From c4644584c13362eb67c9db8f6fbe8c109728fbd4 Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Date: Wed, 9 Jun 2021 08:04:12 -0700
Subject: [PATCH v7] Remove similar ereport calls in parse_subscription_options

Remove similar code (with the only difference in the option) when
throwing errors for mutually exclusive and disallowed combination
of options. Have variables for the options and use them in the
error messages. It makes the code look better with lesser ereport(ERROR
statements.
---
 src/backend/commands/subscriptioncmds.c | 63 +++++++++++++------------
 1 file changed, 32 insertions(+), 31 deletions(-)

diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index 9ec344cb0f..dd33196e0f 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -261,28 +261,26 @@ parse_subscription_options(List *stmt_options, SubOpts *opts)
 	 */
 	if (!opts->connect && IsSet(supported_opts, SUBOPT_CONNECT))
 	{
+		char *incompat_opt = NULL;
+
 		/* Check for incompatible options from the user. */
 		if (opts->enabled && IsSet(supported_opts, SUBOPT_ENABLED) &&
 			IsSet(specified_opts, SUBOPT_ENABLED))
+			incompat_opt = "enabled = true";
+		else if (opts->create_slot &&
+				 IsSet(supported_opts, SUBOPT_CREATE_SLOT) &&
+				 IsSet(specified_opts, SUBOPT_CREATE_SLOT))
+			incompat_opt = "create_slot = true";
+		else if (opts->copy_data && IsSet(supported_opts, SUBOPT_COPY_DATA) &&
+				 IsSet(specified_opts, SUBOPT_COPY_DATA))
+			incompat_opt = "copy_data = true";
+
+		if (incompat_opt)
 			ereport(ERROR,
 					(errcode(ERRCODE_SYNTAX_ERROR),
 			/*- translator: both %s are strings of the form "option = value" */
 					 errmsg("%s and %s are mutually exclusive options",
-							"connect = false", "enabled = true")));
-
-		if (opts->create_slot && IsSet(supported_opts, SUBOPT_CREATE_SLOT) &&
-			IsSet(specified_opts, SUBOPT_CREATE_SLOT))
-			ereport(ERROR,
-					(errcode(ERRCODE_SYNTAX_ERROR),
-					 errmsg("%s and %s are mutually exclusive options",
-							"connect = false", "create_slot = true")));
-
-		if (opts->copy_data && IsSet(supported_opts, SUBOPT_COPY_DATA) &&
-			IsSet(specified_opts, SUBOPT_COPY_DATA))
-			ereport(ERROR,
-					(errcode(ERRCODE_SYNTAX_ERROR),
-					 errmsg("%s and %s are mutually exclusive options",
-							"connect = false", "copy_data = true")));
+							"connect = false", incompat_opt)));
 
 		/* Change the defaults of other options. */
 		opts->enabled = false;
@@ -297,35 +295,38 @@ parse_subscription_options(List *stmt_options, SubOpts *opts)
 	if (!opts->slot_name && IsSet(supported_opts, SUBOPT_SLOT_NAME) &&
 		IsSet(specified_opts, SUBOPT_SLOT_NAME))
 	{
+		char *incompat_opt = NULL;
+		char *required_opt = NULL;
+
 		if (opts->enabled && IsSet(supported_opts, SUBOPT_ENABLED) &&
 			IsSet(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")));
+			incompat_opt = "enabled = true";
+		else if (opts->create_slot &&
+				 IsSet(supported_opts, SUBOPT_CREATE_SLOT) &&
+				 IsSet(specified_opts, SUBOPT_CREATE_SLOT))
+			incompat_opt = "create_slot = true";
 
-		if (opts->create_slot && IsSet(supported_opts, SUBOPT_CREATE_SLOT) &&
-			IsSet(specified_opts, SUBOPT_CREATE_SLOT))
+		if (incompat_opt)
 			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")));
+							"slot_name = NONE", incompat_opt)));
 
 		if (opts->enabled && IsSet(supported_opts, SUBOPT_ENABLED) &&
 			!IsSet(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")));
+			required_opt = "enabled = false";
+		else if (opts->create_slot &&
+				 IsSet(supported_opts, SUBOPT_CREATE_SLOT) &&
+				!IsSet(specified_opts, SUBOPT_CREATE_SLOT))
+			required_opt = "create_slot = false";
 
-		if (opts->create_slot && IsSet(supported_opts, SUBOPT_CREATE_SLOT) &&
-			!IsSet(specified_opts, SUBOPT_CREATE_SLOT))
+		if (required_opt)
 			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")));
+							"slot_name = NONE", required_opt)));
 	}
 
 	opts->specified_opts = specified_opts;
-- 
2.25.1

