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

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? 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?

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.

-- 
With Regards,
Amit Kapila.