Subject: Re: [PATCH] Support automatic sequence replication

From: shveta malik <[email-masked]>
Date: Fri, 6 Feb 2026 14:45:32 +0530

On Thu, Feb 5, 2026 at 10:33 AM Ajin Cherian <[email-masked]> wrote:
>
> On Tue, Feb 3, 2026 at 9:22 PM shveta malik <[email-masked]> wrote:
> >
> > On Tue, Feb 3, 2026 at 9:18 AM Ajin Cherian <[email-masked]> wrote:
> > Thanks for the patch.
> >
> > +1 for the overall idea of patch that once a subscription is created
> > which subscribes to sequences, a sequence sync worker is started which
> > continuously syncs the sequences. This makes usage of REFRESH
> > SEQUENCES redundant and thus it is removed. I am still reviewing the
> > design choice here, and will post my comments soon (if any).
> >
>
> Thanks!
>
> > By quick validation, few issues in current implementation:
> >
> > 1)
> > If the sequence sync worker exits due to some issue (or killed or
> > server restarts), sequence-sync worker is not started again by apply
> > worker unless there is a sequence in INIT state i.e. synchronization
> > of sequences in READY state stops. IIUC, the logic of
> > ProcessSequencesForSync() needs to change to start seq sync worker
> > irrespective of state of sequences.
> >
>
> Yes, I fixed this. I've changed FetchRelationStates to fetch sequences
> in ANY state and not just ones in NON READY state.
>
> > 2)
> > There is some issue in how LOGs (DEBUGs) are getting generated.
> >
> > a) Even if there is no drift, it still keeps on dumping:
> > "logical replication sequence synchronization for subscription "sub1"
> > - total unsynchronized: 3"
> >
>
> Removed this.
>
> > b)
> > When there is a drift in say single sequence, it puts rest (which are
> > in sync) to "missing" section:
> > "logical replication sequence synchronization for subscription "sub1"
> > - batch #1 = 3 attempted, 1 succeeded, 0 mismatched, 0 insufficient
> > permission, 2 missing from publisher, 0 skipped"
> >
>
> Fixed, and added a new section called "no drift". Also now this debug
> message is printed every time the worker attempts to synchronize
> sequences. also mentioning the state of the sequences being synced.
>
> > 3)
> > If a sequence sync worker is taking a nap, and subscription is
> > disabled or the server is stopped just before upgrade, how is the user
> > supposed to know that sequences are synced at the end?
>
> Well, one way is to wait for a debug message that says that all the
> attempted sequences are in the "no drift" state. Also remote
> sequence's LSN is updated in pg_subscription_rel for each sequence.
> Let me know if you have anything more in mind. One option is to leave
> the ALTER SUBSCRIPTION..REFRESH SEQUENCE in place, that will change
> the state of all the sequences to the INIT state, and the user can
> then wait for the sequences to change state to READY.

I think this point needs more analysis. I am analyzing and discussing
it further.

> Attaching patch v3 addressing the above comments.
>

Thank You for the patch. I haven’t reviewed the details yet, but it
would be good to evaluate whether we really need to start the sequence
sync worker so deep inside the apply worker. Currently, the caller of
ProcessSequencesForSync(), namely ProcessSyncingRelations() is invoked
for every apply message to process possible state-changes of relation
and start worker (tablesync/sequence-sync etc). Since monitoring
state-change behavior is no longer required for sequences, should we
consider moving this logic to the main loop of the apply worker,
possibly within LogicalRepApplyLoop()? There, we could check whether
the table has any sequences and, if a worker is not already running,
start one. Thoughts?

thanks
Shveta

---

From: shveta malik <[email-masked]>
Date: Fri, 6 Feb 2026 14:47:26 +0530

> the table has any sequences and, if a worker is not already running,

Correction to last line:
There, we could check whether *the current susbcription* has any
sequences and, if a worker is not already running,start one.

thanks
Shveta

---

From: shveta malik <[email-masked]>
Date: Wed, 11 Feb 2026 09:00:02 +0530

> sequences and, if a worker is not already running,start one.

Few more comments:

1)
+ /* Run sync for sequences in READY state */
+ sequence_copied |= LogicalRepSyncSequences(LogRepWorkerWalRcvConn,
SUBREL_STATE_READY);
+
+ /* Call initial sync for sequences in INIT state */
+ sequence_copied |= LogicalRepSyncSequences(LogRepWorkerWalRcvConn,
SUBREL_STATE_INIT);

Above logic means we ping primary twice for one seq-sync cycle? Is it
possible to do it in below way:

Get all sequences from the publisher in one go.
If local-seq's state is INIT, copy those sequences, move the state to READY.
If local-seq's state is READY, check the drift and proceed accordingly.

i.e, there should be a single call to LogicalRepSyncSequences() and
relstate shouldn't even be an argument.

2)
GetSequence:
+ /* Open and lock the sequence relation */
+ seqrel = table_open(relid, AccessShareLock);

GetSequence is called from check_sequence_drift and
check_sequence_drift() from copy_sequences(). In copy_sequences(), we
already open the seq's (localrelid) table in
get_and_validate_seq_info() and give it as output (see sequence_rel).
Same can be passed to this instead of trying opening the table again.

3)
+ /* Verify it's actually a sequence */
+ if (seqrel->rd_rel->relkind != RELKIND_SEQUENCE)
+ ereport(ERROR,
+ (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+ errmsg("\"%s\" is not a sequence",
+ RelationGetRelationName(seqrel))));

Do we need this check? IIUC, sequence_rel is opened using
RowExclusiveLock in get_and_validate_seq_info() and thus should not
hit this case so that it becomes non-sequence later on. If required,
we can have Assert.

I can review further once these and previous comments are addressed.

thanks
Shveta

---

From: Ajin Cherian <[email-masked]>
Date: Thu, 12 Feb 2026 20:23:59 +1100

> the table has any sequences and, if a worker is not already running,

I've moved this to LogicalRepApplyLoop()

On Mon, Feb 9, 2026 at 10:55 AM Peter Smith <[email-masked]> wrote:
>
> Some review comments for v3-0001.
>
> ======
> .../replication/logical/sequencesync.c
>
> copy_sequences:
>
> 1.
> - if (sync_status == COPYSEQ_SUCCESS)
> +
> + /*
> + * For sequences in READY state, only sync if there's drift
> + */
> + if (relstate == SUBREL_STATE_READY && sync_status == COPYSEQ_SUCCESS)
> + {
> + should_sync = check_sequence_drift(seqinfo);
> + if (should_sync)
> + drift_detected = true;
> + }
> +
> + if (sync_status == COPYSEQ_SUCCESS && should_sync)
>   sync_status = copy_sequence(seqinfo,
> - sequence_rel->rd_rel->relowner);
> + sequence_rel->rd_rel->relowner,
> + relstate);
> + else if (sync_status == COPYSEQ_SUCCESS && !should_sync)
> + sync_status = COPYSEQ_NOWORK;
>
> I'm struggling somewhat to follow this logic.
>
> For example, every condition refers to COPYSEQ_SUCCESS -- is that
> really necessary; can't this all be boiled down to something like
> below?
>
> SUGGESTION
>
> /*
>  * For sequences in INIT state, always sync.
>  * Otherwise, for sequences in READY state, only sync if there's drift.
>  */
> if (sync_status == COPYSEQ_SUCCESS)
> {
>   if ((relstate == SUBREL_STATE_INIT) || check_sequence_drift(seqinfo))
>     sync_status = copy_sequence(...);
>   else
>     sync_status = COPYSEQ_NOWORK;
> }

Changed as suggested.

> relstate shouldn't even be an argument.

Changed as suggested.

> + RelationGetRelationName(seqrel))));

Changed these.

Other than these, I've changed seqinfos from being a global static
list to a list that gets passed around and destroyed in each
iteration.
I haven't addressed the upgrade issue raised by Vignesh and Shveta in
this patch. I will address that in the next patch.
 Here's patch v4 addressing the above comments.

regards,
Ajin Cherian
Fujitsu Australia

---

From: shveta malik <[email-masked]>
Date: Thu, 12 Feb 2026 15:32:56 +0530

> this patch. I will address that in the next patch.

Thanks Ajin, I will review. I will suggest waiting for others'
feedback before doing anything about the upgrade issue. Meanwhile, we
can try to improve the current patch itself.

thanks
Shveta

---

From: Peter Smith <[email-masked]>
Date: Fri, 13 Feb 2026 16:42:35 +1100

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.

======

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.

---

From: Ashutosh Sharma <[email-masked]>
Date: Tue, 17 Feb 2026 16:51:32 +0530

Hi,

>  Here's patch v4 addressing the above comments.

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.

Thoughts?

--

One minor comment:

 * Sequence state transitions follow this pattern:
- *   INIT -> READY
+ *  INIT --> READY ->-+
+ *             ^      | (check/synchronzize)
+ *             |      |
+ *             +--<---+

"synchronzize" → "synchronize"

--
With Regards,
Ashutosh Sharma.

---

From: Amit Kapila <[email-masked]>
Date: Wed, 18 Feb 2026 10:18:49 +0530

> How about retaining ALTER SUBSCRIPTION ... REFRESH SEQUENCES command?

Yes, Shveta and myself also advocated the same for the upgrade case.

> way to manually synchronize the sequences.

Good point. I think we won't be able to launch sync workers even when
we reach max_logical_replication_workers limit. So, this is one more
reason to retain the REFRESH SEQUENCES command.

-- 
With Regards,
Amit Kapila.

