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

On Thu, Aug 5, 2021 at 2:08 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> 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:

Regarding regression tests, since ADD/DROP PUBLICATION logic could be
complicated it seems to me that we can add tests to a new file, say
alter_sub_pub.pl, that includes some scenarios as I mentioned in the
previous message.

Regards,

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