On Thu, Oct 30, 2025 at 8:12 AM Peter Smith <[email protected]> wrote:
>
> 3.
> +static void
> +report_sequence_errors(StringInfo insuffperm_seqs, StringInfo
> mismatched_seqs,
> + StringInfo insuffperm_seqs)
>
> Perhaps this function should also have an assertion:
>
> Assert(insuffperm_seqs->len || mismatched_seqs->len || insuffperm_seqs->len);
>
I don't think such an assertion is helpful. As of now, this is called
from only one place where we ensure that one of the conditions
mentioned in assert is true but in future even if we call that without
any condition being true, it will just be a harmless call.
Other review comments:
===================
1. In copy_sequences, we start a transaction before getting the
changes from the publisher and keep using it till we copy all
sequences. If we don't need to retain lock while querying from a
publisher, then can't we even commit the xact before that and start a
new one for the copy_sequence part? Is it possible that we fetch the
required sequence information in LogicalRepSyncSequences()?
2. Isn't it better to add some comments as to why we didn't decide to
retain locks on required sequences while querying the publisher, and
rather re-validate them later?
3. To avoid a lot of parameters in get_remote_sequence_info and
validate_sequence(), can we have one function call say
get_and_validate_seq_info()? It will retrieve only the parameters
required by copy_sequence.
4.
validate_sequence()
{
...
+ /* Sequence was concurrently invalidated? */
+ if (!seqinfo->entry_valid)
+ {
+ ReleaseSysCache(tup);
+ return COPYSEQ_SKIPPED;
+ }
So, if the sequence is renamed concurrently, we get the cause as
SKIPPED but shouldn't it be COPYSEQ_MISMATCH? Is it possible to
compare the local sequence name with the remote name in
validate_sequence() after taking lock on sequence relation? If so, we
can return the state as COPYSEQ_MISMATCH.
Apart from the above, I have modified comments at various places in
the patch, see attached.
--
With Regards,
Amit Kapila.
diff --git a/src/backend/commands/sequence.c b/src/backend/commands/sequence.c
index 9d1dc87ceb1..8d671b7a29d 100644
--- a/src/backend/commands/sequence.c
+++ b/src/backend/commands/sequence.c
@@ -1796,9 +1796,9 @@ pg_sequence_parameters(PG_FUNCTION_ARGS)
/*
* Return the sequence tuple along with its page LSN.
*
- * This is primarily intended for use by pg_dump to gather sequence data
- * without needing to individually query each sequence relation. This will also
- * be used by logical replication while synchronizing sequences.
+ * This is primarily used by pg_dump to efficiently collect sequence data
+ * without querying each sequence individually, and is also leveraged by
+ * logical replication while synchronizing sequences.
*/
Datum
pg_get_sequence_data(PG_FUNCTION_ARGS)
@@ -1842,11 +1842,6 @@ pg_get_sequence_data(PG_FUNCTION_ARGS)
values[0] = Int64GetDatum(seq->last_value);
values[1] = BoolGetDatum(seq->is_called);
-
- /*
- * For details about recording the LSN, see the
- * UpdateSubscriptionRelState() call in copy_sequence().
- */
values[2] = LSNGetDatum(PageGetLSN(page));
UnlockReleaseBuffer(buf);
diff --git a/src/backend/replication/logical/sequencesync.c
b/src/backend/replication/logical/sequencesync.c
index 7927aeac054..5418327c237 100644
--- a/src/backend/replication/logical/sequencesync.c
+++ b/src/backend/replication/logical/sequencesync.c
@@ -28,12 +28,13 @@
* to handle synchronization.
*
* A single sequencesync worker is responsible for synchronizing all sequences
- * marked in pg_subscription_rel. It begins by retrieving the list of sequences
- * flagged for synchronization. These sequences are then processed in batches,
- * allowing multiple entries to be synchronized within a single transaction.
- * The worker fetches the current sequence values and page LSNs from the remote
- * publisher, updates the corresponding sequences on the local subscriber, and
- * finally marks each sequence as READY upon successful synchronization.
+ * in INIT state in pg_subscription_rel. It begins by retrieving the list of
+ * sequences flagged for synchronization. These sequences are then processed
+ * in batches, allowing multiple entries to be synchronized within a single
+ * transaction. The worker fetches the current sequence values and page LSNs
+ * from the remote publisher, updates the corresponding sequences on the local
+ * subscriber, and finally marks each sequence as READY upon successful
+ * synchronization.
*
* Sequence state transitions follow this pattern:
* INIT → READY
@@ -42,19 +43,9 @@
* sequences are synchronized per transaction. The locks on the sequence
* relation will be periodically released at each transaction commit.
*
- * XXX: An alternative design was considered where the launcher process would
- * periodically check for sequences that need syncing and then start the
- * sequencesync worker. However, the approach of having the apply worker
- * manage the sequencesync worker was chosen for the following reasons:
- * a) The apply worker can access the sequences that need to be synchronized
- * from the pg_subscription_rel system catalog. Whereas the launcher process
- * operates without direct database access so would need a framework to
- * establish connections with the databases to retrieve the sequences for
- * synchronization.
- * b) It utilizes the existing tablesync worker code to start the sequencesync
- * process, thus preventing code duplication in the launcher.
- * c) It simplifies code maintenance by consolidating changes to a single
- * location rather than multiple components.
+ * XXX: We didn't choose launcher process to maintain the launch of
sequencesync
+ * worker as it didn't have database connection to access the sequences from
the
+ * pg_subscription_rel system catalog that need to be synchronized.
*-------------------------------------------------------------------------
*/
@@ -338,8 +329,7 @@ copy_sequence(LogicalRepSequenceInfo *seqinfo, int64
last_value,
/*
* Record the remote sequence’s LSN in pg_subscription_rel and mark the
- * sequence as READY. The LSN represents the WAL position of the remote
- * sequence at the time it was synchronized.
+ * sequence as READY.
*/
UpdateSubscriptionRelState(MySubscription->oid, seqoid,
SUBREL_STATE_READY,
page_lsn, false);
