Re: pg_receivewal starting position

On Wed, Oct 20, 2021 at 02:58:26PM +0200, Ronan Dunklau wrote:
> After sending the previous patch suite, I figured it would be worthwhile to 
> also have tests covering timeline switches, which was not covered before.

That seems independent to me.  I'll take a look.

> So please find attached a new version with an additional patch for those tests, 
> covering both  "resume from last know archive" and "resume from the 
> replication slots position" cases.

So, taking things in order, I have looked at 0003 and 0001, and
attached are refined versions for both of them.

0003 is an existing hole in the docs, which I think we had better
address first and backpatch, taking into account that the starting
point calculation considers compressed segments when looking for
completed segments.

Regarding 0001, I have found the last test to check for NULL values
returned by READ_REPLICATION_SLOT after dropping the slot overlaps
with the first test, so I have removed that.  I have expanded a bit
the use of like(), and there were some confusion with
PostgresNode::psql and some extra arguments (see DROP_REPLICATION_SLOT
and CREATE_REPLICATION_SLOT, and no need for return values in the 
CREATE case either).  Some comments, docs and code have been slightly
tweaked.

Here are some comments about 0002.

+       /* The commpand should always return precisely one tuple */
s/commpand/command/

+       pg_log_error("could not fetch replication slot: got %d rows and %d fields, expected %d rows and %d or more fields",
+                    PQntuples(res), PQnfields(res), 1, 3);
Should this be "could not read" instead?

+       if (sscanf(PQgetvalue(res, 0, 1), "%X/%X", &hi, &lo) != 2)
+       {
+           pg_log_error("could not parse slot's restart_lsn \"%s\"",
+                        PQgetvalue(res, 0, 1));
+           PQclear(res);
+           return false;
+       }
Wouldn't it be saner to initialize *restart_lsn and *restart_tli to
some default values at the top of GetSlotInformation() instead, if
they are specified by the caller?  And I think that we should still
complain even if restart_lsn is NULL.

On a quick read of 0004, I find the split of the logic with
change_timeline() a bit hard to understand.  It looks like we should
be able to make a cleaner split, but I am not sure how that would
look, though.
--
Michael