On Tue, Sep 22, 2026 at 11:29 PM vignesh C <[email protected]> wrote: > > On Tue, 22 Sept 2026 at 19:21, Zhijie Hou <[email protected]> wrote: > > > > > > Attached is v2, which takes a slightly different approach to the same > > > problem. Instead of stopping the sequence sync worker when a refresh > > > removes a sequence, copy_sequence() now checks whether the sequence is > > > still part of the subscription before updating it and skips it if it > > > is no longer subscribed. > > > > This approach makes sense to me. > > > > I didn't find any major issues in the patch, but I have a few questions: > > > > 1. > > > > + /* > > + * The sequence may no longer be part of the subscription. There is > > + * nothing left to synchronize, so leave the local sequence alone and let > > + * the caller skip it. > > + */ > > + if (GetSubscriptionRelState(MySubscription->oid, seqoid, > > + &statelsn) == SUBREL_STATE_UNKNOWN) > > > > Can we simply use SearchSysCacheExists2 to check for the subrel entry here ? > > Yes, it can be used. > > > 2. > > > > + rel = table_open(SubscriptionRelRelationId, RowExclusiveLock); > > ... > > + table_close(rel, NoLock); > > + table_close(rel, NoLock); > > + table_close(rel, NoLock); > > > > The patch adds 3 table_close calls in each return branch. Would it be > > possible > > to delay the table_open to just before UpdateSubscriptionRelState, so that > > only > > one close call is needed? > > Modified > > The attached v3 version patch has the changes for the same. >
I liked the patch's idea; I verified it, and it works well. I have no further comments. Thanks. Shveta
