On Mon, Jul 13, 2026 at 4:38 PM vignesh C <[email protected]> wrote:
>
> On Mon, 13 Jul 2026 at 11:08, shveta malik <[email protected]> wrote:
> >
> > On Sat, Jul 11, 2026 at 12:46 PM vignesh C <[email protected]> wrote:
> > >
> > > On Sat, 11 Jul 2026 at 10:31, Amit Kapila <[email protected]> wrote:
> > > >
> > > > On Fri, Jul 10, 2026 at 10:22 AM Noah Misch <[email protected]> wrote:
> > > > >
> > > > > A Fable 5 review of logical replication of sequences found a way to
> > > > > get
> > > > > subscribed sequences into READY state despite the subscriber side
> > > > > having data
> > > > > older than the last REFRESH SEQUENCES. I'm attaching the test case
> > > > > it wrote.
> > > > > I reviewed the test, and I think it identifies a genuine defect.
> > > > >
> > > >
> > > > Good catch. We have following ways to fix: (a) As mentioned by
> > > > Kuroda-san, during REFRESH SEQUENCES command, if we detect that the
> > > > sequencesync worker is in progress, we can either make the command
> > > > wait till the sequencesync is finished, return ERROR suggesting
> > > > sequence sync already in-progress, or first stop the sequencesync
> > > > worker and then complete the command and let the worker restart after
> > > > REFRESH command is finished; (b) raise a WARNING+HINT for sequences
> > > > that are not in ready state as proposed by Vignesh. Shall we
> > > > additionally add a Note for user to ensure seuencesync worker is not
> > > > in-progress before REFRESH SEQUENCES command?
> > > >
> > > > Do you have any preference? I think WARNING+HINT should be sufficient
> > > > for users as this shouldn't be a common scenario but going the other
> > > > way is also fine.
> > >
> > > Both approaches seem reasonable to me. One downside of the WARNING
> > > approach is that if a subscription contains many sequences and the
> > > user immediately reruns ALTER SUBSCRIPTION ... REFRESH SEQUENCES, they
> > > could receive a large number of warnings one for each sequence that is
> > > already being synchronized which may be noisy and not particularly
> > > useful.
> > >
> > > Here is a patch implementing approach (a), which detects whether a
> > > sequence synchronization worker is already running for the
> > > subscription. If a synchronization is already in progress, ALTER
> > > SUBSCRIPTION ... REFRESH SEQUENCES reports an error and asks the user
> > > to rerun the command after the current synchronization completes.
> > >
> >
> > Please find my comments:
> >
> > 1)
> > + /*
> > + * Disallow a concurrent REFRESH SEQUENCES while a sequencesync worker
> > + * for this subscription is still synchronizing the sequences from a
> > + * previous REFRESH SEQUENCES. Without this check, this command would
> > + * reset the sequences' state back to INIT while the running worker is
> > + * midway through applying the values it already fetched from the
> > + * publisher, which could leave the sequences marked READY with stale
> > + * data.
> > + */
> >
> > The wording "reset the sequences' state back to INIT" is incorrect.
> > There will be no issue if REFRESH resets the state back to INIT (from
> > READY) as those will then be picked up in the next cycle. The problem
> > is that there is no reset haappening. I think we can improve this
> > comment.
> >
> > Suggestion:
> >
> > /*
> > * Disallow a concurrent REFRESH SEQUENCES while a sequence sync worker
> > * for this subscription is still running. This avoids a race where the
> > * publisher's sequence advances after the current worker has fetched its
> > * value but before it marks the sequence READY. A user may then issue
> > * another REFRESH SEQUENCES to synchronize the updated value. Since the
> > * affected sequences are already in the INIT state, the running worker
> > * has no indication that a new synchronization has been requested. It
> > * would then apply the stale value it already fetched and mark the
> > * sequence READY, causing the new synchronization request to be lost and
> > * preventing the updated publisher values from being synchronized.
> > */
>
> Modified
>
> > 2)
> > I am unsure if a testcase is really needed here as it is a very simple
> > fix. But I'd like to see what others think here.
> > If a test is needed, I can review it, currently I have skipped it.
>
> Even I feel a test case is not needed for this.
>
> The attached v2 version patch has the changes for the same.
> In addition, it includes a fix for Finding 2
> (default_transaction_read_only) reported by Noah at [1], following the
> approach suggested by Amit at [2].
> This version also addresses Findings 6 and 17 by reporting an error
> when ALTER SUBSCRIPTION ... REFRESH SEQUENCES is executed against
> subscriptions created prior to PostgreSQL 19, and documents this
> behavior accordingly.
>
> [1] -
> https://www.postgresql.org/message-id/20260710045217.f0.noahmisch%40microsoft.com
> [2] -
> https://www.postgresql.org/message-id/CAA4eK1K8LD243UzHgVNCm4skJZ4UCjR3vowDhKp%3DcWnK5oBT-Q%40mail.gmail.com
>
Thanks. A few comments:
001:
1)
+ if (walrcv_server_version(wrconn) < 190000)
+ ereport(ERROR,
+ errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("cannot synchronize sequences for subscription \"%s\" because
the publisher is running a version earlier than PostgreSQL 19",
+ sub->name));
+
I think the error can be independent of subscription name, we can give
a generic error, similar to:
"cannot enable retain_dead_tuples if the publisher is running a
version earlier than PostgreSQL 19"
2)
The fix will solve the purpose. But I have a doubt for the check in
copy_sequences(). Say a few sequences are already in the INIT state,
and the subscription is then repointed to a pre-PG19 publisher. Would
the apply worker keep launching the sequence sync worker, only for it
to exit each time with this error of copy_sequences? Is my
understanding correct? If so, is there a way to avoid launching the
sequence sync worker altogether when connected to a pre-PG19
publisher? We do have 'LogRepWorkerWalRcvConn' in the apply worker to
check the version.
002: looks good.
003: yet to review.
thanks
Shveta
