Re: pg_receivewal starting position - Mailing list pgsql-hackers

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