Date: Mon, 16 Sep 2019 18:45:31 -0300
From: Alvaro Herrera <alvherre@2ndquadrant.com>
To: Nikolay Shaplov <dhyan@nataraj.su>
Cc: pgsql-hackers@lists.postgresql.org, Tom Lane <tgl@sss.pgh.pa.us>,
	Robert Haas <robertmhaas@gmail.com>,
	Nikita Glukhov <n.gluhov@postgrespro.ru>,
	"pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>
Subject: Re: [PATCH][PROPOSAL] Add enum releation option type
Message-ID: <20190916214531.GA15390@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="fdj2RfSjLxBAspz7"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <2049090.vyOS18DeL7@x200m>
User-Agent: Mutt/1.9.4 (2018-02-28)
Precedence: bulk


--fdj2RfSjLxBAspz7
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit

On 2019-Sep-13, Nikolay Shaplov wrote:

> I've played with it around, and did some googling, but without much success.
> If we are moving this way (an this way seems to be good one), I need you help, 
> because this thing is beyond my C knowledge, I will not manage it myself.

So I kinda did it ... and didn't like the result very much.

Partly, maybe the problem is not one that this patch introduces, but
just a pre-existing problem in reloptions: namely, does an access/
module (gist, rel) depend on reloptions, or does reloptions.c depend on
access modules?  It seems that there is no actual modularity separation
between these; currently both seem to depend on each other, if only
because there's a central list (arrays) of valid reloptions.  If we did
away with that and instead had each module defined its own reloptions,
maybe that problem would go away.  But how would that work?

Also: I'm not sure about the stuff that coerces the enum value to an int
generically; that works fine with my compiler but I'm not sure it's
kosher.

Thirdly: I'm not sure that what I suggested is syntactically valid,
because C doesn't let you define an array in an initializator in the
middle of another array.  If there is, it requires some syntax trick
that I'm not familiar with.  So I had to put the valid values arrays
separate from the main enum options, after all, which kinda sucks.

Finally (but this is easily fixable), with this patch the
allocate_reloption stuff is broken (needs to pstrdup() the "Valid values
are" message, which needs to be passed as a new argument).

All in all, I don't know what to think of this.  It seemed like a good
idea, but maybe in practice it's not so great.

Do I hear other opinions?

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

--fdj2RfSjLxBAspz7
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="enum-reloptions-7.patch"

diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
index 20f4ed3c38..7b57eab1c5 100644
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
@@ -433,7 +433,22 @@ static relopt_real realRelOpts[] =
 	{{NULL}}
 };
 
-static relopt_string stringRelOpts[] =
+relopt_enum_elt_def gistBufferingOptValues[] =
+{
+	{ "on", GIST_OPTION_BUFFERING_ON },
+	{ "off", GIST_OPTION_BUFFERING_OFF },
+	{ "auto", GIST_OPTION_BUFFERING_AUTO },
+	{ (const char *) NULL }
+};
+
+relopt_enum_elt_def viewCheckOptValues[] =
+{
+	{ "local", VIEW_OPTION_CHECK_OPTION_LOCAL },
+	{ "cascaded", VIEW_OPTION_CHECK_OPTION_CASCADED },
+	{ (const char *) NULL }
+};
+
+static relopt_enum enumRelOpts[] =
 {
 	{
 		{
@@ -442,10 +457,9 @@ static relopt_string stringRelOpts[] =
 			RELOPT_KIND_GIST,
 			AccessExclusiveLock
 		},
-		4,
-		false,
-		gistValidateBufferingOption,
-		"auto"
+		gistBufferingOptValues,
+		GIST_OPTION_BUFFERING_AUTO,
+		"Valid values are \"on\", \"off\", and \"auto\"."
 	},
 	{
 		{
@@ -454,11 +468,19 @@ static relopt_string stringRelOpts[] =
 			RELOPT_KIND_VIEW,
 			AccessExclusiveLock
 		},
-		0,
-		true,
-		validateWithCheckOption,
-		NULL
+		viewCheckOptValues,
+		VIEW_OPTION_CHECK_OPTION_NOT_SET,
+		"Valid values are \"local\" and \"cascaded\"."
 	},
+	{
+		{
+			NULL
+		}
+	}
+};
+
+static relopt_string stringRelOpts[] =
+{
 	/* list terminator */
 	{{NULL}}
 };
