v4-0001-fix-ALTER-SUBSCRIPTION-ADD-DROP-PUBLICATION.patch

From 6fd398473ee27dd72f6b17d766a7bc12ff86b24f Mon Sep 17 00:00:00 2001
From: "houzj.fnst" <houzj.fnst@fujitsu.com>
Date: Sat, 7 Aug 2021 19:38:49 +0800
Subject: [PATCH] fix ALTER SUBSCRIPTION ADD DROP PUBLICATION

Currently, in function AlterSubscription_refresh(), it will compare
the tables from the target publication with the tables in local
pg_subscription_rel. And then add the tables to pg_subscription_rel
which not exists yet, and delele the tables from pg_subscription_rel
which doesn't exists in target publication.

But When ALTER SUB ADD/DROP PUBLICATION, the code only pass added or dropped
publications as the target publication to AlterSubscription_refresh() which
could result in wrong record in pg_subscription_rel and unexpected table sync.

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), It's impossible to
achieve that DROP PUBLICATION drops relations that are associated with only
the publication being dropped.

So, change the behaviour of ADD/DROP PUBLICATION to refresh all publications
like SET PUBLICATION.

---
 doc/src/sgml/ref/alter_subscription.sgml   |  4 +-
 src/backend/commands/subscriptioncmds.c    |  8 ++-
 src/test/regress/expected/subscription.out |  3 --
 src/test/regress/sql/subscription.sql      |  3 --
 src/test/subscription/t/001_rep_changes.pl | 57 +++++++++++++++++++++-
 5 files changed, 60 insertions(+), 15 deletions(-)

diff --git a/doc/src/sgml/ref/alter_subscription.sgml b/doc/src/sgml/ref/alter_subscription.sgml
index a6f994450d..76eb011c74 100644
--- a/doc/src/sgml/ref/alter_subscription.sgml
+++ b/doc/src/sgml/ref/alter_subscription.sgml
@@ -111,9 +111,7 @@ ALTER SUBSCRIPTION <replaceable class="parameter">name</replaceable> RENAME TO <
       publications, and <literal>DROP</literal> removes the publications from
       the list of publications.  See <xref linkend="sql-createsubscription"/>
       for more information.  By default, this command will also act like
-      <literal>REFRESH PUBLICATION</literal>, except that in case of
-      <literal>ADD</literal> or <literal>DROP</literal>, only the added or
-      dropped publications are refreshed.
+      <literal>REFRESH PUBLICATION</literal>.
      </para>
 
      <para>
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index 5157f44058..8aaa735926 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -1006,9 +1006,7 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
 				List	   *publist;
 				bool		isadd = stmt->kind == ALTER_SUBSCRIPTION_ADD_PUBLICATION;
 
-				supported_opts = SUBOPT_REFRESH;
-				if (isadd)
-					supported_opts |= SUBOPT_COPY_DATA;
+				supported_opts = SUBOPT_REFRESH | SUBOPT_COPY_DATA;
 
 				parse_subscription_options(pstate, stmt->options,
 										   supported_opts, &opts);
@@ -1042,8 +1040,8 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
 
 					PreventInTransactionBlock(isTopLevel, "ALTER SUBSCRIPTION with refresh");
 
-					/* Only refresh the added/dropped list of publications. */
-					sub->publications = stmt->publication;
+					/* Refresh the new list of publications. */
+					sub->publications = publist;
 
 					AlterSubscription_refresh(sub, opts.copy_data);
 				}
diff --git a/src/test/regress/expected/subscription.out b/src/test/regress/expected/subscription.out
index 77b4437b69..15a1ac6398 100644
--- a/src/test/regress/expected/subscription.out
+++ b/src/test/regress/expected/subscription.out
@@ -230,9 +230,6 @@ ERROR:  cannot drop all the publications from a subscription
 -- fail - publication does not exist in subscription
 ALTER SUBSCRIPTION regress_testsub DROP PUBLICATION testpub3 WITH (refresh = false);
 ERROR:  publication "testpub3" is not in subscription "regress_testsub"
--- fail - do not support copy_data option
-ALTER SUBSCRIPTION regress_testsub DROP PUBLICATION testpub1 WITH (refresh = false, copy_data = true);
-ERROR:  unrecognized subscription parameter: "copy_data"
 -- ok - delete publications
 ALTER SUBSCRIPTION regress_testsub DROP PUBLICATION testpub1, testpub2 WITH (refresh = false);
 \dRs+
