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

From 78b8e3e959d83e898ad5befaf55fe46c6fdd6f20 Mon Sep 17 00:00:00 2001
From: Hou Zhijie <HouZhijie@foxmail.com>
Date: Wed, 4 Aug 2021 15:33:15 +0800
Subject: [PATCH] fix-ALTER-SUB-ADD-DROP-PUBLICATION

---
 src/backend/commands/subscriptioncmds.c    | 76 +++++++++++++++-------
 src/test/subscription/t/001_rep_changes.pl | 21 +++++-
 2 files changed, 71 insertions(+), 26 deletions(-)

diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index 5157f44058..68a128ced0 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -606,7 +606,8 @@ CreateSubscription(ParseState *pstate, CreateSubscriptionStmt *stmt,
 }
 
 static void
-AlterSubscription_refresh(Subscription *sub, bool copy_data)
+AlterSubscription_refresh(Subscription *sub, bool copy_data,
+						  AlterSubscriptionType type)
 {
 	char	   *err;
 	List	   *pubrel_names;
@@ -617,6 +618,7 @@ AlterSubscription_refresh(Subscription *sub, bool copy_data)
 	int			off;
 	int			remove_rel_len;
 	Relation	rel = NULL;
+	int			ntables_to_drop = 0;
 	typedef struct SubRemoveRels
 	{
 		Oid			relid;
@@ -625,6 +627,11 @@ AlterSubscription_refresh(Subscription *sub, bool copy_data)
 	SubRemoveRels *sub_remove_rels;
 	WalReceiverConn *wrconn;
 
+	Assert(type == ALTER_SUBSCRIPTION_SET_PUBLICATION ||
+		   type == ALTER_SUBSCRIPTION_ADD_PUBLICATION ||
+		   type == ALTER_SUBSCRIPTION_DROP_PUBLICATION ||
+		   type == ALTER_SUBSCRIPTION_REFRESH);
+
 	/* Load the library providing us libpq calls. */
 	load_file("libpqwalreceiver", false);
 
@@ -656,8 +663,10 @@ AlterSubscription_refresh(Subscription *sub, bool copy_data)
 
 			subrel_local_oids[off++] = relstate->relid;
 		}
-		qsort(subrel_local_oids, list_length(subrel_states),
-			  sizeof(Oid), oid_cmp);
+
+		if (type != ALTER_SUBSCRIPTION_DROP_PUBLICATION)
+			qsort(subrel_local_oids, list_length(subrel_states),
+				sizeof(Oid), oid_cmp);
 
 		/*
 		 * Rels that we want to remove from subscription and drop any slots
@@ -681,22 +690,25 @@ AlterSubscription_refresh(Subscription *sub, bool copy_data)
 			Oid			relid;
 
 			relid = RangeVarGetRelid(rv, AccessShareLock, false);
-
-			/* Check for supported relkind. */
-			CheckSubscriptionRelkind(get_rel_relkind(relid),
-									 rv->schemaname, rv->relname);
-
 			pubrel_local_oids[off++] = relid;
 
-			if (!bsearch(&relid, subrel_local_oids,
-						 list_length(subrel_states), sizeof(Oid), oid_cmp))
+			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);
+
+				if (!bsearch(&relid, subrel_local_oids,
+							 list_length(subrel_states), sizeof(Oid), oid_cmp))
+				{
+					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)));
+				}
 			}
 		}
 
@@ -704,16 +716,30 @@ AlterSubscription_refresh(Subscription *sub, bool copy_data)
 		 * Next remove state for tables we should not care about anymore using
 		 * the data we collected above
 		 */
-		qsort(pubrel_local_oids, list_length(pubrel_names),
-			  sizeof(Oid), oid_cmp);
+		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);
+		}
 
 		remove_rel_len = 0;
-		for (off = 0; off < list_length(subrel_states); off++)
+		for (off = 0; off < ntables_to_drop; off++)
 		{
 			Oid			relid = subrel_local_oids[off];
-
-			if (!bsearch(&relid, pubrel_local_oids,
-						 list_length(pubrel_names), sizeof(Oid), oid_cmp))
+			bool		drop_table = false;
+
+			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);
+
+			if (drop_table)
 			{
 				char		state;
 				XLogRecPtr	statelsn;
@@ -994,7 +1020,7 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
 					/* Make sure refresh sees the new list of publications. */
 					sub->publications = stmt->publication;
 
-					AlterSubscription_refresh(sub, opts.copy_data);
+					AlterSubscription_refresh(sub, opts.copy_data, stmt->kind);
 				}
 
 				break;
@@ -1045,7 +1071,7 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
 					/* Only refresh the added/dropped list of publications. */
 					sub->publications = stmt->publication;
 
-					AlterSubscription_refresh(sub, opts.copy_data);
+					AlterSubscription_refresh(sub, opts.copy_data, stmt->kind);
 				}
 
 				break;
@@ -1087,7 +1113,7 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
 
 				PreventInTransactionBlock(isTopLevel, "ALTER SUBSCRIPTION ... REFRESH");
 
-				AlterSubscription_refresh(sub, opts.copy_data);
+				AlterSubscription_refresh(sub, opts.copy_data, stmt->kind);
 
 				break;
 			}
diff --git a/src/test/subscription/t/001_rep_changes.pl b/src/test/subscription/t/001_rep_changes.pl
index 0c84d87873..f04df8b8d9 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');
@@ -262,6 +262,25 @@ $result =
   $node_subscriber->safe_psql('postgres', "SELECT count(*) FROM temp1");
 is($result, qq(1), 'check rows on subscriber with multiple publications');
 
+# Test changing the list of subscribed publications
+# Removes publications from the list of publications
+$node_subscriber->safe_psql('postgres', "ALTER SUBSCRIPTION tap_sub_temp1 DROP PUBLICATION tap_pub_temp1");
+
+# pg_subscription_rel should only have one row
+$result = $node_subscriber->safe_psql('postgres',
+	"SELECT count(*) FROM pg_subscription_rel t1, pg_subscription t2 WHERE t1.srsubid = t2.oid AND t2.subname = 'tap_sub_temp1'");
+is($result, qq(1),
+	'check one relation was removed from subscribed list');
+
+# Add additional publications
+$node_subscriber->safe_psql('postgres', "ALTER SUBSCRIPTION tap_sub_temp1 ADD PUBLICATION tap_pub_temp1");
+
+# pg_subscription_rel should only have two rows
+$result = $node_subscriber->safe_psql('postgres',
+	"SELECT count(*) FROM pg_subscription_rel t1, pg_subscription t2 WHERE t1.srsubid = t2.oid AND t2.subname = 'tap_sub_temp1'");
+is($result, qq(2),
+	'check one more relation was subscribed');
+
 # Drop subscription as we don't need it anymore
 $node_subscriber->safe_psql('postgres', "DROP SUBSCRIPTION tap_sub_temp1");
 
-- 
2.27.0

From ac986d987b1eae6dae320deb76686cac2d904bf2 Mon Sep 17 00:00:00 2001
From: Hou Zhijie <HouZhijie@foxmail.com>
Date: Thu, 5 Aug 2021 18:29:26 +0800
Subject: [PATCH] 0001-fix-ALTER-SUB-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.

Fix the use of AlterSubscription_refresh() by passing correct publications
to it and distinguish the refresh logic of ADD and DROP.

---
 src/backend/commands/subscriptioncmds.c      |  71 +++++++----
 src/test/subscription/t/024_alter_sub_pub.pl | 124 +++++++++++++++++++
 2 files changed, 171 insertions(+), 24 deletions(-)
 create mode 100644 src/test/subscription/t/024_alter_sub_pub.pl

diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index 5157f44058..6b14ef1fbc 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -606,7 +606,8 @@ CreateSubscription(ParseState *pstate, CreateSubscriptionStmt *stmt,
 }
 
 static void
