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

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

After looking into this problem, I think the reason is the
[alter sub add/drop publication] misused the function
AlterSubscription_refresh().

For DROP cases:

Currently, in function AlterSubscription_refresh(), it will first fetch the
target tables from the target publication, and also fetch the tables in
subscriber side from pg_subscription_rel. Then it will check each table from
local pg_subscription_rel, if the table does not exists in the tables fetched
from the target publication then drop it.

The logic above only works for SET PUBLICATION. However, When DROP PUBLICATION,
the tables fetched from target publication is actually the tables that need to
be dropped. If reuse the above logic, it will drop the wrong table which result
in unexpected behavioud in (1) and (2).(ADD PUBLICATION have similar problem).

So, I think we'd better fix this problem. I tried add some additional check in
AlterSubscription_refresh() which can avoid the problem like the attached
patch. Not sure do we need to further refactor.

Best regards,
houzj

Hi,

On Mon, Aug 2, 2021 at 10:52 PM houzj.fnst@fujitsu.com
<houzj.fnst@fujitsu.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.

>
> After looking into this problem, I think the reason is the
> [alter sub add/drop publication] misused the function
> AlterSubscription_refresh().
>
> For DROP cases:
>
> Currently, in function AlterSubscription_refresh(), it will first fetch the
> target tables from the target publication, and also fetch the tables in
> subscriber side from pg_subscription_rel. Then it will check each table from
> local pg_subscription_rel, if the table does not exists in the tables fetched
> from the target publication then drop it.
>
> The logic above only works for SET PUBLICATION. However, When DROP PUBLICATION,
> the tables fetched from target publication is actually the tables that need to
> be dropped. If reuse the above logic, it will drop the wrong table which result
> in unexpected behavioud in (1) and (2).(ADD PUBLICATION have similar problem).

Yes. For example, suppose that the publisher has two publications pub1
and pub2 for table1 and table2, respecitively. And we create a
subscription subscribing to those two publications.

postgres(1:74454)=# \dRs sub
          List of subscriptions
 Name |  Owner   | Enabled | Publication
------+----------+---------+-------------
 sub  | masahiko | t       | {pub1,pub2}
(1 row)

postgres(1:74454)=# select srsubid, srrelid::regclass::text,
srsubstate, srsublsn from pg_subscription_rel;
 srsubid | srrelid | srsubstate | srsublsn
---------+---------+------------+-----------
   16394 | table1  | r          | 0/170F388
   16394 | table2  | r          | 0/170F388
(2 rows)

After dropping pub1 from the subscription, 'table2' associated with
'pub2' is removed:

postgres(1:74454)=# alter subscription sub drop publication pub1;
ALTER SUBSCRIPTION
postgres(1:74454)=# select srsubid, srrelid::regclass::text,
srsubstate, srsublsn from pg_subscription_rel;
 srsubid | srrelid | srsubstate | srsublsn
---------+---------+------------+-----------
   16394 | table1  | r          | 0/170F388
(1 row)

> So, I think we'd better fix this problem. I tried add some additional check in
> AlterSubscription_refresh() which can avoid the problem like the attached
> patch. Not sure do we need to further refactor.

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);
                }

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.

Regards,

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

On Wednesday, August 4, 2021 1:47 PM Masahiko Sawada <sawada.mshk@gmail.com>
> On Mon, Aug 2, 2021 at 10:52 PM houzj.fnst@fujitsu.com
> <houzj.fnst@fujitsu.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

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>
> > On Mon, Aug 2, 2021 at 10:52 PM houzj.fnst@fujitsu.com
> > <houzj.fnst@fujitsu.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/

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.

Best regards,
houzj

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/

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/

On Thursday, August 5, 2021 1:09 PM Masahiko Sawada <sawada.mshk@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

On Thu, Aug 5, 2021 at 11:40 PM houzj.fnst@fujitsu.com
<houzj.fnst@fujitsu.com> wrote:
>
> On Thursday, August 5, 2021 1:09 PM Masahiko Sawada <sawada.mshk@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).
>

Thank you for updating the patch!

Here are some comments:

I think that it still could happen that DROP PULIBCATION misses
dropping relations. 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 pub3 add table t1;

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 | t1      | r          | 0/1707D18

With v3 patch, pg_subscription_rel has entries for t3 and t1. But if
ALTER SUBSCRIPTION ... DROP PUBLICATION pub12 refreshes only relations
associated with pub12, t1 should be removed and be added when we
refresh pub3.

---
-AlterSubscription_refresh(Subscription *sub, bool copy_data)
+AlterSubscription_refresh(Subscription *sub, bool copy_data,
+                         AlterSubscriptionType type)

I think it's better to add comments to this function. The meaning of
*sub varies depending on 'type'. For example, only in ADD PUBLICATION
cases, 'sub' has only newly-added publications to this function. In
other cases, 'sub' has a new set of publications.