diff --git a/src/test/regress/sql/subscription.sql b/src/test/regress/sql/subscription.sql
index d42104c191..7faa935a2a 100644
--- a/src/test/regress/sql/subscription.sql
+++ b/src/test/regress/sql/subscription.sql
@@ -171,9 +171,6 @@ ALTER SUBSCRIPTION regress_testsub DROP PUBLICATION testpub, testpub1, testpub2
 -- fail - publication does not exist in subscription
 ALTER SUBSCRIPTION regress_testsub DROP PUBLICATION testpub3 WITH (refresh = false);
 
--- fail - do not support copy_data option
-ALTER SUBSCRIPTION regress_testsub DROP PUBLICATION testpub1 WITH (refresh = false, copy_data = true);
-
 -- ok - delete publications
 ALTER SUBSCRIPTION regress_testsub DROP PUBLICATION testpub1, testpub2 WITH (refresh = false);
 
diff --git a/src/test/subscription/t/001_rep_changes.pl b/src/test/subscription/t/001_rep_changes.pl
index 0c84d87873..62e5b169f6 100644
--- a/src/test/subscription/t/001_rep_changes.pl
+++ b/src/test/subscription/t/001_rep_changes.pl
@@ -6,7 +6,7 @@ use strict;
 use warnings;
 use PostgresNode;
 use TestLib;
-use Test::More tests => 32;
+use Test::More tests => 34;
 
 # Initialize publisher node
 my $node_publisher = PostgresNode->new('publisher');
@@ -178,6 +178,61 @@ is( $node_subscriber->safe_psql(
 $node_publisher->safe_psql('postgres',
 	"INSERT INTO tab_full SELECT generate_series(1,10)");
 
+# Test behaviour of ALTER SUBSCRIPTION ... ADD/DROP PUBLICATION
+# The list of publication should be refreshed
+
+# Setup logical replication
+$node_publisher->safe_psql('postgres', "CREATE TABLE tab_ins_for_readd (a int)");
+
+$node_publisher->safe_psql('postgres',
+	"INSERT INTO tab_ins_for_readd SELECT generate_series(1,10)");
+
+$node_publisher->safe_psql('postgres',
+	"CREATE PUBLICATION tap_pub_for_readd FOR TABLE tab_ins_for_readd");
+
+$node_subscriber->safe_psql('postgres', "CREATE TABLE tab_ins_for_readd (a int)");
+
+# Add tap_pub_for_readd to publication list
+$node_subscriber->safe_psql('postgres',
+	"ALTER SUBSCRIPTION tap_sub ADD PUBLICATION tap_pub_for_readd");
+
+# Wait for initial table sync to finish
+$node_subscriber->poll_query_until('postgres', $synced_query)
+  or die "Timed out while waiting for subscriber to synchronize data";
+
+$node_publisher->wait_for_catchup('tap_sub');
+
+# Check the initial data of tab_ins_for_readd is copied to subscriber
+$result = $node_subscriber->safe_psql('postgres',
+	"SELECT count(*), min(a), max(a) FROM tab_ins_for_readd");
+is($result, qq(10|1|10), 'check initial data is copied to subscriber');
+
+# Drop tap_pub_for_readd from publication list
+$node_subscriber->safe_psql('postgres',
+	"ALTER SUBSCRIPTION tap_sub DROP PUBLICATION tap_pub_for_readd");
+
+# Readd tap_pub_for_readd to publication list
+$node_subscriber->safe_psql('postgres',
+	"ALTER SUBSCRIPTION tap_sub ADD PUBLICATION tap_pub_for_readd");
+
+# Wait for initial table sync to finish
+$node_subscriber->poll_query_until('postgres', $synced_query)
+  or die "Timed out while waiting for subscriber to synchronize data";
+
+$node_publisher->wait_for_catchup('tap_sub');
+
+# Check the initial data of tab_ins_for_readd was copied to subscriber again
+$result = $node_subscriber->safe_psql('postgres',
+	"SELECT count(*), min(a), max(a) FROM tab_ins_for_readd");
+is($result, qq(20|1|10), 'check initial data is copied to subscriber');
+
+# Clean unused tables and publication
+$node_subscriber->safe_psql('postgres',
+	"ALTER SUBSCRIPTION tap_sub DROP PUBLICATION tap_pub_for_readd");
+$node_subscriber->safe_psql('postgres', "DROP TABLE tab_ins_for_readd");
+$node_publisher->safe_psql('postgres', "DROP PUBLICATION tap_pub_for_readd");
+$node_publisher->safe_psql('postgres', "DROP TABLE tab_ins_for_readd");
+
 # Test behaviour of ALTER PUBLICATION ... DROP TABLE
 #
 # When a publisher drops a table from publication, it should also stop sending
-- 
2.18.4