-AlterSubscription_refresh(Subscription *sub, bool copy_data)
+AlterSubscription_refresh(Subscription *sub, bool copy_data,
+						  AlterSubscriptionType type)
 {
 	char	   *err;
 	List	   *pubrel_names;
@@ -617,6 +618,7 @@ AlterSubscription_refresh(Subscription *sub, bool copy_data)
 	int			off;
 	int			remove_rel_len;
 	Relation	rel = NULL;
+	int			ntables_to_drop = 0;
 	typedef struct SubRemoveRels
 	{
 		Oid			relid;
@@ -625,6 +627,11 @@ AlterSubscription_refresh(Subscription *sub, bool copy_data)
 	SubRemoveRels *sub_remove_rels;
 	WalReceiverConn *wrconn;
 
+	Assert(type == ALTER_SUBSCRIPTION_SET_PUBLICATION ||
+		   type == ALTER_SUBSCRIPTION_ADD_PUBLICATION ||
+		   type == ALTER_SUBSCRIPTION_DROP_PUBLICATION ||
+		   type == ALTER_SUBSCRIPTION_REFRESH);
+
 	/* Load the library providing us libpq calls. */
 	load_file("libpqwalreceiver", false);
 
@@ -656,8 +663,14 @@ AlterSubscription_refresh(Subscription *sub, bool copy_data)
 
 			subrel_local_oids[off++] = relstate->relid;
 		}
-		qsort(subrel_local_oids, list_length(subrel_states),
-			  sizeof(Oid), oid_cmp);
+
+		/*
+		 * Don't need to lookup oid in subrel_local_oids if we are DROPing
+		 * PUBLICATION, so skip the sort.
+		 */
+		if (type != ALTER_SUBSCRIPTION_DROP_PUBLICATION)
+			qsort(subrel_local_oids, list_length(subrel_states),
+				sizeof(Oid), oid_cmp);
 
 		/*
 		 * Rels that we want to remove from subscription and drop any slots
@@ -681,34 +694,41 @@ AlterSubscription_refresh(Subscription *sub, bool copy_data)
 			Oid			relid;
 
 			relid = RangeVarGetRelid(rv, AccessShareLock, false);
-
-			/* Check for supported relkind. */
-			CheckSubscriptionRelkind(get_rel_relkind(relid),
-									 rv->schemaname, rv->relname);
-
 			pubrel_local_oids[off++] = relid;
 
-			if (!bsearch(&relid, subrel_local_oids,
-						 list_length(subrel_states), sizeof(Oid), oid_cmp))
+			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);
+
+				if (!bsearch(&relid, subrel_local_oids,
+							 list_length(subrel_states), sizeof(Oid), oid_cmp))
+				{
+					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)));
+				}
 			}
 		}
 
 		/*
 		 * Next remove state for tables we should not care about anymore using
-		 * the data we collected above
+		 * the data we collected above, if we are not ADDing PUBLICATION.
 		 */
-		qsort(pubrel_local_oids, list_length(pubrel_names),
-			  sizeof(Oid), oid_cmp);
+		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);
+		}
 
 		remove_rel_len = 0;
-		for (off = 0; off < list_length(subrel_states); off++)
+		for (off = 0; off < ntables_to_drop; off++)
 		{
 			Oid			relid = subrel_local_oids[off];
 
@@ -994,7 +1014,7 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
 					/* Make sure refresh sees the new list of publications. */
 					sub->publications = stmt->publication;
 
-					AlterSubscription_refresh(sub, opts.copy_data);
+					AlterSubscription_refresh(sub, opts.copy_data, stmt->kind);
 				}
 
 				break;
@@ -1043,9 +1063,12 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
 					PreventInTransactionBlock(isTopLevel, "ALTER SUBSCRIPTION with refresh");
 
 					/* Only refresh the added/dropped list of publications. */
-					sub->publications = stmt->publication;
+					if (stmt->kind == ALTER_SUBSCRIPTION_ADD_PUBLICATION)
+						sub->publications = stmt->publication;
+					else
+						sub->publications = publist;
 
-					AlterSubscription_refresh(sub, opts.copy_data);
+					AlterSubscription_refresh(sub, opts.copy_data, stmt->kind);
 				}
 
 				break;
@@ -1087,7 +1110,7 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
 
 				PreventInTransactionBlock(isTopLevel, "ALTER SUBSCRIPTION ... REFRESH");
 
-				AlterSubscription_refresh(sub, opts.copy_data);
+				AlterSubscription_refresh(sub, opts.copy_data, stmt->kind);
 
 				break;
 			}
diff --git a/src/test/subscription/t/024_alter_sub_pub.pl b/src/test/subscription/t/024_alter_sub_pub.pl
new file mode 100644
index 0000000000..974182aebe
--- /dev/null
+++ b/src/test/subscription/t/024_alter_sub_pub.pl
@@ -0,0 +1,124 @@
+
+# Copyright (c) 2021, PostgreSQL Global Development Group
+
+# This test checks that constraints work on subscriber
+use strict;
+use warnings;
+use PostgresNode;
+use TestLib;
+use Test::More tests => 8;
+
+# Initialize publisher node
+my $node_publisher = PostgresNode->new('publisher');
+$node_publisher->init(allows_streaming => 'logical');
+$node_publisher->start;
+
+# Create subscriber node
+my $node_subscriber = PostgresNode->new('subscriber');
+$node_subscriber->init(allows_streaming => 'logical');
+$node_subscriber->start;
+
+# 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)");
+
+# Setup logical replication
+my $publisher_connstr = $node_publisher->connstr . ' dbname=postgres';
+$node_publisher->safe_psql('postgres',
+	"CREATE PUBLICATION tap_pub_temp1 FOR TABLE temp1");
+$node_publisher->safe_psql('postgres',
+	"CREATE PUBLICATION tap_pub_temp2 FOR TABLE temp2");
+$node_publisher->safe_psql('postgres',
+	"CREATE PUBLICATION tap_pub_temp3 FOR TABLE temp3, temp2");
+$node_subscriber->safe_psql('postgres',
+	"CREATE SUBSCRIPTION tap_sub_temp1 CONNECTION '$publisher_connstr' PUBLICATION tap_pub_temp1, tap_pub_temp2, tap_pub_temp3"
+);
+
+# 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');
+
+# Test changing the list of subscribed publications
+# Removes table from publication
+$node_publisher->safe_psql('postgres', "ALTER PUBLICATION tap_pub_temp1 DROP TABLE temp1");
+
+# Remove publications from the list of publications
+$node_subscriber->safe_psql('postgres', "ALTER SUBSCRIPTION tap_sub_temp1 DROP PUBLICATION tap_pub_temp1");
+
+# temp1 should have been deleted from pg_subscription_rel
+$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');
+
+
+# Removes publications from the list of publications
+$node_subscriber->safe_psql('postgres', "ALTER SUBSCRIPTION tap_sub_temp1 DROP PUBLICATION tap_pub_temp3");
+
+# one row should have been deleted from pg_subscription_rel
+$result = $node_subscriber->safe_psql('postgres',
+	"SELECT count(*) FROM pg_subscription_rel");
+is($result, qq(1),
+	'check one relation was removed from subscribed list');
+
+# temp3 should have been deleted from pg_subscription_rel
+$result = $node_subscriber->safe_psql('postgres',
+	"SELECT count(*) FROM pg_subscription_rel where srrelid::regclass::text = 'temp3'");
+is($result, qq(0),
+	'check relation temp1 was removed from subscribed list');
+
+# temp2 should still exists
+$result = $node_subscriber->safe_psql('postgres',
+	"SELECT count(*) FROM pg_subscription_rel where srrelid::regclass::text = 'temp2'");
+is($result, qq(1),
+	'check relation temp1 was removed from subscribed list');
+
+# Readd table to publication
+$node_publisher->safe_psql('postgres', "ALTER PUBLICATION tap_pub_temp1 ADD TABLE temp1");
+
+# Add additional publications
+$node_subscriber->safe_psql('postgres', "ALTER SUBSCRIPTION tap_sub_temp1 ADD PUBLICATION tap_pub_temp1");
+
+# pg_subscription_rel should have two rows(temp1, temp2)
+$result = $node_subscriber->safe_psql('postgres',
+	"SELECT count(*) FROM pg_subscription_rel");
+is($result, qq(2),
+	'check one more relation was subscribed');
+
+$result = $node_subscriber->safe_psql('postgres',
+	"SELECT count(*) FROM pg_subscription_rel where srrelid::regclass::text = 'temp1'");
+is($result, qq(1),
+	'check relation temp1 was added to subscribed list');
+
+# Drop subscription as we don't need it anymore
+$node_subscriber->safe_psql('postgres', "DROP SUBSCRIPTION tap_sub_temp1");
+
+# Drop publications as we don't need them anymore
+$node_publisher->safe_psql('postgres', "DROP PUBLICATION tap_pub_temp1");
+$node_publisher->safe_psql('postgres', "DROP PUBLICATION tap_pub_temp2");
+$node_publisher->safe_psql('postgres', "DROP PUBLICATION tap_pub_temp3");
+
+# Clean up the tables on both publisher and subscriber as we don't need them
+$node_publisher->safe_psql('postgres', "DROP TABLE temp1");
+$node_publisher->safe_psql('postgres', "DROP TABLE temp2");
+$node_publisher->safe_psql('postgres', "DROP TABLE temp3");
+$node_subscriber->safe_psql('postgres', "DROP TABLE temp1");
+$node_subscriber->safe_psql('postgres', "DROP TABLE temp2");
+$node_subscriber->safe_psql('postgres', "DROP TABLE temp3");
+
+$node_subscriber->stop('fast');
+$node_publisher->stop('fast');
-- 
2.27.0

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