---

From: shveta malik <[email-masked]>
Date: Wed, 18 Feb 2026 12:36:31 +0530

I tested a few scenarios on the latest patch. Sequence sync worker did
not stop in below scenarios:

1) When the subscription was disabled.
2) When the only publication for sequences was dropped from
subscription ( ALTER SUBSCRIPTION sub1 DROP PUBLICATION pub_seq;)
3) When all sequences were dropped on sub.

Application worker stops in scenario 1, seq-sync worker should also
stop. See maybe_reread_subscription().

We need to decide the behavior of the seq-sync worker for 2 and 3.

IMO, for scenario 2, we should stop the sequence sync worker. It is of
no use to keep holding one worker slot when there are no publisher
publishing sequences for that subscription.

For scenario 3, it might be acceptable(or may be even expected?) to
keep the seq-sync worker running. But the challenge is how we would
distinguish scenario 2 from scenario 3. I believe scenario 2 will rely
on the absence of sequence entries in pg_subscription_rel to sotp
seq-sync worker. But in both scenario 2 and scenario 3, there would be
no sequence entries in pg_subscription_rel. Given that, to keep the
logic simpler, we can stop the seq-sync worker in scenario 3 as well.
This seems like a corner case, and it should not cause much harm to
stop the worker and restart it later when needed.

Thoughts?

thanks
Shveta

---

From: Amit Kapila <[email-masked]>
Date: Wed, 18 Feb 2026 13:11:48 +0530

> We need to decide the behavior of the seq-sync worker for 2 and 3.

Shouldn't we try to map (2) and (3) to what we do for table publication?

-- 
With Regards,
Amit Kapila.

---

From: shveta malik <[email-masked]>
Date: Wed, 18 Feb 2026 15:34:46 +0530

Few comments:

1)

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.

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.

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.

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

4)
FetchRelationStates():
+ List    *seq_states;

It would be better to initialize it with NIL.

thanks
Shveta

---

From: shveta malik <[email-masked]>
Date: Wed, 18 Feb 2026 16:58:20 +0530

> Shouldn't we try to map (2) and (3) to what we do for table publication?

I thought about it, this is what I have in mind:

1) When there is no sequences and tables to be synced, we will be
blocking 2 workers slot, one for apply worker and one for seq-sync
worker. While only apply-worker is enough, as it may restart seq-sync
worker as soon as it notices a sequence.

2) In case of apply-worker, when no tables are being published, it
hardly does any work. IIUC, it just sends responses to keep-alive
messages. OTOH, seq-sync worker will begin a transaction and query
pg_subscription_rel every few seconds. I feel, we should avoid this
unnecessary activity if possible.

Overall, I feel the sequence sync worker is logically different from
the table apply worker. It behaves more like an auxiliary worker
managed by the apply worker and derives all of its state from the
system catalogs. In contrast, the apply worker actively consumes the
logical replication stream and is responsible for advancing the slot
and origin. If the apply worker is stopped for an extended period, the
slot cannot advance, which may lead to WAL accumulation on the
publisher. There is no such constraint associated with the seq-sync
worker. And thus stopping the seq-sync worker aggressively (in
scenarios 2 and 3) is both safer and simpler as compared to apply
worker and will also avoid some unnecessary transactions and activity
as I stated above. Thoughts?

thanks
Shveta

---

From: Peter Smith <[email-masked]>
Date: Thu, 19 Feb 2026 08:56:29 +1100

On Wed, Feb 18, 2026 at 9:05 PM shveta malik <[email-masked]> wrote:
...
> publisher, 0 skipped, 5 no drift

Those are DEBUG1 messages, not LOG messages, so I think the primary
goal is to ensure that they are full of useful information to help
debugging.  Knowing the prior state information means we know that the
"drift" logic was needed when deciding to sync or not.

If message volume can be reduced without any loss of debugging
usefulness, then great, but we need to take care not to throw the baby
out with the bath water.

If message volume is a problem, then maybe that should be addressed by
using more log levels DEBUG1,DEBUG2,DEBUG3,... etc.

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

---

From: Amit Kapila <[email-masked]>
Date: Thu, 19 Feb 2026 07:45:36 +0530

On Wed, Feb 18, 2026 at 4:58 PM shveta malik <[email-masked]> wrote:
> system catalogs.

I understand that sequence-sync worker won't be doing anything useful
in such corner cases but the chances of such situations are rare and
the consequences are not that bad. Similarly, one can say that we can
exit the launcher process when no subscription is present and the
system should figure out and restart when required. In this case also,
the apply-worker needs to check from time-to-time or needs to be
informed to launch the new sequence-sync worker. I think we can
evaluate these cases separately and can decide to write a top-up patch
if found useful to make sequence-sync worker detect and exit.

-- 
With Regards,
Amit Kapila.

---

From: Amit Kapila <[email-masked]>
Date: Thu, 19 Feb 2026 07:52:26 +0530

> usefulness, then great, but we need to take care not to throw the baby

I think it is useful to discuss how much DEBUG information is
required. However, I would like to know if this is related to the
patch being discussed or a case in HEAD? If later, then it is better
to discuss it separately and address it as a separate patch if
required.

-- 
With Regards,
Amit Kapila.

---

From: shveta malik <[email-masked]>
Date: Thu, 19 Feb 2026 09:35:03 +0530

> using more log levels DEBUG1,DEBUG2,DEBUG3,... etc.

I am not totally against it, but I don’t find it particularly useful
to dump the state (INIT, READY), since we already log exactly which
sequences were updated individually (see below). The output below
covers both cases—sequences with drift and those in the INIT state. We
also include the “no drift” count. So all of these effectively
summarizes the overall status.

logical replication sync for subscription "sub2", sequence
"public.myseq1" has been updated
logical replication sync for subscription "sub2", sequence
"public.myseq2" has been updated

 Even if we plan to retain multiple messages for INIT vs READY, I
guess, with copy_sequences() now suggested to be called only once
irrespective of 'state' will make it further tricky to separate out
these log messages for INIT and READY. But let's see.

> to discuss it separately and address it as a separate patch if

Amit, this is related to the new patch only.

thanks
Shveta

---

From: shveta malik <[email-masked]>
Date: Thu, 19 Feb 2026 09:49:34 +0530

On Thu, Feb 19, 2026 at 7:45 AM Amit Kapila <[email-masked]> wrote:
> system should figure out and restart when required.

No, I don’t think this is comparable to our case. For that scenario to
work, we would need to expose subscription-related awareness and state
to the postmaster, which is not recommended. In our case, the apply
worker is already responsible for monitoring the sequence-sync worker.
It is already checking two things:
--whether the subscription includes sequences, and
--whether the sequence-sync worker is running and if not, it starts it.

So there is no additional logic required. None. We don’t need any
extra awareness in the apply worker. Given that, I don’t think the two
cases are comparable at all.

But I also agree that it is a corner case, and running additional
seq-sync worker per subscription may not harm that much.

> In this case also,
> informed to launch the new sequence-sync worker.

It is already doing that. There is always a case where seq-sync worker
errors out or exits unexpectedly. In such a case, the apply worker is
the one to start it again. And for that, apply worker has to keep
checking it.

> I think we can

Sure, we can do that.

thanks
Shveta

---

From: Ajin Cherian <[email-masked]>
Date: Fri, 20 Feb 2026 21:57:16 +1100

> anymore, contrary to what that comment says.

Fixed.

> committed independently of the new seq-sync logic.

Keeping it as is for the time being. Let's see what everybody else thinks.

> How is this possible? Should it just be Assert?

Fixed.

> Why is talking about the processing order relevant?

Removed.

> part, should you also remove the earlier INSERT for 'regress_s1'?

Fixed both the above comments.

> 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.

> + *             ^      | (check/synchronzize)

Fixed.

> 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()

> worker if one is not already running.

Fixed.

> allow us to fetch all required information from the primary in a

Changed it.

> 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

---

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Mon, 23 Feb 2026 01:26:10 +0000

Dear Ajin,

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".

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"?

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.

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?

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.

06.
Not sure if the issue should be discussed here, but I found that sequences more
likely to go backward if users use sequences on the subscriber side.
Previously, the sync could happen based on the request, and users could understand
the risk. But now everything would be done automatically, thus they may be
surprised more.

Should we consider some ratchet mechanisms, or retain it now because it's not
expected usage?

E.g., `nextval()` is called three times, and synchronization occurs between them.

```
subscriber=# SELECT nextval('seq');
 nextval
---------
       2
(1 row)

subscriber=# SELECT nextval('seq');
 nextval
---------
       3
(1 row)

subscriber=# -- synchronization happened
subscriber=# SELECT nextval('seq');
 nextval
---------
       1
(1 row)
```

07.
Question: Can we introduce an intermediate state, such as SYNC, to clarify
whether synchronization is proceeding?

Best regards,
Hayato Kuroda
FUJITSU LIMITED

---

From: Amit Kapila <[email-masked]>
Date: Mon, 23 Feb 2026 11:14:19 +0530

> Should we consider some ratchet mechanisms, or retain it now because it's not

We discussed this case upthread. We ideally can handle it via
conflict/resolution strategy or simply avoid updating the sequences
that are synced from the publisher. If we do later it would be tricky
because we need to maintain a persistent state and then after
failover, that state should be cleared. We discussed to have it
documented that users should use such sequences only during upgrade or
for failover cases, allowing to update it actively on multiple nodes
can lead to inconsistency.

-- 
With Regards,
Amit Kapila.

---