@@ -505,6 +527,12 @@ initialize_reloptions(void)
 								   realRelOpts[i].gen.lockmode));
 		j++;
 	}
+	for (i = 0; enumRelOpts[i].gen.name; i++)
+	{
+		Assert(DoLockModesConflict(enumRelOpts[i].gen.lockmode,
+								   enumRelOpts[i].gen.lockmode));
+		j++;
+	}
 	for (i = 0; stringRelOpts[i].gen.name; i++)
 	{
 		Assert(DoLockModesConflict(stringRelOpts[i].gen.lockmode,
@@ -543,6 +571,14 @@ initialize_reloptions(void)
 		j++;
 	}
 
+	for (i = 0; enumRelOpts[i].gen.name; i++)
+	{
+		relOpts[j] = &enumRelOpts[i].gen;
+		relOpts[j]->type = RELOPT_TYPE_ENUM;
+		relOpts[j]->namelen = strlen(relOpts[j]->name);
+		j++;
+	}
+
 	for (i = 0; stringRelOpts[i].gen.name; i++)
 	{
 		relOpts[j] = &stringRelOpts[i].gen;
@@ -640,6 +676,9 @@ allocate_reloption(bits32 kinds, int type, const char *name, const char *desc)
 		case RELOPT_TYPE_REAL:
 			size = sizeof(relopt_real);
 			break;
+		case RELOPT_TYPE_ENUM:
+			size = sizeof(relopt_enum);
+			break;
 		case RELOPT_TYPE_STRING:
 			size = sizeof(relopt_string);
 			break;
@@ -718,6 +757,24 @@ add_real_reloption(bits32 kinds, const char *name, const char *desc, double defa
 	add_reloption((relopt_gen *) newoption);
 }
 
+/*
+ * add_enuum_reloption
+ *		Add a new enum reloption
+ */
+void
+add_enum_reloption(bits32 kinds, const char *name, const char *desc,
+				   relopt_enum_elt_def *members, int default_val)
+{
+	relopt_enum *newoption;
+
+	newoption = (relopt_enum *) allocate_reloption(kinds, RELOPT_TYPE_ENUM,
+												   name, desc);
+	newoption->members = members;
+	newoption->default_val = default_val;
+
+	add_reloption((relopt_gen *) newoption);
+}
+
 /*
  * add_string_reloption
  *		Add a new string reloption
@@ -1234,6 +1291,35 @@ parse_one_reloption(relopt_value *option, char *text_str, int text_len,
 									   optreal->min, optreal->max)));
 			}
 			break;
+		case RELOPT_TYPE_ENUM:
+			{
+				relopt_enum *optenum = (relopt_enum *) option->gen;
+				relopt_enum_elt_def *elt;
+
+				parsed = false;
+				for (elt = optenum->members; elt->string_val; elt++)
+				{
+					if (pg_strcasecmp(value, elt->string_val) == 0)
+					{
+						option->values.enum_val = elt->symbol_val;
+						parsed = true;
+						break;
+					}
+				}
+				if (validate && !parsed)
+					ereport(ERROR,
+							(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+							 errmsg("invalid value for \"%s\" option",
+									option->gen->name),
+							 errdetail("%s", optenum->detailmsg)));
+				/*
+				 * If value is not among the allowed string values, but we are
+				 * not asked to validate, just use the default numeric value.
+				 */
+				if (!parsed)
+					option->values.enum_val = optenum->default_val;
+			}
+			break;
 		case RELOPT_TYPE_STRING:
 			{
 				relopt_string *optstring = (relopt_string *) option->gen;
@@ -1327,6 +1413,11 @@ fillRelOptions(void *rdopts, Size basesize,
 							options[i].values.real_val :
 							((relopt_real *) options[i].gen)->default_val;
 						break;
+					case RELOPT_TYPE_ENUM:
+						*(int *) itempos = options[i].isset ?
+							options[i].values.enum_val :
+							((relopt_enum *) options[i].gen)->default_val;
+						break;
 					case RELOPT_TYPE_STRING:
 						optstring = (relopt_string *) options[i].gen;
 						if (options[i].isset)
@@ -1443,8 +1534,8 @@ view_reloptions(Datum reloptions, bool validate)
 	static const relopt_parse_elt tab[] = {
 		{"security_barrier", RELOPT_TYPE_BOOL,
 		offsetof(ViewOptions, security_barrier)},
-		{"check_option", RELOPT_TYPE_STRING,
-		offsetof(ViewOptions, check_option_offset)}
+		{"check_option", RELOPT_TYPE_ENUM,
+		offsetof(ViewOptions, check_option)}
 	};
 
 	options = parseRelOptions(reloptions, validate, RELOPT_KIND_VIEW, &numoptions);
diff --git a/src/backend/access/gist/gistbuild.c b/src/backend/access/gist/gistbuild.c
index ecef0ff072..b835779089 100644
--- a/src/backend/access/gist/gistbuild.c
+++ b/src/backend/access/gist/gistbuild.c
@@ -129,11 +129,10 @@ gistbuild(Relation heap, Relation index, IndexInfo *indexInfo)
 	{
 		/* Get buffering mode from the options string */
 		GiSTOptions *options = (GiSTOptions *) index->rd_options;
-		char	   *bufferingMode = (char *) options + options->bufferingModeOffset;
 
-		if (strcmp(bufferingMode, "on") == 0)
+		if (options->buffering_mode == GIST_OPTION_BUFFERING_ON)
 			buildstate.bufferingMode = GIST_BUFFERING_STATS;
-		else if (strcmp(bufferingMode, "off") == 0)
+		else if (options->buffering_mode == GIST_OPTION_BUFFERING_OFF)
 			buildstate.bufferingMode = GIST_BUFFERING_DISABLED;
 		else
 			buildstate.bufferingMode = GIST_BUFFERING_AUTO;
@@ -236,25 +235,6 @@ gistbuild(Relation heap, Relation index, IndexInfo *indexInfo)
 	return result;
 }
 
-/*
- * Validator for "buffering" reloption on GiST indexes. Allows "on", "off"
- * and "auto" values.
- */
-void
-gistValidateBufferingOption(const char *value)
-{
-	if (value == NULL ||
-		(strcmp(value, "on") != 0 &&
-		 strcmp(value, "off") != 0 &&
-		 strcmp(value, "auto") != 0))
-	{
-		ereport(ERROR,
-				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-				 errmsg("invalid value for \"buffering\" option"),
-				 errdetail("Valid values are \"on\", \"off\", and \"auto\".")));
-	}
-}
-
 /*
  * Attempt to switch to buffering mode.
  *
diff --git a/src/backend/access/gist/gistutil.c b/src/backend/access/gist/gistutil.c
index 97260201dc..45804d7a91 100644
--- a/src/backend/access/gist/gistutil.c
+++ b/src/backend/access/gist/gistutil.c
@@ -913,7 +913,7 @@ gistoptions(Datum reloptions, bool validate)
 	int			numoptions;
 	static const relopt_parse_elt tab[] = {
 		{"fillfactor", RELOPT_TYPE_INT, offsetof(GiSTOptions, fillfactor)},
-		{"buffering", RELOPT_TYPE_STRING, offsetof(GiSTOptions, bufferingModeOffset)}
+		{"buffering", RELOPT_TYPE_ENUM, offsetof(GiSTOptions, buffering_mode)}
 	};
 
 	options = parseRelOptions(reloptions, validate, RELOPT_KIND_GIST,
diff --git a/src/backend/commands/view.c b/src/backend/commands/view.c
index 9773bdc1c3..bea890f177 100644
--- a/src/backend/commands/view.c
+++ b/src/backend/commands/view.c
@@ -38,24 +38,6 @@
 
 static void checkViewTupleDesc(TupleDesc newdesc, TupleDesc olddesc);
 
-/*---------------------------------------------------------------------
- * Validator for "check_option" reloption on views. The allowed values
- * are "local" and "cascaded".
- */
-void
-validateWithCheckOption(const char *value)
-{
-	if (value == NULL ||
-		(strcmp(value, "local") != 0 &&
-		 strcmp(value, "cascaded") != 0))
-	{
-		ereport(ERROR,
-				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-				 errmsg("invalid value for \"check_option\" option"),
-				 errdetail("Valid values are \"local\" and \"cascaded\".")));
-	}
-}
-
 /*---------------------------------------------------------------------
  * DefineVirtualRelation
  *
diff --git a/src/include/access/gist_private.h b/src/include/access/gist_private.h
index ed5b643885..719c9482a9 100644
--- a/src/include/access/gist_private.h
+++ b/src/include/access/gist_private.h
@@ -395,6 +395,19 @@ typedef struct GISTBuildBuffers
 	int			rootlevel;
 } GISTBuildBuffers;
 
+/*
+ * Buffering is an enum option
+ * gist_option_buffering_numeric_values defines a numeric representation of
+ * option values, and GIST_OPTION_BUFFERING_ENUM_DEF defines enum string values
+ * and maps them to numeric one.
+ */
+typedef enum GistOptBufferingMode
+{
+	GIST_OPTION_BUFFERING_ON = 0,
+	GIST_OPTION_BUFFERING_OFF = 1,
+	GIST_OPTION_BUFFERING_AUTO = 2
+} GistOptBufferingMode;
+
 /*
  * Storage type for GiST's reloptions
  */
@@ -402,7 +415,7 @@ typedef struct GiSTOptions
 {
 	int32		vl_len_;		/* varlena header (do not touch directly!) */
 	int			fillfactor;		/* page fill factor in percent (0..100) */
-	int			bufferingModeOffset;	/* use buffering build? */
+	GistOptBufferingMode buffering_mode;	/* buffering build mode */
 } GiSTOptions;
 
 /* gist.c */
diff --git a/src/include/access/reloptions.h b/src/include/access/reloptions.h
index 6d392e4d5a..13dbac3eca 100644
--- a/src/include/access/reloptions.h
+++ b/src/include/access/reloptions.h
@@ -31,6 +31,7 @@ typedef enum relopt_type
 	RELOPT_TYPE_BOOL,
 	RELOPT_TYPE_INT,
 	RELOPT_TYPE_REAL,
+	RELOPT_TYPE_ENUM,
 	RELOPT_TYPE_STRING
 } relopt_type;
 
@@ -80,6 +81,7 @@ typedef struct relopt_value
 		bool		bool_val;
 		int			int_val;
 		double		real_val;
+		int			enum_val;
 		char	   *string_val; /* allocated separately */
 	}			values;
 } relopt_value;