From 98d500330c19b6b3f6a0aa1487ed88aa9288749f Mon Sep 17 00:00:00 2001
From: "houzj.fnst" <houzj.fnst@cn.fujitsu.com>
Date: Mon, 23 Aug 2021 17:03:03 +0800
Subject: [PATCH] Fix Alter Subscription Add/Drop Publication refresh behavior.

The current refresh behavior tries to just refresh added/dropped
publications but that leads to removing wrong tables from subscription. We
can't refresh just the dropped publication because it is quite possible
that some of the tables are removed from publication by that time and now
those will remain as part of the subscription. Also, there is a chance
that the tables that were part of the publication being dropped are also
part of another publication, so we can't remove those.

So, we decided that by default, add/drop commands will also act like
REFRESH PUBLICATION which means they will refresh all the publications. We
can keep the old behavior for "add publication" but it is better to be
consistent with "drop publication".

Author: Hou Zhijie
Reviewed-by: Masahiko Sawada, Amit Kapila
Backpatch-through: 14, where it was introduced
Discussion: https://postgr.es/m/OS0PR01MB5716935D4C2CC85A6143073F94EF9@OS0PR01MB5716.jpnprd01.prod.outlook.com
---
 doc/src/sgml/ref/alter_subscription.sgml     |  7 +-
 src/backend/commands/subscriptioncmds.c      |  8 +--
 src/bin/psql/tab-complete.c                  |  8 +--
 src/test/regress/expected/subscription.out   |  3 -
 src/test/regress/sql/subscription.sql        |  3 -
 src/test/subscription/t/024_alter_sub_pub.pl | 99 ++++++++++++++++++++++++++++
 6 files changed, 106 insertions(+), 22 deletions(-)
 create mode 100644 src/test/subscription/t/024_alter_sub_pub.pl

diff --git a/doc/src/sgml/ref/alter_subscription.sgml b/doc/src/sgml/ref/alter_subscription.sgml
index a6f9944..835be0d 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>
@@ -134,8 +132,7 @@ ALTER SUBSCRIPTION <replaceable class="parameter">name</replaceable> RENAME TO <
       </variablelist>
 
       Additionally, refresh options as described
-      under <literal>REFRESH PUBLICATION</literal> may be specified,
-      except in the case of <literal>DROP PUBLICATION</literal>.
+      under <literal>REFRESH PUBLICATION</literal> may be specified.
      </para>
     </listitem>
    </varlistentry>
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index 5157f44..8aaa735 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/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 0750f70..b48d193 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -1675,14 +1675,10 @@ psql_completion(const char *text, int start, int end)
 	else if (HeadMatches("ALTER", "SUBSCRIPTION", MatchAny) &&
 			 TailMatches("ADD|DROP|SET", "PUBLICATION", MatchAny))
 		COMPLETE_WITH("WITH (");
-	/* ALTER SUBSCRIPTION <name> ADD|SET PUBLICATION <name> WITH ( */
+	/* ALTER SUBSCRIPTION <name> ADD|DROP|SET PUBLICATION <name> WITH ( */
 	else if (HeadMatches("ALTER", "SUBSCRIPTION", MatchAny) &&
-			 TailMatches("ADD|SET", "PUBLICATION", MatchAny, "WITH", "("))
+			 TailMatches("ADD|DROP|SET", "PUBLICATION", MatchAny, "WITH", "("))
 		COMPLETE_WITH("copy_data", "refresh");
-	/* ALTER SUBSCRIPTION <name> DROP PUBLICATION <name> WITH ( */
-	else if (HeadMatches("ALTER", "SUBSCRIPTION", MatchAny) &&
-			 TailMatches("DROP", "PUBLICATION", MatchAny, "WITH", "("))
-		COMPLETE_WITH("refresh");
 
 	/* ALTER SCHEMA <name> */
 	else if (Matches("ALTER", "SCHEMA", MatchAny))
