From 52327b2d3540a4dabe397a8e6be0459cf57524ea Mon Sep 17 00:00:00 2001
From: Nathan Bossart <bossartn@amazon.com>
Date: Thu, 2 Dec 2021 23:50:12 +0000
Subject: [PATCH v13 1/1] Simplify recent parse_subscription_option changes.

Make the parse_subscription_options function reponsible for zapping the SubOpts
param up-front, instead of hoping the caller may do it.

Remove redundant condition checks for "supported_opts" where we already know the
option MUST be supported.
---
 src/backend/commands/subscriptioncmds.c    | 77 ++++++++++++++----------------
 src/test/regress/expected/subscription.out |  2 +-
 src/test/regress/sql/subscription.sql      |  2 +-
 3 files changed, 37 insertions(+), 44 deletions(-)

diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index 9427e86fee..0dff3e66ad 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -95,8 +95,6 @@ static void ReportSlotConnectionError(List *rstates, Oid subid, char *slotname,
  *
  * Since not all options can be specified in both commands, this function
  * will report an error if mutually exclusive options are specified.
- *
- * Caller is expected to have cleared 'opts'.
  */
 static void
 parse_subscription_options(ParseState *pstate, List *stmt_options,
@@ -104,6 +102,9 @@ parse_subscription_options(ParseState *pstate, List *stmt_options,
 {
 	ListCell   *lc;
 
+	/* Start out with cleared opts. */
+	memset(opts, 0, sizeof(SubOpts));
+
 	/* caller must expect some option */
 	Assert(supported_opts != 0);
 
@@ -262,7 +263,6 @@ parse_subscription_options(ParseState *pstate, List *stmt_options,
 	{
 		/* Check for incompatible options from the user. */
 		if (opts->enabled &&
-			IsSet(supported_opts, SUBOPT_ENABLED) &&
 			IsSet(opts->specified_opts, SUBOPT_ENABLED))
 			ereport(ERROR,
 					(errcode(ERRCODE_SYNTAX_ERROR),
@@ -271,7 +271,6 @@ parse_subscription_options(ParseState *pstate, List *stmt_options,
 							"connect = false", "enabled = true")));
 
 		if (opts->create_slot &&
-			IsSet(supported_opts, SUBOPT_CREATE_SLOT) &&
 			IsSet(opts->specified_opts, SUBOPT_CREATE_SLOT))
 			ereport(ERROR,
 					(errcode(ERRCODE_SYNTAX_ERROR),
@@ -279,7 +278,6 @@ parse_subscription_options(ParseState *pstate, List *stmt_options,
 							"connect = false", "create_slot = true")));
 
 		if (opts->copy_data &&
-			IsSet(supported_opts, SUBOPT_COPY_DATA) &&
 			IsSet(opts->specified_opts, SUBOPT_COPY_DATA))
 			ereport(ERROR,
 					(errcode(ERRCODE_SYNTAX_ERROR),
@@ -297,44 +295,39 @@ parse_subscription_options(ParseState *pstate, List *stmt_options,
 	 * was used.
 	 */
 	if (!opts->slot_name &&
-		IsSet(supported_opts, SUBOPT_SLOT_NAME) &&
 		IsSet(opts->specified_opts, SUBOPT_SLOT_NAME))
 	{
-		if (opts->enabled &&
-			IsSet(supported_opts, SUBOPT_ENABLED) &&
-			IsSet(opts->specified_opts, SUBOPT_ENABLED))
-			ereport(ERROR,
-					(errcode(ERRCODE_SYNTAX_ERROR),
-			/*- translator: both %s are strings of the form "option = value" */
-					 errmsg("%s and %s are mutually exclusive options",
-							"slot_name = NONE", "enabled = true")));
-
-		if (opts->create_slot &&
-			IsSet(supported_opts, SUBOPT_CREATE_SLOT) &&
-			IsSet(opts->specified_opts, SUBOPT_CREATE_SLOT))
-			ereport(ERROR,
-					(errcode(ERRCODE_SYNTAX_ERROR),
-			/*- translator: both %s are strings of the form "option = value" */
-					 errmsg("%s and %s are mutually exclusive options",
-							"slot_name = NONE", "create_slot = true")));
-
-		if (opts->enabled &&
-			IsSet(supported_opts, SUBOPT_ENABLED) &&
-			!IsSet(opts->specified_opts, SUBOPT_ENABLED))
-			ereport(ERROR,
-					(errcode(ERRCODE_SYNTAX_ERROR),
-			/*- translator: both %s are strings of the form "option = value" */
-					 errmsg("subscription with %s must also set %s",
-							"slot_name = NONE", "enabled = false")));
+		if (opts->enabled)
+		{
+			if (IsSet(opts->specified_opts, SUBOPT_ENABLED))
+				ereport(ERROR,
+						(errcode(ERRCODE_SYNTAX_ERROR),
+				/*- translator: both %s are strings of the form "option = value" */
+						 errmsg("%s and %s are mutually exclusive options",
+								"slot_name = NONE", "enabled = true")));
+			else
+				ereport(ERROR,
+						(errcode(ERRCODE_SYNTAX_ERROR),
+				/*- translator: both %s are strings of the form "option = value" */
+						 errmsg("subscription with %s must also set %s",
+								"slot_name = NONE", "enabled = false")));
+		}
 
-		if (opts->create_slot &&
-			IsSet(supported_opts, SUBOPT_CREATE_SLOT) &&
-			!IsSet(opts->specified_opts, SUBOPT_CREATE_SLOT))
-			ereport(ERROR,
-					(errcode(ERRCODE_SYNTAX_ERROR),
-			/*- translator: both %s are strings of the form "option = value" */
-					 errmsg("subscription with %s must also set %s",
-							"slot_name = NONE", "create_slot = false")));
+		if (opts->create_slot)
+		{
+			if (IsSet(opts->specified_opts, SUBOPT_CREATE_SLOT))
+				ereport(ERROR,
+						(errcode(ERRCODE_SYNTAX_ERROR),
+				/*- translator: both %s are strings of the form "option = value" */
+						 errmsg("%s and %s are mutually exclusive options",
+								"slot_name = NONE", "create_slot = true")));
+			else
+				ereport(ERROR,
+						(errcode(ERRCODE_SYNTAX_ERROR),
+				/*- translator: both %s are strings of the form "option = value" */
+						 errmsg("subscription with %s must also set %s",
+								"slot_name = NONE", "create_slot = false")));
+		}
 	}
 }
 
@@ -387,7 +380,7 @@ CreateSubscription(ParseState *pstate, CreateSubscriptionStmt *stmt,
 	char		originname[NAMEDATALEN];
 	List	   *publications;
 	bits32		supported_opts;
-	SubOpts		opts = {0};
+	SubOpts		opts;
 
 	/*
 	 * Parse and check options.
@@ -833,7 +826,7 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
 	Subscription *sub;
 	Form_pg_subscription form;
 	bits32		supported_opts;
-	SubOpts		opts = {0};
+	SubOpts		opts;
 
 	rel = table_open(SubscriptionRelationId, RowExclusiveLock);
 
diff --git a/src/test/regress/expected/subscription.out b/src/test/regress/expected/subscription.out
index 15a1ac6398..80aae83562 100644
--- a/src/test/regress/expected/subscription.out
+++ b/src/test/regress/expected/subscription.out
@@ -54,7 +54,7 @@ CREATE SUBSCRIPTION regress_testsub2 CONNECTION 'dbname=regress_doesnotexist' PU
 ERROR:  connect = false and create_slot = true are mutually exclusive options
 CREATE SUBSCRIPTION regress_testsub2 CONNECTION 'dbname=regress_doesnotexist' PUBLICATION testpub WITH (slot_name = NONE, enabled = true);
 ERROR:  slot_name = NONE and enabled = true are mutually exclusive options
-CREATE SUBSCRIPTION regress_testsub2 CONNECTION 'dbname=regress_doesnotexist' PUBLICATION testpub WITH (slot_name = NONE, create_slot = true);
+CREATE SUBSCRIPTION regress_testsub2 CONNECTION 'dbname=regress_doesnotexist' PUBLICATION testpub WITH (slot_name = NONE, enabled = false, create_slot = true);
 ERROR:  slot_name = NONE and create_slot = true are mutually exclusive options
 CREATE SUBSCRIPTION regress_testsub2 CONNECTION 'dbname=regress_doesnotexist' PUBLICATION testpub WITH (slot_name = NONE);
 ERROR:  subscription with slot_name = NONE must also set enabled = false
diff --git a/src/test/regress/sql/subscription.sql b/src/test/regress/sql/subscription.sql
index 7faa935a2a..bd0f4af1e4 100644
--- a/src/test/regress/sql/subscription.sql
+++ b/src/test/regress/sql/subscription.sql
@@ -43,7 +43,7 @@ CREATE SUBSCRIPTION regress_testsub2 CONNECTION 'dbname=regress_doesnotexist' PU
 CREATE SUBSCRIPTION regress_testsub2 CONNECTION 'dbname=regress_doesnotexist' PUBLICATION testpub WITH (connect = false, enabled = true);
 CREATE SUBSCRIPTION regress_testsub2 CONNECTION 'dbname=regress_doesnotexist' PUBLICATION testpub WITH (connect = false, create_slot = true);
 CREATE SUBSCRIPTION regress_testsub2 CONNECTION 'dbname=regress_doesnotexist' PUBLICATION testpub WITH (slot_name = NONE, enabled = true);
-CREATE SUBSCRIPTION regress_testsub2 CONNECTION 'dbname=regress_doesnotexist' PUBLICATION testpub WITH (slot_name = NONE, create_slot = true);
+CREATE SUBSCRIPTION regress_testsub2 CONNECTION 'dbname=regress_doesnotexist' PUBLICATION testpub WITH (slot_name = NONE, enabled = false, create_slot = true);
 CREATE SUBSCRIPTION regress_testsub2 CONNECTION 'dbname=regress_doesnotexist' PUBLICATION testpub WITH (slot_name = NONE);
 CREATE SUBSCRIPTION regress_testsub2 CONNECTION 'dbname=regress_doesnotexist' PUBLICATION testpub WITH (slot_name = NONE, enabled = false);
 CREATE SUBSCRIPTION regress_testsub2 CONNECTION 'dbname=regress_doesnotexist' PUBLICATION testpub WITH (slot_name = NONE, create_slot = false);
-- 
2.16.6

