Hi,
While I was reviewing one of the logical decoding features, I found a
few issues in alter subscription drop publication.
Alter subscription drop publication does not support copy_data option,
that needs to be removed from tab completion.
Dropping all the publications present in the subscription using alter
subscription drop publication would throw "subscription must contain
at least one publication". This message was slightly confusing to me.
As even though some publication was present on the subscription I was
not able to drop. Instead I feel we could throw an error message
something like "dropping specified publication will result in
subscription without any publication, this is not supported".
merge_publications can be called after validation of the options
specified, I think we should check if the options specified are
correct or not before checking the actual publications.
Attached a patch which contains the fixes for the same.
Thoughts?
Regards,
Vignesh
From 516f7b933f06b64ebb21e3f55bdd223703879a3a Mon Sep 17 00:00:00 2001
From: vignesh <vignes...@gmail.com>
Date: Mon, 10 May 2021 12:15:28 +0530
Subject: [PATCH v1] Fixes in alter subscription drop publication.
Fixes in alter subscription drop publication.
---
src/backend/commands/subscriptioncmds.c | 6 +++---
src/bin/psql/tab-complete.c | 9 ++++++---
src/test/regress/expected/subscription.out | 2 +-
3 files changed, 10 insertions(+), 7 deletions(-)
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index bbb2f5d029..55ce24e613 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -951,8 +951,6 @@ AlterSubscription(AlterSubscriptionStmt *stmt, bool isTopLevel)
bool refresh;
List *publist;
- publist = merge_publications(sub->publications, stmt->publication, isadd, stmt->subname);
-
parse_subscription_options(stmt->options,
NULL, /* no "connect" */
NULL, NULL, /* no "enabled" */
@@ -965,6 +963,8 @@ AlterSubscription(AlterSubscriptionStmt *stmt, bool isTopLevel)
NULL, NULL, /* no "binary" */
NULL, NULL); /* no "streaming" */
+ publist = merge_publications(sub->publications, stmt->publication, isadd, stmt->subname);
+
values[Anum_pg_subscription_subpublications - 1] =
publicationListToArray(publist);
replaces[Anum_pg_subscription_subpublications - 1] = true;
@@ -1671,7 +1671,7 @@ merge_publications(List *oldpublist, List *newpublist, bool addpub, const char *
if (!oldpublist)
ereport(ERROR,
(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
- errmsg("subscription must contain at least one publication")));
+ errmsg("dropping specified publication will result in subscription without any publication, this is not supported")));
return oldpublist;
}
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index d917987fd5..8ec78dc734 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -1675,11 +1675,14 @@ 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|DROP|SET PUBLICATION <name> WITH ( */
+ /* ALTER SUBSCRIPTION <name> ADD|SET PUBLICATION <name> WITH ( */
else if (HeadMatches("ALTER", "SUBSCRIPTION", MatchAny) &&
- TailMatches("ADD|DROP|SET", "PUBLICATION", MatchAny, "WITH", "("))
+ TailMatches("ADD|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))
COMPLETE_WITH("OWNER TO", "RENAME TO");
diff --git a/src/test/regress/expected/subscription.out b/src/test/regress/expected/subscription.out
index 09576c176b..c71e86a797 100644
--- a/src/test/regress/expected/subscription.out
+++ b/src/test/regress/expected/subscription.out
@@ -223,7 +223,7 @@ ALTER SUBSCRIPTION regress_testsub DROP PUBLICATION testpub1, testpub1 WITH (ref
ERROR: publication name "testpub1" used more than once
-- fail - all publications are deleted
ALTER SUBSCRIPTION regress_testsub DROP PUBLICATION testpub, testpub1, testpub2 WITH (refresh = false);
-ERROR: subscription must contain at least one publication
+ERROR: dropping specified publication will result in subscription without any publication, this is not supported
-- 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"
--
2.25.1