diff --git a/src/test/regress/expected/subscription.out b/src/test/regress/expected/subscription.out
index 77b4437..15a1ac6 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 d42104c..7faa935 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/024_alter_sub_pub.pl b/src/test/subscription/t/024_alter_sub_pub.pl
new file mode 100644
index 0000000..3550941
--- /dev/null
+++ b/src/test/subscription/t/024_alter_sub_pub.pl
@@ -0,0 +1,99 @@
+
+# Copyright (c) 2021, PostgreSQL Global Development Group
+
+# This test checks behaviour of ALTER SUBSCRIPTION ... ADD/DROP PUBLICATION
+use strict;
+use warnings;
+use PostgresNode;
+use TestLib;
+use Test::More tests => 3;
+
+# Initialize publisher node
+my $node_publisher = PostgresNode->new('publisher');
+$node_publisher->init(allows_streaming => 'logical');
+$node_publisher->start;
+
+# Create subscriber node
+my $node_subscriber = PostgresNode->new('subscriber');
+$node_subscriber->init(allows_streaming => 'logical');
+$node_subscriber->start;
+
+# Create tables on publisher
+$node_publisher->safe_psql('postgres', "CREATE TABLE tab_readd_refresh (a int)");
+$node_publisher->safe_psql('postgres',
+	"INSERT INTO tab_readd_refresh SELECT generate_series(1,10)");
+
+# Create tables on subscriber
+$node_subscriber->safe_psql('postgres', "CREATE TABLE tab_readd_refresh (a int)");
+
+# Setup logical replication
+my $publisher_connstr = $node_publisher->connstr . ' dbname=postgres';
+$node_publisher->safe_psql('postgres',
+	"CREATE PUBLICATION tap_pub_readd FOR TABLE tab_readd_refresh");
+$node_publisher->safe_psql('postgres',
+	"CREATE PUBLICATION tap_pub");
+
+$node_subscriber->safe_psql('postgres',
+	"CREATE SUBSCRIPTION tap_sub CONNECTION '$publisher_connstr' PUBLICATION tap_pub_readd, tap_pub"
+);
+
+# Wait for initial table sync to finish
+my $synced_query =
+  "SELECT count(1) = 0 FROM pg_subscription_rel WHERE srsubstate NOT IN ('r', 's');";
+$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_readd_refresh is copied to subscriber
+my $result = $node_subscriber->safe_psql('postgres',
+	"SELECT count(*), min(a), max(a) FROM tab_readd_refresh");
+is($result, qq(10|1|10), 'check initial data is copied to subscriber');
+
+# Create a new table on publisher
+$node_publisher->safe_psql('postgres', "CREATE TABLE tab_drop_refresh (a int)");
+$node_publisher->safe_psql('postgres',
+	"INSERT INTO tab_drop_refresh SELECT generate_series(1,10)");
+
+# Create a new table on subscriber
+$node_subscriber->safe_psql('postgres', "CREATE TABLE tab_drop_refresh (a int)");
+
+# Add the table to publication
+$node_publisher->safe_psql('postgres',
+	"ALTER PUBLICATION tap_pub ADD TABLE tab_drop_refresh");
+
+# Drop tap_pub_readd from publication list
+# the publication list should be refreshed
+$node_subscriber->safe_psql('postgres',
+	"ALTER SUBSCRIPTION tap_sub DROP PUBLICATION tap_pub_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_drop_refresh was copied to subscriber
+$result = $node_subscriber->safe_psql('postgres',
+	"SELECT count(*), min(a), max(a) FROM tab_drop_refresh");
+is($result, qq(10|1|10), 'check initial data is copied to subscriber');
+
+# Re-add tap_pub_readd to publication list
+# the publication list should be refreshed again
+$node_subscriber->safe_psql('postgres',
+	"ALTER SUBSCRIPTION tap_sub ADD PUBLICATION tap_pub_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_readd_refresh was copied to subscriber again
+$result = $node_subscriber->safe_psql('postgres',
+	"SELECT count(*), min(a), max(a) FROM tab_readd_refresh");
+is($result, qq(20|1|10), 'check initial data is copied to subscriber');
+
+# shutdown
+$node_subscriber->stop('fast');
+$node_publisher->stop('fast');
-- 
2.7.2.windows.1

From bd16fa3b8c7e30db04d30f89244b3a0f3dcea061 Mon Sep 17 00:00:00 2001
From: Amit Kapila <akapila@postgresql.org>
Date: Mon, 23 Aug 2021 17:05:07 +0530
Subject: [PATCH v6] Fix Alter Subscription Add/Drop Publication refresh
 behavior.

The current refresh behavior tries to just refresh added/dropped
publications but that leads to removing wrong tables from subscription. We
can't refresh just the dropped publication because it is quite possible
that some of the tables are removed from publication by that time and now
those will remain as part of the subscription. Also, there is a chance
that the tables that were part of the publication being dropped are also
part of another publication, so we can't remove those.

So, we decided that by default, add/drop commands will also act like
REFRESH PUBLICATION which means they will refresh all the publications. We
can keep the old behavior for "add publication" but it is better to be
consistent with "drop publication".

Author: Hou Zhijie
Reviewed-by: Masahiko Sawada, Amit Kapila
Backpatch-through: 14, where it was introduced
Discussion: https://postgr.es/m/OS0PR01MB5716935D4C2CC85A6143073F94EF9@OS0PR01MB5716.jpnprd01.prod.outlook.com
---
 doc/src/sgml/ref/alter_subscription.sgml    |  7 +-
 src/backend/commands/subscriptioncmds.c     |  9 +-
 src/bin/psql/tab-complete.c                 |  8 +-
 src/test/regress/expected/subscription.out  |  3 -
 src/test/regress/sql/subscription.sql       |  3 -
 src/test/subscription/t/024_add_drop_pub.pl | 98 +++++++++++++++++++++
 6 files changed, 105 insertions(+), 23 deletions(-)
 create mode 100644 src/test/subscription/t/024_add_drop_pub.pl

diff --git a/doc/src/sgml/ref/alter_subscription.sgml b/doc/src/sgml/ref/alter_subscription.sgml
index a6f994450d..835be0d2a4 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>
@@ -134,8 +132,7 @@ ALTER SUBSCRIPTION <replaceable class="parameter">name</replaceable> RENAME TO <
       </variablelist>
 
       Additionally, refresh options as described
-      under <literal>REFRESH PUBLICATION</literal> may be specified,
-      except in the case of <literal>DROP PUBLICATION</literal>.
+      under <literal>REFRESH PUBLICATION</literal> may be specified.
      </para>
     </listitem>
    </varlistentry>
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index 5157f44058..c47ba26369 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -1006,10 +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 +1039,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/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 0750f70273..b48d193595 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -1675,14 +1675,10 @@ psql_completion(const char *text, int start, int end)
 	else if (HeadMatches("ALTER", "SUBSCRIPTION", MatchAny) &&
 			 TailMatches("ADD|DROP|SET", "PUBLICATION", MatchAny))
 		COMPLETE_WITH("WITH (");
-	/* ALTER SUBSCRIPTION <name> ADD|SET PUBLICATION <name> WITH ( */
+	/* ALTER SUBSCRIPTION <name> ADD|DROP|SET PUBLICATION <name> WITH ( */
 	else if (HeadMatches("ALTER", "SUBSCRIPTION", MatchAny) &&
-			 TailMatches("ADD|SET", "PUBLICATION", MatchAny, "WITH", "("))
+			 TailMatches("ADD|DROP|SET", "PUBLICATION", MatchAny, "WITH", "("))
 		COMPLETE_WITH("copy_data", "refresh");
-	/* ALTER SUBSCRIPTION <name> DROP PUBLICATION <name> WITH ( */
-	else if (HeadMatches("ALTER", "SUBSCRIPTION", MatchAny) &&
-			 TailMatches("DROP", "PUBLICATION", MatchAny, "WITH", "("))
-		COMPLETE_WITH("refresh");
 
 	/* ALTER SCHEMA <name> */
 	else if (Matches("ALTER", "SCHEMA", MatchAny))
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/024_add_drop_pub.pl b/src/test/subscription/t/024_add_drop_pub.pl
new file mode 100644
index 0000000000..24493a9c4e
--- /dev/null
+++ b/src/test/subscription/t/024_add_drop_pub.pl
@@ -0,0 +1,98 @@
+
+# Copyright (c) 2021, PostgreSQL Global Development Group
+
+# This test checks behaviour of ALTER SUBSCRIPTION ... ADD/DROP PUBLICATION
+use strict;
+use warnings;
+use PostgresNode;
+use TestLib;
+use Test::More tests => 3;
+
+# Initialize publisher node
+my $node_publisher = PostgresNode->new('publisher');
+$node_publisher->init(allows_streaming => 'logical');
+$node_publisher->start;
+
+# Create subscriber node
+my $node_subscriber = PostgresNode->new('subscriber');
+$node_subscriber->init(allows_streaming => 'logical');
+$node_subscriber->start;
+
+# Create table on publisher
+$node_publisher->safe_psql('postgres', "CREATE TABLE tab_1 (a int)");
+$node_publisher->safe_psql('postgres',
+	"INSERT INTO tab_1 SELECT generate_series(1,10)");
+
+# Create table on subscriber
+$node_subscriber->safe_psql('postgres', "CREATE TABLE tab_1 (a int)");
+
+# Setup logical replication
+my $publisher_connstr = $node_publisher->connstr . ' dbname=postgres';
+$node_publisher->safe_psql('postgres',
+	"CREATE PUBLICATION tap_pub_1 FOR TABLE tab_1");
+$node_publisher->safe_psql('postgres',
+	"CREATE PUBLICATION tap_pub_2");
+
+$node_subscriber->safe_psql('postgres',
+	"CREATE SUBSCRIPTION tap_sub CONNECTION '$publisher_connstr' PUBLICATION tap_pub_1, tap_pub_2"
+);
+
+# Wait for initial table sync to finish
+my $synced_query =
+  "SELECT count(1) = 0 FROM pg_subscription_rel WHERE srsubstate NOT IN ('r', 's');";
+
+$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_1 is copied to subscriber
+my $result = $node_subscriber->safe_psql('postgres',
+	"SELECT count(*), min(a), max(a) FROM tab_1");
+is($result, qq(10|1|10), 'check initial data is copied to subscriber');
+
+# Create a new table on publisher
+$node_publisher->safe_psql('postgres', "CREATE TABLE tab_2 (a int)");
+$node_publisher->safe_psql('postgres',
+	"INSERT INTO tab_2 SELECT generate_series(1,10)");
+
+# Create a new table on subscriber
+$node_subscriber->safe_psql('postgres', "CREATE TABLE tab_2 (a int)");
+
+# Add the table to publication
+$node_publisher->safe_psql('postgres',
+	"ALTER PUBLICATION tap_pub_2 ADD TABLE tab_2");
+
+# Dropping tap_pub_1 will refresh the entire publication list
+$node_subscriber->safe_psql('postgres',
+	"ALTER SUBSCRIPTION tap_sub DROP PUBLICATION tap_pub_1");
+
+# 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_drop_refresh was copied to subscriber
+$result = $node_subscriber->safe_psql('postgres',
+	"SELECT count(*), min(a), max(a) FROM tab_2");
+is($result, qq(10|1|10), 'check initial data is copied to subscriber');
+
+# Re-adding tap_pub_1 will refresh the entire publication list
+$node_subscriber->safe_psql('postgres',
+	"ALTER SUBSCRIPTION tap_sub ADD PUBLICATION tap_pub_1");
+
+# 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_1 was copied to subscriber again
+$result = $node_subscriber->safe_psql('postgres',
+	"SELECT count(*), min(a), max(a) FROM tab_1");
+is($result, qq(20|1|10), 'check initial data is copied to subscriber');
+
+# shutdown
+$node_subscriber->stop('fast');
+$node_publisher->stop('fast');
-- 
2.28.0.windows.1

From 478c73a0943cfd1b2bf7683384dd067a920f3fc7 Mon Sep 17 00:00:00 2001
From: "houzj.fnst" <houzj.fnst@cn.fujitsu.com>
Date: Mon, 23 Aug 2021 21:49:30 +0800
Subject: [PATCH] Fix Alter Subscription Add/Drop Publication refresh behavior.

The current refresh behavior tries to just refresh added/dropped
publications but that leads to removing wrong tables from subscription. We
can't refresh just the dropped publication because it is quite possible
that some of the tables are removed from publication by that time and now
those will remain as part of the subscription. Also, there is a chance
that the tables that were part of the publication being dropped are also
part of another publication, so we can't remove those.

So, we decided that by default, add/drop commands will also act like
REFRESH PUBLICATION which means they will refresh all the publications. We
can keep the old behavior for "add publication" but it is better to be
consistent with "drop publication".

Author: Hou Zhijie
Reviewed-by: Masahiko Sawada, Amit Kapila
Backpatch-through: 14, where it was introduced
Discussion: https://postgr.es/m/OS0PR01MB5716935D4C2CC85A6143073F94EF9@OS0PR01MB5716.jpnprd01.prod.outlook.com
---
 doc/src/sgml/ref/alter_subscription.sgml     |  7 +-
 src/backend/commands/subscriptioncmds.c      |  5 +-
 src/bin/psql/tab-complete.c                  |  8 +--
 src/test/regress/expected/subscription.out   |  3 -
 src/test/regress/sql/subscription.sql        |  3 -
 src/test/subscription/t/021_alter_sub_pub.pl | 98 ++++++++++++++++++++++++++++
 6 files changed, 104 insertions(+), 20 deletions(-)
 create mode 100644 src/test/subscription/t/021_alter_sub_pub.pl

diff --git a/doc/src/sgml/ref/alter_subscription.sgml b/doc/src/sgml/ref/alter_subscription.sgml
index b3d1731..acccde9 100644
--- a/doc/src/sgml/ref/alter_subscription.sgml
+++ b/doc/src/sgml/ref/alter_subscription.sgml
@@ -106,9 +106,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>
@@ -129,8 +127,7 @@ ALTER SUBSCRIPTION <replaceable class="parameter">name</replaceable> RENAME TO <
       </variablelist>
 
       Additionally, refresh options as described
-      under <literal>REFRESH PUBLICATION</literal> may be specified,
-      except in the case of <literal>DROP PUBLICATION</literal>.
+      under <literal>REFRESH PUBLICATION</literal> may be specified.
      </para>
     </listitem>
    </varlistentry>
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index e096d5f..0b3a17e 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -960,8 +960,7 @@ AlterSubscription(AlterSubscriptionStmt *stmt, bool isTopLevel)
 										   NULL, NULL,	/* no "enabled" */
 										   NULL,	/* no "create_slot" */
 										   NULL, NULL,	/* no "slot_name" */
-										   isadd ? &copy_data : NULL,	/* for drop, no
-																		 * "copy_data" */
+										   &copy_data,
 										   NULL,	/* no "synchronous_commit" */
 										   &refresh,
 										   NULL, NULL,	/* no "binary" */
@@ -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;
 
 					AlterSubscription_refresh(sub, copy_data);
 				}
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 0ebd5aa..38af568 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -1675,14 +1675,10 @@ psql_completion(const char *text, int start, int end)
 	else if (HeadMatches("ALTER", "SUBSCRIPTION", MatchAny) &&
 			 TailMatches("ADD|DROP|SET", "PUBLICATION", MatchAny))
 		COMPLETE_WITH("WITH (");
