RE: Race condition in FetchTableStates() breaks synchronization of subscription tables

On Wednesday, April 24, 2024 6:29 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:
>
> On Wed, 24 Apr 2024 at 11:59, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> >
> > On Wed, Mar 13, 2024 at 9:19 AM vignesh C <vignesh21(at)gmail(dot)com> wrote:
> > >
> > > On Tue, 12 Mar 2024 at 09:34, Ajin Cherian <itsajin(at)gmail(dot)com> wrote:
> > > >
> > > >
> > > >
> > > > On Tue, Mar 12, 2024 at 2:59 PM vignesh C <vignesh21(at)gmail(dot)com>
> wrote:
> > > >>
> > > >>
> > > >> Thanks, I have created the following Commitfest entry for this:
> > > >> https://commitfest.postgresql.org/47/4816/
> > > >>
> > > >> Regards,
> > > >> Vignesh
> > > >
> > > >
> > > > Thanks for the patch, I have verified that the fix works well by following
> the steps mentioned to reproduce the problem.
> > > > Reviewing the patch, it seems good and is well documented. Just one
> minor comment I had was probably to change the name of the variable
> table_states_valid to table_states_validity. The current name made sense when
> it was a bool, but now that it is a tri-state enum, it doesn't fit well.
> > >
> > > Thanks for reviewing the patch, the attached v6 patch has the
> > > changes for the same.
> > >
> >
> > v6_0001* looks good to me. This should be backpatched unless you or
> > others think otherwise.
>
> This patch needs to be committed in master,PG16 and PG15.
> This is not required from PG14 onwards because they don't have
> HasSubscriptionRelations call before updating table_states_valid:
>     /*
>      * Does the subscription have tables?
>      *
>      * If there were not-READY relations found then we know it does. But
>      * if table_states_not_ready was empty we still need to check again to
>      * see if there are 0 tables.
>      */
>     has_subrels = (table_states_not_ready != NIL) ||
>       HasSubscriptionRelations(MySubscription->oid);
>
> So the invalidation function will not be called here.
>
> Whereas for PG15 and newer versions this is applicable:
> HasSubscriptionRelations calls table_open function which will get the
> invalidate callback like in:
> HasSubscriptionRelations -> table_open -> relation_open -> LockRelationOid
> -> AcceptInvalidationMessages -> ReceiveSharedInvalidMessages ->
> LocalExecuteInvalidationMessage -> CallSyscacheCallbacks ->
> invalidate_syncing_table_states
>
> The attached patch
> v7-0001-Table-sync-missed-due-to-race-condition-in-subscr.patch
> applies for master and PG16 branch,
> v7-0001-Table-sync-missed-due-to-race-condition-in-subscr_PG15.patch
> applies for PG15 branch.

Thanks, I have verified that the patches can be applied cleanly and fix the
issue on each branch. The regression test can also pass after applying the patch
on my machine.

Best Regards,
Hou zj