From: Amit Kapila <[email-masked]>
Date: Tue, 24 Feb 2026 11:06:15 +0530

> 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.

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.

> It describes why the copying is skipped. How about "COPYSEQ_NO_DRIFT"?

+1.

> 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);

> 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.

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.

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?

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.

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.

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.

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.

-- 
With Regards,
Amit Kapila.

---

From: Amit Kapila <[email-masked]>
Date: Tue, 24 Feb 2026 11:47:24 +0530

> 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. 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?

...
> 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?

-- 
With Regards,
Amit Kapila.

---

From: Ajin Cherian <[email-masked]>
Date: Tue, 24 Feb 2026 22:04:26 +1100

> 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.

> will be required to get local_sequence definition changes , if any.

I will need to investigate this further.

Fixed.

> context as SequenceSyncContext.

Done. Changed as requested.

> Few other miscellaneous comments

I have re-added them and modified them.

Removed.

Fixed.

Fixed.

Removed.

Fixed.

On Tue, Feb 24, 2026 at 5:17 PM Amit Kapila <[email-masked]> wrote:
> ProcessSequencesForSync() should be sufficient.

Added it back as previous.

 Can we find some cheap
> and also detect sequence worker exit without much delay?

I will need to investigate this further.

> worker is active? Does it still try to refresh sequences, if so, is

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

---

From: Amit Kapila <[email-masked]>
Date: Tue, 24 Feb 2026 16:55:20 +0530

> the time of an upgrade. If so, then this might not be a good approach.

The point I was trying to make is that with automatic sequence sync,
we won't need to execute REFRESH SEQUENCES before upgrade or failover
as the sequences will be in sync.

> compare that with the values in the publisher.

Yeah, this point needs more thoughts as the existing method for
REFRESH SEQUENCES also allows users to easily verify if the sequences
are in sync.

-- 
With Regards,
Amit Kapila.

---

From: Dilip Kumar <[email-masked]>
Date: Tue, 24 Feb 2026 18:48:43 +0530

> as the sequences will be in sync.

That is a valid goal, however, the sequence sync worker is an
asynchronous process triggered at specific intervals. For a switchover
to guarantee consistency, we must disconnect all connections from the
current publisher and wait for all pending data to sync. Relying on an
automated, interval-based worker at this critical time is risky, as it
directly extends our downtime. To minimize the cutover window, we
should either treat sequence data the same as relational data
(synchronous streaming) or manually trigger for the sequence sync
during the switchover phase, i.e. REFRESH SEQUENCES.  Anyway this is
my understanding, do you have anything else in mind that how you are
trying to completely avoid REFRESH SEQUENCES.  Having said that I
agree that in many cases where sequences are not frequently modified
it is very much possible that when we are in switchover phase all
sequences are already in sync and if we can identify that then we can
avoid REFRESH SEQUENCES, but my point is we can not completely avoid
that in all caes.  And the cases where we are frequently inserting
into relation which has a sequence type field the sequences will be
freqnectly modified and we have to call REFRESH SEQUENCES.

-- 
Regards,
Dilip Kumar
Google

---

From: Amit Kapila <[email-masked]>
Date: Wed, 25 Feb 2026 08:32:14 +0530

On Tue, Feb 24, 2026 at 6:49 PM Dilip Kumar <[email-masked]> wrote:
> that in all caes.

Right, I am also of the point that we can't completely avoid REFRESH
SEQUENCES in all cases. Having sequence sync worker syncing
periodically reduces the chances that one need to perform REFRESH
SEQUENCES, so we need both. Do we agree with that?

Additionally, I am thinking that after sync-worker we change REFRESH
SEQUENCES functionality such that it will refresh all sequences during
command. And, even we fail to copy one sequence, the command should
fail. This should use almost the same functionality (like we sync only
the required ones) as syncworker but online. This is because the case
where it would be used will be less likely, say when due to some
reason like max_logical_workers limit, we are not able to invoke
sequencesync worker or just before upgrade (when sequences are not
already synced), so doing sync during refresh command sounds
reasonable. What do you think?

-- 
With Regards,
Amit Kapila.

---

From: shveta malik <[email-masked]>
Date: Wed, 25 Feb 2026 09:27:06 +0530

> reasonable. What do you think?

I agree with the overall direction of the discussion. But this is how
I look at it:

Given that sequences can be large, it’s not always practical and may
place an unnecessary burden on users to manually verify whether all
sequences are in sync before an upgrade. Requiring users to inspect
sequence values and manually determine drift before deciding whether
to run REFRESH introduces unnecessary operational complexity. So
instead of trying to find ways to avoid REFRESH, we can position it as
the final safety step before upgrade, optimized to refresh only
drifted sequences and to fail if synchronization of any required
sequence does not succeed. This approach gives us several advantages:

--Users don’t need to manually check for drift.
--REFRESH becomes a safe, final synchronization step before upgrade.
--In most real-world scenarios, sequences are likely already in sync
due to the periodic sync worker. When everything is already
synchronized, REFRESH effectively becomes a lightweight confirmation
check.
--If some sequences are out of sync (due to high activity, worker
limits such as max_logical_replication_workers, or other edge cases),
REFRESH will correct only those.

This creates a practical win-win situation: fast execution when
sequences are already in sync, guaranteed correctness when they are
not, and no additional manual validation burden on the user. Thoughts?

thanks
Shveta

---

From: Ajin Cherian <[email-masked]>
Date: Thu, 26 Feb 2026 18:37:08 +1100

> and also detect sequence worker exit without much delay?

Added this.

> will be required to get local_sequence definition changes , if any.

Changed.

Thanks Hou-san for helping me with these changes.
I also did some performance testing on HEAD to see how long REFRESH
SEQUENCES takes for a large number of sequences.
I ran these on a 2× Intel Xeon E5-2699 v4 (22 cores each, 44 cores
total / 88 threads) 512 GB RAM. I didn't see much value in
differentiating between cases where half the sequences were different
or all the sequences were different as REFRESH SEQUENCES updates all
sequences after changing the state of all of them to INIT, it doesn't
matter if they drifted or not.

On HEAD:
time to sync 10000 sequences: 1.080s (1080ms)
time to sync 100000 sequences: 12.069s (12069ms)
time to sync 1000000 sequences: 139.414s (139414ms)

testing script attached (pass in the number of sequences as a run time
parameter).

regards,
Ajin Cherian
Fujitsu Australia

---

From: Nisha Moond <[email-masked]>
Date: Thu, 26 Feb 2026 19:43:12 +0530

> testing script attached (pass in the number of sequences as a run time

Hi Ajin,
Thanks for sharing the performance results. I ran the same tests using
your scripts on a different machine with the configuration:
 - Chip: Apple M4 Pro, 14 CPU cores
 - RAM: 24 GB
 - Postgres installation on pg_Head - commit 77c7a17a6e5

For these tests, I used shared_buffers = 4GB. The time taken for 1M
sequences is increased significantly:
  time to sync 10000 sequences: .994s (994ms)
  time to sync 100000 sequences: 11.032s (11032ms)
  time to sync 1000000 sequences: 426.850s (426850ms)

I also tested with shared_buffers = 8GB, and the time for 1M sequences
was 441.794s (441794 ms)

--
Thanks,
Nisha

---

From: Amit Kapila <[email-masked]>
Date: Fri, 27 Feb 2026 11:31:14 +0530

> I also tested with shared_buffers = 8GB, and the time for 1M sequences

IIUC, the tests by Ajin and Nisha show the time to sync sequences
varies from a few seconds to minutes depending on the number of
sequences and machine configuration. This indicates that having an
autosync worker can avoid downtime before upgrade, otherwise, users
always need to use REFRESH SEQUENCES before upgrade.

-- 
With Regards,
Amit Kapila.

---

From: Amit Kapila <[email-masked]>
Date: Fri, 27 Feb 2026 16:37:28 +0530

On Thu, Feb 26, 2026 at 1:07 PM Ajin Cherian <[email-masked]> wrote:

Few comments:
=============
1.
+ oldctx = MemoryContextSwitchTo(SequenceSyncContext);

- initStringInfo(&app_name);
- appendStringInfo(&app_name, "pg_%u_sequence_sync_" UINT64_FORMAT,
- MySubscription->oid, GetSystemIdentifier());
+ /* Process sequences */
+ sequence_copied = copy_sequences(conn, seqinfos);

- /*
- * Establish the connection to the publisher for sequence synchronization.
- */
- LogRepWorkerWalRcvConn =
- walrcv_connect(MySubscription->conninfo, true, true,
-    must_use_password,
-    app_name.data, &err);
- if (LogRepWorkerWalRcvConn == NULL)
- ereport(ERROR,
- errcode(ERRCODE_CONNECTION_FAILURE),
- errmsg("sequencesync worker for subscription \"%s\" could not
connect to the publisher: %s",
-    MySubscription->name, err));
-
- pfree(app_name.data);
-
- copy_sequences(LogRepWorkerWalRcvConn);
+ MemoryContextSwitchTo(oldctx);

It is better to switch to SequenceSyncContext at the caller of
LogicalRepSyncSequences similar to what we are doing for
ApplyMessageContext.

2.
@@ -4221,6 +4221,7 @@ LogicalRepApplyLoop(XLogRecPtr last_received)
  ProcessConfigFile(PGC_SIGHUP);
  }

+
  if (rc & WL_TIMEOUT)

Spurious line addition.

3. Apart from above, the attached patch contains comments and cosmetic changes.