-	/* ALTER SUBSCRIPTION <name> ADD|SET PUBLICATION <name> WITH ( */
+	/* ALTER SUBSCRIPTION <name> ADD|DROP|SET PUBLICATION <name> WITH ( */
 	else if (HeadMatches("ALTER", "SUBSCRIPTION", MatchAny) &&
-			 TailMatches("ADD|SET", "PUBLICATION", MatchAny, "WITH", "("))
+			 TailMatches("ADD|DROP|SET", "PUBLICATION", MatchAny, "WITH", "("))
 		COMPLETE_WITH("copy_data", "refresh");
-	/* ALTER SUBSCRIPTION <name> DROP PUBLICATION <name> WITH ( */
-	else if (HeadMatches("ALTER", "SUBSCRIPTION", MatchAny) &&
-			 TailMatches("DROP", "PUBLICATION", MatchAny, "WITH", "("))
-		COMPLETE_WITH("refresh");
 
 	/* ALTER SCHEMA <name> */
 	else if (Matches("ALTER", "SCHEMA", MatchAny))
diff --git a/src/test/regress/expected/subscription.out b/src/test/regress/expected/subscription.out
index 4ac7259..590d7f1 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 c9d1bd8..855a341 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/021_alter_sub_pub.pl b/src/test/subscription/t/021_alter_sub_pub.pl
new file mode 100644
index 0000000..104eddb
--- /dev/null
+++ b/src/test/subscription/t/021_alter_sub_pub.pl
@@ -0,0 +1,98 @@
+
+# Copyright (c) 2021, PostgreSQL Global Development Group
+
+# This test checks behaviour of ALTER SUBSCRIPTION ... ADD/DROP PUBLICATION
+use strict;
+use warnings;
+use PostgresNode;
+use TestLib;
+use Test::More tests => 3;
+
+# Initialize publisher node
+my $node_publisher = get_new_node('publisher');
+$node_publisher->init(allows_streaming => 'logical');
+$node_publisher->start;
+
+# Create subscriber node
+my $node_subscriber = get_new_node('subscriber');
+$node_subscriber->init(allows_streaming => 'logical');
+$node_subscriber->start;
+
+# Create table on publisher
+$node_publisher->safe_psql('postgres', "CREATE TABLE tab_1 (a int)");
+$node_publisher->safe_psql('postgres',
+	"INSERT INTO tab_1 SELECT generate_series(1,10)");
+
+# Create table on subscriber
+$node_subscriber->safe_psql('postgres', "CREATE TABLE tab_1 (a int)");
+
+# Setup logical replication
+my $publisher_connstr = $node_publisher->connstr . ' dbname=postgres';
+$node_publisher->safe_psql('postgres',
+	"CREATE PUBLICATION tap_pub_1 FOR TABLE tab_1");
+$node_publisher->safe_psql('postgres',
+	"CREATE PUBLICATION tap_pub_2");
+
+$node_subscriber->safe_psql('postgres',
+	"CREATE SUBSCRIPTION tap_sub CONNECTION '$publisher_connstr' PUBLICATION tap_pub_1, tap_pub_2"
+);
+
+# Wait for initial table sync to finish
+my $synced_query =
+  "SELECT count(1) = 0 FROM pg_subscription_rel WHERE srsubstate NOT IN ('r', 's');";
+
+$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_1 is copied to subscriber
+my $result = $node_subscriber->safe_psql('postgres',
+	"SELECT count(*), min(a), max(a) FROM tab_1");
+is($result, qq(10|1|10), 'check initial data is copied to subscriber');
+
+# Create a new table on publisher
+$node_publisher->safe_psql('postgres', "CREATE TABLE tab_2 (a int)");
+$node_publisher->safe_psql('postgres',
+	"INSERT INTO tab_2 SELECT generate_series(1,10)");
+
+# Create a new table on subscriber
+$node_subscriber->safe_psql('postgres', "CREATE TABLE tab_2 (a int)");
+
+# Add the table to publication
+$node_publisher->safe_psql('postgres',
+	"ALTER PUBLICATION tap_pub_2 ADD TABLE tab_2");
+
+# Dropping tap_pub_1 will refresh the entire publication list
+$node_subscriber->safe_psql('postgres',
+	"ALTER SUBSCRIPTION tap_sub DROP PUBLICATION tap_pub_1");
+
+# 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_drop_refresh was copied to subscriber
+$result = $node_subscriber->safe_psql('postgres',
+	"SELECT count(*), min(a), max(a) FROM tab_2");
+is($result, qq(10|1|10), 'check initial data is copied to subscriber');
+
+# Re-adding tap_pub_1 will refresh the entire publication list
+$node_subscriber->safe_psql('postgres',
+	"ALTER SUBSCRIPTION tap_sub ADD PUBLICATION tap_pub_1");
+
+# 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_1 was copied to subscriber again
+$result = $node_subscriber->safe_psql('postgres',
+	"SELECT count(*), min(a), max(a) FROM tab_1");
+is($result, qq(20|1|10), 'check initial data is copied to subscriber');
+
+# shutdown
+$node_subscriber->stop('fast');
+$node_publisher->stop('fast');
-- 
2.7.2.windows.1