---
+           if (type != ALTER_SUBSCRIPTION_DROP_PUBLICATION)
            {
-               AddSubscriptionRelState(sub->oid, relid,
-                                       copy_data ? SUBREL_STATE_INIT
: SUBREL_STATE_READY,
-                                       InvalidXLogRecPtr);
-               ereport(DEBUG1,
-                       (errmsg_internal("table \"%s.%s\" added to
subscription \"%s\"",
-                                        rv->schemaname, rv->relname,
sub->name)));
+               /* Check for supported relkind. */
+               CheckSubscriptionRelkind(get_rel_relkind(relid),
+                                       rv->schemaname, rv->relname);

I think CheckSubscriptionRelkind() is necessary even in
non-DROP-PUBLICATION cases.

---
+       if (type != ALTER_SUBSCRIPTION_ADD_PUBLICATION)
+       {
+           qsort(pubrel_local_oids, list_length(pubrel_names),
+                 sizeof(Oid), oid_cmp);
+           ntables_to_drop = list_length(subrel_states);
+       }

I think we can break here in ADD PUBLICATION cases, which seems more
readable to me. That way, we don't need ntables_to_drop. Also, we
palloc the memory for sub_remove_rels even in ADD PUBLICATION cases
but I think we don't need to do that.

---
+# Create tables on publisher
+$node_publisher->safe_psql('postgres', "CREATE TABLE temp1 (a int)");
+$node_publisher->safe_psql('postgres', "CREATE TABLE temp2 (a int)");
+$node_publisher->safe_psql('postgres', "CREATE TABLE temp3 (a int)");
+
+# Create tables on subscriber
+$node_subscriber->safe_psql('postgres', "CREATE TABLE temp1 (a int)");
+$node_subscriber->safe_psql('postgres', "CREATE TABLE temp2 (a int)");
+$node_subscriber->safe_psql('postgres', "CREATE TABLE temp3 (a int)");

Can we execute multiple statements in one safe_psql?

---
+# check initial pg_subscription_rel
+my $result = $node_subscriber->safe_psql('postgres',
+ "SELECT count(*) FROM pg_subscription_rel");
+is($result, qq(3),
+ 'three relation was subscribed');

s/three relation/three relations/

---
+$result = $node_subscriber->safe_psql('postgres',
+ "SELECT count(*) FROM pg_subscription_rel");
+
+is($result, qq(2),
+ 'check one relation was removed from subscribed list');
+
+$result = $node_subscriber->safe_psql('postgres',
+ "SELECT count(*) FROM pg_subscription_rel where
srrelid::regclass::text = 'temp1'");
+is($result, qq(0),
+ 'check relation temp1 was removed from subscribed list');

At some places, the test checks the number of total tuples and then
does an existence check. I think we can check the contents of
pg_subscription_rel instead. For example, in the above checks, we can
check if pg_subscription_rel has entries for temp2 and temp3.

Regards,

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

On Fri, Aug 6, 2021 at 5:09 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Thu, Aug 5, 2021 at 11:40 PM houzj.fnst@fujitsu.com
> <houzj.fnst@fujitsu.com> wrote:
> >
> > > 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).
> >
>
> Thank you for updating the patch!
>
> Here are some comments:
>
> I think that it still could happen that DROP PULIBCATION misses
> dropping relations. 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 pub3 add table t1;
>
> 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 | t1      | r          | 0/1707D18
>
> With v3 patch, pg_subscription_rel has entries for t3 and t1. But if
> ALTER SUBSCRIPTION ... DROP PUBLICATION pub12 refreshes only relations
> associated with pub12, t1 should be removed and be added when we
> refresh pub3.
>

But, isn't this happening because of your suggestion to compare the
current set of relations with relations from publications that doesn't
need to be removed? Do we have better ideas to fix or shall we just
say that we will refresh based on whatever current set of relations
are present in publication to be dropped?

One more thing, I think it would be better to write a separate refresh
function for add/drop rather than adding checks in the current refresh
functionality. We can extract common functionality code which can be
called from the different refresh functions.

-- 
With Regards,
Amit Kapila.

On Fri, Aug 6, 2021 at 10:09 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Fri, Aug 6, 2021 at 5:09 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
>
> But, isn't this happening because of your suggestion to compare the
> current set of relations with relations from publications that doesn't
> need to be removed? Do we have better ideas to fix or shall we just
> say that we will refresh based on whatever current set of relations
> are present in publication to be dropped?
>

The other options could be that (a) for drop publication, we refresh
all the publications, (b) for both add/drop publication, we refresh
all the publications.

Any better ideas?

As this is introduced in PG-14 via the below commit, I am adding
authors and committer to see if they have any better ideas.

commit 82ed7748b710e3ddce3f7ebc74af80fe4869492f
Author: Peter Eisentraut <peter@eisentraut.org>
Date:   Tue Apr 6 10:44:26 2021 +0200

    ALTER SUBSCRIPTION ... ADD/DROP PUBLICATION

 Author: Japin Li <japinli@hotmail.com>
 Author: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>


-- 
With Regards,
Amit Kapila.

On Fri, Aug 6, 2021 at 1:39 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Fri, Aug 6, 2021 at 5:09 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > On Thu, Aug 5, 2021 at 11:40 PM houzj.fnst@fujitsu.com
> > <houzj.fnst@fujitsu.com> wrote:
> > >
> > > > 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).
> > >
> >
> > Thank you for updating the patch!
> >
> > Here are some comments:
> >
> > I think that it still could happen that DROP PULIBCATION misses
> > dropping relations. 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 pub3 add table t1;
> >
> > 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 | t1      | r          | 0/1707D18
> >
> > With v3 patch, pg_subscription_rel has entries for t3 and t1. But if
> > ALTER SUBSCRIPTION ... DROP PUBLICATION pub12 refreshes only relations
> > associated with pub12, t1 should be removed and be added when we
> > refresh pub3.
> >
>
> But, isn't this happening because of your suggestion to compare the
> current set of relations with relations from publications that doesn't
> need to be removed?

