Re: [PATCH] Support automatic sequence replication

public inbox for [email protected]  
help / color / mirror / Atom feed

From: Peter Smith <[email protected]>
To: Ajin Cherian <[email protected]>
Cc: shveta malik <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Subject: Re: [PATCH] Support automatic sequence replication
Date: Fri, 13 Feb 2026 16:42:35 +1100
Message-ID: <CAHut+PvbkDGj8qHMdeQftqgds2v71KNG9PeC6TDCKYQn_AAVCA@mail.gmail.com> (raw)
In-Reply-To: <CAFPTHDZwEhxhDAeqcPi0GuYN6xBs8gFXHOMUnbg3u2Xigcz4Zg@mail.gmail.com>
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>

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.

view thread (58+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected]
  Subject: Re: [PATCH] Support automatic sequence replication
  In-Reply-To: <CAHut+PvbkDGj8qHMdeQftqgds2v71KNG9PeC6TDCKYQn_AAVCA@mail.gmail.com>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox