В письме от пятница, 2 ноября 2018 г. 23:52:13 MSK пользователь Nikolay
Shaplov написал:
> > In this case the only solution I can see is
> >
> > DETAIL: Valid values are: "value1", "value2", "value3".
> >
> > Where list '"value1", "value2", "value3"' is built in runtime but have no
> > any bindnings to any specific language. And the rest of the message is
> > 'DETAIL: Valid values are: %s' which can be properly translated.
>
> I've fiex the patch. Now it does not add "and" at all.
New version of the patch. Nothing is changed, just rebased against current
master.
The big story of the patch:
I've started to rewriting reloption code so it can be used in any kind of
options (my original task was opclass parameters, Nikita Glukhov now doing it)
While rewriting I also find places that can be done in a better way, and also
change them.
Final patch was big, so it was desided to split it into parts.
This is one part of it. It brings more login to some reloptions that
implemented as string options, but logically they are enum options. I think is
is good idea to make them actual enums.
diff --git a/.gitignore b/.gitignore
index 794e35b..37331c2 100644
diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
index eece89a..22906bb 100644
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
@@ -422,7 +422,11 @@ static relopt_real realRelOpts[] =
{{NULL}}
};
-static relopt_string stringRelOpts[] =
+static relopt_enum_element_definition gist_buffering_enum_def[] =
+ GIST_OPTION_BUFFERING_ENUM_DEF;
+static relopt_enum_element_definition view_check_option_enum_def[] =
+ VIEW_OPTION_CHECK_OPTION_ENUM_DEF;
+static relopt_enum enumRelOpts[] =
{
{
{
@@ -431,10 +435,8 @@ static relopt_string stringRelOpts[] =
RELOPT_KIND_GIST,
AccessExclusiveLock
},
- 4,
- false,
- gistValidateBufferingOption,
- "auto"
+ gist_buffering_enum_def,
+ GIST_OPTION_BUFFERING_AUTO
},
{
{
@@ -443,11 +445,14 @@ static relopt_string stringRelOpts[] =
RELOPT_KIND_VIEW,
AccessExclusiveLock
},
- 0,
- true,
- validateWithCheckOption,
- NULL
+ view_check_option_enum_def,
+ VIEW_OPTION_CHECK_OPTION_NOT_SET
},
+ {{NULL}}
+};
+
+static relopt_string stringRelOpts[] =
+{
/* list terminator */
{{NULL}}
};
@@ -494,6 +499,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,
@@ -532,6 +543,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;
@@ -629,6 +648,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;
@@ -708,6 +730,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,
+ relopt_enum_element_definition *enum_def, int default_val)
+{
+ relopt_enum *newoption;
+
+ newoption = (relopt_enum *) allocate_reloption(kinds, RELOPT_TYPE_ENUM,
+ name, desc);
+ newoption->enum_def = enum_def;
+ newoption->default_val = default_val;
+
+ add_reloption((relopt_gen *) newoption);
+}
+
+/*
* add_string_reloption
* Add a new string reloption
*
@@ -1223,6 +1263,51 @@ 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_element_definition *el_def;
+
+ parsed = false;
+ for(el_def = optenum->enum_def; el_def->text_value; el_def++)
+ {
+ if (pg_strcasecmp(value, el_def->text_value) == 0)
+ {
+ option->values.enum_val = el_def->numeric_value;
+ parsed = true;
+ break;
+ }
+ }
+ if (!parsed)
+ {
+ /*
+ * If value is not among allowed enum text values, but we
+ * are not up to validating, just use default numeric value,
+ * otherwise we raise an error
+ */
+ if (!validate)
+ option->values.enum_val = optenum->default_val;
+ else
+ {
+ StringInfoData buf;
+ initStringInfo(&buf);
+ for(el_def = optenum->enum_def; el_def->text_value;
+ el_def++)
+ {
+ appendStringInfo(&buf,"\"%s\"",el_def->text_value);
+ if (el_def[1].text_value)
+ appendStringInfo(&buf,", ");
+ }
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("invalid value for \"%s\" option",
+ option->gen->name),
+ errdetail("Valid values are: %s.", buf.data)));
+ pfree(buf.data);
+ }
+ }
+ }
+ break;
case RELOPT_TYPE_STRING:
{
relopt_string *optstring = (relopt_string *) option->gen;
@@ -1316,6 +1401,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)
@@ -1428,8 +1518,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 b9c4e27..9fd53b0 100644
--- a/src/backend/access/gist/gistbuild.c
+++ b/src/backend/access/gist/gistbuild.c
@@ -128,11 +128,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)
}
/*
- * 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 70627e5..a73b355 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 00e85ed..44a25a4 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 ||
- (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
*
* 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 a73716d..46cb487 100644
--- a/src/include/access/gist_private.h
+++ b/src/include/access/gist_private.h
@@ -369,13 +369,33 @@ typedef struct GISTBuildBuffers
} 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 gist_option_buffering_numeric_values
+{
+ GIST_OPTION_BUFFERING_ON = 0,
+ GIST_OPTION_BUFFERING_OFF = 1,
+ GIST_OPTION_BUFFERING_AUTO = 2,
+} gist_option_buffering_numeric_values;
+
+#define GIST_OPTION_BUFFERING_ENUM_DEF { \
+ { "on", GIST_OPTION_BUFFERING_ON }, \
+ { "off", GIST_OPTION_BUFFERING_OFF }, \
+ { "auto", GIST_OPTION_BUFFERING_AUTO }, \
+ { (const char *) NULL, 0 } \
+}
+
+/*
* 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 50690b9..2838945 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;
@@ -81,6 +82,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;
@@ -108,6 +110,25 @@ typedef struct relopt_real
double max;
} relopt_real;
+/*
+ * relopt_enum_element_definition - An element that defines enum name-value
+ * pair. An array of such elements defines acceptable values of enum option,
+ * and their internal numeric representation
+ */
+typedef struct relopt_enum_element_definition
+{
+ const char *text_value;
+ int numeric_value;
+} relopt_enum_element_definition;
+
+typedef struct relopt_enum
+{
+ relopt_gen gen;
+ relopt_enum_element_definition *enum_def; /* Null terminated array of enum
+ * elements definitions */
+ int default_val; /* Value that is used when option is not defined */
+} relopt_enum;
+
/* validation routines for strings */
typedef void (*validate_string_relopt) (const char *value);
@@ -253,6 +274,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_element_definition *enum_def, 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 2217081..4db2186 100644
--- a/src/include/utils/rel.h
+++ b/src/include/utils/rel.h
@@ -318,6 +318,25 @@ 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 view_option_check_option_numeric_values
+{
+ 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;
+
+#define VIEW_OPTION_CHECK_OPTION_ENUM_DEF { \
+ { "local", VIEW_OPTION_CHECK_OPTION_LOCAL}, \
+ { "cascaded", VIEW_OPTION_CHECK_OPTION_CASCADED }, \
+ { (const char *) NULL, 0 } \
+}
/*
* ViewOptions
@@ -327,7 +346,7 @@ typedef struct ViewOptions
{
int32 vl_len_; /* varlena header (do not touch directly!) */
bool security_barrier;
- int check_option_offset;
+ int check_option;
} ViewOptions;
/*
@@ -346,7 +365,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
@@ -355,10 +375,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
@@ -367,11 +385,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..32f5e25 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 e64d693..c7ab1a8 100644
--- a/src/test/regress/expected/updatable_views.out
+++ b/src/test/regress/expected/updatable_views.out
@@ -1675,7 +1675,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"