Hmm yes, it cannot cover all cases. I had somehow misunderstood that
the subscriber knows which relations are associated with which
publications. Given that the subscriber doesn’t know which relations
are associated with which publications (and the set of subscribed
relations on the subscriber becomes out of date once the publication
is updated), I think it's impossible to achieve that DROP PUBLICATION
drops relations that are associated with only the publication being
dropped.

> Do we have better ideas to fix or shall we just
> say that we will refresh based on whatever current set of relations
> are present in publication to be dropped?

I don't have better ideas. It seems to me that it's prudent that we
accept the approach in v3 patch and refresh all publications in DROP
PUBLICATION cases.

>
> One more thing, I think it would be better to write a separate refresh
> function for add/drop rather than adding checks in the current refresh
> functionality. We can extract common functionality code which can be
> called from the different refresh functions.

+1

Regards,

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

Hi,

Sorry for the late reply.  Having read this thread, the problem is caused by
misunderstanding the AlterSubscription_refresh().  My apologies.

On Fri, 06 Aug 2021 at 14:12, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> On Fri, Aug 6, 2021 at 1:39 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>>
>> On Fri, Aug 6, 2021 at 5:09 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>> >
>> > On Thu, Aug 5, 2021 at 11:40 PM houzj.fnst@fujitsu.com
>> > <houzj.fnst@fujitsu.com> wrote:
>> > >
>> > > > 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).
>> > >
>> >
>> > Thank you for updating the patch!
>> >
>> > Here are some comments:
>> >
>> > I think that it still could happen that DROP PULIBCATION misses
>> > dropping relations. 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 pub3 add table t1;
>> >
>> > 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 | t1      | r          | 0/1707D18
>> >
>> > With v3 patch, pg_subscription_rel has entries for t3 and t1. But if
>> > ALTER SUBSCRIPTION ... DROP PUBLICATION pub12 refreshes only relations
>> > associated with pub12, t1 should be removed and be added when we
>> > refresh pub3.
>> >
>>
>> But, isn't this happening because of your suggestion to compare the
>> current set of relations with relations from publications that doesn't
>> need to be removed?
>
> Hmm yes, it cannot cover all cases. I had somehow misunderstood that
> the subscriber knows which relations are associated with which
> publications. Given that the subscriber doesn’t know which relations
> are associated with which publications (and the set of subscribed
> relations on the subscriber becomes out of date once the publication
> is updated), I think it's impossible to achieve that DROP PUBLICATION
> drops relations that are associated with only the publication being
> dropped.
>
>> Do we have better ideas to fix or shall we just
>> say that we will refresh based on whatever current set of relations
>> are present in publication to be dropped?
>
> I don't have better ideas. It seems to me that it's prudent that we
> accept the approach in v3 patch and refresh all publications in DROP
> PUBLICATION cases.
>

Maybe refreshing all publications in PUBLICATION cases is simple way to
fix the problem.

>>
>> One more thing, I think it would be better to write a separate refresh
>> function for add/drop rather than adding checks in the current refresh
>> functionality. We can extract common functionality code which can be
>> called from the different refresh functions.
>
> +1
>

Agreed.

> Regards,


--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.

On Fri, Aug 6, 2021 at 2:50 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Fri, Aug 6, 2021 at 10:09 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Fri, Aug 6, 2021 at 5:09 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > >
> >
> > But, isn't this happening because of your suggestion to compare the
> > current set of relations with relations from publications that doesn't
> > need to be removed? Do we have better ideas to fix or shall we just
> > say that we will refresh based on whatever current set of relations
> > are present in publication to be dropped?
> >
>
> The other options could be that (a) for drop publication, we refresh
> all the publications, (b) for both add/drop publication, we refresh
> all the publications.
>
> Any better ideas?
>
> As this is introduced in PG-14 via the below commit, I am adding
> authors and committer to see if they have any better ideas.
>
> commit 82ed7748b710e3ddce3f7ebc74af80fe4869492f
> Author: Peter Eisentraut <peter@eisentraut.org>
> Date:   Tue Apr 6 10:44:26 2021 +0200
>
>     ALTER SUBSCRIPTION ... ADD/DROP PUBLICATION
>
>  Author: Japin Li <japinli@hotmail.com>
>  Author: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
>

I've added this to Open Items.

Regards,

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

