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>
In-Reply-To: <CAJpy0uDsuNqjWd-TmGBxqSS1rnVCJ3B8=SYrtxQ=Vs8kb71QFA@mail.gmail.com>
From: shveta malik <shveta.malik@gmail.com>
Date: Wed, 11 Feb 2026 09:00:02 +0530
Message-ID: <CAJpy0uAMWg3KcXtVBS7B0rnchLNrCCVYBByJCzAp=u5LERgtfA@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/CAJpy0uAMWg3KcXtVBS7B0rnchLNrCCVYBByJCzAp%3Du5LERgtfA%40mail.gmail.com>
Precedence: bulk

On Fri, Feb 6, 2026 at 2:47=E2=80=AFPM shveta malik <shveta.malik@gmail.com=
> wrote:
>
> On Fri, Feb 6, 2026 at 2:45=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:
> > >
> > > On Tue, Feb 3, 2026 at 9:22=E2=80=AFPM shveta malik <shveta.malik@gma=
il.com> wrote:
> > > >
> > > > On Tue, Feb 3, 2026 at 9:18=E2=80=AFAM Ajin Cherian <itsajin@gmail.=
com> wrote:
> > > > Thanks for the patch.
> > > >
> > > > +1 for the overall idea of patch that once a subscription is create=
d
> > > > which subscribes to sequences, a sequence sync worker is started wh=
ich
> > > > continuously syncs the sequences. This makes usage of REFRESH
> > > > SEQUENCES redundant and thus it is removed. I am still reviewing th=
e
> > > > 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 appl=
y
> > > > worker unless there is a sequence in INIT state i.e. synchronizatio=
n
> > > > 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 sequence=
s
> > > 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 "sub=
1"
> > > > - total unsynchronized: 3"
> > > >
> > >
> > > Removed this.
> > >
> > > > b)
> > > > When there is a drift in say single sequence, it puts rest (which a=
re
> > > > in sync) to "missing" section:
> > > > "logical replication sequence synchronization for subscription "sub=
1"
> > > > - batch #1 =3D 3 attempted, 1 succeeded, 0 mismatched, 0 insufficie=
nt
> > > > 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 u=
ser
> > > > 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, 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?
>
> 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.
>

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 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.

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))));

Do we need this check? IIUC, sequence_rel is opened using
RowExclusiveLock in get_and_validate_seq_info() and thus should not
hit this case so that it becomes non-sequence later on. If required,
we can have Assert.

I can review further once these and previous comments are addressed.

thanks
Shveta