-- 
With Regards,
Amit Kapila.
diff --git a/src/backend/replication/logical/sequencesync.c b/src/backend/replication/logical/sequencesync.c
index ffbbd1257d0..3898b315bb8 100644
--- a/src/backend/replication/logical/sequencesync.c
+++ b/src/backend/replication/logical/sequencesync.c
@@ -111,8 +111,8 @@ static MemoryContext SequenceSyncContext = NULL;
  * unsynchronized after it exits, a new worker can be started in the next
  * iteration.
  *
- * The pointer to the sequencesync worker is cached to avoid acquiring an
- * LWLock and scanning the workers array each time via logicalrep_worker_find().
+ * The pointer to the sequencesync worker is cached to avoid scanning the
+ * workers array each time via logicalrep_worker_find().
  */
 void
 MaybeLaunchSequenceSyncWorker(void)
@@ -137,7 +137,7 @@ MaybeLaunchSequenceSyncWorker(void)
 	LWLockAcquire(LogicalRepWorkerLock, LW_SHARED);
 
 	/*
-	 * Return if the sequence sync worker for the current subscription is
+	 * Quick exit if the sequence sync worker for the current subscription is
 	 * already alive.
 	 */
 	if (sequencesync_worker &&
@@ -586,9 +586,8 @@ copy_sequences(WalReceiverConn *conn, List *seqinfos)
 			{
 				case COPYSEQ_SUCCESS:
 					elog(DEBUG1,
-						 "logical replication synchronizatio for subscription \"%s\", sequence \"%s.%s\" has been updated",
-						 MySubscription->name, seqinfo->nspname,
-						 seqinfo->seqname);
+						 "logical replication synchronization has updated sequence \"%s.%s\" in subscription \"%s\"",
+						 seqinfo->nspname, seqinfo->seqname, MySubscription->name);
 					batch_succeeded_count++;
 					sequence_copied = true;
 					break;
@@ -765,14 +764,12 @@ LogicalRepSyncSequences(WalReceiverConn *conn)
 		 * allocate them under SequenceSyncContext.
 		 */
 		oldctx = MemoryContextSwitchTo(SequenceSyncContext);
-
 		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);
 
 		table_close(sequence_rel, NoLock);
@@ -860,7 +857,6 @@ start_sequence_sync(void)
 
 			/* Process any invalidation messages that might have accumulated */
 			AcceptInvalidationMessages();
-
 			maybe_reread_subscription();
 
 			/*
@@ -875,7 +871,6 @@ start_sequence_sync(void)
 			}
 			else
 			{
-
 				/*
 				 * Double the sleep time, but not beyond
 				 * the maximum allowable value.

---

From: "Zhijie Hou (Fujitsu)" <[email-masked]>
Date: Sat, 28 Feb 2026 08:40:31 +0000

On Friday, February 27, 2026 7:07 PM Amit Kapila <[email-masked]> wrote:
> - errmsg("sequencesync worker for subscription \"%s\" could not connect to
> the publisher: %s",

Agreed. I changed as suggested.

> 3. Apart from above, the attached patch contains comments and cosmetic

Thanks for the patch, I have merged them into 0001.

Here is the V8 patch set addressing the previous review comments:

- For 0001, I noticed that the GetSequence() function added in the patch
  fetches the local sequence value without any privilege check. This
  allows the worker to read sequence data even without proper SELECT
  privilege, which seems unsafe. I've added a SELECT privilege check
  before fetching the sequence value. Additionally, I've updated several
  comments, made cosmetic changes, commit message update, and run
  pgindent on all patches.

- 0002 includes the changes to synchronize sequences directly in the
  REFRESH SEQUENCES command

Best Regards,
Hou zj

---

From: "Zhijie Hou (Fujitsu)" <[email-masked]>
Date: Mon, 2 Mar 2026 07:58:54 +0000

>   REFRESH SEQUENCES command

Rebased the patch to silence compile warning due to a recent commit
a2c89835.

Best Regards,
Hou zj

---

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Wed, 4 Mar 2026 04:20:47 +0000

Dear Hou,

01.
```
   <para>
    A <firstterm>sequence synchronization worker</firstterm> will be started
-   after executing any of the above subscriber commands, and will exit once the
-   sequences are synchronized.
+   after executing any of the above subscriber commands. The worker will
+   remain running for the life of the subscription, periodically
+   synchronizing all published sequences.
   </para>
```

I think it's not accurate, because REFRESH SEQUENCE command does not need the
sequencesync worker anymore.

02.
```
void
GetSequence(Relation seqrel, int64 *last_value, bool *is_called)
```

I think GetSequence() itself should conitan the permission check like SetSequence().
My idea is to set NULL for last_value and is_called in this case.

03.
```
		/*
		 * Verify that the current user has SELECT privilege on the sequence.
		 * This is required to read the sequence state below.
		 */
		aclresult = pg_class_aclcheck(seqoid, GetUserId(), ACL_SELECT);

		if (aclresult != ACLCHECK_OK)
			return COPYSEQ_INSUFFICIENT_PERM;

		/* Get current local sequence state */
		GetSequence(sequence_rel, &local_last_value, &local_is_called);
```

If you accept above comment, this part can be simplified.

04.
```
/*
 * get_and_validate_seq_info
 *
 * Extracts remote sequence information from the tuple slot received from the
 * publisher, and validates it against the corresponding local sequence
 * definition.
 */
static CopySeqResult
get_and_validate_seq_info(TupleTableSlot *slot, Relation *sequence_rel,
						  LogicalRepSequenceInfo **seqinfo, int *seqidx,
						  List *seqinfos)
```

It can return COPYSEQ_SUCCESS, but it might be misleading; copying is not
happened yet here. How about returning boolean and add another argument to
indicate the reason if the validation is failed?

05.

LogicalRepSyncSequences() starts the transaction and read sequences every time.
Can we cache the seqinfos to reuse in the next iteration? My idea is to introduce
a syscache callback for the pg_subscription_relto invalidate the cached list.

How about measuring performance once and considering it's a good improvement?

Best regards,
Hayato Kuroda
FUJITSU LIMITED

---

From: shveta malik <[email-masked]>
Date: Wed, 4 Mar 2026 15:54:35 +0530

> Rebased the patch to silence compile warning due to a recent commit

Thanks for the patch. Please find a few comments for 001:

1)
  /*
  * Record the remote sequence's LSN in pg_subscription_rel and mark the
- * sequence as READY.
+ * sequence as READY if updating a sequence that is in INIT state.
  */
- UpdateSubscriptionRelState(MySubscription->oid, seqoid, SUBREL_STATE_READY,
-    seqinfo->page_lsn, false);
+ if (seqinfo->relstate == SUBREL_STATE_INIT)
+ UpdateSubscriptionRelState(MySubscription->oid, seqoid, SUBREL_STATE_READY,
+    seqinfo->page_lsn, false);

What if page-lsn has changed and we are in READY state, don't we need
to update that in pg_subscription_rel? Or is that done somewhere else?

2)
+ *
+ * If relstate is SUBREL_STATE_READY, only synchronize sequences that
+ * have drifted from their publisher values. Otherwise, synchronize
+ * all sequences.
+ *
+ * Returns true/false if any sequences were actually copied.
  */
+static bool
+copy_sequences(WalReceiverConn *conn, List *seqinfos)

There is no relstate, comments need correction.

3)
Currently we use same state 'COPYSEQ_SUCCESS' for 2 cases: allowed to
copy (as returned by validate_seqsync_state and
get_and_validate_seq_info) and copy-done (by copy_sequences,
copy_sequence). It is slightly confusing. Shall we add one more state
for 'allowed' case, could be COPYSEQ_ELIGIBLE or COPYSEQ_PROCEED or
COPYSEQ_ALLOWED? COPYSEQ_SUCCESS was used for such a case in previous
seq-sync commands too (on HEAD), but now its usage is more in
'allowed' case as compared to HEAD, so perhaps we can change in this
patch. But I would like to know what others think here.

4)
+ * Preliminary check to determine if copying the sequence is allowed.

How about this comment:
Check whether the user has required privileges on the sequence and
whether the sequence has drifted.

5)
validate_seqsync_state():
+ /*
+ * Skip synchronization if the sequence is already in READY state and
+ * has not drifted from the publisher's value.
+ */
+ if (local_last_value == seqinfo->last_value &&
+ local_is_called == seqinfo->is_called)
+ return COPYSEQ_NO_DRIFT;

Since we already have a comment where we check READY state in outer
if-block and since we are not checking READY state here, perhaps we
can change the above comment to simply:
 "Skip synchronization if it has not drifted from the publisher's value."

thanks
Shveta

---

From: "Zhijie Hou (Fujitsu)" <[email-masked]>
Date: Thu, 5 Mar 2026 02:45:45 +0000

> Thanks for updating the patch. Here are my comments.

Thanks for the comments.

> I think it's not accurate, because REFRESH SEQUENCE command does not
> need the sequencesync worker anymore.

Since the command is changed in 0002, I updated the doc there.

> 
> 02.
> ```
> void
> GetSequence(Relation seqrel, int64 *last_value, bool *is_called) ```
> 

Changed.

> 		GetSequence(sequence_rel, &local_last_value,
> &local_is_called); ```
> 

Changed.

> happened yet here. How about returning boolean and add another argument
> to indicate the reason if the validation is failed?

I have added one more enum value COPYSEQ_ALLOWED for cases where
the validation passes to avoid changing the function signature.