On Fri, Aug 6, 2021 at 9:57 PM Japin Li <japinli@hotmail.com> wrote:
>
> >
> > Hmm yes, it cannot cover all cases. I had somehow misunderstood that
> > the subscriber knows which relations are associated with which
> > publications. Given that the subscriber doesn’t know which relations
> > are associated with which publications (and the set of subscribed
> > relations on the subscriber becomes out of date once the publication
> > is updated), I think it's impossible to achieve that DROP PUBLICATION
> > drops relations that are associated with only the publication being
> > dropped.
> >
> >> Do we have better ideas to fix or shall we just
> >> say that we will refresh based on whatever current set of relations
> >> are present in publication to be dropped?
> >
> > I don't have better ideas. It seems to me that it's prudent that we
> > accept the approach in v3 patch and refresh all publications in DROP
> > PUBLICATION cases.
> >
>
> Maybe refreshing all publications in PUBLICATION cases is simple way to
> fix the problem.
>

Do you mean to say that do it for both Add and Drop or just for Drop?
Actually, doing it both will make the behavior consistent but doing it
just for Drop might be preferable by some users. I think it is better
to be consistent here but I am fine if you and others feel it is
better to refresh all publications only in Drop case.

--
With Regards,
Amit Kapila.

> On Aug 7, 2021, at 1:35 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> 
> On Fri, Aug 6, 2021 at 9:57 PM Japin Li <japinli@hotmail.com> wrote:
>> 
>>> 
>>> Hmm yes, it cannot cover all cases. I had somehow misunderstood that
>>> the subscriber knows which relations are associated with which
>>> publications. Given that the subscriber doesn’t know which relations
>>> are associated with which publications (and the set of subscribed
>>> relations on the subscriber becomes out of date once the publication
>>> is updated), I think it's impossible to achieve that DROP PUBLICATION
>>> drops relations that are associated with only the publication being
>>> dropped.
>>> 
>>>> Do we have better ideas to fix or shall we just
>>>> say that we will refresh based on whatever current set of relations
>>>> are present in publication to be dropped?
>>> 
>>> I don't have better ideas. It seems to me that it's prudent that we
>>> accept the approach in v3 patch and refresh all publications in DROP
>>> PUBLICATION cases.
>>> 
>> 
>> Maybe refreshing all publications in PUBLICATION cases is simple way to
>> fix the problem.
>> 
> 
> Do you mean to say that do it for both Add and Drop or just for Drop?
> Actually, doing it both will make the behavior consistent but doing it
> just for Drop might be preferable by some users. I think it is better
> to be consistent here but I am fine if you and others feel it is
> better to refresh all publications only in Drop case.
> 

I prefer to refresh all PUBLICATIONS for both ADD and DROP operations, I think
this is more consistent and makes the code simple.

--
Best regards
Japin Li

On Sat, Aug 7, 2021 1:36 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Fri, Aug 6, 2021 at 9:57 PM Japin Li <japinli@hotmail.com> wrote:
> >
> > >
> > > Hmm yes, it cannot cover all cases. I had somehow misunderstood that
> > > the subscriber knows which relations are associated with which
> > > publications. Given that the subscriber doesn’t know which relations
> > > are associated with which publications (and the set of subscribed
> > > relations on the subscriber becomes out of date once the publication
> > > is updated), I think it's impossible to achieve that DROP
> > > PUBLICATION drops relations that are associated with only the
> > > publication being dropped.
> > >
> > >> Do we have better ideas to fix or shall we just say that we will
> > >> refresh based on whatever current set of relations are present in
> > >> publication to be dropped?
> > >
> > > I don't have better ideas. It seems to me that it's prudent that we
> > > accept the approach in v3 patch and refresh all publications in DROP
> > > PUBLICATION cases.
> > >
> >
> > Maybe refreshing all publications in PUBLICATION cases is simple way
> > to fix the problem.
> >
> 
> Do you mean to say that do it for both Add and Drop or just for Drop?
> Actually, doing it both will make the behavior consistent but doing it just for
> Drop might be preferable by some users. I think it is better to be consistent here
> but I am fine if you and others feel it is better to refresh all publications only in
> Drop case.

Personally, I also think it will be better to make the behavior consistent.
Attach the new version patch make both ADD and DROP behave the same as SET PUBLICATION
which refresh all the publications.

Best regards,
houzj

On Sat, Aug 7, 2021 at 6:53 PM houzj.fnst@fujitsu.com
<houzj.fnst@fujitsu.com> wrote:
>
> On Sat, Aug 7, 2021 1:36 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > On Fri, Aug 6, 2021 at 9:57 PM Japin Li <japinli@hotmail.com> wrote:
> >
> > Do you mean to say that do it for both Add and Drop or just for Drop?
> > Actually, doing it both will make the behavior consistent but doing it just for
> > Drop might be preferable by some users. I think it is better to be consistent here
> > but I am fine if you and others feel it is better to refresh all publications only in
> > Drop case.
>
> Personally, I also think it will be better to make the behavior consistent.
> Attach the new version patch make both ADD and DROP behave the same as SET PUBLICATION
> which refresh all the publications.
>