@@ -107,6 +109,25 @@ typedef struct relopt_real
 	double		max;
 } relopt_real;
 
+/*
+ * relopt_enum_elt_def -- One member of the array of acceptable values
+ * of an enum reloption.
+ */
+typedef struct relopt_enum_elt_def
+{
+	const char *string_val;
+	int			symbol_val;
+} relopt_enum_elt_def;
+
+typedef struct relopt_enum
+{
+	relopt_gen	gen;
+	relopt_enum_elt_def *members;
+	int			default_val;
+	const char *detailmsg;
+	/* null-terminated array of members */
+} relopt_enum;
+
 /* validation routines for strings */
 typedef void (*validate_string_relopt) (const char *value);
 
@@ -252,6 +273,8 @@ extern void add_int_reloption(bits32 kinds, const char *name, const char *desc,
 							  int default_val, int min_val, int max_val);
 extern void add_real_reloption(bits32 kinds, const char *name, const char *desc,
 							   double default_val, double min_val, double max_val);
+extern void add_enum_reloption(bits32 kinds, const char *name, const char *desc,
+				   relopt_enum_elt_def *members, int default_val);
 extern void add_string_reloption(bits32 kinds, const char *name, const char *desc,
 								 const char *default_val, validate_string_relopt validator);
 
diff --git a/src/include/commands/view.h b/src/include/commands/view.h
index 13a58017ba..663e096a7a 100644
--- a/src/include/commands/view.h
+++ b/src/include/commands/view.h
@@ -17,8 +17,6 @@
 #include "catalog/objectaddress.h"
 #include "nodes/parsenodes.h"
 
