From 697c4cbdd386a4bd856614de6cedf5af8d1b63be Mon Sep 17 00:00:00 2001 From: Peter Eisentraut Date: Fri, 2 Jun 2017 21:59:07 -0400 Subject: [PATCH] Fix ALTER SUBSCRIPTION grammar ambiguity There was a grammar ambiguity between SET PUBLICATION name REFRESH and SET PUBLICATION SKIP REFRESH, because SKIP is not a reserved word. To resolve that, fold the refresh choice into the WITH options. Refreshing is the default now. Author: tushar --- doc/src/sgml/catalogs.sgml | 2 +- doc/src/sgml/ref/alter_subscription.sgml | 35 ++++++++++++++++++++---------- src/backend/commands/subscriptioncmds.c | 34 +++++++++++++++++++++-------- src/backend/parser/gram.y | 14 ++---------- src/include/nodes/parsenodes.h | 1 - src/test/regress/expected/subscription.out | 2 +- src/test/regress/sql/subscription.sql | 2 +- src/test/subscription/t/001_rep_changes.pl | 2 +- 8 files changed, 54 insertions(+), 38 deletions(-) diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml index b2fae027f5..5723be744d 100644 --- a/doc/src/sgml/catalogs.sgml +++ b/doc/src/sgml/catalogs.sgml @@ -6609,7 +6609,7 @@ <structname>pg_subscription_rel</structname> This catalog only contains tables known to the subscription after running either CREATE SUBSCRIPTION or - ALTER SUBSCRIPTION ... REFRESH. + ALTER SUBSCRIPTION ... REFRESH PUBLICATION. diff --git a/doc/src/sgml/ref/alter_subscription.sgml b/doc/src/sgml/ref/alter_subscription.sgml index a3471a0442..bead99622e 100644 --- a/doc/src/sgml/ref/alter_subscription.sgml +++ b/doc/src/sgml/ref/alter_subscription.sgml @@ -22,7 +22,7 @@ ALTER SUBSCRIPTION name CONNECTION 'conninfo' -ALTER SUBSCRIPTION name SET PUBLICATION publication_name [, ...] { REFRESH [ WITH ( refresh_option [= value] [, ... ] ) ] | SKIP REFRESH } +ALTER SUBSCRIPTION name SET PUBLICATION publication_name [, ...] [ WITH ( set_publication_option [= value] [, ... ] ) ] ALTER SUBSCRIPTION name REFRESH PUBLICATION [ WITH ( refresh_option [= value] [, ... ] ) ] ALTER SUBSCRIPTION name ENABLE ALTER SUBSCRIPTION name DISABLE @@ -80,18 +80,29 @@ Parameters Changes list of subscribed publications. See for more information. + By default this command will also act like REFRESH + PUBLICATION. - When REFRESH is specified, this command will also act - like REFRESH - PUBLICATION. refresh_option specifies - additional options for the refresh operation, as described - under REFRESH PUBLICATION. When - SKIP REFRESH is specified, the command will not try - to refresh table information. Note that - either REFRESH or SKIP REFRESH - must be specified. + set_publication_option specifies additional + options for this operation. The supported options are: + + + + refresh (boolean) + + + When false, the command will not try to refresh table information. + REFRESH PUBLICATION should then be executed separately. + The default is true. + + + + + + Additionally, refresh options as described + under REFRESH PUBLICATION may be specified. @@ -107,7 +118,7 @@ Parameters - refresh_option specifies additional options for the + refresh_option specifies additional options for the refresh operation. The supported options are: @@ -185,7 +196,7 @@ Examples Change the publication subscribed by a subscription to insert_only: -ALTER SUBSCRIPTION mysub SET PUBLICATION insert_only REFRESH; +ALTER SUBSCRIPTION mysub SET PUBLICATION insert_only; diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c index 86eb31df93..ad98b38efe 100644 --- a/src/backend/commands/subscriptioncmds.c +++ b/src/backend/commands/subscriptioncmds.c @@ -64,12 +64,14 @@ static void parse_subscription_options(List *options, bool *connect, bool *enabled_given, bool *enabled, bool *create_slot, bool *slot_name_given, char **slot_name, - bool *copy_data, char **synchronous_commit) + bool *copy_data, char **synchronous_commit, + bool *refresh) { ListCell *lc; bool connect_given = false; bool create_slot_given = false; bool copy_data_given = false; + bool refresh_given = false; /* If connect is specified, the others also need to be. */ Assert(!connect || (enabled && create_slot && copy_data)); @@ -92,6 +94,8 @@ parse_subscription_options(List *options, bool *connect, bool *enabled_given, *copy_data = true; if (synchronous_commit) *synchronous_commit = NULL; + if (refresh) + *refresh = true; /* Parse options */ foreach(lc, options) @@ -167,6 +171,16 @@ parse_subscription_options(List *options, bool *connect, bool *enabled_given, PGC_BACKEND, PGC_S_TEST, GUC_ACTION_SET, false, 0, false); } + else if (strcmp(defel->defname, "refresh") == 0 && refresh) + { + if (refresh_given) + ereport(ERROR, + (errcode(ERRCODE_SYNTAX_ERROR), + errmsg("conflicting or redundant options"))); + + refresh_given = true; + *refresh = defGetBoolean(defel); + } else ereport(ERROR, (errcode(ERRCODE_SYNTAX_ERROR), @@ -315,7 +329,8 @@ CreateSubscription(CreateSubscriptionStmt *stmt, bool isTopLevel) */ parse_subscription_options(stmt->options, &connect, &enabled_given, &enabled, &create_slot, &slotname_given, - &slotname, ©_data, &synchronous_commit); + &slotname, ©_data, &synchronous_commit, + NULL); /* * Since creating a replication slot is not transactional, rolling back @@ -645,7 +660,7 @@ AlterSubscription(AlterSubscriptionStmt *stmt) parse_subscription_options(stmt->options, NULL, NULL, NULL, NULL, &slotname_given, &slotname, - NULL, &synchronous_commit); + NULL, &synchronous_commit, NULL); if (slotname_given) { @@ -680,7 +695,7 @@ AlterSubscription(AlterSubscriptionStmt *stmt) parse_subscription_options(stmt->options, NULL, &enabled_given, &enabled, NULL, - NULL, NULL, NULL, NULL); + NULL, NULL, NULL, NULL, NULL); Assert(enabled_given); if (!sub->slotname && enabled) @@ -712,13 +727,13 @@ AlterSubscription(AlterSubscriptionStmt *stmt) break; case ALTER_SUBSCRIPTION_PUBLICATION: - case ALTER_SUBSCRIPTION_PUBLICATION_REFRESH: { bool copy_data; + bool refresh; parse_subscription_options(stmt->options, NULL, NULL, NULL, NULL, NULL, NULL, ©_data, - NULL); + NULL, &refresh); values[Anum_pg_subscription_subpublications - 1] = publicationListToArray(stmt->publication); @@ -727,12 +742,13 @@ AlterSubscription(AlterSubscriptionStmt *stmt) update_tuple = true; /* Refresh if user asked us to. */ - if (stmt->kind == ALTER_SUBSCRIPTION_PUBLICATION_REFRESH) + if (refresh) { if (!sub->enabled) ereport(ERROR, (errcode(ERRCODE_SYNTAX_ERROR), - errmsg("ALTER SUBSCRIPTION ... REFRESH is not allowed for disabled subscriptions"))); + errmsg("ALTER SUBSCRIPTION with refresh is not allowed for disabled subscriptions"), + errhint("Use ALTER SUBSCRIPTION ... SET PUBLICATION ... WITH (refresh = false)."))); /* Make sure refresh sees the new list of publications. */ sub->publications = stmt->publication; @@ -754,7 +770,7 @@ AlterSubscription(AlterSubscriptionStmt *stmt) parse_subscription_options(stmt->options, NULL, NULL, NULL, NULL, NULL, NULL, ©_data, - NULL); + NULL, NULL); AlterSubscription_refresh(sub, copy_data); diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y index 7e03624eb4..ada95e5bc3 100644 --- a/src/backend/parser/gram.y +++ b/src/backend/parser/gram.y @@ -9279,24 +9279,14 @@ AlterSubscriptionStmt: n->options = $6; $$ = (Node *)n; } - | ALTER SUBSCRIPTION name SET PUBLICATION publication_name_list REFRESH opt_definition - { - AlterSubscriptionStmt *n = - makeNode(AlterSubscriptionStmt); - n->kind = ALTER_SUBSCRIPTION_PUBLICATION_REFRESH; - n->subname = $3; - n->publication = $6; - n->options = $8; - $$ = (Node *)n; - } - | ALTER SUBSCRIPTION name SET PUBLICATION publication_name_list SKIP REFRESH + | ALTER SUBSCRIPTION name SET PUBLICATION publication_name_list opt_definition { AlterSubscriptionStmt *n = makeNode(AlterSubscriptionStmt); n->kind = ALTER_SUBSCRIPTION_PUBLICATION; n->subname = $3; n->publication = $6; - n->options = NIL; + n->options = $7; $$ = (Node *)n; } | ALTER SUBSCRIPTION name ENABLE_P diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h index 8720e713c4..2d2e2c0fbc 100644 --- a/src/include/nodes/parsenodes.h +++ b/src/include/nodes/parsenodes.h @@ -3382,7 +3382,6 @@ typedef enum AlterSubscriptionType ALTER_SUBSCRIPTION_OPTIONS, ALTER_SUBSCRIPTION_CONNECTION, ALTER_SUBSCRIPTION_PUBLICATION, - ALTER_SUBSCRIPTION_PUBLICATION_REFRESH, ALTER_SUBSCRIPTION_REFRESH, ALTER_SUBSCRIPTION_ENABLED } AlterSubscriptionType; diff --git a/src/test/regress/expected/subscription.out b/src/test/regress/expected/subscription.out index 91ba8ab95a..4fcbf7efe9 100644 --- a/src/test/regress/expected/subscription.out +++ b/src/test/regress/expected/subscription.out @@ -82,7 +82,7 @@ ERROR: invalid connection string syntax: missing "=" after "foobar" in connecti testsub | regress_subscription_user | f | {testpub} | off | dbname=doesnotexist (1 row) -ALTER SUBSCRIPTION testsub SET PUBLICATION testpub2, testpub3 SKIP REFRESH; +ALTER SUBSCRIPTION testsub SET PUBLICATION testpub2, testpub3 WITH (refresh = false); ALTER SUBSCRIPTION testsub CONNECTION 'dbname=doesnotexist2'; ALTER SUBSCRIPTION testsub SET (slot_name = 'newname'); -- fail diff --git a/src/test/regress/sql/subscription.sql b/src/test/regress/sql/subscription.sql index 4b694a357e..36fa1bbac8 100644 --- a/src/test/regress/sql/subscription.sql +++ b/src/test/regress/sql/subscription.sql @@ -61,7 +61,7 @@ CREATE SUBSCRIPTION testsub3 CONNECTION 'dbname=doesnotexist' PUBLICATION testpu \dRs+ -ALTER SUBSCRIPTION testsub SET PUBLICATION testpub2, testpub3 SKIP REFRESH; +ALTER SUBSCRIPTION testsub SET PUBLICATION testpub2, testpub3 WITH (refresh = false); ALTER SUBSCRIPTION testsub CONNECTION 'dbname=doesnotexist2'; ALTER SUBSCRIPTION testsub SET (slot_name = 'newname'); diff --git a/src/test/subscription/t/001_rep_changes.pl b/src/test/subscription/t/001_rep_changes.pl index e5638d3322..f9cf5e4392 100644 --- a/src/test/subscription/t/001_rep_changes.pl +++ b/src/test/subscription/t/001_rep_changes.pl @@ -143,7 +143,7 @@ "SELECT pid FROM pg_stat_replication WHERE application_name = '$appname';" ); $node_subscriber->safe_psql('postgres', -"ALTER SUBSCRIPTION tap_sub SET PUBLICATION tap_pub_ins_only REFRESH WITH (copy_data = false)" +"ALTER SUBSCRIPTION tap_sub SET PUBLICATION tap_pub_ins_only WITH (copy_data = false)" ); $node_publisher->poll_query_until('postgres', "SELECT pid != $oldpid FROM pg_stat_replication WHERE application_name = '$appname';" -- 2.13.0