There is a bug reported on pgsql-bugs [1] which seems to be happening
due to the same reason. Can you please test that the other bug is also
fixed with your patch?

[1] - https://www.postgresql.org/message-id/17132-6a702189086c6243%40postgresql.org

-- 
With Regards,
Amit Kapila.

On Monday, August 9, 2021 11:10 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> 
> On Sat, Aug 7, 2021 at 6:53 PM houzj.fnst@fujitsu.com <houzj.fnst@fujitsu.com>
> wrote:
> >
> > On Sat, Aug 7, 2021 1:36 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > > On Fri, Aug 6, 2021 at 9:57 PM Japin Li <japinli@hotmail.com> wrote:
> > >
> > > Do you mean to say that do it for both Add and Drop or just for Drop?
> > > Actually, doing it both will make the behavior consistent but doing
> > > it just for Drop might be preferable by some users. I think it is
> > > better to be consistent here but I am fine if you and others feel it
> > > is better to refresh all publications only in Drop case.
> >
> > Personally, I also think it will be better to make the behavior consistent.
> > Attach the new version patch make both ADD and DROP behave the same as
> > SET PUBLICATION which refresh all the publications.
> >
> 
> There is a bug reported on pgsql-bugs [1] which seems to be happening due to
> the same reason. Can you please test that the other bug is also fixed with your
> patch?
> 
> [1] -
> https://www.postgresql.org/message-id/17132-6a702189086c6243%40postgres
> ql.org

Thanks for the reminder, I have checked and tested the reported bug.
The reported bug is that when drop and then re-add a publication on subscriber side,
the table in the publication wasn't synced. And after applying the patch here, the table
will be synced as expected if re-added(behaves the same as SET PUBLICATION).

Best regards,
Hou zj

On Sat, Aug 7, 2021 at 2:36 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Fri, Aug 6, 2021 at 9:57 PM Japin Li <japinli@hotmail.com> wrote:
> >
> > >
> > > Hmm yes, it cannot cover all cases. I had somehow misunderstood that
> > > the subscriber knows which relations are associated with which
> > > publications. Given that the subscriber doesn’t know which relations
> > > are associated with which publications (and the set of subscribed
> > > relations on the subscriber becomes out of date once the publication
> > > is updated), I think it's impossible to achieve that DROP PUBLICATION
> > > drops relations that are associated with only the publication being
> > > dropped.
> > >
> > >> Do we have better ideas to fix or shall we just
> > >> say that we will refresh based on whatever current set of relations
> > >> are present in publication to be dropped?
> > >
> > > I don't have better ideas. It seems to me that it's prudent that we
> > > accept the approach in v3 patch and refresh all publications in DROP
> > > PUBLICATION cases.
> > >
> >
> > Maybe refreshing all publications in PUBLICATION cases is simple way to
> > fix the problem.
> >
>
> Actually, doing it both will make the behavior consistent but doing it
> just for Drop might be preferable by some users.

I agree that ADD/DROP PUBLICATION is convenient to just add/drop
publications instead of providing the complete publication list. But
I'm concerned that during developing this feature most people didn't
like the behavior of refreshing new and existing publications. Also,
there was pointing out that there will be an overhead of a SQL with
more publications when AlterSubscription_refresh() is called with all
the existing publications. If we feel this behavior is unnatural, the
users will feel the same. I personally think there is a benefit only
in terms of syntax. And we can work on the part of refreshing only
publications being added/dropped on v15 development. But it might be
better to consider also if there will be use cases for users to
add/remove publications in the subscription even with such downsides.

Regards,

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

On Tue, Aug 10, 2021 at 8:05 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Sat, Aug 7, 2021 at 2:36 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Fri, Aug 6, 2021 at 9:57 PM Japin Li <japinli@hotmail.com> wrote:
> > >
> > > >
> > > > Hmm yes, it cannot cover all cases. I had somehow misunderstood that
> > > > the subscriber knows which relations are associated with which
> > > > publications. Given that the subscriber doesn’t know which relations
> > > > are associated with which publications (and the set of subscribed
> > > > relations on the subscriber becomes out of date once the publication
> > > > is updated), I think it's impossible to achieve that DROP PUBLICATION
> > > > drops relations that are associated with only the publication being
> > > > dropped.
> > > >
> > > >> Do we have better ideas to fix or shall we just
> > > >> say that we will refresh based on whatever current set of relations
> > > >> are present in publication to be dropped?
> > > >
> > > > I don't have better ideas. It seems to me that it's prudent that we
> > > > accept the approach in v3 patch and refresh all publications in DROP
> > > > PUBLICATION cases.
> > > >
> > >
> > > Maybe refreshing all publications in PUBLICATION cases is simple way to
> > > fix the problem.
> > >
> >
> > Actually, doing it both will make the behavior consistent but doing it
> > just for Drop might be preferable by some users.
>
> I agree that ADD/DROP PUBLICATION is convenient to just add/drop
> publications instead of providing the complete publication list. But
> I'm concerned that during developing this feature most people didn't
> like the behavior of refreshing new and existing publications. Also,
> there was pointing out that there will be an overhead of a SQL with
> more publications when AlterSubscription_refresh() is called with all
> the existing publications.
>