> Can we cache the seqinfos to reuse in the next iteration? My idea is to
> introduce a syscache callback for the pg_subscription_relto invalidate the
> cached list.
> 

I think the time of scanning sequence might be negligible when most of sequence
needs to be updated, since the most cost would be on sequence update. There
might be some value to save the CPU cycle when the most of sequence
has not drifted in which case the cost of scanning might be noticeable. But we
will do some more tests and share later.

Best Regards,
Hou zj

---

From: "Zhijie Hou (Fujitsu)" <[email-masked]>
Date: Thu, 5 Mar 2026 02:46:00 +0000

On Wednesday, March 4, 2026 6:25 PM shveta malik <[email-masked]> wrote:
> 
> On Mon, Mar 2, 2026 at 1:28 PM Zhijie Hou (Fujitsu) <[email-masked]>
> wrote:
> >

Thanks for the comments.

> What if page-lsn has changed and we are in READY state, don't we need to
> update that in pg_subscription_rel? Or is that done somewhere else?

The check was done in 0002, I moved it to 0001 now.

Changed.

> copy_sequence). It is slightly confusing. Shall we add one more state for
> 'allowed' case, could be COPYSEQ_ELIGIBLE or COPYSEQ_PROCEED or
> COPYSEQ_ALLOWED? COPYSEQ_SUCCESS was used for such a case in
> previous seq-sync commands too (on HEAD), but now its usage is more in
> 'allowed' case as compared to HEAD, so perhaps we can change in this patch.
> But I would like to know what others think here.

I agree that adding a new value can make the code easier to read.

> Check whether the user has required privileges on the sequence and whether
> the sequence has drifted.

Changed as suggested.

> + if (local_last_value == seqinfo->last_value && local_is_called ==
> + seqinfo->is_called) return COPYSEQ_NO_DRIFT;
> 
> Since we already have a comment where we check READY state in outer if-
> block and since we are not checking READY state here, perhaps we can
> change the above comment to simply:

Changed.

Here is V10 patch set which addressed all comments.

I also fixed a bug that Kuroda-San reported off-list, where we were not
resetting the StringInfo used to build the query in the copy_sequences loop,
which caused the built query to be incorrect.

Best Regards,
Hou zj

---

From: shveta malik <[email-masked]>
Date: Thu, 5 Mar 2026 09:35:21 +0530

> Here is V10 patch set which addressed all comments.

Thank You. Please find a few comments on 001:

1)
+ /*
+ * Skip synchronization if the current user does not have sufficient
+ * privileges to read the sequence data.
+ */
+ if (local_last_value == 0)
+ return COPYSEQ_INSUFFICIENT_PERM;

I don't think it is the right way to handle this. The local_last_value
can be genuinely 0 for some cases and we may end up giving the wrong
ERROR.

Try this:
CREATE SEQUENCE my_seq START WITH 0 INCREMENT BY 1 MINVALUE 0;
And then set-up pub-sub.

We get:
2026-03-05 08:57:39.591 IST [92281] WARNING:  insufficient privileges
on sequence ("public.my_seq")
2026-03-05 08:57:39.591 IST [92281] ERROR:  logical replication
sequence synchronization failed for subscription "subi1"

Either we shall move back the acl check to the caller of GetSequence
or pass the info of acl-check failure in  a new argument or return
value.

2)
+ /*
+ * For sequences in INIT state, always sync. Otherwise, for
+ * sequences in READY state, only sync if there's drift.
+ */
  if (sync_status == COPYSEQ_SUCCESS)
- sync_status = copy_sequence(seqinfo,
- sequence_rel->rd_rel->relowner);
+ sync_status = copy_sequence(seqinfo, sequence_rel);

We shall add such a comment atop copy_sequence as well. I am unsure if
we need it here or not.

3)
+ /* Sleep for the configured interval */
+ (void) WaitLatch(MyLatch,
+ WL_LATCH_SET | WL_TIMEOUT | WL_EXIT_ON_PM_DEATH,
+ sleep_ms,
+ WAIT_EVENT_LOGICAL_SYNC_STATE_CHANGE);

I don't think this wait-event is appropriate. Unlike tablesync, we are
not waiting for any state change here. Shall we add a new one for our
case? How about WAIT_EVENT_LOGICAL_SEQSYNC_MAIN? Thoughts?

4)
+ relstate = subrel->srsubstate;

it will be good to move it just after below part:

/* Skip if the relation is not a sequence */

5)

  }
+ /* Check if there are any sequences. */
+ has_subsequences = (seq_states != NIL);

One blank line before new change will improve readability.

6)

 ##########
 ## ALTER SUBSCRIPTION ... REFRESH PUBLICATION should cause sync of new
-# sequences of the publisher, but changes to existing sequences should
-# not be synced.
+# sequences of the publisher.
 ##########

 # Create a new sequence 'regress_s2', and update existing sequence 'regress_s1'

Last comment needs to be changed. Remove this please:  'and update
existing sequence 'regress_s1''

thanks
Shveta

---

From: shveta malik <[email-masked]>
Date: Thu, 5 Mar 2026 11:18:20 +0530

> Thank You. Please find a few comments on 001:

A concern in 002:

I realized that below might not be the correct logic to avoid
overwriting sequences at sub which are already at latest values.

+ /*
+ * Skip synchronization if the local sequence value is already ahead of
+ * the publisher's value.
...
+ */
+ if (local_last_value > seqinfo->last_value)
+ {
+ ereport(WARNING,
+ errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("skipped synchronizing the sequence \"%s.%s\"",
+    seqinfo->nspname, seqinfo->seqname),
+ errdetail("The local last_value %lld is ahead of the one on publisher",
+   (long long int) local_last_value));
+
+ return COPYSEQ_NO_DRIFT;
+ }

A sequence could be descending one too and thus we may wrongly end up
avoiding synchronization. We should first check if it is descending or
ascending (perhaps by checking if increment_by < 0 or >0), then decide
to manage conflict.

Example:
postgres=# CREATE SEQUENCE desc_seq START WITH 1000 INCREMENT BY -1
MINVALUE 1 MAXVALUE 1000;
CREATE SEQUENCE
postgres=# select nextval('desc_seq');
nextval
---------
    1000

postgres=# select nextval('desc_seq');
nextval
---------
     999

Doc also mentions descending sequences. See [1] (search for descending).

[1]: https://www.postgresql.org/docs/18/sql-createsequence.html

thanks
Shveta

---

From: Amit Kapila <[email-masked]>
Date: Thu, 5 Mar 2026 16:16:50 +0530

> case? How about WAIT_EVENT_LOGICAL_SEQSYNC_MAIN? Thoughts?

+1 for a new wait event. Few other minor comments:

1.
+ * Check if the subscription includes sequences and start a sequencesync
+ * worker if one is not already running. The active sequencesync worker will
+ * handle all pending sequence synchronization. If any sequences remain
+ * unsynchronized after it exits, a new worker can be started in the next
+ * iteration.
  *
- * Start a sequencesync worker if one is not already running. The active
- * sequencesync worker will handle all pending sequence synchronization. If any
- * sequences remain unsynchronized after it exits, a new worker can be started
- * in the next iteration.

Why did this comment change? The earlier one sounds okay to me.

2.
  break;
+
  case COPYSEQ_INSUFFICIENT_PERM:

Why does this patch add additional new lines? We use both styles
(existing and what this patch does) in the code but it seems
unnecessary to change for this patch.

3.
- ProcessSequencesForSync();
+
+ /* Check if sequence worker needs to be started */
+ MaybeLaunchSequenceSyncWorker();

No need for an additional line and a comment here.

-- 
With Regards,
Amit Kapila.

---

From: "Zhijie Hou (Fujitsu)" <[email-masked]>
Date: Thu, 5 Mar 2026 11:34:51 +0000

On Thursday, March 5, 2026 6:47 PM Amit Kapila <[email-masked]> wrote:
> > + WL_LATCH_SET | WL_TIMEOUT | WL_EXIT_ON_PM_DEATH, sleep_ms,

Added.

> 
> 1.
> + * Check if the subscription includes sequences and start a
> + sequencesync
> + * worker if one is not already running. The active sequencesync worker
> + will
> + * unsynchronized after it exits, a new worker can be started in the
> + next
> + * iteration.
>   *

I think either version is fine, so reverted this change now.

> Why does this patch add additional new lines? We use both styles (existing
> and what this patch does) in the code but it seems unnecessary to change for
> this patch.

Removed.

Removed.

Here is the V11 patch which addressed all above comments and [1][2].

[1] https://www.postgresql.org/message-id/CAJpy0uAfu-VPqCknLLvJ%2BPUx_cyoR-b70xUNT6Pyv8N-odKizQ%40mail.gmail.com
[2] https://www.postgresql.org/message-id/CAJpy0uBeAdz6-3P26Eryeq3TyjA-GiKY3z0hFMxzZD%3DAYGqQ3Q%40mail.gmail.com

Best Regards,
Hou zj

---

From: "Zhijie Hou (Fujitsu)" <[email-masked]>
Date: Thu, 5 Mar 2026 11:34:53 +0000

