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

On Thursday, August 5, 2021 1:09 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote
> I've reviewed v2 patch. Here are some comments:
> 
> +           if (type == ALTER_SUBSCRIPTION_SET_PUBLICATION ||
> +               type == ALTER_SUBSCRIPTION_REFRESH)
> +               drop_table = !bsearch(&relid, pubrel_local_oids,
> +                                     list_length(pubrel_names),
> +                                     sizeof(Oid), oid_cmp);
> +           else if (type == ALTER_SUBSCRIPTION_DROP_PUBLICATION)
> +               drop_table = bsearch(&relid, pubrel_local_oids,
> +                                    list_length(pubrel_names),
> +                                    sizeof(Oid), oid_cmp);
> 
> IIUC, in DROP PUBLICATION cases, we don't need to refer to the tables in the
> publication on the publisher that we're dropping in order to check whether to
> remove the relation. If we drop a table from the publication on the publisher
> before dropping the publication from the subscription on the subscriber, the
> pg_subscription_rel entry for the table remains. For example, 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 pub12 drop table t2;
> 
> 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 | t2      | r          | 0/1707CE0
> 
> With v2 patch, the pg_subscription_rel has the entry for 't2' but it should not
> exist.
> 
> But that doesn't mean that drop_table should always be true in DROP
> PULIBCATION cases. Since the tables associated with two publications can
> overlap, we cannot remove the relation from pg_subscription_rel if it is also
> associated with the remaining publication. For example:
> 
> 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 pub13 for table t1, t3;
> 
> On subscriber:
> create subscription sub connection 'dbname=postgres' publication pub12,
> pub13;
> 
> 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/17080B0
> (1 row)
> 
> With v2 patch, the pg_subscription_rel has only one entry for t3 but it should
> have also the one for t1 since it's still subscribing to pub13.
> 
> 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).

Best regards,
Houzj