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. Here's patch v4 addressing the above comments. regards, Ajin Cherian Fujitsu Australia
v4-0001-Support-automatic-sequence-replication.patch
Description: Binary data
