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>
In-Reply-To: <CAJpy0uBz7MCSUkvFJD9ij65vBahNmY+bfCgdGKRqXovYs+K_TA@mail.gmail.com>
From: shveta malik <shveta.malik@gmail.com>
Date: Fri, 6 Feb 2026 14:47:26 +0530
Message-ID: <CAJpy0uDsuNqjWd-TmGBxqSS1rnVCJ3B8=SYrtxQ=Vs8kb71QFA@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/CAJpy0uDsuNqjWd-TmGBxqSS1rnVCJ3B8%3DSYrtxQ%3DVs8kb71QFA%40mail.gmail.com>
Precedence: bulk

On Fri, Feb 6, 2026 at 2:45=E2=80=AFPM shveta malik <shveta.malik@gmail.com=
> wrote:
>
> On Thu, Feb 5, 2026 at 10:33=E2=80=AFAM Ajin Cherian <itsajin@gmail.com> =
wrote:
> >
> > On Tue, Feb 3, 2026 at 9:22=E2=80=AFPM shveta malik <shveta.malik@gmail=
.com> wrote:
> > >
> > > On Tue, Feb 3, 2026 at 9:18=E2=80=AFAM Ajin Cherian <itsajin@gmail.co=
m> 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 whic=
h
> > > 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 =3D 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 use=
r
> > > 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=E2=80=99t 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.

thanks
Shveta