From: Ronan Dunklau <ronan.dunklau@aiven.io>
To: Kyotaro Horiguchi <horikyota.ntt@gmail.com>,
 Michael Paquier <michael@paquier.xyz>
Cc: bharath.rupireddyforpostgres@gmail.com,
 pgsql-hackers@lists.postgresql.org, sawada.mshk@gmail.com
Subject: Re: pg_receivewal starting position
Date: Thu, 02 Sep 2021 10:08:26 +0200
Message-ID: <3214881.26WV74EoJP@aivenronan>
Organization: aiven
In-Reply-To: <YTB9HeN7nc6dQkxz@paquier.xyz>
References: 
 <CALj2ACW6YA0SfqwPm1-FRRdeNj1e9isiSax3jZ8MmyQYYQQWNw@mail.gmail.com>
 <20210902.144554.1303720268994714850.horikyota.ntt@gmail.com>
 <YTB9HeN7nc6dQkxz@paquier.xyz>
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset="iso-8859-1"
Archived-At: 
 <https://www.postgresql.org/message-id/3214881.26WV74EoJP%40aivenronan>
Precedence: bulk

Le jeudi 2 septembre 2021, 09:28:29 CEST Michael Paquier a =E9crit :
> On Thu, Sep 02, 2021 at 02:45:54PM +0900, Kyotaro Horiguchi wrote:
> > At Wed, 1 Sep 2021 10:30:05 +0530, Bharath Rupireddy
> > <bharath.rupireddyforpostgres@gmail.com> wrote in If I read the patch
> > correctly the situation above is warned by the following message then
> > continue to the next step giving up to identify start position from slot
> > data.
>=20
> Better to fallback to the past behavior if attempting to use a
> pg_receivewal >=3D 15 with a PG instance older than 14.
>=20
> >> "server does not suport fetching the slot's position, resuming from the
> >> current server position instead">=20
> > (The message looks a bit too long, though..)
>=20
> Agreed.  Falling back to a warning is not the best answer we can have
> here, as there could be various failure types, and for some of them a
> hard failure is adapted;
> - Failure in the backend while running READ_REPLICATION_SLOT.  This
> should imply a hard failure, no?
> - Slot that does not exist.  In this case, we could fall back to the
> current write position of the server.
>=20
> by default if the slot information cannot be retrieved.
> Something that's disturbing me in patch 0002 is that we would ignore
> the results of GetSlotInformation() if any error happens, even if
> there is a problem in the backend, like an OOM.  We should be careful
> about the semantics here.

Ok !

>=20
> > However, if the operator doesn't know the server is old, pg_receivewal
> > starts streaming from unexpected position, which is a kind of
> > disaster. So I'm inclined to agree to Bharath, but rather I imagine of
> > an option to explicitly specify how to determine the start position.
> >=20
> > --start-source=3D[server,wal,slot]  specify starting-LSN source, defaul=
t is
> >=20
> >                      trying all of them in the order of wal, slot, serv=
er.
> >=20
> > I don't think the option doesn't need to accept multiple values at once.
>=20
> What is the difference between "wal" and "server"?  "wal" stands for
> the start position of the set of files stored in the location
> directory, and "server" is the location that we'd receive from the
> server?  I don't think that we need that because, when using a slot,
> we know that we can rely on the LSN that the slot retains for
> pg_receivewal as that should be the same point as what has been
> streamed last.  Could there be an argument instead for changing the
> default and rely on the slot information rather than scanning the
> local WAL archive path for the start point when using --slot?  When
> using pg_receivewal as a service, relying on a scan of the WAL archive
> directory if there is no slot and fallback to an invalid LSN if there
> is nothing is fine by me, but I think that just relying on the slot
> information is saner as the backend makes sure that nothing is
> missing.  That's also more useful when streaming changes from a single
> slot from multiple locations (stream to location 1 with a slot, stop
> pg_receivewal, stream to location 2 that completes 1 with the same
> slot).

One benefit I see from first trying to get it from the local WAL stream is =
that=20
we may end up in a state where it has been flushed to disk but we couldn't=
=20
advance the replication slot. In that case it is better to resume from the=
=20
point on disk. Maybe taking the max(slot_lsn, local_file_lsn) would work be=
st=20
for the use case you're describing.

>=20
> +        pg_log_error("Slot \"%s\" is not a physical replication slot",
> +                     replication_slot);
> In 0003, the format of this error is not really project-like.
> Something like that perhaps would be more adapted:
> "cannot use the slot provided, physical slot expected."
>=20
> I am not really convinced about the need of getting the active state
> and the PID used in the backend when fetcing the slot data,
> particularly if that's just for some frontend-side checks.  The
> backend has safeguards already for all that.

I could see the use for sending active_pid for use within pg_basebackup: at=
=20
least we could fail early if the slot is already in use. But at the same ti=
me,=20
maybe it won't be in use anymore once we need it.

>=20
> While looking at that, I have applied de1d4fe to add
> PostgresNode::command_fails_like(), coming from 0003, and put my hands
> on 0001 as per the attached, as the starting point.  That basically
> comes down to all the points raised upthread, plus some tweaks for
> things I bumped into to get the semantics of the command to what looks
> like the right shape.

Thanks, I was about to send a new patchset with basically the same thing. I=
t=20
would be nice to know we work on the same thing concurrently in the future =
to=20
avoid duplicate efforts. I'll rebase and send the updated version for patch=
es=20
0002 and 0003 of my original proposal once we reach an agreement over the=20
behaviour / options of pg_receivewal.

Also considering the number of different fields to be filled by the=20
GetSlotInformation function, my local branch groups them into a dedicated=20
struct which is more convenient than having X possibly null arguments.

=2D-=20
Ronan Dunklau