On Fri, Feb 6, 2026 at 2:47 PM shveta malik <[email protected]> wrote:
>
> On Fri, Feb 6, 2026 at 2:45 PM shveta malik <[email protected]> wrote:
> >
> > On Thu, Feb 5, 2026 at 10:33 AM Ajin Cherian <[email protected]> wrote:
> > >
> > > On Tue, Feb 3, 2026 at 9:22 PM shveta malik <[email protected]>
> > > wrote:
> > > >
> > > > On Tue, Feb 3, 2026 at 9:18 AM Ajin Cherian <[email protected]> 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?
>
> 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.
>
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
