Re: [PATCH] Support automatic sequence replication

On Fri, Feb 6, 2026 at 8:15 PM shveta malik <shveta(dot)malik(at)gmail(dot)com> wrote:
>
> On Thu, Feb 5, 2026 at 10:33 AM Ajin Cherian <itsajin(at)gmail(dot)com> 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 <smithpb2250(at)gmail(dot)com> 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 <shveta(dot)malik(at)gmail(dot)com> wrote:
>
> On Fri, Feb 6, 2026 at 2:47 PM shveta malik <shveta(dot)malik(at)gmail(dot)com> 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