Re: sequencesync worker race with REFRESH SEQUENCES

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

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.

thanks
Shveta