On Thursday, August 5, 2021 1:09 PM Masahiko Sawada <sawada.m...@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
v3-0001-fix-ALTER-SUB-ADD-DROP-PUBLICATION.patch
Description: v3-0001-fix-ALTER-SUB-ADD-DROP-PUBLICATION.patch
