MIME-Version: 1.0
References: <CAFPTHDZXX9WQ_X1ZfEvS248T+pKuk6SmCnXcvgPM059N1xPUfA@mail.gmail.com>
 <CAJpy0uDLUEjHHME8om1vAf6qkXCeRR6cBvkpK8yWBAC=T0ZFLA@mail.gmail.com>
 <CAFPTHDZg1JrunGgOj332hr+gUuH_Jm7skqPpYSvd-QE3yEdRDQ@mail.gmail.com>
 <CAJpy0uBz7MCSUkvFJD9ij65vBahNmY+bfCgdGKRqXovYs+K_TA@mail.gmail.com>
 <CAJpy0uDsuNqjWd-TmGBxqSS1rnVCJ3B8=SYrtxQ=Vs8kb71QFA@mail.gmail.com>
 <CAJpy0uAMWg3KcXtVBS7B0rnchLNrCCVYBByJCzAp=u5LERgtfA@mail.gmail.com> <CAFPTHDZwEhxhDAeqcPi0GuYN6xBs8gFXHOMUnbg3u2Xigcz4Zg@mail.gmail.com>
In-Reply-To: <CAFPTHDZwEhxhDAeqcPi0GuYN6xBs8gFXHOMUnbg3u2Xigcz4Zg@mail.gmail.com>
From: shveta malik <shveta.malik@gmail.com>
Date: Thu, 12 Feb 2026 15:32:56 +0530
Message-ID: <CAJpy0uCz8BxcBKAevcRw+ShrjWVUDoW6H9iPY+9kCaKiN9APpw@mail.gmail.com>
Subject: Re: [PATCH] Support automatic sequence replication
To: Ajin Cherian <itsajin@gmail.com>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>, 
	shveta malik <shveta.malik@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: <https://www.postgresql.org/message-id/CAJpy0uCz8BxcBKAevcRw%2BShrjWVUDoW6H9iPY%2B9kCaKiN9APpw%40mail.gmail.com>
Precedence: bulk

On Thu, Feb 12, 2026 at 2:54=E2=80=AFPM Ajin Cherian <itsajin@gmail.com> wr=
ote:
>
> On Fri, Feb 6, 2026 at 8:15=E2=80=AFPM shveta malik <shveta.malik@gmail.c=
om> wrote:
> >
> > On Thu, Feb 5, 2026 at 10:33=E2=80=AFAM Ajin Cherian <itsajin@gmail.com=
> wrote:
>
> > Thank You for the patch. I haven=E2=80=99t reviewed the details yet, bu=
t 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=E2=80=AFAM Peter Smith <smithpb2250@gmail.co=
m> wrote:
> >
> > Some review comments for v3-0001.
> >
> > =3D=3D=3D=3D=3D=3D
> > .../replication/logical/sequencesync.c
> >
> > copy_sequences:
> >
> > 1.
> > - if (sync_status =3D=3D COPYSEQ_SUCCESS)
> > +
> > + /*
> > + * For sequences in READY state, only sync if there's drift
> > + */
> > + if (relstate =3D=3D SUBREL_STATE_READY && sync_status =3D=3D COPYSEQ_=
SUCCESS)
> > + {
> > + should_sync =3D check_sequence_drift(seqinfo);
> > + if (should_sync)
> > + drift_detected =3D true;
> > + }
> > +
> > + if (sync_status =3D=3D COPYSEQ_SUCCESS && should_sync)
> >   sync_status =3D copy_sequence(seqinfo,
> > - sequence_rel->rd_rel->relowner);
> > + sequence_rel->rd_rel->relowner,
> > + relstate);
> > + else if (sync_status =3D=3D COPYSEQ_SUCCESS && !should_sync)
> > + sync_status =3D 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 =3D=3D COPYSEQ_SUCCESS)
> > {
> >   if ((relstate =3D=3D SUBREL_STATE_INIT) || check_sequence_drift(seqin=
fo))
> >     sync_status =3D copy_sequence(...);
> >   else
> >     sync_status =3D COPYSEQ_NOWORK;
> > }
>
> Changed as suggested.
>
>
>
> On Wed, Feb 11, 2026 at 2:30=E2=80=AFPM shveta malik <shveta.malik@gmail.=
com> wrote:
> >
> > On Fri, Feb 6, 2026 at 2:47=E2=80=AFPM shveta malik <shveta.malik@gmail=
.com> wrote:
> > >
> > Few more comments:
> >
> > 1)
> > + /* Run sync for sequences in READY state */
> > + sequence_copied |=3D LogicalRepSyncSequences(LogRepWorkerWalRcvConn,
> > SUBREL_STATE_READY);
> > +
> > + /* Call initial sync for sequences in INIT state */
> > + sequence_copied |=3D 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 R=
EADY.
> > 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 =3D 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 !=3D 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