Re: Refactor pg_rewind code and make it work against a standby - Mailing list pgsql-hackers
From | Heikki Linnakangas |
---|---|
Subject | Re: Refactor pg_rewind code and make it work against a standby |
Date | |
Msg-id | da768d3a-dddc-fd3f-4b64-e951a1926bfa@iki.fi Whole thread Raw |
In response to | Re: Refactor pg_rewind code and make it work against a standby (Soumyadeep Chakraborty <soumyadeep2007@gmail.com>) |
Responses |
Re: Refactor pg_rewind code and make it work against a standby
(Heikki Linnakangas <hlinnaka@iki.fi>)
|
List | pgsql-hackers |
On 25/09/2020 02:56, Soumyadeep Chakraborty wrote: > On Thu, Sep 24, 2020 at 10:27 AM Heikki Linnakangas <hlinnaka@iki.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 a problem. >>> 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. >>> 16. >>> >>>> conn = PQconnectdb(connstr_source); >>>> >>>> if (PQstatus(conn) == CONNECTION_BAD) >>>> pg_fatal("could not connect to server: %s", >>>> PQerrorMessage(conn)); >>>> >>>> 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! - Heikki
pgsql-hackers by date: