On 25/09/2020 02:56, Soumyadeep Chakraborty wrote:
On Thu, Sep 24, 2020 at 10:27 AM Heikki Linnakangas <hlinn...@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
