On Thu, Feb 12, 2026 at 2:54 PM Ajin Cherian <[email protected]> wrote: > > On Fri, Feb 6, 2026 at 8:15 PM shveta malik <[email protected]> wrote: > > > > On Thu, Feb 5, 2026 at 10:33 AM Ajin Cherian <[email protected]> wrote: > > > 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? > > > > I've moved this to LogicalRepApplyLoop() > > > > On Mon, Feb 9, 2026 at 10:55 AM Peter Smith <[email protected]> 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. > > > > On Wed, Feb 11, 2026 at 2:30 PM shveta malik <[email protected]> wrote: > > > > On Fri, Feb 6, 2026 at 2:47 PM shveta malik <[email protected]> wrote: > > > > > 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. > > > > Changed as suggested. > > > 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)))); > > > > 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.
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
