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

On Wed, Aug 4, 2021 at 9:19 PM houzj.fnst@fujitsu.com
<houzj.fnst@fujitsu.com> wrote:
>
> On Wednesday, August 4, 2021 7:00 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote
> > On Wed, Aug 4, 2021 at 5:06 PM houzj.fnst@fujitsu.com <houzj.fnst@fujitsu.com> wrote:
> > >
> > > On Wednesday, August 4, 2021 1:47 PM Masahiko Sawada <sawada.mshk@gmail.com>
> > > >
> > > > I've not looked at the patch deeply yet but I think that the
> > > > following one line change seems to fix the cases you reported,
> > > > although I migit be missing
> > > > something:
> > > >
> > > > diff --git a/src/backend/commands/subscriptioncmds.c
> > > > b/src/backend/commands/subscriptioncmds.c
> > > > index 22ae982328..c74d6d51d6 100644
> > > > --- a/src/backend/commands/subscriptioncmds.c
> > > > +++ b/src/backend/commands/subscriptioncmds.c
> > > > @@ -1068,7 +1068,7 @@ AlterSubscription(ParseState *pstate,
> > > > AlterSubscriptionStmt *stmt,
> > > >                     PreventInTransactionBlock(isTopLevel, "ALTER
> > > > SUBSCRIPTION with refresh");
> > > >
> > > >                     /* Only refresh the added/dropped list of publications.
> > */
> > > > -                   sub->publications = stmt->publication;
> > > > +                   sub->publications = publist;
> > > >
> > > >                     AlterSubscription_refresh(sub, opts.copy_data);
> > > >                 }
> > >
> > > I considered the same fix before, but with the above fix, it seems
> > > refresh both existsing publications and new publication, which most of
> > > people didn't like in previous discussion[1]. From the doc[2], IIRC,
> > > Currently, the ADD/DROP PUBLICATION is supposed to only refresh the new
> > publication.
> >
> > What do you mean by refreshing both existing and new publications? I dropped
> > and created one publication out of three publications that the subscription is
> > subscribing to but the corresponding entries in pg_subscription_rel for tables
> > associated with the other two publications were not changed and the table sync
> > workers also were not started for them.
> >
>
> +                   sub->publications = publist;
> -                   sub->publications = stmt->publication;
> With the above fix, When ADD/DROP PUBLICATION, I meant the table that doesn't
> belong to the dropped or added publication could be updated in
> pg_subscription_rel.
>
> I can see the effect in the following example:
>
> 1)-publisher-
> CREATE TABLE test,test2,test3
> CREATE PUBLICATION pub FOR TABLE test;
> CREATE PUBLICATION pub2 FOR TABLE test2;
> 2)-subscriber-
> CREATE TABLE test,test2,test3
> CREATE SUBSCRIPTION sub CONNECTION 'dbname=postgres port=10000' PUBLICATION pub,pub2;
> select * from pg_subscription_rel;
> -[ RECORD 1 ]---------
> srsubid    | 16393
> srrelid    | 16387
> srsubstate | r
> srsublsn   | 0/150A2D8
> -[ RECORD 2 ]---------
> srsubid    | 16393
> srrelid    | 16384
> srsubstate | r
> srsublsn   | 0/150A310
>
> 3)-publisher-
> alter publication pub add table test3;
> 4)-subscriber-
> ALTER SUBSCRIPTION sub drop PUBLICATION pub2;
> select * from pg_subscription_rel;
> -[ RECORD 1 ]---------
> srsubid    | 16393
> srrelid    | 16384
> srsubstate | r
> srsublsn   | 0/150A310
> -[ RECORD 2 ]---------
> srsubid    | 16393
> srrelid    | 16390
> srsubstate | r
> srsublsn   |
>
> I can see the [RECORD 2] which is the new registered table in 'pub2' (see step 3)
> has been added to the pg_subscription_rel. Based pervious discussion and
> document, that table seems shouldn't be refreshed when drop another
> publication.

Thanks for the explanation! I understood the problem.

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.

Regards,

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