Date: Mon, 6 Sep 2021 13:22:30 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Ronan Dunklau <ronan.dunklau@aiven.io>
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
Message-ID: <YTWXhvITxLexDomF@paquier.xyz>
References: 
 <CALj2ACW6YA0SfqwPm1-FRRdeNj1e9isiSax3jZ8MmyQYYQQWNw@mail.gmail.com>
 <3214881.26WV74EoJP@aivenronan>
 <YTCNQAFA2HUPtL+p@paquier.xyz>
 <4129596.LXfQO9unRY@aivenronan>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha256;
	protocol="application/pgp-signature"; boundary="TX7cKc9zV4iPDNT6"
Content-Disposition: inline
In-Reply-To: <4129596.LXfQO9unRY@aivenronan>
Archived-At: 
 <https://www.postgresql.org/message-id/YTWXhvITxLexDomF%40paquier.xyz>
Precedence: bulk


--TX7cKc9zV4iPDNT6
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Fri, Sep 03, 2021 at 11:58:27AM +0200, Ronan Dunklau wrote:
>=20
> 0002 for pg_receivewal tried to simplify the logic of information to retu=
rn,=20
> by using a dedicated struct for this. This accounts for Bahrath's demands=
 to=20
> return every possible field.
> In particular, an is_logical field simplifies the detection of the type o=
f slot.=20
> In my opinion it makes sense to simplify it like this on the client side =
while=20
> being more open-minded on the server side if we ever need to provide a ne=
w=20
> type of slot. Also, GetSlotInformation now returns an enum to be able to=
=20
> handle the different modes of failures, which differ between pg_receivewa=
l and=20
> 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;

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.

> There is still the concern raised by Bharath about being able to select t=
he=20
> way to fetch the replication slot information for the user, and what shou=
ld or=20
> should not be included in the documentation. I am in favor of documenting=
 the=20
> process of selecting the wal start, and maybe include a --start-lsn optio=
n for=20
> the user to override it, but that's maybe for another patch.

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.

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.

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-F927AA6340C8@=
yesql.se
We should not mimic in the frontend errors that are safely trapped in
the backend with the proper locks, in any case.

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.
--
Michael

--TX7cKc9zV4iPDNT6
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAEBCAAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmE1l4YACgkQnvQgOdby
QH1npQ/+OKiwOwHTQrtoasyyp1mm5ZUwepplROuBnMwAz/M3Cl3sNsY50nTmlBjI
EUxlnKQqG+/GoKRSSQsIHBJ7/YQPnsVihpR+hfxQgavJjGzZvlpQM4/Ir1wdt+Nv
g3fEwueAPDgfpXKueF91U7ojqH6Ojind5FqMN/EveBBhak92+ymHqkrZVNr90oDV
8ykPGmBpCaowgxSFLLfqSivdgy8Q7c6mPRoVjiCOMzyEImL4M414bLwJkSaSQpvI
gEhu86Dq2wO4h3jPswRsv+Yz1BhjZ2b+O8r8XR/5Ah1kQkBS76zMtQWnlHBF5jAU
c/CaYMUpY+ySmPRIEwR7y0nEj6tsis+hU0KjtRN3Podyn22Ms//bPVtiQS/HxHTc
6drljw1+VTi31UDm8Yz/XWl2jdSeFBpk0cCAa9aHD206dJhVqgkwlIgtROlAyN8d
Fxu2Fx1xsg7oU3UaLM70wg9UvRO0rNdBvREMqpztkQkyzIHstzOs/6P8vgmi00jG
Zdbz10tjDM9kdXDWDnarHcaL36truGy5HpD5/WYhPRyxLd8E4OFkNUP/BY9v11uD
mPCySnAB8z65Bs2dQN5W6yj4t4gIMwiEKWTad0OGXgj/fEkVPw9+4KGTcJj7tbPE
kfv1NSlq3/wHtm5LuoEuIww8slkv3HIWT+9ritFZzvuvvQTrWY8=
=58Q+
-----END PGP SIGNATURE-----

--TX7cKc9zV4iPDNT6--