Re: pg_receivewal starting position

On Tue, Oct 19, 2021 at 05:32:55PM +0200, Ronan Dunklau wrote:
> Following recommendations, I stripped most of the features from the patch. For
> now we support only physical replication slots, and only provide the two fields
> of interest (restart_lsn, restart_tli) in addition to the slot type (fixed at
> physical) to not paint ourselves in a corner.
>
> I also removed the part about pg_basebackup since other fixes have been
> proposed for that case.

Patch 0001 looks rather clean. I have a couple of comments.

+ if (OidIsValid(slot_contents.data.database))
+ elog(ERROR, "READ_REPLICATION_SLOT is only supported for physical slots");

elog() can only be used for internal errors. Errors that can be
triggered by a user should use ereport() instead.

+ok($stdout eq '||',
+ "READ_REPLICATION_SLOT returns NULL values if slot does not exist");
[...]
+ok($stdout =~ 'physical\|[^|]*\|1',
+ "READ_REPLICATION_SLOT returns tuple corresponding to the slot");
Isn't result pattern matching something we usually test with like()?

+($ret, $stdout, $stderr) = $node_primary->psql(
+ 'postgres',
+ "READ_REPLICATION_SLOT $slotname;",
+ extra_params => [ '-d', $connstr_rep ]);
No need for extra_params in this test. You can just pass down
"replication => 1" instead, no?

--- a/src/test/recovery/t/006_logical_decoding.pl
+++ b/src/test/recovery/t/006_logical_decoding.pl
[...]
+ok($stderr=~ 'READ_REPLICATION_SLOT is only supported for physical slots',
+ 'Logical replication slot is not supported');
This one should use like().

+ <para>
+ The slot's <literal>restart_lsn</literal> can also beused as a starting
+ point if the target directory is empty.
+ </para>
I am not sure that there is a need for this addition as the same thing
is said when describing the lookup ordering.

+ If nothing is found and a slot is specified, use the
+ <command>READ_REPLICATION_SLOT</command>
+ command.
It may be clearer to say that the position is retrieved from the
command.

+bool
+GetSlotInformation(PGconn *conn, const char *slot_name, XLogRecPtr
*restart_lsn, TimeLineID* restart_tli)
+{
Could you extend that so as we still run the command but don't crash
if the caller specifies NULL for any of the result fields? This would
be handy.

+ if (PQgetisnull(res, 0, 0))
+ {
+ PQclear(res);
+ pg_log_error("replication slot \"%s\" does not exist",
slot_name);
+ return false;
+ }
+ if (PQntuples(res) != 1 || PQnfields(res) < 3)
+ {
+ 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);
+ PQclear(res);
+ return false;
+ }
Wouldn't it be better to reverse the order of these two checks?

I don't mind the addition of the slot type being part of the result of
READ_REPLICATION_SLOT even if it is not mandatory (?), but at least
GetSlotInformation() should check after it.

+# Setup the slot, and connect to it a first time
+$primary->run_log(
+ [ 'pg_receivewal', '--slot', $slot_name, '--create-slot' ],
+ 'creating a replication slot');
+$primary->psql('postgres',
+ 'INSERT INTO test_table VALUES (generate_series(1,100));');
+$primary->psql('postgres', 'SELECT pg_switch_wal();');
+$nextlsn =
+ $primary->safe_psql('postgres', 'SELECT pg_current_wal_insert_lsn();');
+chomp($nextlsn);
Wouldn't it be simpler to use CREATE_REPLICATION_SLOT with RESERVE_WAL
here, rather than going through pg_receivewal? It seems to me that
this would be cheaper without really impacting the coverage.
--
Michael