From bd16fa3b8c7e30db04d30f89244b3a0f3dcea061 Mon Sep 17 00:00:00 2001
From: Amit Kapila <akapila@postgresql.org>
Date: Mon, 23 Aug 2021 17:05:07 +0530
Subject: [PATCH v7] Fix Alter Subscription Add/Drop Publication refresh
 behavior.

The current refresh behavior tries to just refresh added/dropped
publications but that leads to removing wrong tables from subscription. We
can't refresh just the dropped publication because it is quite possible
that some of the tables are removed from publication by that time and now
those will remain as part of the subscription. Also, there is a chance
that the tables that were part of the publication being dropped are also
part of another publication, so we can't remove those.

So, we decided that by default, add/drop commands will also act like
REFRESH PUBLICATION which means they will refresh all the publications. We
can keep the old behavior for "add publication" but it is better to be
consistent with "drop publication".

Author: Hou Zhijie
Reviewed-by: Masahiko Sawada, Amit Kapila
Backpatch-through: 14, where it was introduced
Discussion: https://postgr.es/m/OS0PR01MB5716935D4C2CC85A6143073F94EF9@OS0PR01MB5716.jpnprd01.prod.outlook.com
---
 doc/src/sgml/ref/alter_subscription.sgml    |  7 +-
 src/backend/commands/subscriptioncmds.c     |  9 +-
 src/bin/psql/tab-complete.c                 |  8 +-
 src/test/regress/expected/subscription.out  |  3 -
 src/test/regress/sql/subscription.sql       |  3 -
 src/test/subscription/t/024_add_drop_pub.pl | 98 +++++++++++++++++++++
 6 files changed, 105 insertions(+), 23 deletions(-)
 create mode 100644 src/test/subscription/t/024_add_drop_pub.pl

diff --git a/doc/src/sgml/ref/alter_subscription.sgml b/doc/src/sgml/ref/alter_subscription.sgml
index a6f994450d..835be0d2a4 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>
@@ -134,8 +132,7 @@ ALTER SUBSCRIPTION <replaceable class="parameter">name</replaceable> RENAME TO <
       </variablelist>
 
       Additionally, refresh options as described
-      under <literal>REFRESH PUBLICATION</literal> may be specified,
-      except in the case of <literal>DROP PUBLICATION</literal>.
+      under <literal>REFRESH PUBLICATION</literal> may be specified.
      </para>
     </listitem>
    </varlistentry>
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index 5157f44058..c47ba26369 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -1006,10 +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 +1039,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/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 0750f70273..b48d193595 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -1675,14 +1675,10 @@ psql_completion(const char *text, int start, int end)
 	else if (HeadMatches("ALTER", "SUBSCRIPTION", MatchAny) &&
 			 TailMatches("ADD|DROP|SET", "PUBLICATION", MatchAny))
 		COMPLETE_WITH("WITH (");
-	/* ALTER SUBSCRIPTION <name> ADD|SET PUBLICATION <name> WITH ( */
+	/* ALTER SUBSCRIPTION <name> ADD|DROP|SET PUBLICATION <name> WITH ( */
 	else if (HeadMatches("ALTER", "SUBSCRIPTION", MatchAny) &&
-			 TailMatches("ADD|SET", "PUBLICATION", MatchAny, "WITH", "("))
+			 TailMatches("ADD|DROP|SET", "PUBLICATION", MatchAny, "WITH", "("))
 		COMPLETE_WITH("copy_data", "refresh");
-	/* ALTER SUBSCRIPTION <name> DROP PUBLICATION <name> WITH ( */
-	else if (HeadMatches("ALTER", "SUBSCRIPTION", MatchAny) &&
-			 TailMatches("DROP", "PUBLICATION", MatchAny, "WITH", "("))
-		COMPLETE_WITH("refresh");
 
 	/* ALTER SCHEMA <name> */
 	else if (Matches("ALTER", "SCHEMA", MatchAny))
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/024_add_drop_pub.pl b/src/test/subscription/t/024_add_drop_pub.pl
new file mode 100644
index 0000000000..24493a9c4e
--- /dev/null
+++ b/src/test/subscription/t/024_add_drop_pub.pl
@@ -0,0 +1,98 @@
+
+# Copyright (c) 2021, PostgreSQL Global Development Group
+
+# This test checks behaviour of ALTER SUBSCRIPTION ... ADD/DROP PUBLICATION
+use strict;
+use warnings;
+use PostgresNode;
+use TestLib;
+use Test::More tests => 3;
+
+# Initialize publisher node
+my $node_publisher = PostgresNode->new('publisher');
+$node_publisher->init(allows_streaming => 'logical');
+$node_publisher->start;
+
+# Create subscriber node
+my $node_subscriber = PostgresNode->new('subscriber');
+$node_subscriber->init(allows_streaming => 'logical');
+$node_subscriber->start;
+
+# Create table on publisher
+$node_publisher->safe_psql('postgres', "CREATE TABLE tab_1 (a int)");
+$node_publisher->safe_psql('postgres',
+	"INSERT INTO tab_1 SELECT generate_series(1,10)");
+
+# Create table on subscriber
+$node_subscriber->safe_psql('postgres', "CREATE TABLE tab_1 (a int)");
+
+# Setup logical replication
+my $publisher_connstr = $node_publisher->connstr . ' dbname=postgres';
+$node_publisher->safe_psql('postgres',
+	"CREATE PUBLICATION tap_pub_1 FOR TABLE tab_1");
+$node_publisher->safe_psql('postgres',
+	"CREATE PUBLICATION tap_pub_2");
+
+$node_subscriber->safe_psql('postgres',
+	"CREATE SUBSCRIPTION tap_sub CONNECTION '$publisher_connstr' PUBLICATION tap_pub_1, tap_pub_2"
+);
+
+# Wait for initial table sync to finish
+my $synced_query =
+  "SELECT count(1) = 0 FROM pg_subscription_rel WHERE srsubstate NOT IN ('r', 's');";
+
+$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_1 is copied to subscriber
+my $result = $node_subscriber->safe_psql('postgres',
+	"SELECT count(*), min(a), max(a) FROM tab_1");
+is($result, qq(10|1|10), 'check initial data is copied to subscriber');
+
+# Create a new table on publisher
+$node_publisher->safe_psql('postgres', "CREATE TABLE tab_2 (a int)");
+$node_publisher->safe_psql('postgres',
+	"INSERT INTO tab_2 SELECT generate_series(1,10)");
+
+# Create a new table on subscriber
+$node_subscriber->safe_psql('postgres', "CREATE TABLE tab_2 (a int)");
+
+# Add the table to publication
+$node_publisher->safe_psql('postgres',
+	"ALTER PUBLICATION tap_pub_2 ADD TABLE tab_2");
+
+# Dropping tap_pub_1 will refresh the entire publication list
+$node_subscriber->safe_psql('postgres',
+	"ALTER SUBSCRIPTION tap_sub DROP PUBLICATION tap_pub_1");
+
+# 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_drop_refresh was copied to subscriber
+$result = $node_subscriber->safe_psql('postgres',
+	"SELECT count(*), min(a), max(a) FROM tab_2");
+is($result, qq(10|1|10), 'check initial data is copied to subscriber');
+
+# Re-adding tap_pub_1 will refresh the entire publication list
+$node_subscriber->safe_psql('postgres',
+	"ALTER SUBSCRIPTION tap_sub ADD PUBLICATION tap_pub_1");
+
+# 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_1 was copied to subscriber again
+$result = $node_subscriber->safe_psql('postgres',
+	"SELECT count(*), min(a), max(a) FROM tab_1");
+is($result, qq(20|1|10), 'check initial data is copied to subscriber');
+
+# shutdown
+$node_subscriber->stop('fast');
+$node_publisher->stop('fast');
-- 
2.28.0.windows.1

