Re: Logical Replication of sequences

Hi Vignesh,

There are still pending changes from my previous review of the
0720-0003 patch [1], but here are some new review comments for your
latest patch v20240525-0003.

======
doc/src/sgml/catalogs.sgml

nitpick - fix plurals and tweak the description.

~~~

1.
   <para>
-   This catalog only contains tables known to the subscription after running
-   either <link linkend="sql-createsubscription"><command>CREATE
SUBSCRIPTION</command></link> or
-   <link linkend="sql-altersubscription"><command>ALTER SUBSCRIPTION
... REFRESH
+   This catalog only contains tables and sequences known to the subscription
+   after running either
+   <link linkend="sql-createsubscription"><command>CREATE
SUBSCRIPTION</command></link>
+   or <link linkend="sql-altersubscription"><command>ALTER
SUBSCRIPTION ... REFRESH
    PUBLICATION</command></link>.
   </para>

Shouldn't this mention "REFRESH PUBLICATION SEQUENCES" too?

======
src/backend/commands/sequence.c

SetSequenceLastValue:
nitpick - maybe change: /log_cnt/new_log_cnt/ for consistency with the
other parameter, and to emphasise the old log_cnt is overwritten

======
src/backend/replication/logical/sequencesync.c

2.
+/*
+ * fetch_remote_sequence_data
+ *
+ * Fetch sequence data (current state) from the remote node, including
+ * the latest sequence value from the publisher and the Page LSN for the
+ * sequence.
+ */
+static int64
+fetch_remote_sequence_data(WalReceiverConn *conn, Oid remoteid,
+    int64 *log_cnt, XLogRecPtr *lsn)

2a.
Now you are also returning the 'log_cnt' but that is not mentioned by
the function comment.

~

2b.
Is it better to name these returned by-ref ptrs like 'ret_log_cnt',
and 'ret_lsn' to emphasise they are output variables? YMMV.

~~~

3.
+ /* Process the sequence. */
+ slot = MakeSingleTupleTableSlot(res->tupledesc, &TTSOpsMinimalTuple);
+ while (tuplestore_gettupleslot(res->tuplestore, true, false, slot))

This will have one-and-only-one tuple for the discovered sequence,
won't it? So, why is this a while loop?

======
src/include/commands/sequence.h

nitpick - maybe change: /log_cnt/new_log_cnt/ (same as earlier in this post)

======
src/test/subscription/t/034_sequences.pl

4.
Q. Should we be suspicious that log_cnt changes from '32' to '31', or
is there a valid explanation? It smells like some calculation is
off-by-one, but without debugging I can't tell if it is right or
wrong.

======
Please also see the attached diffs patch, which implements the
nitpicks mentioned above.

======
[1] 0720-0003 review -
https://www.postgresql.org/message-id/CAHut%2BPsfsfzyBrmo8E43qFMp9_bmen2tuCsNYN8sX%3Dfa86SdfA%40mail.gmail.com

Kind Regards,
Peter Smith.
Fujitsu Australia