Re: [BUG] wrong refresh when ALTER SUBSCRIPTION ADD/DROP PUBLICATION

On Wed, Aug 4, 2021 at 5:06 PM houzj(dot)fnst(at)fujitsu(dot)com
<houzj(dot)fnst(at)fujitsu(dot)com> wrote:
>
> On Wednesday, August 4, 2021 1:47 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com>
> > On Mon, Aug 2, 2021 at 10:52 PM houzj(dot)fnst(at)fujitsu(dot)com
> > <houzj(dot)fnst(at)fujitsu(dot)com> wrote:
> > >
> > >
> > > Hi hackers,
> > >
> > > When testing some other logical replication related patches, I found
> > > two unexpected behaviours about ALTER SUBSCRIPTION ADD/DROP
> > PUBLICATION.
> > >
> > > (1)
> > > when I execute the following sqls[1], the data of tables that
> > > registered to 'pub' wasn't copied to the subscriber side which means
> > > tablesync worker didn't start.
> > >
> > > -----sub originally had 2 pub nodes(pub,pub2) ALTER SUBSCRIPTION sub
> > > drop PUBLICATION pub; ALTER SUBSCRIPTION sub add PUBLICATION pub;
> > > -----
> > >
> > > (2)
> > > And when I execute the following sqls, the data of table registered to 'pub2'
> > > are synced again.
> > >
> > > -----sub originally had 2 pub nodes(pub,pub2) ALTER SUBSCRIPTION sub
> > > drop PUBLICATION pub; ALTER SUBSCRIPTION sub REFRESH PUBLICATION;
> > > -----
> >
> > I could reproduce this issue by the above steps.
>
> Thanks for looking into this problem.
>
> >
> > 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.

>
> > Also, I think we need to add some regression tests for this.
> > Currently, subscription.sql has tests for ADD/DROP PUBLICATION but they don't
> > check pg_subscription_rel.
>
> Currently, the subscription.sql doesn't have actual tables to
> be subscribed which means the pg_subscription_rel is empty. I am not sure where
> to place the testcases, temporarily placed in 001_rep_changes.pl.

Right. Adding tests to 001_rep_changes.pl seems good to me.

Regards,

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