From: Ronan Dunklau <ronan.dunklau@aiven.io>
To: Michael Paquier <michael@paquier.xyz>
Cc: pgsql-hackers@lists.postgresql.org,
 Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>,
 Kyotaro Horiguchi <horikyota.ntt@gmail.com>,
 Masahiko Sawada <sawada.mshk@gmail.com>
Subject: Re: pg_receivewal starting position
Date: Mon, 25 Oct 2021 10:24:46 +0200
Message-ID: <21259852.EfDdHjke4D@aivenronan>
Organization: aiven
In-Reply-To: <YXZmZU6c081lRJ7z@paquier.xyz>
References: 
 <CALj2ACW6YA0SfqwPm1-FRRdeNj1e9isiSax3jZ8MmyQYYQQWNw@mail.gmail.com>
 <3162614.44csPzL39Z@aivenronan> <YXZmZU6c081lRJ7z@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/21259852.EfDdHjke4D%40aivenronan>
Precedence: bulk

Le lundi 25 octobre 2021, 10:10:13 CEST Michael Paquier a =E9crit :
> On Mon, Oct 25, 2021 at 09:50:01AM +0200, Ronan Dunklau wrote:
> > Le lundi 25 octobre 2021, 09:40:10 CEST Michael Paquier a =E9crit :
> >> On Mon, Oct 25, 2021 at 09:15:32AM +0200, Ronan Dunklau wrote:
> >>> Does it make sense though ? The NULL slot_name case handling is pretty
> >>> straight forward has it will be handled by string formatting, but in =
the
> >>> case of a null restart_lsn, we have no way of knowing if the command =
was
> >>> issued at all.
> >>=20
> >> If I am following your point, I don't think that it matters much here,
> >> and it seems useful to me to be able to pass NULL for both of them, so
> >> as one can check if the slot exists or not with an API designed this
> >> way.
> >=20
> > You're right, but I'm afraid we would have to check the server version
> > twice in any case different from the basic pg_receivewal on (once in the
> > function itself, and one before calling it if we want a meaningful
> > result). Maybe we should move the version check outside the
> > GetSlotInformation function to avoid this, and let it fail with a syntax
> > error when the server doesn't support it ?
> With the approach taken by the patch, we fall down silently to the
> previous behavior if we connect to a server <=3D 14, and rely on the new
> behavior with a server >=3D 15, ensuring compatibility.  Why would you
> want to make sure that the command is executed when we should just
> enforce that the old behavior is what happens when there is a slot
> defined and a backend <=3D 14?

Sorry I haven't been clear. For the use case of this patch, the current=20
approach is perfect (and we supply the restart_lsn).

However, if we want to support the case of "just check if the slot exists",=
 we=20
need to make sure the command is actually executed, and check the version=20
before calling the function, which would make the check executed twice.

What I'm proposing is just that we let the responsibility of checking=20
PQServerVersion() to the caller, and remove it from GetSlotInformation, ie:

=2D               if (replication_slot !=3D NULL)
+               if (replication_slot !=3D NULL && PQserverVersion(conn) >=
=3D=20
150000)
                {
                        if (!GetSlotInformation(conn, replication_slot,=20
&stream.startpos,
                                                                        &st=
ream.timeline))

That way, if we introduce a caller wanting to use this function as an API t=
o=20
check a slot exists, the usage of checking the server version beforehand wi=
ll=20
be consistent.


=2D-=20
Ronan Dunklau