MIME-Version: 1.0
References: <CAFPTHDZXX9WQ_X1ZfEvS248T+pKuk6SmCnXcvgPM059N1xPUfA@mail.gmail.com>
 <CAJpy0uDLUEjHHME8om1vAf6qkXCeRR6cBvkpK8yWBAC=T0ZFLA@mail.gmail.com>
 <CAFPTHDZg1JrunGgOj332hr+gUuH_Jm7skqPpYSvd-QE3yEdRDQ@mail.gmail.com>
 <CAJpy0uBz7MCSUkvFJD9ij65vBahNmY+bfCgdGKRqXovYs+K_TA@mail.gmail.com>
 <CAJpy0uDsuNqjWd-TmGBxqSS1rnVCJ3B8=SYrtxQ=Vs8kb71QFA@mail.gmail.com>
 <CAJpy0uAMWg3KcXtVBS7B0rnchLNrCCVYBByJCzAp=u5LERgtfA@mail.gmail.com> <CAFPTHDZwEhxhDAeqcPi0GuYN6xBs8gFXHOMUnbg3u2Xigcz4Zg@mail.gmail.com>
In-Reply-To: <CAFPTHDZwEhxhDAeqcPi0GuYN6xBs8gFXHOMUnbg3u2Xigcz4Zg@mail.gmail.com>
From: Peter Smith <smithpb2250@gmail.com>
Date: Fri, 13 Feb 2026 16:42:35 +1100
Message-ID: <CAHut+PvbkDGj8qHMdeQftqgds2v71KNG9PeC6TDCKYQn_AAVCA@mail.gmail.com>
Subject: Re: [PATCH] Support automatic sequence replication
To: Ajin Cherian <itsajin@gmail.com>
Cc: shveta malik <shveta.malik@gmail.com>, 
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Archived-At: <https://www.postgresql.org/message-id/CAHut%2BPvbkDGj8qHMdeQftqgds2v71KNG9PeC6TDCKYQn_AAVCA%40mail.gmail.com>
Precedence: bulk

Hi Ajin.

Some review comments for patch v4-0001

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

GetSequence:

1.
+/*
+ * Read the current sequence values (last_value and is_called)
+ *
+ * This is a read-only operation that acquires AccessShareLock on the sequence.
+ * Used by logical replication sequence synchronization to detect drift.
+ */

The comment seems stale. e.g. the function is not acquiring a lock
anymore, contrary to what that comment says.


======
.../replication/logical/sequencesync.c

2.
-static List *seqinfos = NIL;

The removal of this global was not strictly part of this patch; it is
more like a prerequisite to make everything tidier, so your new code
does not go further down that track of side-affecting a global. From
that POV, I thought this global removal should be implemented as a
first/separate (0001) patch so that it might be quickly reviewed and
committed independently of the new seq-sync logic.

~~~

LogicalRepSyncSequences:

3.
+ /* Error on unexpected states */
+ if (relstate != SUBREL_STATE_INIT && relstate != SUBREL_STATE_READY)
+ {
+ table_close(sequence_rel, NoLock);
+ ereport(ERROR,
+ (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("unexpected relstate '%c' for sequence \"%s.%s\" in
subscription \"%s\"",
+ relstate,
+ get_namespace_name(RelationGetNamespace(sequence_rel)),
+ RelationGetRelationName(sequence_rel),
+ MySubscription->name)));
+ }
+

How is this possible? Should it just be Assert?

~~~

start_sequence_sync:

4.
+ /*
+ * Synchronize all sequences (both READY and INIT states).
+ * The function will process INIT sequences first, then READY sequences.
+ */
+ sequence_copied = LogicalRepSyncSequences(LogRepWorkerWalRcvConn);

Why is talking about the processing order relevant?


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

5.
+ /* Check if any new sequences need syncing */
+ ProcessSequencesForSync();
+

Maybe don't say "new" because IIUC it also handles older sequences
where the values have drifted.

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

6.
-$result = $node_publisher->safe_psql(
- 'postgres', qq(
- SELECT last_value, is_called FROM regress_s1;
-));
-is($result, '200|t', 'Check sequence value in the publisher');
-
-# Check - existing sequence ('regress_s1') is not synced
-$result = $node_subscriber->safe_psql(
- 'postgres', qq(
- SELECT last_value, is_called FROM regress_s1;
-));
-is($result, '100|t', 'REFRESH PUBLICATION will not sync existing sequence');
-

Since you are no longer testing "existing sequences" in this test
part, should you also remove the earlier INSERT for 'regress_s1'?

======
Kind Regards,
Peter Smith.
Fujitsu Australia.