From 607e99ad8a2312ec0e7bac67a64473b661923538 Mon Sep 17 00:00:00 2001
From: "houzj.fnst" <houzj.fnst@cn.fujitsu.com>
Date: Tue, 24 Aug 2021 08:55:58 +0800
Subject: [PATCH] Fix Alter Subscription Add/Drop Publication refresh behavior.

The current refresh behavior tries to just refresh added/dropped
publications but that leads to removing wrong tables from subscription. We
can't refresh just the dropped publication because it is quite possible
that some of the tables are removed from publication by that time and now
those will remain as part of the subscription. Also, there is a chance
that the tables that were part of the publication being dropped are also
part of another publication, so we can't remove those.

So, we decided that by default, add/drop commands will also act like
REFRESH PUBLICATION which means they will refresh all the publications. We
can keep the old behavior for "add publication" but it is better to be
consistent with "drop publication".

Author: Hou Zhijie
Reviewed-by: Masahiko Sawada, Amit Kapila
Backpatch-through: 14, where it was introduced
Discussion: https://postgr.es/m/OS0PR01MB5716935D4C2CC85A6143073F94EF9@OS0PR01MB5716.jpnprd01.prod.outlook.com
---
 doc/src/sgml/ref/alter_subscription.sgml     |  7 +-
 src/backend/commands/subscriptioncmds.c      |  7 +-
 src/bin/psql/tab-complete.c                  |  8 +--
 src/test/regress/expected/subscription.out   |  3 -
 src/test/regress/sql/subscription.sql        |  3 -
 src/test/subscription/t/021_alter_sub_pub.pl | 98 ++++++++++++++++++++++++++++
 6 files changed, 105 insertions(+), 21 deletions(-)
 create mode 100644 src/test/subscription/t/021_alter_sub_pub.pl

diff --git a/doc/src/sgml/ref/alter_subscription.sgml b/doc/src/sgml/ref/alter_subscription.sgml
index b3d1731..acccde9 100644
--- a/doc/src/sgml/ref/alter_subscription.sgml
+++ b/doc/src/sgml/ref/alter_subscription.sgml
@@ -106,9 +106,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>
@@ -129,8 +127,7 @@ ALTER SUBSCRIPTION <replaceable class="parameter">name</replaceable> RENAME TO <
       </variablelist>
 
       Additionally, refresh options as described
-      under <literal>REFRESH PUBLICATION</literal> may be specified,
-      except in the case of <literal>DROP PUBLICATION</literal>.
+      under <literal>REFRESH PUBLICATION</literal> may be specified.
      </para>
     </listitem>
    </varlistentry>
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index e096d5f..1719f04 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -960,8 +960,7 @@ AlterSubscription(AlterSubscriptionStmt *stmt, bool isTopLevel)
 										   NULL, NULL,	/* no "enabled" */
 										   NULL,	/* no "create_slot" */
 										   NULL, NULL,	/* no "slot_name" */
-										   isadd ? &copy_data : NULL,	/* for drop, no
-																		 * "copy_data" */
+										   &copy_data,
 										   NULL,	/* no "synchronous_commit" */
 										   &refresh,
 										   NULL, NULL,	/* no "binary" */
@@ -986,8 +985,8 @@ AlterSubscription(AlterSubscriptionStmt *stmt, bool isTopLevel)
 
 					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, copy_data);
 				}
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 0ebd5aa..38af568 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -1675,14 +1675,10 @@ psql_completion(const char *text, int start, int end)
 	else if (HeadMatches("ALTER", "SUBSCRIPTION", MatchAny) &&
 			 TailMatches("ADD|DROP|SET", "PUBLICATION", MatchAny))
 		COMPLETE_WITH("WITH (");
-	/* ALTER SUBSCRIPTION <name> ADD|SET PUBLICATION <name> WITH ( */
+	/* ALTER SUBSCRIPTION <name> ADD|DROP|SET PUBLICATION <name> WITH ( */
 	else if (HeadMatches("ALTER", "SUBSCRIPTION", MatchAny) &&
-			 TailMatches("ADD|SET", "PUBLICATION", MatchAny, "WITH", "("))
+			 TailMatches("ADD|DROP|SET", "PUBLICATION", MatchAny, "WITH", "("))
 		COMPLETE_WITH("copy_data", "refresh");
-	/* ALTER SUBSCRIPTION <name> DROP PUBLICATION <name> WITH ( */
-	else if (HeadMatches("ALTER", "SUBSCRIPTION", MatchAny) &&
-			 TailMatches("DROP", "PUBLICATION", MatchAny, "WITH", "("))
-		COMPLETE_WITH("refresh");
 
 	/* ALTER SCHEMA <name> */
 	else if (Matches("ALTER", "SCHEMA", MatchAny))
diff --git a/src/test/regress/expected/subscription.out b/src/test/regress/expected/subscription.out
index 4ac7259..590d7f1 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 c9d1bd8..855a341 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/021_alter_sub_pub.pl b/src/test/subscription/t/021_alter_sub_pub.pl
new file mode 100644
index 0000000..104eddb
--- /dev/null
+++ b/src/test/subscription/t/021_alter_sub_pub.pl
@@ -0,0 +1,98 @@
+
+# Copyright (c) 2021, PostgreSQL Global Development Group
+
+# This test checks behaviour of ALTER SUBSCRIPTION ... ADD/DROP PUBLICATION
+use strict;
+use warnings;
+use PostgresNode;
+use TestLib;
+use Test::More tests => 3;
+
+# Initialize publisher node
+my $node_publisher = get_new_node('publisher');
+$node_publisher->init(allows_streaming => 'logical');
+$node_publisher->start;
+
+# Create subscriber node
+my $node_subscriber = get_new_node('subscriber');
+$node_subscriber->init(allows_streaming => 'logical');
+$node_subscriber->start;
+
+# Create table on publisher
+$node_publisher->safe_psql('postgres', "CREATE TABLE tab_1 (a int)");
+$node_publisher->safe_psql('postgres',
+	"INSERT INTO tab_1 SELECT generate_series(1,10)");
+
+# Create table on subscriber
+$node_subscriber->safe_psql('postgres', "CREATE TABLE tab_1 (a int)");
+
+# Setup logical replication
+my $publisher_connstr = $node_publisher->connstr . ' dbname=postgres';
+$node_publisher->safe_psql('postgres',
+	"CREATE PUBLICATION tap_pub_1 FOR TABLE tab_1");
+$node_publisher->safe_psql('postgres',
+	"CREATE PUBLICATION tap_pub_2");
+
+$node_subscriber->safe_psql('postgres',
+	"CREATE SUBSCRIPTION tap_sub CONNECTION '$publisher_connstr' PUBLICATION tap_pub_1, tap_pub_2"
+);
+
+# Wait for initial table sync to finish
+my $synced_query =
+  "SELECT count(1) = 0 FROM pg_subscription_rel WHERE srsubstate NOT IN ('r', 's');";
+
+$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_1 is copied to subscriber
+my $result = $node_subscriber->safe_psql('postgres',
+	"SELECT count(*), min(a), max(a) FROM tab_1");
+is($result, qq(10|1|10), 'check initial data is copied to subscriber');
+
+# Create a new table on publisher
+$node_publisher->safe_psql('postgres', "CREATE TABLE tab_2 (a int)");
+$node_publisher->safe_psql('postgres',
+	"INSERT INTO tab_2 SELECT generate_series(1,10)");
+
+# Create a new table on subscriber
+$node_subscriber->safe_psql('postgres', "CREATE TABLE tab_2 (a int)");
+
+# Add the table to publication
+$node_publisher->safe_psql('postgres',
+	"ALTER PUBLICATION tap_pub_2 ADD TABLE tab_2");
+
+# Dropping tap_pub_1 will refresh the entire publication list
+$node_subscriber->safe_psql('postgres',
+	"ALTER SUBSCRIPTION tap_sub DROP PUBLICATION tap_pub_1");
+
+# 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_drop_refresh was copied to subscriber
+$result = $node_subscriber->safe_psql('postgres',
+	"SELECT count(*), min(a), max(a) FROM tab_2");
+is($result, qq(10|1|10), 'check initial data is copied to subscriber');
+
+# Re-adding tap_pub_1 will refresh the entire publication list
+$node_subscriber->safe_psql('postgres',
+	"ALTER SUBSCRIPTION tap_sub ADD PUBLICATION tap_pub_1");
+
+# 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_1 was copied to subscriber again
+$result = $node_subscriber->safe_psql('postgres',
+	"SELECT count(*), min(a), max(a) FROM tab_1");
+is($result, qq(20|1|10), 'check initial data is copied to subscriber');
+
+# shutdown
+$node_subscriber->stop('fast');
+$node_publisher->stop('fast');
-- 
2.7.2.windows.1

