From 6fd398473ee27dd72f6b17d766a7bc12ff86b24f Mon Sep 17 00:00:00 2001 From: "houzj.fnst" 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 name RENAME TO < publications, and DROP removes the publications from the list of publications. See for more information. By default, this command will also act like - REFRESH PUBLICATION, except that in case of - ADD or DROP, only the added or - dropped publications are refreshed. + REFRESH PUBLICATION. 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