Re: Logical Replication of sequences

[Peter Smith]

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

diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index 19d04b1..dcd0b98 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -8102,8 +8102,8 @@ SCRAM-SHA-256$<replaceable>&lt;iteration 
count&gt;</replaceable>:<replaceable>&l
   </indexterm>
 
   <para>
-   The catalog <structname>pg_subscription_rel</structname> contains the
-   state for each replicated tables and sequences in each subscription.  This
+   The catalog <structname>pg_subscription_rel</structname> stores the
+   state of each replicated table and sequence for each subscription.  This
    is a many-to-many mapping.
   </para>
 
diff --git a/src/backend/commands/sequence.c b/src/backend/commands/sequence.c
index a3e7c79..f292fbc 100644
--- a/src/backend/commands/sequence.c
+++ b/src/backend/commands/sequence.c
@@ -342,7 +342,7 @@ ResetSequence(Oid seq_relid)
  * logical replication.
  */
 void
-SetSequenceLastValue(Oid seq_relid, int64 new_last_value, int64 log_cnt)
+SetSequenceLastValue(Oid seq_relid, int64 new_last_value, int64 new_log_cnt)
 {
        SeqTable        elm;
        Relation        seqrel;
@@ -370,7 +370,7 @@ SetSequenceLastValue(Oid seq_relid, int64 new_last_value, 
int64 log_cnt)
 
        seq->last_value = new_last_value;
        seq->is_called = true;
-       seq->log_cnt = log_cnt;
+       seq->log_cnt = new_log_cnt;
 
        MarkBufferDirty(buf);
 
diff --git a/src/include/commands/sequence.h b/src/include/commands/sequence.h
index a302890..4c6aee0 100644
--- a/src/include/commands/sequence.h
+++ b/src/include/commands/sequence.h
@@ -60,7 +60,7 @@ extern ObjectAddress AlterSequence(ParseState *pstate, 
AlterSeqStmt *stmt);
 extern void SequenceChangePersistence(Oid relid, char newrelpersistence);
 extern void DeleteSequenceTuple(Oid relid);
 extern void ResetSequence(Oid seq_relid);
-extern void SetSequenceLastValue(Oid seq_relid, int64 new_last_value, int64 
log_cnt);
+extern void SetSequenceLastValue(Oid seq_relid, int64 new_last_value, int64 
new_log_cnt);
 extern void ResetSequenceCaches(void);
 
 extern void seq_redo(XLogReaderState *record);