-extern void validateWithCheckOption(const char *value);
-
 extern ObjectAddress DefineView(ViewStmt *stmt, const char *queryString,
 								int stmt_location, int stmt_len);
 
diff --git a/src/include/utils/rel.h b/src/include/utils/rel.h
index 91b3b1b902..9061716cc2 100644
--- a/src/include/utils/rel.h
+++ b/src/include/utils/rel.h
@@ -327,6 +327,19 @@ typedef struct StdRdOptions
 	((relation)->rd_options ? \
 	 ((StdRdOptions *) (relation)->rd_options)->parallel_workers : (defaultpw))
 
+/*
+ * check_option is an enum option
+ * view_option_check_option_numeric_values defines a numeric representation of
+ * option values, and VIEW_OPTION_CHECK_OPTION_ENUM_DEF defines enum string
+ * values and maps them to numeric one.
+ */
+
+typedef enum ViewOptCheckOption
+{
+	VIEW_OPTION_CHECK_OPTION_NOT_SET = -1,
+	VIEW_OPTION_CHECK_OPTION_LOCAL = 0,
+	VIEW_OPTION_CHECK_OPTION_CASCADED = 1,
+} ViewOptCheckOption;
 
 /*
  * ViewOptions
@@ -336,7 +349,7 @@ typedef struct ViewOptions
 {
 	int32		vl_len_;		/* varlena header (do not touch directly!) */
 	bool		security_barrier;