From bd16fa3b8c7e30db04d30f89244b3a0f3dcea061 Mon Sep 17 00:00:00 2001
From: Amit Kapila <akapila@postgresql.org>
Date: Mon, 23 Aug 2021 17:05:07 +0530
Subject: [PATCH v7] Fix Alter Subscription Add/Drop Publication refresh
 behavior.

The current refresh behavior tries to just refresh added/dropped
publications but that leads to removing wrong tables from subscription. We
can't refresh just the dropped publication because it is quite possible
that some of the tables are removed from publication by that time and now
those will remain as part of the subscription. Also, there is a chance
that the tables that were part of the publication being dropped are also
part of another publication, so we can't remove those.

So, we decided that by default, add/drop commands will also act like
REFRESH PUBLICATION which means they will refresh all the publications. We
can keep the old behavior for "add publication" but it is better to be
consistent with "drop publication".

Author: Hou Zhijie
Reviewed-by: Masahiko Sawada, Amit Kapila
Backpatch-through: 14, where it was introduced
Discussion: https://postgr.es/m/OS0PR01MB5716935D4C2CC85A6143073F94EF9@OS0PR01MB5716.jpnprd01.prod.outlook.com
---
 doc/src/sgml/ref/alter_subscription.sgml    |  7 +-
 src/backend/commands/subscriptioncmds.c     |  9 +-
 src/bin/psql/tab-complete.c                 |  8 +-
 src/test/regress/expected/subscription.out  |  3 -
 src/test/regress/sql/subscription.sql       |  3 -
 src/test/subscription/t/024_add_drop_pub.pl | 98 +++++++++++++++++++++
 6 files changed, 105 insertions(+), 23 deletions(-)
 create mode 100644 src/test/subscription/t/024_add_drop_pub.pl

diff --git a/doc/src/sgml/ref/alter_subscription.sgml b/doc/src/sgml/ref/alter_subscription.sgml
index a6f994450d..835be0d2a4 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>
@@ -134,8 +132,7 @@ ALTER SUBSCRIPTION <replaceable class="parameter">name</replaceable> RENAME TO <
       </variablelist>
 
       Additionally, refresh options as described
-      under <literal>REFRESH PUBLICATION</literal> may be specified,
-      except in the case of <literal>DROP PUBLICATION</literal>.
+      under <literal>REFRESH PUBLICATION</literal> may be specified.
      </para>
     </listitem>
    </varlistentry>
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index 5157f44058..c47ba26369 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -1006,10 +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 +1039,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/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 0750f70273..b48d193595 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -1675,14 +1675,10 @@ psql_completion(const char *text, int start, int end)
 	else if (HeadMatches("ALTER", "SUBSCRIPTION", MatchAny) &&
 			 TailMatches("ADD|DROP|SET", "PUBLICATION", MatchAny))
 		COMPLETE_WITH("WITH (");
-	/* ALTER SUBSCRIPTION <name> ADD|SET PUBLICATION <name> WITH ( */
+	/* ALTER SUBSCRIPTION <name> ADD|DROP|SET PUBLICATION <name> WITH ( */
 	else if (HeadMatches("ALTER", "SUBSCRIPTION", MatchAny) &&
-			 TailMatches("ADD|SET", "PUBLICATION", MatchAny, "WITH", "("))
+			 TailMatches("ADD|DROP|SET", "PUBLICATION", MatchAny, "WITH", "("))
 		COMPLETE_WITH("copy_data", "refresh");
-	/* ALTER SUBSCRIPTION <name> DROP PUBLICATION <name> WITH ( */
-	else if (HeadMatches("ALTER", "SUBSCRIPTION", MatchAny) &&
-			 TailMatches("DROP", "PUBLICATION", MatchAny, "WITH", "("))
-		COMPLETE_WITH("refresh");
 
 	/* ALTER SCHEMA <name> */
 	else if (Matches("ALTER", "SCHEMA", MatchAny))
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/024_add_drop_pub.pl b/src/test/subscription/t/024_add_drop_pub.pl
new file mode 100644
index 0000000000..24493a9c4e
--- /dev/null
+++ b/src/test/subscription/t/024_add_drop_pub.pl
@@ -0,0 +1,98 @@
+
+# Copyright (c) 2021, PostgreSQL Global Development Group
+
+# This test checks behaviour of ALTER SUBSCRIPTION ... ADD/DROP PUBLICATION
+use strict;
+use warnings;
+use PostgresNode;
+use TestLib;
+use Test::More tests => 3;
+
+# Initialize publisher node
+my $node_publisher = PostgresNode->new('publisher');
+$node_publisher->init(allows_streaming => 'logical');
+$node_publisher->start;
+
+# Create subscriber node
+my $node_subscriber = PostgresNode->new('subscriber');
+$node_subscriber->init(allows_streaming => 'logical');
+$node_subscriber->start;
+
+# Create table on publisher
+$node_publisher->safe_psql('postgres', "CREATE TABLE tab_1 (a int)");
+$node_publisher->safe_psql('postgres',
+	"INSERT INTO tab_1 SELECT generate_series(1,10)");
+
+# Create table on subscriber
+$node_subscriber->safe_psql('postgres', "CREATE TABLE tab_1 (a int)");
+
+# Setup logical replication
+my $publisher_connstr = $node_publisher->connstr . ' dbname=postgres';
+$node_publisher->safe_psql('postgres',
+	"CREATE PUBLICATION tap_pub_1 FOR TABLE tab_1");
+$node_publisher->safe_psql('postgres',
+	"CREATE PUBLICATION tap_pub_2");
+
+$node_subscriber->safe_psql('postgres',
+	"CREATE SUBSCRIPTION tap_sub CONNECTION '$publisher_connstr' PUBLICATION tap_pub_1, tap_pub_2"
+);
+
+# Wait for initial table sync to finish
+my $synced_query =
+  "SELECT count(1) = 0 FROM pg_subscription_rel WHERE srsubstate NOT IN ('r', 's');";
+
+$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_1 is copied to subscriber
+my $result = $node_subscriber->safe_psql('postgres',
+	"SELECT count(*), min(a), max(a) FROM tab_1");
+is($result, qq(10|1|10), 'check initial data is copied to subscriber');
+
+# Create a new table on publisher
+$node_publisher->safe_psql('postgres', "CREATE TABLE tab_2 (a int)");
+$node_publisher->safe_psql('postgres',
+	"INSERT INTO tab_2 SELECT generate_series(1,10)");
+
+# Create a new table on subscriber
+$node_subscriber->safe_psql('postgres', "CREATE TABLE tab_2 (a int)");
+
+# Add the table to publication
+$node_publisher->safe_psql('postgres',
+	"ALTER PUBLICATION tap_pub_2 ADD TABLE tab_2");
+
+# Dropping tap_pub_1 will refresh the entire publication list
+$node_subscriber->safe_psql('postgres',
+	"ALTER SUBSCRIPTION tap_sub DROP PUBLICATION tap_pub_1");
+
+# 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_drop_refresh was copied to subscriber
+$result = $node_subscriber->safe_psql('postgres',
+	"SELECT count(*), min(a), max(a) FROM tab_2");
+is($result, qq(10|1|10), 'check initial data is copied to subscriber');
+
+# Re-adding tap_pub_1 will refresh the entire publication list
+$node_subscriber->safe_psql('postgres',
+	"ALTER SUBSCRIPTION tap_sub ADD PUBLICATION tap_pub_1");
+
+# 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_1 was copied to subscriber again
+$result = $node_subscriber->safe_psql('postgres',
+	"SELECT count(*), min(a), max(a) FROM tab_1");
+is($result, qq(20|1|10), 'check initial data is copied to subscriber');
+
+# shutdown
+$node_subscriber->stop('fast');
+$node_publisher->stop('fast');
-- 
2.28.0.windows.1