|From:||Heikki Linnakangas <hlinnaka(at)iki(dot)fi>|
|To:||Soumyadeep Chakraborty <soumyadeep2007(at)gmail(dot)com>|
|Cc:||Kyotaro Horiguchi <horikyota(dot)ntt(at)gmail(dot)com>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>|
|Subject:||Re: Refactor pg_rewind code and make it work against a standby|
|Views:||Raw Message | Whole Thread | Download mbox | Resend email|
On 25/09/2020 02:56, Soumyadeep Chakraborty wrote:
> On Thu, Sep 24, 2020 at 10:27 AM Heikki Linnakangas <hlinnaka(at)iki(dot)fi> wrote:
>>> 7. Please address the FIXME for the symlink case:
>>> /* FIXME: Check if it points to the same target? */
>> It's not a new issue. Would be nice to fix, of course. I'm not sure what
>> the right thing to do would be. If you have e.g. replaced
>> postgresql.conf with a symlink that points outside the data directory,
>> would it be appropriate to overwrite it? Or perhaps we should throw an
>> error? We also throw an error if a file is a symlink in the source but a
>> regular file in the target, or vice versa.
> Hmm, I can imagine a use case for 2 different symlink targets on the
> source and target clusters. For example the primary's pg_wal directory
> can have a different symlink target as compared to a standby's
> (different mount points on the same network maybe?). An end user might
> not desire pg_rewind meddling with that setup or may desire pg_rewind to
> treat the source as a source-of-truth with respect to this as well and
> would want pg_rewind to overwrite the target's symlink. Maybe doing a
> check and emitting a warning with hint/detail is prudent here while
> taking no action.
We have special handling for 'pg_wal' to pretend that it's a regular
directory (see process_source_file()), so that's taken care of. But if
you did a something similar with some other subdirectory, that would be
>>> 14. queue_overwrite_range(), finish_overwrite() instead of
>>> queue_fetch_range(), finish_fetch()? Similarly update\
>>> *_fetch_file_range() and *_finish_fetch()
>>> 15. Let's have local_source.c and libpq_source.c instead of *_fetch.c
>> Good idea! And fetch.h -> rewind_source.h.
> +1. You might have missed the changes to rename "fetch" -> "overwrite"
> as was mentioned in 14.
I preferred the "fetch" nomenclature in those function names. They fetch
and overwrite the file ranges, so 'fetch' still seems appropriate.
"fetch" -> "overwrite" would make sense if you wanted to emphasize the
"overwrite" part more. Or we could rename it to "fetch_and_overwrite".
But overall I think "fetch" is fine.
>>>> conn = PQconnectdb(connstr_source);
>>>> if (PQstatus(conn) == CONNECTION_BAD)
>>>> pg_fatal("could not connect to server: %s",
>>>> if (showprogress)
>>>> pg_log_info("connected to server");
>>> The above hunk should be part of init_libpq_source(). Consequently,
>>> init_libpq_source() should take a connection string instead of a conn.
>> The libpq connection is also needed by WriteRecoveryConfig(), that's why
>> it's not fully encapsulated in libpq_source.
> Ah. I find it pretty weird why we need to specify --source-server to
> have ----write-recovery-conf work. From the code, we only need the conn
> for calling PQserverVersion(), something we can easily get by slurping
> pg_controldata on the source side? Maybe we can remove this limitation?
Yeah, perhaps. In another patch :-).
I read through the patches one more time, fixed a bunch of typos and
such, and pushed patches 1-4. I'm going to spend some more time on
testing the last patch. It allows using a standby server as the source,
and we don't have any tests for that yet. Thanks for the review!
|Next Message||Ajin Cherian||2020-11-04 09:26:49||Re: [HACKERS] logical decoding of two-phase transactions|
|Previous Message||Jakub Wartak||2020-11-04 09:07:59||Re: automatic analyze: readahead - add "IO read time" log message|