Re: Refactor pg_rewind code and make it work against a standby

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