True, but I think at that time we didn't know this design problem with
'drop publication'.

> If we feel this behavior is unnatural, the
> users will feel the same. I personally think there is a benefit only
> in terms of syntax.
>

Right, and I think in this particular case, the new syntax is much
easier to use for users.

> And we can work on the part of refreshing only
> publications being added/dropped on v15 development.
>

Yeah, unless we change design drastically we might not be able to do a
refresh for dropped publications, for add it is possible. It seems
most of the people responded on this thread that we can be consistent
in terms of refreshing for add/drop at this stage but we can still go
with another option where we can refresh only newly added publications
for add but for drop refresh all publications.

Peter E., do you have any opinion on this matter?

--
With Regards,
Amit Kapila.

On 10.08.21 05:22, Amit Kapila wrote:
> Yeah, unless we change design drastically we might not be able to do a
> refresh for dropped publications, for add it is possible. It seems
> most of the people responded on this thread that we can be consistent
> in terms of refreshing for add/drop at this stage but we can still go
> with another option where we can refresh only newly added publications
> for add but for drop refresh all publications.
> 
> Peter E., do you have any opinion on this matter?

Refresh everything for both ADD and DROP makes sense to me.

On Sat, Aug 7, 2021 at 6:53 PM houzj.fnst@fujitsu.com
<houzj.fnst@fujitsu.com> wrote:
>
> Personally, I also think it will be better to make the behavior consistent.
> Attach the new version patch make both ADD and DROP behave the same as SET PUBLICATION
> which refresh all the publications.
>

I think we can have tests in the separate test file (alter_sub_pub.pl)
like you earlier had in one of the versions. Use some meaningful names
for tables instead of temp1, temp2 as you had in the previous version.
Otherwise, the code changes look good to me.

-- 
With Regards,
Amit Kapila.

On Wed, Aug 18, 2021 at 7:54 PM Peter Eisentraut
<peter.eisentraut@enterprisedb.com> wrote:
>
> On 10.08.21 05:22, Amit Kapila wrote:
> > Yeah, unless we change design drastically we might not be able to do a
> > refresh for dropped publications, for add it is possible. It seems
> > most of the people responded on this thread that we can be consistent
> > in terms of refreshing for add/drop at this stage but we can still go
> > with another option where we can refresh only newly added publications
> > for add but for drop refresh all publications.
> >
> > Peter E., do you have any opinion on this matter?
>
> Refresh everything for both ADD and DROP makes sense to me.
>

Thanks for your suggestion.

-- 
With Regards,
Amit Kapila.

On Mon, Aug 23, 2021 at 1:59 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Sat, Aug 7, 2021 at 6:53 PM houzj.fnst@fujitsu.com
> <houzj.fnst@fujitsu.com> wrote:
> >
> > Personally, I also think it will be better to make the behavior consistent.
> > Attach the new version patch make both ADD and DROP behave the same as SET PUBLICATION
> > which refresh all the publications.
> >
>
> I think we can have tests in the separate test file (alter_sub_pub.pl)
> like you earlier had in one of the versions. Use some meaningful names
> for tables instead of temp1, temp2 as you had in the previous version.
> Otherwise, the code changes look good to me.

-               supported_opts = SUBOPT_REFRESH;
-               if (isadd)
-                   supported_opts |= SUBOPT_COPY_DATA;
+               supported_opts = SUBOPT_REFRESH | SUBOPT_COPY_DATA;

I think that the currently the doc says copy_data option can be
specified except in DROP PUBLICATION case, which needs to be fixed
corresponding the above change.

Regards,

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

On Mon, Aug 23, 2021 12:59 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> 
> On Sat, Aug 7, 2021 at 6:53 PM houzj.fnst@fujitsu.com <houzj.fnst@fujitsu.com> wrote:
> >
> > Personally, I also think it will be better to make the behavior consistent.
> > Attach the new version patch make both ADD and DROP behave the same as
> > SET PUBLICATION which refresh all the publications.
> >
> 
> I think we can have tests in the separate test file (alter_sub_pub.pl) like you
> earlier had in one of the versions. Use some meaningful names for tables
> instead of temp1, temp2 as you had in the previous version.
> Otherwise, the code changes look good to me.

Thanks for the comment.
Attach new version patch which did the following changes.

* move the tests to a separate test file (024_alter_sub_pub.pl)
* adjust the document of copy_data option
* add tab-complete for copy_data option when ALTER DROP publication.

Best regards,
Hou zj

