Hi! While working with my big reloption patch, https://www.postgresql.org/message-id/flat/2146419.veIEZdk4E4@x200m#2146419.veIEZdk4E4@x200m I found out, that all relation options of string type in current postgres, are actually behaving as "enum" type. But each time this behavior is implemented as validate function plus strcmp to compare option value against one of the possible values.
I think it is not the best practice. It is better to have enum type where it is technically enum, and keep sting type for further usage (for example for some kind of index patterns or something like this). Now strcmp in this case does not really slow down postgres, as both string options are checked when index is created. One check there will not really slow down. But if in future somebody would want to have such option checked on regular basis, it is better to have it as enum type: once "strcmp" it while parsing, and then just "==" when one need to check option value in runtime. The patch is in attachment. I hope the code is quite clear. Possible flaws: 1. I've changed error message from 'Valid values are "XXX", "YYY" and "ZZZ".' to 'Valid values are "XXX", "YYY", "ZZZ".' to make a code a bit simpler. If it is not acceptable, please let me know, I will add "and" to the string. 2. Also about the string with the list of acceptable values: the code that creates this string is inside parse_one_reloption function now. It is a bit too complex. May be there is already exists some helper function that creates a string "XXX", "YYY", "ZZZ" from the list of XXX YYY ZZZ values, I do not know of? Or may be it is reason to create such a function. If so where to create it? Inside "reloptions.c"? Or it can be reused and I'd better put it somewhere else? I hope there would be not further difficulties with this patch. Hope it can be committed in proper time. -- Do code for fun.
diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
index 274f7aa..3d12a30 100644
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
@@ -404,7 +404,9 @@ static relopt_real realRelOpts[] {{NULL}}
};
-static relopt_string stringRelOpts[] +static const char *gist_buffering_enum_names[] = GIST_OPTION_BUFFERING_VALUE_NAMES;
+static const char *view_check_option_names[] = VIEW_OPTION_CHECK_OPTION_VALUE_NAMES;
+static relopt_enum enumRelOpts[] {
{
{
@@ -413,10 +415,8 @@ static relopt_string stringRelOpts[] RELOPT_KIND_GIST,
AccessExclusiveLock
},
- 4,
- false,
- gistValidateBufferingOption,
- "auto"
+ gist_buffering_enum_names,
+ GIST_OPTION_BUFFERING_AUTO
},
{
{
@@ -425,11 +425,14 @@ static relopt_string stringRelOpts[] RELOPT_KIND_VIEW,
AccessExclusiveLock
},
- 0,
- true,
- validateWithCheckOption,
- NULL
+ view_check_option_names,
+ VIEW_OPTION_CHECK_OPTION_NOT_SET
},
+ {{NULL}}
+};
+
+static relopt_string stringRelOpts[] +{
/* list terminator */
{{NULL}}
};
@@ -476,6 +479,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,
@@ -514,6 +523,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;
@@ -611,6 +628,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;
@@ -690,6 +710,24 @@ add_real_reloption(bits32 kinds, const char *name, const char *desc, double defa
}
/*
+ * add_enuum_reloption
+ * Add a new enum reloption
+ */
+void
+add_enum_reloption(bits32 kinds, const char *name, const char *desc,
+ const char **allowed_values, int default_val)
+{
+ relopt_enum *newoption;
+
+ newoption = (relopt_enum *) allocate_reloption(kinds, RELOPT_TYPE_ENUM,
+ name, desc);
+ newoption->allowed_values = allowed_values;
+ newoption->default_val = default_val;
+
+ add_reloption((relopt_gen *) newoption);
+}
+
+/*
* add_string_reloption
* Add a new string reloption
*
@@ -1192,6 +1230,78 @@ parse_one_reloption(relopt_value *option, char *text_str, int text_len,
optreal->min, optreal->max)));
}
break;
+ case RELOPT_TYPE_ENUM:
+ {
+ relopt_enum *opt_enum = (relopt_enum *) option->gen;
+ int i = 0;
+
+ parsed = false;
+ while (opt_enum->allowed_values[i])
+ {
+ if (pg_strcasecmp(value, opt_enum->allowed_values[i]) == 0)
+ {
+ option->values.enum_val = i;
+ parsed = true;
+ break;
+ }
+ i++;
+ }
+ if (!parsed)
+ {
+ /*
+ * If value was not found among enum value list, we should
+ * raise error listing all acceptable values. So we build
+ * the list, and raise error
+ */
+ int length = 0;
+ char *str;
+ char *ptr;
+
+ /*
+ * Generating list of allowed values: "value1", "value2",
+ * ... "valueN"
+ */
+ i = 0;
+ while (opt_enum->allowed_values[i])
+ {
+ length += strlen(opt_enum->allowed_values[i]) + 4;
+ /* +4: two quotes, one comma, one space */
+ i++;
+ }
+
+ /*
+ * one byte not used for comma after the last item will be
+ * used for \0; for another byte will do -1
+ */
+ str = palloc((length - 1) * sizeof(char));
+ i = 0;
+ ptr = str;
+ while (opt_enum->allowed_values[i])
+ {
+ if (i != 0)
+ {
+ ptr[0] = ',';
+ ptr[1] = ' ';
+ ptr += 2;
+ }
+ ptr[0] = '"';
+ ptr++;
+ sprintf(ptr, "%s", opt_enum->allowed_values[i]);
+ ptr += strlen(ptr);
+ ptr[0] = '"';
+ ptr++;
+ i++;
+ }
+ *ptr = '\0';
+
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("invalid value for \"%s\" option",
+ option->gen->name),
+ errdetail("Valid values are %s.", str)));
+ }
+ }
+ break;
case RELOPT_TYPE_STRING:
{
relopt_string *optstring = (relopt_string *) option->gen;
@@ -1285,6 +1395,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)
@@ -1395,8 +1510,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 d22318a..6c81f57 100644
--- a/src/backend/access/gist/gistbuild.c
+++ b/src/backend/access/gist/gistbuild.c
@@ -126,11 +126,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;
@@ -234,25 +233,6 @@ gistbuild(Relation heap, Relation index, IndexInfo *indexInfo)
}
/*
- * 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.
*
* If there is not enough memory for buffering build, sets bufferingMode
diff --git a/src/backend/access/gist/gistutil.c b/src/backend/access/gist/gistutil.c
index 55cccd2..fe49f53 100644
--- a/src/backend/access/gist/gistutil.c
+++ b/src/backend/access/gist/gistutil.c
@@ -840,7 +840,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 04ad76a..61b3820 100644
--- a/src/backend/commands/view.c
+++ b/src/backend/commands/view.c
@@ -39,24 +39,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 ||
- (pg_strcasecmp(value, "local") != 0 &&
- pg_strcasecmp(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
*
* Create a view relation and use the rules system to store the query
diff --git a/src/include/access/gist_private.h b/src/include/access/gist_private.h
index 36ed724..d492ad4 100644
--- a/src/include/access/gist_private.h
+++ b/src/include/access/gist_private.h
@@ -368,13 +368,30 @@ typedef struct GISTBuildBuffers
} GISTBuildBuffers;
/*
+ * Definition of items of enum type. Names and codes. To add or modify item
+ * edit both lists
+ */
+#define GIST_OPTION_BUFFERING_VALUE_NAMES { \
+ "on", \
+ "off", \
+ "auto", \
+ (const char *) NULL \
+}
+typedef enum gist_option_buffering_value_numbers
+{
+ GIST_OPTION_BUFFERING_ON = 0,
+ GIST_OPTION_BUFFERING_OFF = 1,
+ GIST_OPTION_BUFFERING_AUTO = 2,
+} gist_option_buffering_value_numbers;
+
+/*
* Storage type for GiST's reloptions
*/
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? */
+ int buffering_mode; /* use buffering build? */
} GiSTOptions;
/* gist.c */
diff --git a/src/include/access/reloptions.h b/src/include/access/reloptions.h
index 94739f7..6162fdd 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,14 @@ typedef struct relopt_real
double max;
} relopt_real;
+typedef struct relopt_enum
+{
+ relopt_gen gen;
+ const char **allowed_values;/* Null terminated array of allowed values for
+ * the option */
+ int default_val; /* Number of item of allowed_values array */
+} relopt_enum;
+
/* validation routines for strings */
typedef void (*validate_string_relopt) (const char *value);
@@ -252,6 +262,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,
+ const char **allowed_values, 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 4703922..50d45a5 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 aa8add5..9fe1afa 100644
--- a/src/include/utils/rel.h
+++ b/src/include/utils/rel.h
@@ -336,6 +336,22 @@ typedef struct StdRdOptions
((relation)->rd_options ? \
((StdRdOptions *) (relation)->rd_options)->parallel_workers : (defaultpw))
+/*
+ * Definition of items of enum type. Names and codes. To add or modify item
+ * edit both lists
+ */
+#define VIEW_OPTION_CHECK_OPTION_VALUE_NAMES { \
+ "local", \
+ "cascaded", \
+ (const char *) NULL \
+}
+
+typedef enum view_option_check_option_value_numbers
+{
+ VIEW_OPTION_CHECK_OPTION_NOT_SET = -1,
+ VIEW_OPTION_CHECK_OPTION_LOCAL = 0,
+ VIEW_OPTION_CHECK_OPTION_CASCADED = 1,
+} view_option_check_option_value_numbers;
/*
* ViewOptions
@@ -345,7 +361,7 @@ typedef struct ViewOptions
{
int32 vl_len_; /* varlena header (do not touch directly!) */
bool security_barrier;
- int check_option_offset;
+ int check_option;
} ViewOptions;
/*
@@ -364,7 +380,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
@@ -373,10 +390,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
@@ -385,11 +400,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
diff --git a/src/test/regress/expected/gist.out b/src/test/regress/expected/gist.out
index f5a2993..31bb64d 100644
--- a/src/test/regress/expected/gist.out
+++ b/src/test/regress/expected/gist.out
@@ -13,7 +13,7 @@ drop index gist_pointidx2, gist_pointidx3, gist_pointidx4;
-- Make sure bad values are refused
create index gist_pointidx5 on gist_point_tbl using gist(p) with (buffering = invalid_value);
ERROR: invalid value for "buffering" option
-DETAIL: Valid values are "on", "off", and "auto".
+DETAIL: Valid values are "on", "off", "auto".
create index gist_pointidx5 on gist_point_tbl using gist(p) with (fillfactor=9);
ERROR: value 9 out of bounds for option "fillfactor"
DETAIL: Valid values are between "10" and "100".
diff --git a/src/test/regress/expected/updatable_views.out b/src/test/regress/expected/updatable_views.out
index 964c115..07ae187 100644
--- a/src/test/regress/expected/updatable_views.out
+++ b/src/test/regress/expected/updatable_views.out
@@ -1551,7 +1551,7 @@ SELECT * FROM base_tbl;
ALTER VIEW rw_view1 SET (check_option=here); -- invalid
ERROR: invalid value for "check_option" option
-DETAIL: Valid values are "local" and "cascaded".
+DETAIL: Valid values are "local", "cascaded".
ALTER VIEW rw_view1 SET (check_option=local);
INSERT INTO rw_view2 VALUES (-20); -- should fail
ERROR: new row violates check option for view "rw_view1"
signature.asc
Description: This is a digitally signed message part.
