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