Re: pg_receivewal starting position

Le lundi 25 octobre 2021, 10:10:13 CEST Michael Paquier a écrit :
> 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 écrit :
> >> 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.
> >> 
> >> 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.
> > 
> > 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 <= 14, and rely on the new
> behavior with a server >= 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 <= 14?

Sorry I haven't been clear. For the use case of this patch, the current 
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 
need to make sure the command is actually executed, and check the version 
before calling the function, which would make the check executed twice.

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

-               if (replication_slot != NULL)
+               if (replication_slot != NULL && PQserverVersion(conn) >= 
150000)
                {
                        if (!GetSlotInformation(conn, replication_slot, 
&stream.startpos,
                                                                        &stream.timeline))

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



-- 
Ronan Dunklau