On Thursday, March 5, 2026 1:48 PM shveta malik <[email-masked]> wrote:
> I realized that below might not be the correct logic to avoid overwriting
> sequences at sub which are already at latest values.
> 
> + /*
> + if (local_last_value > seqinfo->last_value) { ereport(WARNING,
> +    seqinfo->nspname, seqinfo->seqname), errdetail("The local
> + last_value %lld is ahead of the one on publisher",
> avoiding synchronization. We should first check if it is descending or ascending
> (perhaps by checking if increment_by < 0 or >0), then decide to manage
> conflict.

Thanks for catching this, I changed it to check the increment before reporting warning.

Best Regards,
Hou zj

---

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Thu, 5 Mar 2026 12:57:03 +0000

Dear Hackers,

> How about measuring performance once and considering it's a good

I profiled the sequencesync worker when sequences were less actively updated on
the publisher side. In the actively updated system, copying sequences used most
of the CPU time; thus, we could not observe the effect.

Abstract
--------------
Sequencesync worker spent 20-25% of the working time scanning pg_subscription_rel
in the workload. It's not so large compared with the total CPU time; the worker
can work once per 2 seconds or longer. We may able to consider the optimization
if there are easy ways.

Source
-----------
ea47447 + v8 patch set + attached fix patch.
To simplify the analysis, I extracted the scan part into the function
scan_subscription_relations. No configure options are set at build.

Workload
---------------
Two workloads were tested.

A - profile with no sequence updates

0. Defined 100 sequences on both nodes
1. Built a pub-sub replication system.
2. Attached the sequencesync worker as early as creating the subscription. 
3. Waited 10 minutes.

B - profile with 10% sequences updates

0. Defined 100 sequences on both nodes
1. Built a pub-sub replication system.
2. Waited till the initial sync was done. On my env 100s was enough
3. Attached the sequencesync worker
4. Updated 10 sequences per second.
5. Repeat step 4 for 10 minutes.

Result
----------
The attached profiles show the detailed results: noupdate.out corresponds to
workload A, while 10percent_update.out is for workload B.

In both cases, scan_subscription_relations spends more than 20% of their working
time. Notable points are to open sequence relations with the AccessShareLock,
committing the transaction, starting the catalog scan, etc.

workload A:
```
               |           --20.83%--scan_subscription_relations
               |                     |          
               |                     |--10.83%--try_table_open
               |                     |          try_relation_open
```

workload B:
```
                       |           --24.01%--scan_subscription_relations
                       |                     |          
                       |                     |--12.52%--try_table_open
                       |                     |          try_relation_open
```

Consideration
--------------
Based on that, we may be able to cache seqinfos to avoid starting the
transaction and opening the sequence. But we need to introduce a relcache
callback to invalidate the specific entry of the list, not sure it's beneficial
more than the complexity.

Configuration
----------------------
Each node had shared_buffer=1GB, and others had the default GUC values.

Environment
--------------------
CPU: Intel(R) Xeon(R) Platinum 8358P CPU @ 2.60GHz, 4 cores, 1 thread per core
Memory: 15GiB
OS: AlmaLinux 9.7

Best regards,
Hayato Kuroda
FUJITSU LIMITED

---

From: shveta malik <[email-masked]>
Date: Fri, 6 Mar 2026 10:55:02 +0530

> Thanks for catching this, I changed it to check the increment before reporting warning.

Thanks! I have changed comments atop sequencesync.c to get rid of old
implementation details (worker dependency upon INIT state) and
rephrased a little bit. Please take it if you find it okay.  Attaching
patch as txt file. It is a top-up for 0001.

thanks
Shveta
From 22a575bc2ab43795c72f2240485c189262672035 Mon Sep 17 00:00:00 2001
From: Shveta Malik <[email-masked]>
Date: Fri, 6 Mar 2026 10:45:01 +0530
Subject: [PATCH] header comment improvement

---
 .../replication/logical/sequencesync.c        | 49 +++++++++----------
 1 file changed, 22 insertions(+), 27 deletions(-)

diff --git a/src/backend/replication/logical/sequencesync.c b/src/backend/replication/logical/sequencesync.c
index cde9eaba474..a80d5e5b335 100644
--- a/src/backend/replication/logical/sequencesync.c
+++ b/src/backend/replication/logical/sequencesync.c
@@ -19,37 +19,32 @@
  * CREATE SUBSCRIPTION
  * ALTER SUBSCRIPTION ... REFRESH PUBLICATION
  *
- * The apply worker periodically scans pg_subscription_rel for sequences in
- * INIT state. When such sequences are found, it spawns a sequencesync worker
- * to handle synchronization.
- *
- * A single sequencesync worker is responsible for synchronizing all sequences.
- * It begins by retrieving the list of sequences that are flagged for
- * synchronization, i.e., those in the INIT state. 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
+ * The apply worker periodically scans pg_subscription_rel for sequences
+ * When sequences are found, it spawns a sequencesync worker to handle
  * synchronization.
  *
- * The sequencesync worker then fetches all sequences that are
- * in the READY state, queries the publisher for current sequence values, and
- * updates any sequences that have drifted and then goes to sleep. The sleep
- * interval starts as SEQSYNC_MIN_SLEEP_MS and doubles after each wake cycle
- * (up to SEQSYNC_MAX_SLEEP_MS). When drift is detected, the interval resets to
- * the minimum to ensure timely updates.
- *
- * After CREATE SUBSCRIPTION, sequences begin in the INIT state. Sequences
- * added through ALTER SUBSCRIPTION.. REFRESH PUBLICATION also start in the INIT
- * state. All INIT sequences are synchronized unconditionally, then transition
- * to the READY state. Once in the READY state, sequences are checked for drift
- * from the publisher and synchronized only when drift is detected.
+ * A single sequencesync worker is responsible for synchronizing all sequences
+ * for a subscription. It begins by retrieving the list of sequences. 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 and updates the
+ * corresponding sequences on the local subscriber. Sequences in the INIT
+ * state are unconditionally updated to the latest values from the publisher
+ * and then moved to the READY state. For sequences already in the READY
+ * state, the worker checks for drift and updates them only when needed.
  *
  * Sequence state transitions follow this pattern:
- *  INIT --> READY ->-+
- *             ^      | (check/synchronize)
- *             |      |
- *             +--<---+
+
+ *       (synchronize)
+ *  INIT --------------> READY ->-+
+ *                         ^      | (check-drift/synchronize)
+ *                         |      |
+ *                         +--<---+
+ *
+ * Between cycles, the worker sleeps for SEQSYNC_MIN_SLEEP_MS. If no drift is
+ * observed in any sequence, the sleep interval doubles after each wake cycle
+ * up to SEQSYNC_MAX_SLEEP_MS. When drift is detected, the interval resets to
+ * the minimum to ensure timely updates.
  *
  * To avoid creating too many transactions, up to MAX_SEQUENCES_SYNC_PER_BATCH
  * sequences are synchronized per transaction. The locks on the sequence
-- 
2.34.1

---

From: Chao Li <[email-masked]>
Date: Fri, 6 Mar 2026 16:52:28 +0800

> On Mar 5, 2026, at 19:34, Zhijie Hou (Fujitsu) <[email-masked]> wrote:
> 

Hi,

I just started reviewing this patch and wanted to first discuss the design.

The current approach introduces a long-lived sync worker for any subscription that has at least one sequence. I noticed a previous email suggesting that this approach is “acceptable”, but it still seems like a big runtime cost.

What I had in mind instead is whether we could extend the WAL decoding protocol to send RM_SEQ_ID over the logical replication stream, so that sequence synchronization becomes part of logical replication itself. That would make it essentially event-driven and close to zero cost at runtime, rather than relying on periodic polling.

There is also one case I haven’t seen discussed yet. Suppose the standby side inserts a tuple into a table that is under logical replication. This might not immediately cause a tuple-level replication conflict, but it could advance the sequence locally. In that case, the standby sequence could diverge from the primary sequence and remain out of sync indefinitely. How should that situation be handled?

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

---

From: "Zhijie Hou (Fujitsu)" <[email-masked]>
Date: Fri, 6 Mar 2026 10:27:59 +0000

> I just started reviewing this patch and wanted to first discuss the design.

Thanks for reviewing.

> The current approach introduces a long-lived sync worker for any subscription
> that has at least one sequence. I noticed a previous email suggesting that
> this approach is “acceptable”, but it still seems like a big runtime cost.

Currently in the patch, the sequence sync worker operates at a dynamic interval
(between 2 and 30 seconds), adjusting based on how frequently the sequence is
updated, and only synchronizes when the sequence has actually diverged from the
publisher. So while it's a long-lived worker, I think its runtime impact is
acceptable.

> What I had in mind instead is whether we could extend the WAL decoding
> protocol to send RM_SEQ_ID over the logical replication stream, so that
> sequence synchronization becomes part of logical replication itself.

That approach was explored in depth during earlier discussions, but it was
ultimately set aside due to the complexity and correctness challenges it would
introduce into logical decoding. Adding sequence information to the replication
stream would require significant changes to the decoding machinery, and given
that the primary use case is to support upgrades, the trade-off didn't seem
justified. You can find a detailed breakdown of the design considerations and
the reasoning behind the decision in [1].

> That would make it essentially event-driven and close to zero cost at runtime,
> rather than relying on periodic polling.

I don't think so. Even with an event-driven model, there's still a cost,
decoding sequence changes would add overhead to walsenders on the publisher that
needs to replicate sequences. So it's just a different distribution of the
overhead.

> There is also one case I haven’t seen discussed yet. Suppose the standby side
> inserts a tuple into a table that is under logical replication. This might not
> immediately cause a tuple-level replication conflict, but it could advance the
> sequence locally. In that case, the standby sequence could diverge from the
> primary sequence and remain out of sync indefinitely. How should that
> situation be handled?

This scenario was also considered in earlier discussions, see [2]. The
divergence issue you mentioned is not introduced by the sequence sync worker; It
is considered as a logical conflict and resolving it would require user
intervention or a future enhancement to handle such conflicts automatically.

[1] https://www.postgresql.org/message-id/CAA4eK1LC%2BKJiAkSrpE_NwvNdidw9F2os7GERUeSxSKv71gXysQ%40mail.gmail.com
[2] https://www.postgresql.org/message-id/CAA4eK1LLkxqeZ_GDjquzxY3bwN3yV8Nq7brvgyOBiWOXtWt4Jg%40mail.gmail.com

Best Regards,
Hou zj

---

From: shveta malik <[email-masked]>
Date: Mon, 9 Mar 2026 08:43:06 +0530

> patch as txt file. It is a top-up for 0001.

No major concerns on 001, just a few trivial things. Do these only if
you feel okay about these.

1)
copy_sequence():

+ (void) GetSubscriptionRelState(MySubscription->oid,
+    RelationGetRelid(sequence_rel),
+    &local_page_lsn);

IIUC, we only need it to get local_page_lsn which is used at the end
of copy_sequence(). Shall we move fetching it towards the end. That
way if validate_seqsync_state() returns copy-not-allowed, then there
is no need to even do 'GetSubscriptionRelState'.

2)
validate_seqsync_state() and get_and_validate_seq_info() are a bit
confusing (at least to me). They have similar names but serve
different purposes. Would it make sense to rename the new function
validate_seqsync_state() to check_seq_privileges_and_drift()?

3)
I feel that the section below in the doc needs some change to briefly
mention the role of the SeqSync worker, at least at a high level, to
indicate that it is running to perform incremental synchronization
already. Thoughts?

---------
29.7.2. Refreshing Out-of-Sync Sequences
Subscriber sequence values can become out of sync as the publisher
advances them. To detect this, compare the
pg_subscription_rel.srsublsn on the subscriber with the page_lsn
obtained from the pg_get_sequence_data function for the sequence on
the publisher. Then run ALTER SUBSCRIPTION ... REFRESH SEQUENCES to
re-synchronize if necessary.
---------

thanks
Shveta

---

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Mon, 9 Mar 2026 03:16:47 +0000

Dear Hou,

Thanks for uprating the patch. Few comments.

01.
The patch needs to be rebased, because of the missing inclusion of wait_event.h.
It might be affected by the commit 868825aae.

02. start_sequence_sync
Missing ProcessConfigFile() in the loop. Now it's done only in copy_sequences(),
but we can reach there when some sequences need to be synchronized.

03.
Can we describe needed privileges? IIUC, initial sync needs the UPDATE privilege,
additionally periodic synch needs SELECT privilege.

04.
```
+               long            sleep_ms = SEQSYNC_MIN_SLEEP_MS;
```

This is used in the for loop, make it the loop-specific variable.

Best regards,
Hayato Kuroda
FUJITSU LIMITED

---

From: shveta malik <[email-masked]>
Date: Mon, 9 Mar 2026 15:59:15 +0530

Few comments for 0002:

1)
+ /*
+ * Allow synchronization if the remote sequence data has a more recent
+ * LSN than the local state, or if no local LSN exists yet.
+ */
+ if (!XLogRecPtrIsValid(local_page_lsn) ||
+ local_page_lsn < seqinfo->page_lsn)
+ return COPYSEQ_ALLOWED;
  /*
  * Skip synchronization if the current user does not have sufficient
  * privileges to read the sequence data.
@@ -384,6 +418,40 @@ validate_seqsync_state(LogicalRepSequenceInfo
*seqinfo, Relation sequence_rel)
  if (!GetSequence(sequence_rel, &local_last_value, &local_is_called))
  return COPYSEQ_INSUFFICIENT_PERM;

Shouldn't the 'COPYSEQ_INSUFFICIENT_PERM' check be the first one, else
we may end up allowing the copy due to the first check even if the
user has no privileges? Or let me know if I am missing something here.

2)
+ ereport(WARNING,
+ errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("skipped synchronizing the sequence \"%s.%s\"",
+    seqinfo->nspname, seqinfo->seqname),
+ seqinfo->increment
+ ? errdetail("The local last_value %lld is ahead of the one on publisher",
+ (long long int) local_last_value)
+ : errdetail("The local last_value %lld is behind of the one on publisher",
+ (long long int) local_last_value));

This error message and DETAIL might not be very clear to the user.
IMO, it may not be easy for the user to deduce that a sequence is
descending and thus it being "behind" is the reason for skipping
synchronization.

Shall we update ERROR msg to say ascending/descending
ERROR: skipped synchronizing ascending sequence <name>
ERROR: skipped synchronizing descending sequence <name>

Since we use descending/ascending in sequence doc many times, I feel
it is okay to use it in ERROR messages too to give more clarity.
Thoughts?

3)
    A <firstterm>sequence synchronization worker</firstterm> will be started
-   after executing any of the above subscriber commands. The worker will
-   remain running for the life of the subscription, periodically
-   synchronizing all published sequences.

    A <firstterm>sequence synchronization worker</firstterm> will be started
+   after executing <command>ALTER SUBSCRIPTION ... REFRESH
PUBLICATION</command>
+   command. The worker will remain running for the life of the subscription,
+   periodically synchronizing all published sequences.

Why have we changed this to mention REFRESH PUB alone responsible for
starting worker? From user's point of view, even CREATE SUB (which has
sequences) will also start the worker no?

thanks
Shveta

---

From: "Zhijie Hou (Fujitsu)" <[email-masked]>
Date: Fri, 13 Mar 2026 07:12:09 +0000

On Tuesday, March 10, 2026 2:44 PM Chao Li <[email-masked]> wrote:
> Thanks for your clarification in the other email. I have reviewed v11 and here
> comes my comments:

Thanks for the comments.

> 
> 1 - 0001
> ```
>  static CopySeqResult
> -copy_sequence(LogicalRepSequenceInfo *seqinfo, Oid seqowner)
> +validate_seqsync_state(LogicalRepSequenceInfo *seqinfo, Relation
> sequence_rel)
>  {
> -	UserContext ucxt;
>  	AclResult	aclresult;
> +	Oid			seqoid = seqinfo->localrelid;
> +
> +	/* Perform drift check if it's not the initial sync */
> +	if (seqinfo->relstate == SUBREL_STATE_READY)
> +	{
> +		int64		local_last_value;
> +		bool		local_is_called;
> +
> +		/*
> +		if (!GetSequence(sequence_rel, &local_last_value,
> &local_is_called))
> +		 * Skip synchronization if the sequence has not drifted from
> the
> +		 * publisher's value.
> +		 */
> +	/* Verify that the current user can update the sequence */
> +	aclresult = pg_class_aclcheck(seqoid, GetUserId(), ACL_UPDATE);
> +
> +	if (aclresult != ACLCHECK_OK)
> +}
> I think we can move pg_class_aclcheck() to before the drift check, because it’s
> a local check, should be cheaper than a remote check. If the local check fails,
> we don’t need to touch remote.

