Subject: Re: Refactor pg_rewind code and make it work against a standby
To: Soumyadeep Chakraborty <soumyadeep2007@gmail.com>
Cc: Kyotaro Horiguchi <horikyota.ntt@gmail.com>,
 pgsql-hackers <pgsql-hackers@postgresql.org>
References: <0c5b3783-af52-3ee5-f8fa-6e794061f70d@iki.fi>
 <20200820.173224.2258034742176021277.horikyota.ntt@gmail.com>
 <f155aab5-1323-8d0c-9e3b-32703124bf00@iki.fi>
 <20200918.164150.1688011206252014871.horikyota.ntt@gmail.com>
 <CAE-ML+-uLy_DiS4VSkbsjMEY9ZBob5LqBXPc31_dQ-EsSesgEw@mail.gmail.com>
 <4744063e-1a60-1b24-e1b7-7ace5e65f0d2@iki.fi>
 <CAE-ML+9NG=HnLq8P_sHM=9UFUPnWbQDSRnmZrMPXONERVJS+zQ@mail.gmail.com>
From: Heikki Linnakangas <hlinnaka@iki.fi>
Message-ID: <da768d3a-dddc-fd3f-4b64-e951a1926bfa@iki.fi>
Date: Wed, 4 Nov 2020 11:23:58 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.4.0
MIME-Version: 1.0
In-Reply-To: 
 <CAE-ML+9NG=HnLq8P_sHM=9UFUPnWbQDSRnmZrMPXONERVJS+zQ@mail.gmail.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

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