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

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.

[1] https://www.postgresql.org/message-id/MEYP282MB166921C8622675A5C0701C31B6BB0%40MEYP282MB1669.AUSP282.PROD.OUTLOOK.COM

[2] https://www.postgresql.org/docs/14/sql-altersubscription.html
By default, this command will also act like REFRESH PUBLICATION, except that in
case of ADD or DROP, only the added or dropped publications are refreshed.

> 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.

Best regards,
houzj