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>
 <CAE9k0PmTyCU1A9YEf+MRgfeZ8yK1bAYJu=o0bH8DNUTzXejQyQ@mail.gmail.com>
 <CAA4eK1L6czEzG4mLNZSyjYC5nX0FMSjjk3csKuxPD3Ph5-7Yvw@mail.gmail.com> <CAJpy0uAhGQJ=msVsn2GsqWXr+YESJK6x9NBvrUtKvtvp1OVuKQ@mail.gmail.com>
In-Reply-To: <CAJpy0uAhGQJ=msVsn2GsqWXr+YESJK6x9NBvrUtKvtvp1OVuKQ@mail.gmail.com>
From: shveta malik <shveta.malik@gmail.com>
Date: Wed, 18 Feb 2026 15:34:46 +0530
Message-ID: <CAJpy0uAOuu-M6wobH2wHOdTymm-cX9+MqwPyRNoOt=sPKBdCew@mail.gmail.com>
Subject: Re: [PATCH] Support automatic sequence replication
To: Ajin Cherian <itsajin@gmail.com>
Cc: Ashutosh Sharma <ashu.coek88@gmail.com>, 
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>, Amit Kapila <amit.kapila16@gmail.com>, 
	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/CAJpy0uAOuu-M6wobH2wHOdTymm-cX9%2BMqwPyRNoOt%3DsPKBdCew%40mail.gmail.com>
Precedence: bulk

Few comments:

1)
+ /* Check if any new sequences need syncing */
+ ProcessSequencesForSync();
+

Shall we name it 'maybe_start_seqsync_worker()' (or something better?)
and move it immediately after 'maybe_advance_nonremovable_xid()'.
Thoughts? This is because we do not process sequences here, we just
check if the subscription includes sequences and start worker.

2)
We can also change the comment atop ProcessSequencesForSync to mention
that. Currently it says:
/*
 * Apply worker determines if sequence synchronization is needed.
 *
 * Start a sequencesync worker if one is not already running.

Now, we shall change it to:
Apply worker determines whether a sequence sync worker is needed.

Check if the subscription includes sequences and start a sequencesync
worker if one is not already running.

3)
I noticed that copy_sequences is invoked twice per sync cycle=E2=80=94once =
for
sequences in the INIT state, and once for sequences in the READY state
to detect drift. This means we ping the primary twice during each sync
cycle. We should consider combining this logic into a single
copy_sequences call. Since we already have the state information
(INIT, READY, etc.) for each local sequence, copy_sequences can
internally check the current state and decide whether to transition a
sequence to READY based on its previous state. This approach would
allow us to fetch all required information from the primary in a
single call.

I also think that we do not even need to mention the states here and
we can give a single message instead of 2:
DEBUG:  logical replication sequence synchronization for subscription
"sub1" - for sequences in INIT state batch #1 =3D 5 attempted, 5
succeeded, 0 mismatched, 0 insufficient permission, 0 missing from
publisher, 0 skipped, 0 no drift
DEBUG:  logical replication sequence synchronization for subscription
"sub1" - for sequences in READY state batch #1 =3D 5 attempted, 0
succeeded, 0 mismatched, 0 insufficient permission, 0 missing from
publisher, 0 skipped, 5 no drift

4)
FetchRelationStates():
+ List    *seq_states;

It would be better to initialize it with NIL.

thanks
Shveta