Re: [BUG] wrong refresh when ALTER SUBSCRIPTION ADD/DROP PUBLICATION - Mailing list pgsql-hackers

On Fri, Aug 6, 2021 at 1:39 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Fri, Aug 6, 2021 at 5:09 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > On Thu, Aug 5, 2021 at 11:40 PM houzj.fnst@fujitsu.com
> > <houzj.fnst@fujitsu.com> wrote:
> > >
> > > > To summary, I think that what we want to do in DROP SUBSCRIPTION cases is to
> > > > drop relations from pg_subscription_rel that are no longer included in the set of
> > > > after-calculated publications. In the latter example, when ALTER
> > > > SUBSCRIPTION ... DROP PUBLICATION pub12, we should compare the current
> > > > set of relations associated with {pub12, pub13} to the set of relcations associated
> > > > with pub13, not pub12. Then we can find out t2 is no longer associated with the
> > > > after-calculated publication (i.g., pub13) so remove it.
> > >
> > > Thanks for the review. You are right, I missed this point.
> > > Attach new version patch which fix these problems(Also add a new pl-test).
> > >
> >
> > Thank you for updating the patch!
> >
> > Here are some comments:
> >
> > I think that it still could happen that DROP PULIBCATION misses
> > dropping relations. Please consider the following case:
> >
> > On publisher and subscriber:
> > create table t1 (a int);
> > create table t2 (a int);
> > create table t3 (a int);
> >
> > On publisher:
> > create publication pub12 for table t1, t2;
> > create publication pub3 for table t3;
> >
> > On subscriber:
> > create subscription sub connection 'dbname=postgres' publication pub12, pub3;
> >
> > On publisher:
> > alter publication pub3 add table t1;
> >
> > On subscriber:
> > alter subscription sub drop publication pub12;
> > select srsubid, srrelid::regclass::text, srsubstate, srsublsn from
> > pg_subscription_rel;
> >  srsubid | srrelid | srsubstate | srsublsn
> > ---------+---------+------------+-----------
> >    16393 | t3      | r          | 0/1707CE0
> >    16393 | t1      | r          | 0/1707D18
> >
> > With v3 patch, pg_subscription_rel has entries for t3 and t1. But if
> > ALTER SUBSCRIPTION ... DROP PUBLICATION pub12 refreshes only relations
> > associated with pub12, t1 should be removed and be added when we
> > refresh pub3.
> >
>
> But, isn't this happening because of your suggestion to compare the
> current set of relations with relations from publications that doesn't
> need to be removed?

Hmm yes, it cannot cover all cases. I had somehow misunderstood that
the subscriber knows which relations are associated with which
publications. Given that the subscriber doesn’t know which relations
are associated with which publications (and the set of subscribed
relations on the subscriber becomes out of date once the publication
is updated), I think it's impossible to achieve that DROP PUBLICATION
drops relations that are associated with only the publication being
dropped.

> Do we have better ideas to fix or shall we just
> say that we will refresh based on whatever current set of relations
> are present in publication to be dropped?

I don't have better ideas. It seems to me that it's prudent that we
accept the approach in v3 patch and refresh all publications in DROP
PUBLICATION cases.

>
> One more thing, I think it would be better to write a separate refresh
> function for add/drop rather than adding checks in the current refresh
> functionality. We can extract common functionality code which can be
> called from the different refresh functions.

+1

Regards,

--
Masahiko Sawada
EDB:  https://www.enterprisedb.com/