On Mon, Aug 23, 2021 1:18 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> On Mon, Aug 23, 2021 at 1:59 PM Amit Kapila <amit.kapila16@gmail.com>
> wrote:
> >
> > On Sat, Aug 7, 2021 at 6:53 PM houzj.fnst@fujitsu.com
> > <houzj.fnst@fujitsu.com> wrote:
> > >
> > > Personally, I also think it will be better to make the behavior consistent.
> > > Attach the new version patch make both ADD and DROP behave the same
> > > as SET PUBLICATION which refresh all the publications.
> > >
> >
> > I think we can have tests in the separate test file (alter_sub_pub.pl)
> > like you earlier had in one of the versions. Use some meaningful names
> > for tables instead of temp1, temp2 as you had in the previous version.
> > Otherwise, the code changes look good to me.
> 
> -               supported_opts = SUBOPT_REFRESH;
> -               if (isadd)
> -                   supported_opts |= SUBOPT_COPY_DATA;
> +               supported_opts = SUBOPT_REFRESH | SUBOPT_COPY_DATA;
> 
> I think that the currently the doc says copy_data option can be specified except
> in DROP PUBLICATION case, which needs to be fixed corresponding the above
> change.

Thanks for the comment.
Fixed in the new version patch.

Best regards,
Hou zj

On Mon, Aug 23, 2021 at 2:45 PM houzj.fnst@fujitsu.com
<houzj.fnst@fujitsu.com> wrote:
>
> On Mon, Aug 23, 2021 12:59 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Sat, Aug 7, 2021 at 6:53 PM houzj.fnst@fujitsu.com <houzj.fnst@fujitsu.com> wrote:
> > >
> > > Personally, I also think it will be better to make the behavior consistent.
> > > Attach the new version patch make both ADD and DROP behave the same as
> > > SET PUBLICATION which refresh all the publications.
> > >
> >
> > I think we can have tests in the separate test file (alter_sub_pub.pl) like you
> > earlier had in one of the versions. Use some meaningful names for tables
> > instead of temp1, temp2 as you had in the previous version.
> > Otherwise, the code changes look good to me.
>
> Thanks for the comment.
> Attach new version patch which did the following changes.
>
> * move the tests to a separate test file (024_alter_sub_pub.pl)
> * adjust the document of copy_data option
> * add tab-complete for copy_data option when ALTER DROP publication.
>

Thanks, the patch looks good to me. I have made some cosmetic changes
in the attached version. It makes the test cases easier to understand.
I am planning to push this tomorrow unless there are more comments or
suggestions.

-- 
With Regards,
Amit Kapila.

On Mon, Aug 23, 2021 8:01 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Mon, Aug 23, 2021 at 2:45 PM houzj.fnst@fujitsu.com<houzj.fnst@fujitsu.com> wrote:
> >
> > On Mon, Aug 23, 2021 12:59 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > On Sat, Aug 7, 2021 at 6:53 PM houzj.fnst@fujitsu.com <houzj.fnst@fujitsu.com> wrote:
> > > >
> > > > Personally, I also think it will be better to make the behavior consistent.
> > > > Attach the new version patch make both ADD and DROP behave the
> > > > same as SET PUBLICATION which refresh all the publications.
> > > >
> > >
> > > I think we can have tests in the separate test file
> > > (alter_sub_pub.pl) like you earlier had in one of the versions. Use
> > > some meaningful names for tables instead of temp1, temp2 as you had in the
> previous version.
> > > Otherwise, the code changes look good to me.
> >
> > Thanks for the comment.
> > Attach new version patch which did the following changes.
> >
> > * move the tests to a separate test file (024_alter_sub_pub.pl)
> > * adjust the document of copy_data option
> > * add tab-complete for copy_data option when ALTER DROP publication.
> >
> 
> Thanks, the patch looks good to me. I have made some cosmetic changes in the
> attached version. It makes the test cases easier to understand.
> I am planning to push this tomorrow unless there are more comments or
> suggestions.

Thanks ! The patch looks good to me.

I noticed that the patch cannot be applied to PG14-stable,
so I attach a separate patch for back-patch(I didn’t change the patch for HEAD branch).

Best regards,
Hou zj

On Mon, Aug 23, 2021 at 11:05 PM houzj.fnst@fujitsu.com
<houzj.fnst@fujitsu.com> wrote:
>
> On Mon, Aug 23, 2021 8:01 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > On Mon, Aug 23, 2021 at 2:45 PM houzj.fnst@fujitsu.com<houzj.fnst@fujitsu.com> wrote:
> > >
> > > On Mon, Aug 23, 2021 12:59 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > > >
> > > > On Sat, Aug 7, 2021 at 6:53 PM houzj.fnst@fujitsu.com <houzj.fnst@fujitsu.com> wrote:
> > > > >
> > > > > Personally, I also think it will be better to make the behavior consistent.
> > > > > Attach the new version patch make both ADD and DROP behave the
> > > > > same as SET PUBLICATION which refresh all the publications.
> > > > >
> > > >
> > > > I think we can have tests in the separate test file
> > > > (alter_sub_pub.pl) like you earlier had in one of the versions. Use
> > > > some meaningful names for tables instead of temp1, temp2 as you had in the
> > previous version.
> > > > Otherwise, the code changes look good to me.
> > >
> > > Thanks for the comment.
> > > Attach new version patch which did the following changes.
> > >
> > > * move the tests to a separate test file (024_alter_sub_pub.pl)
> > > * adjust the document of copy_data option
> > > * add tab-complete for copy_data option when ALTER DROP publication.
> > >
> >
> > Thanks, the patch looks good to me. I have made some cosmetic changes in the
> > attached version. It makes the test cases easier to understand.
> > I am planning to push this tomorrow unless there are more comments or
> > suggestions.
>
> Thanks ! The patch looks good to me.
>
> I noticed that the patch cannot be applied to PG14-stable,
> so I attach a separate patch for back-patch(I didn’t change the patch for HEAD branch).

