From: Heikki Linnakangas <hlinnaka@iki.fi>
To: Soumyadeep Chakraborty <soumyadeep2007@gmail.com>
Cc: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: Refactor pg_rewind code and make it work against a standby
Date: Wed, 4 Nov 2020 11:23:58 +0200
Message-ID: <da768d3a-dddc-fd3f-4b64-e951a1926bfa@iki.fi> (raw)
In-Reply-To: <CAE-ML+9NG=HnLq8P_sHM=9UFUPnWbQDSRnmZrMPXONERVJS+zQ@mail.gmail.com>
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>
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: hlinnaka@iki.fi, soumyadeep2007@gmail.com, horikyota.ntt@gmail.com
Subject: Re: Refactor pg_rewind code and make it work against a standby
In-Reply-To: <da768d3a-dddc-fd3f-4b64-e951a1926bfa@iki.fi>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox