Fix Alter Subscription's Add/Drop Publication 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

diff --git a/doc/src/sgml/ref/alter_subscription.sgml b/doc/src/sgml/ref/alter_subscription.sgml
index b3d173179f4cbb79ef6c13c4d35f1b4e6178bd2b..acccde9b74f76c9841215b867beacf1ecf095efa 100644 (file)
--- 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 e096d5fd2d54c90bce95dbacc8db1afcac81116b..1719f045171159cee3bcae578629e2cd8dab3af7 100644 (file)
--- 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 0ebd5aa41a1ada838a3deb145f0afbae09f28d9d..38af5682f2db485a0c4b0eb47b88202a54fc3c3a 100644 (file)
--- 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 4ac7259f3a5b61f871c5a1f0789245b97dda374d..590d7f16dac423ddd39e5981664e1d567b246829 100644 (file)
--- 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 c9d1bd894881ae43c61aa19c8f99a671b241db82..855a341a3d90385420b3a8ee3fee4d6b93e0a556 100644 (file)
--- 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 (file)
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');