Re: pg_receivewal starting position

On Thu, Aug 26, 2021 at 02:14:27PM +0200, Ronan Dunklau wrote:
> Following the discussion at [1], I refactored the implementation into 
> streamutil and added a third patch making use of it in pg_basebackup itself in 
> order to fail early if the replication slot doesn't exist, so please find 
> attached v2 for that.

Thanks for the split.  That helps a lot.

+
+
 /*
  * Run IDENTIFY_SYSTEM through a given connection and give back to caller

The patch series has some noise diffs here and there, you may want to
clean up that for clarity.

+   if (slot == NULL || !slot->in_use)
+   {
+       LWLockRelease(ReplicationSlotControlLock);
+
+       ereport(ERROR,
+               (errcode(ERRCODE_UNDEFINED_OBJECT),
LWLocks are released on ERROR, so no need for LWLockRelease() here.

+    <listitem>
+      <para>
+      Read information about the named replication slot.  This is
useful to determine which WAL location we should be asking the server
to start streaming at.

A nit.  You may want to be more careful with the indentation of the
documentation.  Things are usually limited in width for readability.
More <literal> markups would be nice for the field names used in the
descriptions.

+   if (slot == NULL || !slot->in_use)                                                                                                                                                                                       [...]
+       ereport(ERROR,
+               (errcode(ERRCODE_UNDEFINED_OBJECT),
+                errmsg("replication slot \"%s\" does not exist",
+                       cmd->slotname)));
[...]
+       if (PQntuples(res) == 0)
+       {
+               pg_log_error("replication slot %s does not exist", slot_name);
+               PQclear(0);
+               return false;
So, the backend and ReadReplicationSlot() report an ERROR if a slot
does not exist but pg_basebackup's GetSlotInformation() does the same
if there are no tuples returned.  That's inconsistent.  Wouldn't it be
more instinctive to return a NULL tuple instead if the slot does not
exist to be able to check after real ERRORs in frontends using this
interface?  A slot in use exists, so the error is a bit confusing here
anyway, no?

+    * XXX: should we allow the caller to specify which target timeline it wants
+    * ?
+    */
What are you thinking about here?

-# restarts of pg_receivewal will see this segment as full..
+# restarts of pg_receivewal will see this segment as full../
Typo.

+   TupleDescInitBuiltinEntry(tupdesc, (AttrNumber) 4, "restart_lsn_timeline",
+                             INT4OID, -1, 0);
+   TupleDescInitBuiltinEntry(tupdesc, (AttrNumber) 5, "confirmed_flush_lsn_timeline",
+                             INT4OID, -1, 0);
I would call these restart_tli and confirmed_flush_tli., without the
"lsn" part.

The patch for READ_REPLICATION_SLOT could have some tests using a
connection that has replication=1 in some TAP tests.  We do that in
001_stream_rep.pl with SHOW, as one example.

-               'slot0'
+               'slot0',                     '-p',
+               "$port"
Something we are missing here?
--
Michael