It makes more sense to structure the permission check around the actual
requirement. Since we only update the sequence when a drift is detected, the
permission check should naturally follow that order. If the permission check
fails, an error will be raised anyway, and in that case, performance is less of
a concern.

> 
> 2 - 0001
> ```
> +static CopySeqResult
> +copy_sequence(LogicalRepSequenceInfo *seqinfo, Relation sequence_rel)
> +{
> +	UserContext ucxt;
>  	bool		run_as_owner = MySubscription->runasowner;
>  	Oid			seqoid = seqinfo->localrelid;
> +	CopySeqResult result;
> +	XLogRecPtr	local_page_lsn;
> +
> RelationGetRelid(sequence_rel),
> +
> &local_page_lsn);
> ```
> 
> I think GetSubscriptionRelState is called to get local_page_lsn. But
> local_page_lsn is only used very late in this function, and there is an early
> return branch, so can we move this call to right before where local_page_lsn
> is used?

Yes, we can do that. Changed.

> 3 - 0001
> +	if (seqinfo->relstate == SUBREL_STATE_INIT ||
> +		seqinfo->page_lsn != local_page_lsn)
> +								   seqinfo-
> >page_lsn, false);
> In the comment, I think you don’t have to add “if updating .. that is in INIT
> state”, but if you do, then you should also mention the lsn condition.

Changed.