-	int			check_option_offset;
+	ViewOptCheckOption check_option;
 } ViewOptions;
 
 /*
@@ -355,7 +368,8 @@ typedef struct ViewOptions
  */
 #define RelationHasCheckOption(relation)									\
 	((relation)->rd_options &&												\
-	 ((ViewOptions *) (relation)->rd_options)->check_option_offset != 0)
+	 ((ViewOptions *) (relation)->rd_options)->check_option !=				\
+	 VIEW_OPTION_CHECK_OPTION_NOT_SET)
 
 /*
  * RelationHasLocalCheckOption
@@ -364,10 +378,8 @@ typedef struct ViewOptions
  */
 #define RelationHasLocalCheckOption(relation)								\
 	((relation)->rd_options &&												\
-	 ((ViewOptions *) (relation)->rd_options)->check_option_offset != 0 ?	\
-	 strcmp((char *) (relation)->rd_options +								\
-			((ViewOptions *) (relation)->rd_options)->check_option_offset,	\
-			"local") == 0 : false)
+	 ((ViewOptions *) (relation)->rd_options)->check_option ==				\
+	 VIEW_OPTION_CHECK_OPTION_LOCAL)
 
 /*
  * RelationHasCascadedCheckOption
@@ -376,11 +388,8 @@ typedef struct ViewOptions
  */
 #define RelationHasCascadedCheckOption(relation)							\
 	((relation)->rd_options &&												\
-	 ((ViewOptions *) (relation)->rd_options)->check_option_offset != 0 ?	\
-	 strcmp((char *) (relation)->rd_options +								\
-			((ViewOptions *) (relation)->rd_options)->check_option_offset,	\
-			"cascaded") == 0 : false)
-
+	 ((ViewOptions *) (relation)->rd_options)->check_option ==				\
+	  VIEW_OPTION_CHECK_OPTION_CASCADED)
 
 /*
  * RelationIsValid

--fdj2RfSjLxBAspz7--