On Tue, Feb 24, 2026 at 4:36 PM Amit Kapila <[email protected]> wrote: > > On Mon, Feb 23, 2026 at 6:56 AM Hayato Kuroda (Fujitsu) > <[email protected]> wrote: > > > > Thanks for updating the patch. Here are my comments. > > > > 01. start_sequence_sync() > > ``` > > /* Need to start transaction for cache lookup */ > > StartTransactionCommand(); > > ``` > > > > Here, we must check additional parameter changes, such as conninfo and > > passwordrequired. > > Also, it's inefficient because transactions are started each time. > > > > Can we re-use maybe_reread_subscription() here? Some parameters do not take > > effect for the sequence sync worker, but it is OK to exit even if they > > are changed. If we use the function, no need to include "storage/ipc.h". > > > > I also think it is better to restart the sequencesync worker on > subscription parameter change and probably it is okay to use > maybe_reread_subscription() but need to be careful to avoid anything > apply_worker specific in that function. BTW, what happens now without > this change when apply_worker stops due to parameter change, does > sequencesync worker continue? If so, we should make it restart as we > are doing for apply worker. >
I have changed it to use maybe_reread_subscription() and modified
maybe_reread_subscription() to have have some special handling
sequence sync workers as well.
Yes, the apply worker will again restart the sequence worker when it
finds that it isn't running and there are sequences in the
subscription_rel.
> Also, shouldn't we need to invoke AcceptInvalidationMessages() as we
> are doing in apply worker when not in a remote transaction? I think it
> will be required to get local_sequence definition changes , if any.
I will need to investigate this further.
>
> > 02. match_previous_words
> >
> > No need to remove "REFRESH SEQUENCES" anymore.
> >
> > 03. CopySeqResult
> > ```
> > COPYSEQ_NOWORK,
> > ```
> >
> > It describes why the copying is skipped. How about "COPYSEQ_NO_DRIFT"?
> >
>
> +1.
Fixed.
>
> > 04. LogicalRepSyncSequences()
> >
> > ```
> > oldctx = MemoryContextSwitchTo(ApplyContext);
> >
> > seq = palloc0_object(LogicalRepSequenceInfo);
> > seq->localrelid = subrel->srrelid;
> > seq->nspname = get_namespace_name(RelationGetNamespace(sequence_rel));
> > seq->seqname = pstrdup(RelationGetRelationName(sequence_rel));
> > seq->relstate = relstate;
> > seqinfos = lappend(seqinfos, seq);
> >
> > MemoryContextSwitchTo(oldctx);
> > ```
> >
> > ISTM they are palloc'd but not pfree'd.
> >
>
> They are freed, see following code. But it seems nspname and seqname
> should be freed separately for each sequence element and each sequence
> element also needs to be freed independently.
>
> + /* Clean up */
> + list_free(seqinfos);
>
> > Since the sequencesync worker now has a long lifetime, we must take care of
> > the
> > memory allocation/freeing more carefully. How about introducing
> > per-interaction
> > memory context like ApplyMessageContext?
> >
>
> Yeah, I feel that would be a better approach than retail pfree
> especially because it is also required at other places in sequencesync
> worker (see places where currently ApplyContext is used in
> sequencesync worker). I think it would be better if we name this new
> context as SequenceSyncContext.
>
Done. Changed as requested.
> Few other miscellaneous comments
> =================================
> 1.
> <sect2 id="sequences-out-of-sync">
> - <title>Refreshing Out-of-Sync Sequences</title>
> - <para>
> - Subscriber sequence values will become out of sync as the publisher
> - advances them.
> - </para>
> - <para>
> - To detect this, compare the
> - <link
> linkend="catalog-pg-subscription-rel">pg_subscription_rel</link>.<structfield>srsublsn</structfield>
> - on the subscriber with the <structfield>page_lsn</structfield> obtained
> - from the <link
> linkend="func-pg-get-sequence-data"><function>pg_get_sequence_data</function></link>
> - function for the sequence on the publisher. Then run
> - <link linkend="sql-altersubscription-params-refresh-sequences">
> - <command>ALTER SUBSCRIPTION ... REFRESH SEQUENCES</command></link> to
> - re-synchronize if necessary.
> - </para>
> + <title>Out-of-Sync Sequences</title>
> <warning>
> <para>
> Each sequence caches a block of values (typically 32) in memory before
> @@ -1961,16 +1941,6 @@ Publications:
> ------------+-----------+------------
> 610 | t | 0/017CEDF8
> (1 row)
> -</programlisting></para>
> -
> - <para>
> - The difference between the sequence page LSNs on the publisher and the
> - sequence page LSNs on the subscriber indicates that the sequences are out
> - of sync. Re-synchronize all sequences known to the subscriber using
> - <link linkend="sql-altersubscription-params-refresh-sequences">
> - <command>ALTER SUBSCRIPTION ... REFRESH SEQUENCES</command></link>.
> -<programlisting>
> -/* sub # */ ALTER SUBSCRIPTION sub1 REFRESH SEQUENCES;
>
> ...
> ...
> - <listitem>
> - <para>
> - Incremental sequence changes are not replicated. Although the data in
> - serial or identity columns backed by sequences will be replicated as
> part
> - of the table, the sequences themselves do not replicate ongoing changes.
> - On the subscriber, a sequence will retain the last value it synchronized
> - from the publisher. If the subscriber is used as a read-only database,
> - then this should typically not be a problem. If, however, some kind of
> - switchover or failover to the subscriber database is intended, then the
> - sequences would need to be updated to the latest values, either by
> - executing <link
> linkend="sql-altersubscription-params-refresh-sequences">
> - <command>ALTER SUBSCRIPTION ... REFRESH SEQUENCES</command></link>
> - or by copying the current data from the publisher (perhaps using
> - <command>pg_dump</command>) or by determining a sufficiently high value
> - from the tables themselves.
> - </para>
> - </listitem>
> -
>
> I think this can still happen after this patch but chances are much
> lower, so we need some of this before upgrade. We should reword it
> accordingly. Similarly check other parts of the doc you removed.
>
I have re-added them and modified them.
> 2.
> - "logical replication synchronization for subscription \"%s\",
> sequence \"%s.%s\" has finished",
> + "logical replication sync for subscription \"%s\", sequence
> \"%s.%s\" has been updated",
>
> Is there a need to shorten synchronization to sync in above message?
>
Removed.
> 3.
> elog(DEBUG1,
> - "logical replication sequence synchronization for subscription
> \"%s\" - batch #%d = %d attempted, %d succeeded, %d mismatched, %d
> insufficient permission, %d missing from publisher, %d skipped",
> - MySubscription->name,
> - (cur_batch_base_index / MAX_SEQUENCES_SYNC_PER_BATCH) + 1,
> - batch_size, batch_succeeded_count, batch_mismatched_count,
> - batch_insuffperm_count, batch_missing_count, batch_skipped_count);
> + "logical replication sequence synchronization for subscription
> \"%s\" - batch #%d = %d attempted, %d succeeded, %d mismatched, %d
> insufficient permission, %d missing from publisher, %d skipped, %d no
> drift",
> + MySubscription->name,
> + (cur_batch_base_index / MAX_SEQUENCES_SYNC_PER_BATCH) + 1,
> + batch_size, batch_succeeded_count, batch_mismatched_count,
> + batch_insuffperm_count, batch_missing_count, batch_skipped_count,
> batch_no_drift);
>
> Here, the formatting of message is incorrect.
>
Fixed.
> 4.
> table_states_not_ready = NIL;
> -
> if (!IsTransactionState())
>
> Spurious line removal.
>
> 5.
> MemoryContextSwitchTo(oldctx);
> -
> + list_free_deep(seq_states);
> /*
> * Does the subscription have tables?
> *
> @@ -260,7 +253,6 @@ FetchRelationStates(bool *has_pending_subtables,
> */
> has_subtables = (table_states_not_ready != NIL) ||
> HasSubscriptionTables(MySubscription->oid);
> -
> /*
> * If the subscription relation cache has been invalidated since we
> * entered this routine, we still use and return the relations we just
> @@ -271,10 +263,8 @@ FetchRelationStates(bool *has_pending_subtables,
> if (relation_states_validity == SYNC_RELATIONS_STATE_REBUILD_STARTED)
> relation_states_validity = SYNC_RELATIONS_STATE_VALID;
> }
> -
> if (has_pending_subtables)
> *has_pending_subtables = has_subtables;
> -
> if (has_pending_subsequences)
> ...
> ...
>
> }
>
> +
> if (rc & WL_TIMEOUT)
>
> All above places have spurious line removals and additions which made
> code harder to understand.
>
Fixed.
> 6.
> else if (Matches("ALTER", "SUBSCRIPTION", MatchAny))
> COMPLETE_WITH("CONNECTION", "ENABLE", "DISABLE", "OWNER TO",
> - "RENAME TO", "REFRESH PUBLICATION", "REFRESH SEQUENCES",
> - "SET", "SKIP (", "ADD PUBLICATION", "DROP PUBLICATION");
> + "RENAME TO", "REFRESH PUBLICATION", "SET", "SKIP (",
> + "ADD PUBLICATION", "DROP PUBLICATION");
>
> unrelated change.
>
Removed.
> 7.
> + else
> + {
> +
> + /*
> + * Double the sleep time, but not beyond
> + * the maximum allowable value.
> + */
> + sleep_ms = Min(sleep_ms * 2, SEQSYNC_MAX_SLEEP_MS);
>
> Comments are not properly aligned.
>
Fixed.
On Tue, Feb 24, 2026 at 5:17 PM Amit Kapila <[email protected]> wrote:
>
> On Mon, Feb 23, 2026 at 6:56 AM Hayato Kuroda (Fujitsu)
> <[email protected]> wrote:
> >
> > 05. LogicalRepApplyLoop()
> >
> > MaybeLaunchSequenceSyncWorker() should be called more; otherwise, the
> > sequencesync
> > worker won't be launched if the worker always receives messages and
> > WL_TIMEOUT does
> > not happen. Can you add most of the places under
> > maybe_advance_nonremovable_xid()?
> > Personally considered, no need to add within `else if (c ==
> > PqReplMsg_PrimaryStatusUpdate)`
> > because it just consumes status updates from the primary.
> >
>
> I don't think we need to be as aggressive as
> maybe_advance_nonremovable_xid because not doing that can lead to
> bload if slot is not advanced. The only minor downside with checking
> too frequently is that we need to traverse the all logical replication
> workers to find if sequencesync worker is available. I feel doing in
> ProcessSyncingRelations() where earlier we were doing
> ProcessSequencesForSync() should be sufficient.
Added it back as previous.
Can we find some cheap
> way to detect if sequencesync worker is present or not? Can you think
> some other way to not incur the cost of traversing the worker array
> and also detect sequence worker exit without much delay?
>
I will need to investigate this further.
> ...
> >
> > 07.
> > Question: Can we introduce an intermediate state, such as SYNC, to clarify
> > whether synchronization is proceeding?
> >
>
> What is the advantage of this? For external purposes, the presence of
> sequencesync worker, which can be checked via pg_stat_subscription
> should be sufficient.
>
> BTW, what is the behavior of REFRESH SEQUENCES command if the sequence
> worker is active? Does it still try to refresh sequences, if so, is
> that required/good idea?
>
Currently REFRESH SEQUENCES can only be called if the subscription is
enabled. All it does is change the states of all the sequences in
subscription_rel to INIT, this will prompt the sequence worker to wake
up and unconditionally sync all the sequences. For sequences in the
INIT state, it doesn't check if there is drift or not, it updates all
sequences unconditionally. From your discussions with Dilip, I
understand we want to reduce the time it takes to REFRESH SEQUENCES at
the time of an upgrade. If so, then this might not be a good approach.
As not only does the sequence worker have to update all sequences even
if they have not drifted, it also has to update the relstate of all
these sequences to READY. I propose, we change the logic such that
REFRESH SEQUENCES only wakes up the sequence worker, that will reduce
the time as most sequences will already be in READY state (unless
newly added) and only sequences that have not drifted need to be
updated and no catalog update is required for sequence states already
in READY state. One downside to this is that, earlier users could
issue a REFRESH SEQUENCES and wait to see if all the sequences have
returned to the READY state to confirm that the sync has completed,
with this approach that advantage is not there. Users might have to
use a query to find the sequence values of all the sequences and
compare that with the values in the publisher.
I have addressed the above comments in the attached patch v6.
regards,
Ajin Cherian
Fujitsu Australia
v6-0001-Support-automatic-sequence-replication.patch
Description: Binary data
