Re: sequencesync worker race with REFRESH SEQUENCES

On Mon, 13 Jul 2026 at 11:08, shveta malik <shveta(dot)malik(at)gmail(dot)com> wrote:
>
> On Sat, Jul 11, 2026 at 12:46 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:
> >
> > On Sat, 11 Jul 2026 at 10:31, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > >
> > > On Fri, Jul 10, 2026 at 10:22 AM Noah Misch <noah(at)leadboat(dot)com> 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

Regards,
Vignesh