On Fri, Feb 13, 2026 at 4:43 PM Peter Smith <[email protected]> wrote: > > 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. >
Fixed.
>
> ======
> .../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.
>
Keeping it as is for the time being. Let's see what everybody else thinks.
> ~~~
>
> 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?
>
Fixed.
> ~~~
>
> 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?
>
>
Removed.
>
> ======
> 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'?
>
Fixed both the above comments.
On Tue, Feb 17, 2026 at 10:21 PM Ashutosh Sharma <[email protected]> wrote:
>
>
> How about retaining ALTER SUBSCRIPTION ... REFRESH SEQUENCES command?
>
> It can be useful in scenarios where automatic sequence replication
> cannot be enabled, for example, due to the max_worker_processes limit
> on the server preventing a new worker from being registered. Since
> increasing max_worker_processes requires a server restart, which the
> user may not want to perform immediately, this command would provide a
> way to manually synchronize the sequences.
>
Retained ALTER SUBSCRIPTION ... REFRESH SEQUENCES similar to the
behaviour on HEAD, it only changes the state of the sequence to INIT.
The sequence worker will update these sequences unconditionally.
>
> One minor comment:
>
> * Sequence state transitions follow this pattern:
> - * INIT -> READY
> + * INIT --> READY ->-+
> + * ^ | (check/synchronzize)
> + * | |
> + * +--<---+
>
>
> "synchronzize" → "synchronize"
>
Fixed.
On Wed, Feb 18, 2026 at 9:04 PM shveta malik <[email protected]> wrote:
>
> Few comments:
>
> 1)
> + /* Check if any new sequences need syncing */
> + ProcessSequencesForSync();
> +
>
> Shall we name it 'maybe_start_seqsync_worker()' (or something better?)
> and move it immediately after 'maybe_advance_nonremovable_xid()'.
> Thoughts? This is because we do not process sequences here, we just
> check if the subscription includes sequences and start worker.
>
Since snak_case are used for static functions, I've renamed it to
MaybeLaunchSequenceSyncWorker and called it immediately after
maybe_advance_nonremovable_xid()
> 2)
> We can also change the comment atop ProcessSequencesForSync to mention
> that. Currently it says:
> /*
> * Apply worker determines if sequence synchronization is needed.
> *
> * Start a sequencesync worker if one is not already running.
>
> Now, we shall change it to:
> Apply worker determines whether a sequence sync worker is needed.
>
> Check if the subscription includes sequences and start a sequencesync
> worker if one is not already running.
>
Fixed.
> 3)
> I noticed that copy_sequences is invoked twice per sync cycle—once for
> sequences in the INIT state, and once for sequences in the READY state
> to detect drift. This means we ping the primary twice during each sync
> cycle. We should consider combining this logic into a single
> copy_sequences call. Since we already have the state information
> (INIT, READY, etc.) for each local sequence, copy_sequences can
> internally check the current state and decide whether to transition a
> sequence to READY based on its previous state. This approach would
> allow us to fetch all required information from the primary in a
> single call.
>
Changed it.
> I also think that we do not even need to mention the states here and
> we can give a single message instead of 2:
> DEBUG: logical replication sequence synchronization for subscription
> "sub1" - for sequences in INIT state batch #1 = 5 attempted, 5
> succeeded, 0 mismatched, 0 insufficient permission, 0 missing from
> publisher, 0 skipped, 0 no drift
> DEBUG: logical replication sequence synchronization for subscription
> "sub1" - for sequences in READY state batch #1 = 5 attempted, 0
> succeeded, 0 mismatched, 0 insufficient permission, 0 missing from
> publisher, 0 skipped, 5 no drift
>
Yes, since states are now specific to each sequence and not to
batches, I've removed it.
I've also found that the sequence worker was not being stopped when
SUBSCRIPTION was disabled. I've added checks to do this as well.
All the above changes are part of the attached v5 patch .
regards,
Ajin Cherian
Fujitsu Australia
v5-0001-Support-automatic-sequence-replication.patch
Description: Binary data