> 
> 4 - 0001
> ```
> +			else
> +			{
> +				/*
> +				 * Double the sleep time, but not beyond the
> maximum allowable
> +				 * value.
> +				 */
> Double the sleep time when no drift is an optimization. But looks like the
> doubling happens only when all sequences have no drift. Say, there are 1000
> sequences, and only one is hot, then the it will still fetch the 1000 sequences
> from remote every 2 seconds, making the optimization much less efficient. I
> think the worker can wake up every 2 seconds, but next fetch time should be
> per sequence.

I'm unsure whether the complexity of per-sequence interval adjustment is
justified. We could consider this as a future enhancement based on user
feedback.

> 5 - 0001
> ```
> + * If the state of sequence is SUBREL_STATE_READY, only synchronize
> sequences
> ```
> 
> “The state of sequence is SUBREL_STATE_READY” is inaccurate, I think it
> should be “the state of sequence sync is SUBREL_STATE_READY”.

I slightly reworded the comments.

> 6 - 0001
> 
> start_sequence_sync runs an infinite loop to periodically sync sequences. I
> don’t it has an auto reconnect mechanism. When something wrong happens,
> the sync worker will exit, how can the worker

The comment seems incomplete.

> 
> 7 - 0001
> 
> Say a major version upgrade use case that uses logical replication. Before
> shutdown the publication side (old version), if there are 1000 sequences, how
> can a user decide if all sequences have been synced? From this perspective,
> would it make sense to log a INFO message when all sequences have no drift?
> If next round still no drift, then don’t repeat the message. In other words,
> when the states between (all no drift) and (any drift) switch, log a INFO
> message.

To determine whether a sequence needs to be resynchronized, users should check
srsublsn in pg_subscription_rel, as documented in [1]. To ensure all sequences
are fully synced, they can simply execute REFRESH SEQUENCE as the final step.

> 
> 8 - 0001
> ```
> +bool
> +GetSequence(Relation seqrel, int64 *last_value, bool *is_called)
> ```
> 
> This function name feels too general. How about ReadSequenceState or
> GetSequenceLastValueAndIsCalled?

I am not sure if the suggested name is better, considering that we already
have SetSequence().

> 
> 9 - 0001
> ```
> +					elog(ERROR, "unrecognized Sequence
> replication result: %d", (int) sync_status);
> ```
> Nit: The “S” seems not have to be capital.

Changed.

> 10 - 0002

0002 includes significant changes in this version, though I have made an effort to address
the comments that are still applicable.

[1] https://www.postgresql.org/docs/devel/logical-replication-sequences.html#SEQUENCES-OUT-OF-SYNC

Best Regards,
Hou zj

---

From: "Zhijie Hou (Fujitsu)" <[email-masked]>
Date: Fri, 13 Mar 2026 07:12:49 +0000

On Monday, March 9, 2026 11:17 AM Kuroda, Hayato/黒田 隼人 <[email-masked]> wrote:
> Few comments.

Thanks for the comments.

> 
> 03.
> Can we describe needed privileges? IIUC, initial sync needs the UPDATE
> privilege, additionally periodic synch needs SELECT privilege.

I could not find existing doc for the privilege required for table sync or
sequence sync, so I think we can consider that as a separate patch.

> This is used in the for loop, make it the loop-specific variable.

This value should survive across sync loop as it's dynamically
adjusted in the loop.

Other comments have been addressed in latest version.

Best Regards,
Hou zj

---

From: "Zhijie Hou (Fujitsu)" <[email-masked]>
Date: Fri, 13 Mar 2026 07:13:48 +0000

On Monday, March 9, 2026 11:13 AM shveta malik <[email-masked]> wrote:
> 
> No major concerns on 001, just a few trivial things. Do these only if you feel
> okay about these.
> 

Thanks for the reviews. I've updated the patch set addressing all comments.

In 0001, aside from addressing comments in [1][2][3][4], I've changed the
logic to delay updating the page LSN for each sequence in
pg_subscription_rel. Frequent catalog updates would generate many
invalidations, degrading the performance of the apply worker which
relies on cached data from pg_subscription_rel.

0002 adds caching of sequence information for the current subscription in the
sequence sync worker. The cache is invalidated immediately when
pg_subscription_rel is modified. Concurrent changes to sequence names or
namespaces are detected before synchronization, as the worker verifies the
sequence data at sync time.

0003 (formerly 0002) modifies REFRESH SEQUENCES to synchronize sequence values
directly without launching a worker.

[1] https://www.postgresql.org/message-id/02EDB3D2-4E5A-4EDE-BADF-3DF62D707831%40gmail.com
[2] https://www.postgresql.org/message-id/OS9PR01MB12149E4614DA95963670772EEF579A%40OS9PR01MB12149.jpnprd01.prod.outlook.com
[3] https://www.postgresql.org/message-id/CAJpy0uAmEkjsBS6RxPv9iDcK2kfJ5%3Dbq4Mq1zMCQtaYFoDfbbQ%40mail.gmail.com
[4] https://www.postgresql.org/message-id/CAJpy0uC0T_tp62zxJN_2d_A%3DYpvf14ebjGFepckeJugW5OHOyA%40mail.gmail.com

Best Regards,
Hou zj

---

From: Chao Li <[email-masked]>
Date: Fri, 13 Mar 2026 16:37:15 +0800

> On Mar 13, 2026, at 15:13, Zhijie Hou (Fujitsu) <[email-masked]> wrote:
> 
> <v12-0003-Synchronize-sequences-directly-in-REFRESH-SEQUEN.patch><v12-0001-Support-automatic-sequence-replication.patch><v12-0002-Cache-sequence-information-in-the-sequence-sync-.patch>

I reviewed v12 again. 0001 looks good. A few comments on 0002 and 0003.

1 - 0002
```
+	/*
+	 * Setup callback for syscache so that we know when something changes in
+	 * the subscription relation state.
+	 */
+	CacheRegisterSyscacheCallback(SUBSCRIPTIONRELMAP,
+								  invalidate_syncing_sequence_infos,
+								  (Datum) 0);
```

I wonder if SUBSCRIPTIONRELMAP should be SUBSCRIPTIONREL?

2 - 0003
```
+		/*
+		 * Use the current memory context for synchronization. Since this should
+		 * be short-lived command context that will be cleaned up automatically,
+		 * we can simply assign it as the synchronization context.
+		 */
+		SequenceSyncContext = CurrentMemoryContext;
```

I think it’s still better to create a memory context from CurrentMemoryContext for SequenceSyncContext, and destroy it after copy_sequence.

Today, this is only on the SQL command path, CurrentMemoryContext is supposed to be short-lived. But AlterSubSyncSequences() might be called somewhere else in future, then we could not predict what would be CurrentMemoryContext.

3 - 0003
```
    "output the wanring for the missing sequence regress_s4”);
```

Typo: wanring -> warning

Best regards,

---

From: shveta malik <[email-masked]>
Date: Fri, 13 Mar 2026 16:11:15 +0530

> Thanks for the reviews. I've updated the patch set addressing all comments.

Thanks Hou-San. Please find my concerns on 001:

1)
Consider a case where the page LSN has changed and the sequence has
drifted, but the page LSN was not updated because the update interval
had not yet elapsed. Later, if there is no further drift for a couple
of minutes, we may continue invoking copy_sequence with update_lsn =
true. However, since check_seq_privileges_and_drift() keeps returning
no drift, the LSN might never get updated.

2)
Also, IIUC, we will end up advancing 'next_lsn_update' based on
'update_lsn' even though no actual lsn-update has occurred. As a
result, the next page LSN update may never happen if the update_lsn =
true cases always coincide with the no-drift case.

Shall copy_sequence() call UpdateSubscriptionRelState() even if there
is no drift but need_lsn_update is true?

thanks
Shveta

---

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Fri, 13 Mar 2026 11:35:41 +0000

Dear Hou,

Thanks for updating the patch. Comments for v12-0002.

01.
```
+       /* Free the existing invalid cache entries */
+       foreach_ptr(LogicalRepSequenceInfo, seqinfo, sequence_infos)
+       {
+               pfree(seqinfo->nspname);
+               pfree(seqinfo->seqname);
+               pfree(seqinfo);
+       }
```

According to the comment atop foreach_delete_current, we should not directly pfree()
the iterator.

02.
```
+               /* Cache the information in a permanent memory context */
+               oldctx = MemoryContextSwitchTo(CacheMemoryContext);
```

Do you have a reason to use CacheMemoryContext instead of ApplyContext?
According to the readme, the context can be used for the limited purpose, like catcache
and relcache. Not sure we can easily use it.

03.
I think seqinfo->found_on_pub must be set to false before doing copy_sequences() again.
Otherwise, sequences dropped on the publisher cannot be detected as the missing ones.
Or we may have to have another array to indicate it.
I found a below scenario.

There are 10 sequences on both instances, and sequencesync worker synchronizes once.
Now two of them are dropped on the publisher. In the next iteration by the worker,
it can find that only 8 sequeces are found on the publisher. Then it scans the
cache to check each found_on_pub in sequence_infos, but they were cached as true.
Thus sequencesync worker cannot report anything for missing ones.

Best regards,
Hayato Kuroda
FUJITSU LIMITED

---

From: "Zhijie Hou (Fujitsu)" <[email-masked]>
Date: Mon, 16 Mar 2026 06:58:52 +0000

Thanks for the comments.

After some off-list discussion with Amit, we agreed that further analysis is
needed, which means rescheduling this feature for the next release.

The main issue requiring analysis is how to reduce the impact of invalidations
that can occur once the sequence synchronization worker begins modifying
pg_subscription_rel regularly. The current patch updates pg_subscription_rel
only every 30 seconds, which seems acceptable for an initial version. However,
there are other potential approaches worth exploring, such as: 1) adding a
subscription option to let users control the update frequency, or 2) updating
the catalog only after modifying a specific number of sequences.

It would also be worthwhile to examine other modules for similar issues and
their solutions. For example, autoanalyze and autovacuum also modify the catalog
regularly. We should investigate whether they face the same invalidation
challenges and how those challenges are addressed.

Overall, we will continue working on this to improve the patch set, but will
schedule it for PG20.

Best Regards,
Hou zj

---

From: Ajin Cherian <[email-masked]>
Date: Thu, 26 Mar 2026 15:25:03 +1100

> Overall, we will continue working on this to improve the patch set, but will

Just rebasing the patch so that it doesn't break cfbot.

regards,
Ajin Cherian
Fujitsu Australia

---

From: Ajin Cherian <[email-masked]>
Date: Wed, 15 Apr 2026 20:41:37 +1000

Rebasing patch to keep cfbot happy.

regards,
Ajin Cherian
Fujitsu Australia