From: Ronan Dunklau <ronan.dunklau@aiven.io>
To: Michael Paquier <michael@paquier.xyz>
Cc: Kyotaro Horiguchi <horikyota.ntt@gmail.com>,
 bharath.rupireddyforpostgres@gmail.com, pgsql-hackers@lists.postgresql.org,
 sawada.mshk@gmail.com
Subject: Re: pg_receivewal starting position
Date: Mon, 06 Sep 2021 08:50:21 +0200
Message-ID: <16991660.zIo0mqpRSP@aivenronan>
Organization: aiven
In-Reply-To: <YTWXhvITxLexDomF@paquier.xyz>
References: 
 <CALj2ACW6YA0SfqwPm1-FRRdeNj1e9isiSax3jZ8MmyQYYQQWNw@mail.gmail.com>
 <4129596.LXfQO9unRY@aivenronan> <YTWXhvITxLexDomF@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/16991660.zIo0mqpRSP%40aivenronan>
Precedence: bulk

Le lundi 6 septembre 2021, 06:22:30 CEST Michael Paquier a =E9crit :
> On Fri, Sep 03, 2021 at 11:58:27AM +0200, Ronan Dunklau wrote:
> > 0002 for pg_receivewal tried to simplify the logic of information to
> > return, by using a dedicated struct for this. This accounts for Bahrath=
's
> > demands to return every possible field.
> > In particular, an is_logical field simplifies the detection of the type=
 of
> > slot. In my opinion it makes sense to simplify it like this on the clie=
nt
> > side while being more open-minded on the server side if we ever need to
> > provide a new type of slot. Also, GetSlotInformation now returns an enum
> > to be able to handle the different modes of failures, which differ
> > between pg_receivewal and pg_basebackup.
>=20
> +   if (PQserverVersion(conn) < 150000)
> +       return READ_REPLICATION_SLOT_UNSUPPORTED;
> [...]
> +typedef enum {
> +   READ_REPLICATION_SLOT_OK,
> +   READ_REPLICATION_SLOT_UNSUPPORTED,
> +   READ_REPLICATION_SLOT_ERROR,
> +   READ_REPLICATION_SLOT_NONEXISTENT
> +} ReadReplicationSlotStatus;
>=20
> Do you really need this much complication?  We could treat the
> unsupported case and the non-existing case the same way: we don't know
> so just assign InvalidXlogRecPtr or NULL to the fields of the
> structure, and make GetSlotInformation() return false just on error,
> with some pg_log_error() where adapted in its internals.

I actually started with the implementation you propose, but changed my mind=
=20
while writing it because I realised it's easier to reason about like this,=
=20
instead of failing silently during READ_REPLICATION_SLOT to fail a bit late=
r=20
when actually trying to start the replication slot because it doesn't exist=
s.=20
Either the user expects the replication slot to exists, and in this case we=
=20
should retry the whole loop in the hope of getting an interesting LSN, or t=
he=20
user doesn't and shouldn't even pass a replication_slot name to begin with.

>=20
> > There is still the concern raised by Bharath about being able to select
> > the
> > way to fetch the replication slot information for the user, and what
> > should or should not be included in the documentation. I am in favor of
> > documenting the process of selecting the wal start, and maybe include a
> > --start-lsn option for the user to override it, but that's maybe for
> > another patch.
>=20
> The behavior of pg_receivewal that you are implementing should be
> documented.  We don't say either how the start location is selected
> when working on an existing directory, so I would recommend to add a
> paragraph in the description section to detail all that, as of:
> - First a scan of the existing archives is done.
> - If nothing is found, and if there is a slot, request the slot
> information.
> - If still nothing (aka slot undefined, or command not supported), use
> the last flush location.

Sounds good, I will add another patch for the documentation of this.

>=20
> As a whole, I am not really convinced that we need a new option for
> that as long as we rely on a slot with pg_receivewal as these are used
> to make sure that we avoid holes in the WAL archives.
>=20
> Regarding pg_basebackup, Daniel has proposed a couple of days ago a
> different solution to trap errors earlier, which would cover the case
> dealt with here:
> https://www.postgresql.org/message-id/0F69E282-97F9-4DB7-8D6D-F927AA6340C=
8@y
> esql.se=20

I will take a look.

> We should not mimic in the frontend errors that are safely trapped
> in the backend with the proper locks, in any case.

I don't understand what you mean by this ? My original proposal was for the=
=20
command to actually attach to the replication slot while reading it, thus=20
keeping a lock on it to prevent dropping or concurrent access on the server.
>=20
> While on it, READ_REPLICATION_SLOT returns a confirmed LSN when
> grabbing the data of a logical slot.  We are not going to use that
> with pg_recvlogical as by default START_STREAMING 0/0 would just use
> the confirmed LSN.  Do we have use cases where this information would
> help?  There is the argument of consistency with physical slots and
> that this can be helpful to do sanity checks for clients, but that's
> rather thin.

If we don't, we should rename the command to READ_PHYSICAL_REPLICATION_SLOT=
=20
and error out on the server if the slot is not actually a physical one to=20
spare the client from performing those checks. I still think it's better to=
=20
support both cases as opposed to having two completely different APIs=20
(READ_(PHYSICAL)_REPLICATION_SLOT for physical ones on a replication=20
connection, pg_replication_slots view for logical ones) as it would enable=
=20
more for third-party clients at a relatively low maintenance cost for us. =
=20

=2D-=20
Ronan Dunklau