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 Regards, Vignesh
v2-0002-Reject-concurrent-sequence-refreshes.patch
Description: Binary data
v2-0001-Reject-sequence-synchronization-against-pre-PG19-.patch
Description: Binary data
v2-0003-Allow-logical-replication-workers-to-ignore-defau.patch
Description: Binary data
