Thanks for the review! I'll post a new version shortly, with your
comments incorporated. More detailed response to a few of them below:
On 18/09/2020 10:41, Kyotaro Horiguchi wrote:
> I don't think filemap_finalize needs to iterate over filemap twice.
True, but I thought it's more clear that way, doing one thing at a time.
> hash_string_pointer is a copy of that of pg_verifybackup.c. Is it
> worth having in hashfn.h or .c?
I think it's fine for now. Maybe in the future if more copies crop up.
>> --- a/src/bin/pg_rewind/pg_rewind.c
>> +++ b/src/bin/pg_rewind/pg_rewind.c
>> ...
>> + filemap_t *filemap;
>> ..
>> + filemap_init();
>> ...
>> + filemap = filemap_finalize();
>
> I'm a bit confused by this, and realized that what filemap_init
> initializes is *not* the filemap, but the filehash. So for example,
> the name of the functions might should be something like this?
>
> filehash_init()
> filemap = filehash_finalyze()/create_filemap()
My thinking was that filemap_* is the prefix for the operations in
filemap.c, hence filemap_init(). I can see the confusion, though, and I
think you're right that renaming would be good. I renamed them to
filehash_init(), and decide_file_actions(). I think those names make the
calling code in pg_rewind.c quite clear.
>> /*
>> * Also check that full_page_writes is enabled. We can get torn pages if
>> * a page is modified while we read it with pg_read_binary_file(), and we
>> * rely on full page images to fix them.
>> */
>> str = run_simple_query(conn, "SHOW full_page_writes");
>> if (strcmp(str, "on") != 0)
>> pg_fatal("full_page_writes must be enabled in the source server");
>> pg_free(str);
>
> This is a part not changed by this patch set. But If we allow to
> connect to a standby, this check can be tricked by setting off on the
> primary and "on" on the standby (FWIW, though). Some protection
> measure might be necessary.
Good point, the value in the primary is what matters. In fact, even when
connected to the primary, the value might change while pg_rewind is
running. I'm not sure how we could tighten that up.
> + if (chunksize > rq->length)
> + {
> + pg_fatal("received more than requested for file \"%s\"",
> + rq->path);
> + /* receiving less is OK, though */
>
> Don't we need to truncate the target file, though?
If a file is truncated in the source while pg_rewind is running, there
should be a WAL record about the truncation that gets replayed when you
start the server after pg_rewind has finished. We could truncate the
file if we wanted to, but it's not necessary. I'll add a comment.
- Heikki