Thanks. The patch for HEAD looks good to me. Regarding the patch for
PG14, I think we need to update the comment as well:

@@ -987,7 +986,7 @@ AlterSubscription(AlterSubscriptionStmt *stmt,
bool isTopLevel)
      PreventInTransactionBlock(isTopLevel, "ALTER SUBSCRIPTION with refresh");

      /* Only refresh the added/dropped list of publications. */
-     sub->publications = stmt->publication;
+     sub->publications = publist;

Regards,

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

> -----Original Message-----
> From: Masahiko Sawada <sawada.mshk@gmail.com>
> Sent: Tuesday, August 24, 2021 8:52 AM
> 
> Thanks. The patch for HEAD looks good to me. Regarding the patch for PG14, I
> think we need to update the comment as well:
> 
> @@ -987,7 +986,7 @@ AlterSubscription(AlterSubscriptionStmt *stmt, bool
> isTopLevel)
>       PreventInTransactionBlock(isTopLevel, "ALTER SUBSCRIPTION with
> refresh");
> 
>       /* Only refresh the added/dropped list of publications. */
> -     sub->publications = stmt->publication;
> +     sub->publications = publist;
> 

Thanks for the comment, attach new version patch which fixed it.

Best regards,
Hou zj

On Tue, Aug 24, 2021 at 10:01 AM houzj.fnst@fujitsu.com
<houzj.fnst@fujitsu.com> wrote:
>
>
>
> > -----Original Message-----
> > From: Masahiko Sawada <sawada.mshk@gmail.com>
> > Sent: Tuesday, August 24, 2021 8:52 AM
> >
> > Thanks. The patch for HEAD looks good to me. Regarding the patch for PG14, I
> > think we need to update the comment as well:
> >
> > @@ -987,7 +986,7 @@ AlterSubscription(AlterSubscriptionStmt *stmt, bool
> > isTopLevel)
> >       PreventInTransactionBlock(isTopLevel, "ALTER SUBSCRIPTION with
> > refresh");
> >
> >       /* Only refresh the added/dropped list of publications. */
> > -     sub->publications = stmt->publication;
> > +     sub->publications = publist;
> >
>
> Thanks for the comment, attach new version patch which fixed it.

Thank you for updating the patch! Looks good to me.

Regards,

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

On Tue, Aug 24, 2021 at 7:13 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Tue, Aug 24, 2021 at 10:01 AM houzj.fnst@fujitsu.com
> <houzj.fnst@fujitsu.com> wrote:
> >
> >
> >
> > > -----Original Message-----
> > > From: Masahiko Sawada <sawada.mshk@gmail.com>
> > > Sent: Tuesday, August 24, 2021 8:52 AM
> > >
> > > Thanks. The patch for HEAD looks good to me. Regarding the patch for PG14, I
> > > think we need to update the comment as well:
> > >
> > > @@ -987,7 +986,7 @@ AlterSubscription(AlterSubscriptionStmt *stmt, bool
> > > isTopLevel)
> > >       PreventInTransactionBlock(isTopLevel, "ALTER SUBSCRIPTION with
> > > refresh");
> > >
> > >       /* Only refresh the added/dropped list of publications. */
> > > -     sub->publications = stmt->publication;
> > > +     sub->publications = publist;
> > >
> >
> > Thanks for the comment, attach new version patch which fixed it.
>
> Thank you for updating the patch! Looks good to me.
>

Pushed!

-- 
With Regards,
Amit Kapila.

On Tue, Aug 24, 2021 at 5:38 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Tue, Aug 24, 2021 at 7:13 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > On Tue, Aug 24, 2021 at 10:01 AM houzj.fnst@fujitsu.com
> > <houzj.fnst@fujitsu.com> wrote:
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Masahiko Sawada <sawada.mshk@gmail.com>
> > > > Sent: Tuesday, August 24, 2021 8:52 AM
> > > >
> > > > Thanks. The patch for HEAD looks good to me. Regarding the patch for PG14, I
> > > > think we need to update the comment as well:
> > > >
> > > > @@ -987,7 +986,7 @@ AlterSubscription(AlterSubscriptionStmt *stmt, bool
> > > > isTopLevel)
> > > >       PreventInTransactionBlock(isTopLevel, "ALTER SUBSCRIPTION with
> > > > refresh");
> > > >
> > > >       /* Only refresh the added/dropped list of publications. */
> > > > -     sub->publications = stmt->publication;
> > > > +     sub->publications = publist;
> > > >
> > >
> > > Thanks for the comment, attach new version patch which fixed it.
> >
> > Thank you for updating the patch! Looks good to me.
> >
>
> Pushed!

Thanks! I've marked this open item as resolved.

Regards,

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