[PATCH][PROPOSAL] Add enum releation option type
Hi!
While working with my big reloption patch,
/messages/by-id/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.
Attachments:
enum-reloptions.difftext/x-patch; charset=UTF-8; name=enum-reloptions.diffDownload
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"
Nikolay Shaplov wrote:
I found out, that all relation options of string type in current postgres, are
actually behaving as "enum" type.
If this patch gets in, I wonder if there are any external modules that
use actual strings. An hypothetical example would be something like a
SSL cipher list; it needs to be somewhat free-form that an enum would
not cut it. If there are such modules, then even if we remove all
existing in-core use cases we should keep the support code for strings.
Maybe we need some in-core user to verify the string case still works.
A new module in src/test/modules perhaps? On the other hand, if we can
find no use for these string reloptions, maybe we should just remove the
support, since as I recall it's messy enough.
[...] 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).
Agreed with the goal, for code simplicity and hopefully reducing
code duplication.
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.
I don't think we care about this, but is this still the case if you use
a stringinfo?
2. Also about the string with the list of acceptable values: the code that
creates this string is inside parse_one_reloption function now.
I think you could save most of that mess by using appendStringInfo and
friends.
I don't much like the way you've represented the list of possible values
for each enum. I think it'd be better to have a struct that represents
everything about each value (string name and C symbol. Maybe the
numerical value too if that's needed, but is it? I suppose all code
should use the C symbol only, so why do we care about the numerical
value?).
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
В письме от 9 февраля 2018 18:45:29 пользователь Alvaro Herrera написал:
If this patch gets in, I wonder if there are any external modules that
use actual strings. An hypothetical example would be something like a
SSL cipher list; it needs to be somewhat free-form that an enum would
not cut it. If there are such modules, then even if we remove all
existing in-core use cases we should keep the support code for strings.
I did not remove string option support from the code. It might be needed
later.
Maybe we need some in-core user to verify the string case still works.
A new module in src/test/modules perhaps?
This sound as a good idea. I am too do not feel really comfortable with that
this string options possibility exists, but is not tested. I'll have a look
there, it it will not require a great job, I will add tests for string options
there.
On the other hand, if we can
find no use for these string reloptions, maybe we should just remove the
support, since as I recall it's messy enough.
No, the implementation of string options is quite good. And may be needed
later.
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.I don't think we care about this, but is this still the case if you use
a stringinfo?
May be not. I'll try to do it better.
2. Also about the string with the list of acceptable values: the code that
creates this string is inside parse_one_reloption function now.I think you could save most of that mess by using appendStringInfo and
friends.
Thanks. I will rewrite this part using these functions. That was really
helpful.
I don't much like the way you've represented the list of possible values
for each enum. I think it'd be better to have a struct that represents
everything about each value (string name and C symbol.
I actually do not like it this way too. I would prefer one structure, not two
lists. But I did not find way how to do it in one struct. How to gave have
string value and C symbol in one structure, without defining C symbols
elsewhere. Otherwise it will be two lists again.
I do not have a lot of hardcore C development experience, so I can miss
something. Can you provide an example of the structure you are talking about?
Maybe the
numerical value too if that's needed, but is it? I suppose all code
should use the C symbol only, so why do we care about the numerical
value?).
It is comfortable to have numerical values when debugging. I like to write
something like
elog(WARNING,"buffering_mode=%i",opt.buffering_mode);
to check that everything works as expected. Such cases is the only reason to
keep numerical value.
--
Do code for fun.
В письме от 9 февраля 2018 18:45:29 пользователь Alvaro Herrera написал:
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.I don't think we care about this, but is this still the case if you use
a stringinfo?2. Also about the string with the list of acceptable values: the code that
creates this string is inside parse_one_reloption function now.I think you could save most of that mess by using appendStringInfo and
friends.
Here is the second verion of the patch where I use appendStringInfo to prepare
error message. The code is much more simple here now, and it now create value
list with "and" at the end: '"xxx", "yyy" and "zzz"'
--
Do code for fun.
Attachments:
enum-reloptions2.difftext/x-patch; charset=UTF-8; name=enum-reloptions2.diffDownload
diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
index 46276ce..82a0492 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
*
@@ -1190,6 +1228,50 @@ 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)
+ {
+ StringInfoData buf;
+
+ initStringInfo(&buf);
+ i = 0;
+ while (opt_enum->allowed_values[i])
+ {
+ appendStringInfo(&buf,"\"%s\"",
+ opt_enum->allowed_values[i]);
+ if (opt_enum->allowed_values[i + 1])
+ {
+ if (opt_enum->allowed_values[i + 2])
+ appendStringInfo(&buf,", ");
+ else
+ appendStringInfo(&buf," and ");
+ }
+ i++;
+ }
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("invalid value for \"%s\" option",
+ option->gen->name),
+ errdetail("Valid values are %s.", /*str*/ buf.data)));
+ pfree(buf.data);
+ }
+ }
+ break;
case RELOPT_TYPE_STRING:
{
relopt_string *optstring = (relopt_string *) option->gen;
@@ -1283,6 +1365,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)
@@ -1393,8 +1480,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 434f15f..02c630d 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 7d4511c..5de0654 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 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 b32c1e9..068745f 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..e5cf179 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" and "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".
В письме от 9 февраля 2018 18:45:29 пользователь Alvaro Herrera написал:
Maybe we need some in-core user to verify the string case still works.
A new module in src/test/modules perhaps?
I've looked attentively in src/test/modules... To properly test all reloptions
hooks for modules wee need to create an extension with some dummy index in it.
This way we can properly test everything. Creating dummy index would be fun,
but it a quite big deal to create it, so it will require a separate patch to
deal with. So I suppose string reloptions is better to leave untested for now,
with a notion, that it should be done sooner or later
I don't much like the way you've represented the list of possible values
for each enum. I think it'd be better to have a struct that represents
everything about each value (string name and C symbol. Maybe the
numerical value too if that's needed, but is it? I suppose all code
should use the C symbol only, so why do we care about the numerical
value?).
One more reason to keep numeric value, that came to my mind, is that it seems
to be logical to keep enum value in postgres internals represented as C-enum.
And C-enum is actually an int value (can be easily casted both ways). It is
not strictly necessary, but it seems to be a bit logical...
--
Do code for fun.
Nikolay Shaplov wrote:
В письме от 9 февраля 2018 18:45:29 пользователь Alvaro Herrera написал:
Maybe we need some in-core user to verify the string case still works.
A new module in src/test/modules perhaps?I've looked attentively in src/test/modules... To properly test all reloptions
hooks for modules wee need to create an extension with some dummy index in it.
This way we can properly test everything. Creating dummy index would be fun,
but it a quite big deal to create it, so it will require a separate patch to
deal with. So I suppose string reloptions is better to leave untested for now,
with a notion, that it should be done sooner or later
Do we really need a dummy index? I was thinking in something that just
calls a few C functions to create and parse a string reloption should be
more than enough.
I don't much like the way you've represented the list of possible values
for each enum. I think it'd be better to have a struct that represents
everything about each value (string name and C symbol. Maybe the
numerical value too if that's needed, but is it? I suppose all code
should use the C symbol only, so why do we care about the numerical
value?).One more reason to keep numeric value, that came to my mind, is that it seems
to be logical to keep enum value in postgres internals represented as C-enum.
And C-enum is actually an int value (can be easily casted both ways). It is
not strictly necessary, but it seems to be a bit logical...
Oh, I didn't mean to steer you away from a C enum. I just meant that we
don't need to define the numerical values ourselves -- it should be fine
to use whatever the C compiler chooses for each C symbol (enum member
name). In the code we don't refer to the values by numerical value,
only by the C symbol.
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
В письме от 15 февраля 2018 12:53:27 пользователь Alvaro Herrera написал:
Maybe we need some in-core user to verify the string case still works.
A new module in src/test/modules perhaps?I've looked attentively in src/test/modules... To properly test all
reloptions hooks for modules wee need to create an extension with some
dummy index in it. This way we can properly test everything. Creating
dummy index would be fun, but it a quite big deal to create it, so it
will require a separate patch to deal with. So I suppose string
reloptions is better to leave untested for now, with a notion, that it
should be done sooner or laterDo we really need a dummy index? I was thinking in something that just
calls a few C functions to create and parse a string reloption should be
more than enough.
Technically we can add_reloption_kind(); then add some string options to it
using add_string_reloption, and then call fillRelOptions with some dummy data
and validate argument on and see what will happen.
But I do not think it will test a lot. I think it is better to test it
altogether, not just some part of it.
Moreover when my whole patch is commited these all should be rewritten, as I
changed some options internals for some good reasons.
So I would prefer to keep it untested while we are done with reloptions, and
then test it in a good way, with creating dummy index and so on. I think it
will be needed for more tests and educational purposes...
But if you will insist on it as a reviewer, I will do as you say.
I don't much like the way you've represented the list of possible values
for each enum. I think it'd be better to have a struct that represents
everything about each value (string name and C symbol. Maybe the
numerical value too if that's needed, but is it? I suppose all code
should use the C symbol only, so why do we care about the numerical
value?).One more reason to keep numeric value, that came to my mind, is that it
seems to be logical to keep enum value in postgres internals represented
as C-enum. And C-enum is actually an int value (can be easily casted both
ways). It is not strictly necessary, but it seems to be a bit logical...Oh, I didn't mean to steer you away from a C enum. I just meant that we
don't need to define the numerical values ourselves -- it should be fine
to use whatever the C compiler chooses for each C symbol (enum member
name). In the code we don't refer to the values by numerical value,
only by the C symbol.
Ah that is what you are talking about :-)
I needed this numbers for debug purposes, nothing more. If it is not good to
keep them, they can be removed now...
(I would prefer to keep them for further debugging, but if it is not right, I
can easily remove them, I do not need them right now)
--
Do code for fun.
Nikolay Shaplov wrote:
В письме от 15 февраля 2018 12:53:27 пользователь Alvaro Herrera написал:
So I would prefer to keep it untested while we are done with reloptions, and
then test it in a good way, with creating dummy index and so on. I think it
will be needed for more tests and educational purposes...But if you will insist on it as a reviewer, I will do as you say.
No, I don't, but let's make sure that there really is a test module
closer to the end of the patch series.
Oh, I didn't mean to steer you away from a C enum. I just meant that we
don't need to define the numerical values ourselves -- it should be fine
to use whatever the C compiler chooses for each C symbol (enum member
name). In the code we don't refer to the values by numerical value,
only by the C symbol.Ah that is what you are talking about :-)
I needed this numbers for debug purposes, nothing more. If it is not good to
keep them, they can be removed now...
(I would prefer to keep them for further debugging, but if it is not right, I
can easily remove them, I do not need them right now)
I'd like to give this deeper review to have a better opinion on this.
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Hi.
I have refactored patch by introducing new struct relop_enum_element to make it
possible to use existing C-enum values in option's definition. So, additional
enum GIST_OPTION_BUFFERING_XXX was removed.
Also default option value should be placed now in the first element of
allowed_values[]. This helps not to expose default values definitions (like
GIST_BUFFERING_AUTO defined in gistbuild.c).
My github repository:
https://github.com/glukhovn/postgres/tree/enum-reloptions
--
Nikita Glukhov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
Attachments:
enum-reloptions-v03.patchtext/x-patch; name=enum-reloptions-v03.patchDownload
diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
index 46276ce..7301ed7 100644
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
@@ -404,7 +404,16 @@ static relopt_real realRelOpts[] =
{{NULL}}
};
-static relopt_string stringRelOpts[] =
+static relopt_enum_element
+view_check_option_enum[] =
+{
+ { RELOPT_ENUM_DEFAULT, VIEW_OPTION_CHECK_OPTION_NOT_SET },
+ { "local", VIEW_OPTION_CHECK_OPTION_LOCAL },
+ { "cascaded", VIEW_OPTION_CHECK_OPTION_CASCADED },
+ { NULL, 0 }
+};
+
+static relopt_enum enumRelOpts[] =
{
{
{
@@ -413,10 +422,7 @@ static relopt_string stringRelOpts[] =
RELOPT_KIND_GIST,
AccessExclusiveLock
},
- 4,
- false,
- gistValidateBufferingOption,
- "auto"
+ gist_buffering_option_enum
},
{
{
@@ -425,11 +431,13 @@ static relopt_string stringRelOpts[] =
RELOPT_KIND_VIEW,
AccessExclusiveLock
},
- 0,
- true,
- validateWithCheckOption,
- NULL
+ view_check_option_enum
},
+ {{NULL}}
+};
+
+static relopt_string stringRelOpts[] =
+{
/* list terminator */
{{NULL}}
};
@@ -476,6 +484,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 +528,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 +633,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 +715,23 @@ 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 *allowed_values)
+{
+ relopt_enum *newoption;
+
+ newoption = (relopt_enum *) allocate_reloption(kinds, RELOPT_TYPE_ENUM,
+ name, desc);
+ newoption->allowed_values = allowed_values;
+
+ add_reloption((relopt_gen *) newoption);
+}
+
+/*
* add_string_reloption
* Add a new string reloption
*
@@ -1190,6 +1232,49 @@ 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;
+ relopt_enum_element *elem = opt_enum->allowed_values;
+
+ parsed = false;
+
+ for (elem = opt_enum->allowed_values; elem->name; elem++)
+ {
+ if (elem->name != RELOPT_ENUM_DEFAULT &&
+ !pg_strcasecmp(value, elem->name))
+ {
+ option->values.enum_val = elem->value;
+ parsed = true;
+ break;
+ }
+ }
+
+ if (validate && !parsed)
+ {
+ StringInfoData buf;
+
+ initStringInfo(&buf);
+
+ for (elem = opt_enum->allowed_values; elem->name; elem++)
+ {
+ if (elem->name == RELOPT_ENUM_DEFAULT)
+ continue; /* should be the first in the list */
+
+ appendStringInfo(&buf, "\"%s\"", elem->name);
+
+ if (elem[1].name)
+ appendStringInfo(&buf, elem[2].name ? ", " : " and ");
+ }
+
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("invalid value for \"%s\" option",
+ option->gen->name),
+ errdetail("Valid values are %s.", /*str*/ buf.data)));
+ }
+ }
+ break;
case RELOPT_TYPE_STRING:
{
relopt_string *optstring = (relopt_string *) option->gen;
@@ -1283,6 +1368,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)->allowed_values[0].value;
+ break;
case RELOPT_TYPE_STRING:
optstring = (relopt_string *) options[i].gen;
if (options[i].isset)
@@ -1393,8 +1483,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 434f15f..a5ca2c3 100644
--- a/src/backend/access/gist/gistbuild.c
+++ b/src/backend/access/gist/gistbuild.c
@@ -74,6 +74,16 @@ typedef struct
GistBufferingMode bufferingMode;
} GISTBuildState;
+/* Definition of GiST enum option 'buffering' */
+relopt_enum_element
+gist_buffering_option_enum[] =
+{
+ { "auto", GIST_BUFFERING_AUTO }, /* default */
+ { "on", GIST_BUFFERING_STATS },
+ { "off", GIST_BUFFERING_DISABLED },
+ { NULL, 0 }
+};
+
/* prototypes for private functions */
static void gistInitBuffering(GISTBuildState *buildstate);
static int calculatePagesPerBuffer(GISTBuildState *buildstate, int levelStep);
@@ -126,15 +136,8 @@ 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)
- buildstate.bufferingMode = GIST_BUFFERING_STATS;
- else if (strcmp(bufferingMode, "off") == 0)
- buildstate.bufferingMode = GIST_BUFFERING_DISABLED;
- else
- buildstate.bufferingMode = GIST_BUFFERING_AUTO;
+ buildstate.bufferingMode = options->bufferingMode;
fillfactor = options->fillfactor;
}
else
@@ -234,25 +237,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..389ecb7 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, bufferingMode)}
};
options = parseRelOptions(reloptions, validate, RELOPT_KIND_GIST,
diff --git a/src/backend/commands/view.c b/src/backend/commands/view.c
index 7d4511c..5de0654 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 36ed724..796264d 100644
--- a/src/include/access/gist_private.h
+++ b/src/include/access/gist_private.h
@@ -17,6 +17,7 @@
#include "access/amapi.h"
#include "access/gist.h"
#include "access/itup.h"
+#include "access/reloptions.h"
#include "fmgr.h"
#include "lib/pairingheap.h"
#include "storage/bufmgr.h"
@@ -368,13 +369,18 @@ typedef struct GISTBuildBuffers
} GISTBuildBuffers;
/*
+ * Definition of items of GiST enum option 'buffering'.
+ */
+extern relopt_enum_element gist_buffering_option_enum[];
+
+/*
* 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 bufferingMode; /* use buffering build? */
} GiSTOptions;
/* gist.c */
diff --git a/src/include/access/reloptions.h b/src/include/access/reloptions.h
index b32c1e9..68a10a4 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,22 @@ typedef struct relopt_real
double max;
} relopt_real;
+typedef struct relopt_enum_element
+{
+ const char *name;
+ int value;
+} relopt_enum_element;
+
+#define RELOPT_ENUM_DEFAULT ((const char *) -1) /* pseudo-name for default value */
+
+typedef struct relopt_enum
+{
+ relopt_gen gen;
+ relopt_enum_element *allowed_values; /* Null terminated array of allowed
+ * values for the option. Its first
+ * element contains default value. */
+} relopt_enum;
+
/* validation routines for strings */
typedef void (*validate_string_relopt) (const char *value);
@@ -252,6 +270,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 *allowed_values);
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..5e7b60d 100644
--- a/src/include/utils/rel.h
+++ b/src/include/utils/rel.h
@@ -336,6 +336,13 @@ typedef struct StdRdOptions
((relation)->rd_options ? \
((StdRdOptions *) (relation)->rd_options)->parallel_workers : (defaultpw))
+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 +352,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 +371,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 +381,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 +391,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..c0ced28 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 "auto", "on" and "off".
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".
Nikita Glukhov wrote:
I have refactored patch by introducing new struct relop_enum_element to make it
possible to use existing C-enum values in option's definition. So, additional
enum GIST_OPTION_BUFFERING_XXX was removed.Also default option value should be placed now in the first element of
allowed_values[]. This helps not to expose default values definitions (like
GIST_BUFFERING_AUTO defined in gistbuild.c).
Cool, yeah this is more in line with what I was thinking.
The "int enum_val" in relopt_value makes me a little nervous. Would it
work to use a relopt_enum_element pointer instead?
I see you lost the Oxford comma:
-DETAIL: Valid values are "on", "off", and "auto".
+DETAIL: Valid values are "auto", "on" and "off".Please put these back.
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
В письме от 1 марта 2018 19:11:05 пользователь Nikita Glukhov написал:
Hi.
I have refactored patch by introducing new struct relop_enum_element to make
it possible to use existing C-enum values in option's definition. So,
additional enum GIST_OPTION_BUFFERING_XXX was removed.
Hi! I've imported yours relopt_enum_element solution. Since Alvaro liked it,
and I like it to. But I called it relopt_enum_element_definition, as it is not
an element itself, but a description of possibilities.
But I do not want to import the rest of it.
#define RELOPT_ENUM_DEFAULT ((const char *) -1) /* pseudo-name for default
value */
I've discussed this solution with my C-experienced friends, and each of them
said, that it will work, but it is better not to use ((const char *) -1) as it
will stop working without any warning, because it is not standard C solution
and newer C-compiler can stop accepting such thing without further notice.
I would avoid using of such thing if possible.
Also default option value should be placed now in the first element of
allowed_values[]. This helps not to expose default values definitions (like
GIST_BUFFERING_AUTO defined in gistbuild.c).
For all other reloption types, default value is a part of relopt_* structure.
I see no reason to do otherwise for enum.
As for exposing GIST_BUFFERING_AUTO. We do the same for default fillfactor
value. I see no reason why we should do otherwise here...
And if we keep default value on relopt_enum, we will not need
RELOPT_ENUM_DEFAULT that, as I said above, I found dubious.
for (elem = opt_enum->allowed_values; elem->name; elem++)
It is better then I did. I imported that too.
if (validate && !parsed)
Oh, yes... There was my mistake. I did not consider validate == false case.
I should do it. Thank you for pointing this out.
But I think that we should return default value if the data in pg_class is
brocken.
May be I later should write an additional patch, that throw WARNING if
reloptions from pg_class can't be parsed. DB admin should know about it, I
think...
The rest I've kept as I do before. If you think that something else should be
changed, I'd like to see, not only the changes, but also some explanations. I
am not sure, for example that we should use same enum for reloptions and
metapage buffering mode representation for example. This seems to be logical,
but it may also confuse. I wan to hear all opinions before doing it, for
example.
May be
typedef enum gist_option_buffering_numeric_values
{
GIST_OPTION_BUFFERING_ON = GIST_BUFFERING_STATS,
GIST_OPTION_BUFFERING_OFF = GIST_BUFFERING_DISABLED,
GIST_OPTION_BUFFERING_AUTO = GIST_BUFFERING_AUTO,
} gist_option_buffering_value_numbers;
will do, and then we can assign one enum to another, keeping the traditional
variable naming?
I also would prefer to keep all enum definition in an .h file, not enum part
in .h file, and array part in .c.
--
Do code for fun.
Attachments:
enum-reloptions4.difftext/x-patch; charset=UTF-8; name=enum-reloptions4.diffDownload
diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
index 46276ce..4b775ab 100644
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
@@ -404,7 +404,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[] =
{
{
{
@@ -413,10 +417,8 @@ static relopt_string stringRelOpts[] =
RELOPT_KIND_GIST,
AccessExclusiveLock
},
- 4,
- false,
- gistValidateBufferingOption,
- "auto"
+ gist_buffering_enum_def,
+ GIST_OPTION_BUFFERING_AUTO
},
{
{
@@ -425,11 +427,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}}
};
@@ -476,6 +481,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 +525,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 +630,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 +712,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
*
@@ -1190,6 +1230,56 @@ 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;
+ relopt_enum_element_definition *el_def;
+
+ parsed = false;
+ for(el_def = opt_enum->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 validateing, just use default nueric value,
+ * otherwise we raise an error
+ */
+ if (!validate)
+ option->values.enum_val = opt_enum->default_val;
+ else
+ {
+ StringInfoData buf;
+ initStringInfo(&buf);
+ for(el_def = opt_enum->enum_def; el_def->text_value;
+ el_def++)
+ {
+ appendStringInfo(&buf,"\"%s\"",el_def->text_value);
+ if (el_def[1].text_value)
+ {
+ if (el_def[2].text_value)
+ appendStringInfo(&buf,", ");
+ else
+ appendStringInfo(&buf," and ");
+ }
+ }
+ 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;
@@ -1283,6 +1373,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)
@@ -1393,8 +1488,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 434f15f..02c630d 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 7d4511c..5de0654 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 36ed724..2d631de 100644
--- a/src/include/access/gist_private.h
+++ b/src/include/access/gist_private.h
@@ -368,13 +368,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_value_numbers;
+
+#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 b32c1e9..f6e645f 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_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);
@@ -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_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 aa8add5..e3f3e7b 100644
--- a/src/include/utils/rel.h
+++ b/src/include/utils/rel.h
@@ -336,6 +336,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
@@ -345,7 +364,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 +383,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 +393,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 +403,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..e5cf179 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" and "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".
В письме от 1 марта 2018 14:47:35 пользователь Alvaro Herrera написал:
I see you lost the Oxford comma:
-DETAIL: Valid values are "on", "off", and "auto". +DETAIL: Valid values are "auto", "on" and "off".Please put these back.
Actually that's me who have lost it. The code with oxford comma would be a
bit more complicated. We should put such coma when we have 3+ items and do not
put it when we have 2.
Does it worth it?
As I've read oxford using of comma is not mandatory and used to avoid
ambiguity.
"XXX, YYY and ZZZ" can be read as "XXX, YYY, ZZZ" or as "XXX, (YYY and ZZZ)".
oxford comma is used to make sure that YYY and ZZZ are separate items of the
list, not an expression inside one item.
But here we hardly have such ambiguity.
So I'll ask again, do you really think it worth it?
--
Do code for fun.
Nikolay Shaplov wrote:
Actually that's me who have lost it.
Yeah, I realized today when I saw your reply to Nikita. I didn't
realize it was him submitting a new version of the patch.
The code with oxford comma would be a
bit more complicated. We should put such coma when we have 3+ items and do not
put it when we have 2.Does it worth it?
As I've read oxford using of comma is not mandatory and used to avoid
ambiguity.
"XXX, YYY and ZZZ" can be read as "XXX, YYY, ZZZ" or as "XXX, (YYY and ZZZ)".
oxford comma is used to make sure that YYY and ZZZ are separate items of the
list, not an expression inside one item.But here we hardly have such ambiguity.
Gracious goodness -- the stuff these Brits come up with!
So I'll ask again, do you really think it worth it?
I'm not qualified to answer this question.
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Hi Nikolay,
I did a quick look at yout patch and have some questions/points to
discuss. I like the idea of the patch and think that enum reloptions
can be useful. Especially for some frequently checked values, as it was
mentioned before.
There are few typos in comments, like 'validateing'.
I have two questions about naming of variables/structures:
1) variable opt_enum in parse_one_reloption named in different way
other similar variables named (without underscore).
2) enum gist_option_buffering_numeric_values/gist_option_buffering_value_numbers.
Firstly, it has two names. Secondly, can we name it
gist_option_buffering, why not?
As you mentioned in previous mail, you prefer to keep enum and
relopt_enum_element_definition array in the same .h file. I'm not sure,
but I think it is done to keep everything related to enum in one place
to avoid inconsistency in case of changes in some part (e.g. change of
enum without change of array). On the other hand, array content created
without array creation itself in .h file. Can we move actual array
creation into same .h file? What is the point to separate array content
definition and array definition?
--
Aleksandr Parfenov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company
В письме от 10 сентября 2018 18:02:10 пользователь Aleksandr Parfenov написал:
I did a quick look at yout patch and have some questions/points to
discuss. I like the idea of the patch and think that enum reloptions
can be useful. Especially for some frequently checked values, as it was
mentioned before.
Thanks.
There are few typos in comments, like 'validateing'.
Yeah. Thats my problem. I've rechecked them with spellchecker, and found two
typos. If there are more, please point me to it.
I have two questions about naming of variables/structures:
1) variable opt_enum in parse_one_reloption named in different way
other similar variables named (without underscore).
I've renamed it. If it confuses you, it may confuse others. No reason to
confuse anybody.
2) enum
gist_option_buffering_numeric_values/gist_option_buffering_value_numbers.
Firstly, it has two names.
My mistake. Fixed it.
Secondly, can we name it gist_option_buffering, why not?
From my point of view, it is not "Gist Buffering Option" itself. It is only a
part of C-code actually that creates "Gist Buffering Option". This enum
defines "Numeric values for Gist Buffering Enum Option". So the logic of the
name is following
(((Gist options)->Buffering)->Numeric Values)
May be "Numeric Values" is not the best name, but this type should be named
gist_option_buffering_[something]. If you have any better idea what this
"something" can be, I will follow your recommendations...
As you mentioned in previous mail, you prefer to keep enum and
relopt_enum_element_definition array in the same .h file. I'm not sure,
but I think it is done to keep everything related to enum in one place
to avoid inconsistency in case of changes in some part (e.g. change of
enum without change of array). On the other hand, array content created
without array creation itself in .h file. Can we move actual array
creation into same .h file? What is the point to separate array content
definition and array definition?
Hm... This would be good. We really can do it? ;-) I am not C-expert, some
aspects of C-development is still mysterious for me. So if it is really ok to
create array in .h file, I would be happy to move it there (This patch does
not include this change, I still want to be sure we can do it)
--
Do code for fun.
Attachments:
enum-reloptions5.difftext/x-patch; charset=UTF-8; name=enum-reloptions5.diffDownload
diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
index db84da0..1801ebf 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
*
@@ -1208,6 +1248,56 @@ 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)
+ {
+ if (el_def[2].text_value)
+ appendStringInfo(&buf,", ");
+ else
+ appendStringInfo(&buf," and ");
+ }
+ }
+ 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;
@@ -1301,6 +1391,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)
@@ -1413,8 +1508,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 434f15f..02c630d 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 dddfe0a..de950c1 100644
--- a/src/backend/access/gist/gistutil.c
+++ b/src/backend/access/gist/gistutil.c
@@ -839,7 +839,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 ffb71c0..5d61b13 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 36ed724..d7f9330 100644
--- a/src/include/access/gist_private.h
+++ b/src/include/access/gist_private.h
@@ -368,13 +368,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 4022c14..9632faf 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 6ecbdb6..ff652e1 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..e5cf179 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" and "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".
В письме от 12 сентября 2018 21:40:49 пользователь Nikolay Shaplov написал:
As you mentioned in previous mail, you prefer to keep enum and
relopt_enum_element_definition array in the same .h file. I'm not sure,
but I think it is done to keep everything related to enum in one place
to avoid inconsistency in case of changes in some part (e.g. change of
enum without change of array). On the other hand, array content created
without array creation itself in .h file. Can we move actual array
creation into same .h file? What is the point to separate array content
definition and array definition?Hm... This would be good. We really can do it? ;-) I am not C-expert, some
aspects of C-development is still mysterious for me. So if it is really ok
to create array in .h file, I would be happy to move it there (This patch
does not include this change, I still want to be sure we can do it)
I've discussed this issue with a colleague, who IS C-expert, and his advice
was not to include this static const into .h file. Because copy of this const
would be created in all objective files where this .h were included. And it is
not the best way...
--
Do code for fun.
On Wed, Mar 7, 2018 at 10:23 AM Nikolay Shaplov <dhyan@nataraj.su> wrote:
I see you lost the Oxford comma:
-DETAIL: Valid values are "on", "off", and "auto". +DETAIL: Valid values are "auto", "on" and "off".Please put these back.
Actually that's me who have lost it. The code with oxford comma would be a
bit more complicated. We should put such coma when we have 3+ items and do not
put it when we have 2.Does it worth it?
In my opinion, it is worth it.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
On Wed, Mar 7, 2018 at 10:23 AM Nikolay Shaplov <dhyan@nataraj.su> wrote:
I see you lost the Oxford comma:
-DETAIL: Valid values are "on", "off", and "auto". +DETAIL: Valid values are "auto", "on" and "off".Please put these back.
Actually that's me who have lost it. The code with oxford comma would be a
bit more complicated. We should put such coma when we have 3+ items and do not
put it when we have 2.Does it worth it?
In my opinion, it is worth it.
Uh ... I've not been paying attention to this thread, but this exchange
seems to be about somebody constructing a message like that piece-by-piece
in code. This has got to be a complete failure from the translatability
standpoint. See
https://www.postgresql.org/docs/devel/static/nls-programmer.html#NLS-GUIDELINES
regards, tom lane
В письме от 1 ноября 2018 11:10:14 пользователь Tom Lane написал:
I see you lost the Oxford comma:
-DETAIL: Valid values are "on", "off", and "auto". +DETAIL: Valid values are "auto", "on" and "off".Please put these back.
Actually that's me who have lost it. The code with oxford comma would be
a
bit more complicated. We should put such coma when we have 3+ items and
do not put it when we have 2.Does it worth it?
In my opinion, it is worth it.
Uh ... I've not been paying attention to this thread, but this exchange
seems to be about somebody constructing a message like that piece-by-piece
in code. This has got to be a complete failure from the translatability
standpoint. Seehttps://www.postgresql.org/docs/devel/static/nls-programmer.html#NLS-GUIDELI
NES
It's a very good reason...
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.
--
Do code for fun.
В письме от 1 ноября 2018 18:26:20 пользователь 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.
--
Do code for fun.
Attachments:
enum-reloptions6.difftext/x-patch; charset=UTF-8; name=enum-reloptions6.diffDownload
diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
index db84da0..b266c86 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
*
@@ -1208,6 +1248,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;
@@ -1301,6 +1386,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)
@@ -1413,8 +1503,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 434f15f..02c630d 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 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 b670cad..35b3d75 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 36ed724..d7f9330 100644
--- a/src/include/access/gist_private.h
+++ b/src/include/access/gist_private.h
@@ -368,13 +368,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 4022c14..9632faf 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 84469f5..ffed97a 100644
--- a/src/include/utils/rel.h
+++ b/src/include/utils/rel.h
@@ -319,6 +319,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
@@ -328,7 +347,7 @@ typedef struct ViewOptions
{
int32 vl_len_; /* varlena header (do not touch directly!) */
bool security_barrier;
- int check_option_offset;
+ int check_option;
} ViewOptions;
/*
@@ -347,7 +366,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
@@ -356,10 +376,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
@@ -368,11 +386,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"
В письме от пятница, 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.
Attachments:
enum-reloptions6a.difftext/x-patch; charset=UTF-8; name=enum-reloptions6a.diffDownload
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"
Attached version 7, with some renaming and rewording of comments.
(I also pgindented. Some things are not pretty because of lack of
typedefs.list patching ... a minor issue at worst.)
I'm still not satisfied with the way the builtin enum tables are passed.
Can we settle for using add_enum_reloption instead at initialization
time? Maybe that would be less ugly. Alternatively, we can have
another member in relopt_enum which is a function pointer that returns
the array of relopt_enum_elt_def. Not sure at which point to call that
function, though.
I think it would be great to have a new enum option in, say,
contrib/bloom, to make sure the add_enum_reloption stuff works
correctly. If there's nothing obvious to add there, let's add something
to src/test/modules.
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachments:
enum-reloptions-7.difftext/x-diff; charset=us-asciiDownload
diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
index f5efe94b7b..0aebb8ffc3 100644
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
@@ -422,32 +422,41 @@ static relopt_real realRelOpts[] =
{{NULL}}
};
-static relopt_string stringRelOpts[] =
+static relopt_enum_elt_def gist_buffering_enum_def[] =
+GIST_OPTION_BUFFERING_ENUM_DEF;
+static relopt_enum_elt_def view_check_option_enum_def[] =
+VIEW_OPTION_CHECK_OPTION_ENUM_DEF;
+static relopt_enum enumRelOpts[] =
{
{
{
"buffering",
- "Enables buffering build for this GiST index",
- RELOPT_KIND_GIST,
- AccessExclusiveLock
+ "Enables buffering build for this GiST index",
+ RELOPT_KIND_GIST,
+ AccessExclusiveLock
},
- 4,
- false,
- gistValidateBufferingOption,
- "auto"
+ gist_buffering_enum_def,
+ GIST_OPTION_BUFFERING_AUTO
},
{
{
"check_option",
- "View has WITH CHECK OPTION defined (local or cascaded).",
- RELOPT_KIND_VIEW,
- AccessExclusiveLock
+ "View has WITH CHECK OPTION defined (local or cascaded).",
+ 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 +503,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 +547,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 +652,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 +734,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_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
*
@@ -1223,6 +1267,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_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 (!parsed)
+ {
+ /*
+ * If value is not among the allowed string values, but we
+ * are not asked to validate, just use default numeric
+ * value. Otherwise raise an error.
+ */
+ if (!validate)
+ option->values.enum_val = optenum->default_val;
+ else
+ {
+ StringInfoData buf;
+
+ initStringInfo(&buf);
+ for (elt = optenum->members; elt->string_val; elt++)
+ {
+ appendStringInfo(&buf, "\"%s\"", elt->string_val);
+ if (elt[1].string_val)
+ 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 +1405,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 +1522,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 94463ffb99..055d3fc8b3 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 8d3dfad27b..2d7669acbc 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 c346630267..4dbafc77eb 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 3698942f9d..3d9c948b69 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 96b457fbe4..df9131877c 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,24 @@ 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;
+ /* null-terminated array of members */
+ relopt_enum_elt_def *members;
+ int default_val;
+} relopt_enum;
+
/* validation routines for strings */
typedef void (*validate_string_relopt) (const char *value);
@@ -253,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 77d8271aec..bcd5e2b687 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 a7c3aa95c2..db1611b984 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 f5a2993aaf..32f5e2512a 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 e64d693e9c..c7ab1a8a8d 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"
В письме от четверг, 3 января 2019 г. 18:12:05 MSK пользователь Alvaro Herrera
написал:
Attached version 7, with some renaming and rewording of comments.
(I also pgindented. Some things are not pretty because of lack of
typedefs.list patching ... a minor issue at worst.)
Thanks! Imported it into my code tree..
I'm still not satisfied with the way the builtin enum tables are passed.
Can we settle for using add_enum_reloption instead at initialization
time? Maybe that would be less ugly. Alternatively, we can have
another member in relopt_enum which is a function pointer that returns
the array of relopt_enum_elt_def. Not sure at which point to call that
function, though.
Actually I am not satisfied with it too... That is why I started that bigger
reloptions refactoring work. So for now I would offer to keep initialization
as it was for other types: in initialize_reloptions using [type]RelOpts[]
arrays. And then I will offer patch that changes it for all types and remove
difference between biult-in reloptions and reloptions in extensions.
I think it would be great to have a new enum option in, say,
contrib/bloom, to make sure the add_enum_reloption stuff works
correctly. If there's nothing obvious to add there, let's add something
to src/test/modules.
As far as I can see there is no obvious place in bloom where we can add enum
options.
So I see several options here:
1. I can try to create some dummy index in src/test/modules. And it would be
very useful for many other tests. For example we will have no real string
reloption when this patch is applied. But it may take a reasonable time to do,
because I've never created indexes before, and I do not have many time-slots
for postgres development in my schedule.
2. Actually I am going to offer patch that will remove difference between
build-in and extension reloptions, all reloptions will use same API. After
applying that patch we would not need to test add_[type]_reloption calls
separately. So may be it would be good enough to check that add_enum_reloption
works right now (add an enum option to bloom, check that it works, and do not
commit that code anywhere) and then wait until we get rid of
add_[type]_reloption API.
This will save us some time. I am a bit worrying about Nikita Glukhov patch it
is better to have reloptions reworked before adding opclass options.
2.5 May be this src/test/modules dummy index is subject to another patch. So
I will start working on it right now, but we will do this work not dependent
to any other patches. And just add there what we need when it is ready. I
would actually add there some code that checks all types of options, because
we actually never check that option value from reloptons successfully reaches
extension code. I did this checks manually, when I wrote that bigger reloption
patch, but there is no facilities to do regularly check is via test suit.
Creating dummy index will create such facilities.
For me all options are good enough, so it is for you too choose, I will do as
you advices.
The following review has been posted through the commitfest application:
make installcheck-world: not tested
Implements feature: not tested
Spec compliant: not tested
Documentation: not tested
I believe this patch simplifies infrastructure. As non native speaker, I'm okay with presented version of comma.
On Sun, Jan 06, 2019 at 06:28:11PM +0300, Nikolay Shaplov wrote:
Actually I am not satisfied with it too... That is why I started that bigger
reloptions refactoring work. So for now I would offer to keep initialization
as it was for other types: in initialize_reloptions using [type]RelOpts[]
arrays. And then I will offer patch that changes it for all types and remove
difference between biult-in reloptions and reloptions in extensions.
Should this be switched as waiting on author or just marked as
returned with feedback then?
2.5 May be this src/test/modules dummy index is subject to another patch. So
I will start working on it right now, but we will do this work not dependent
to any other patches. And just add there what we need when it is ready. I
would actually add there some code that checks all types of options, because
we actually never check that option value from reloptons successfully reaches
extension code. I did this checks manually, when I wrote that bigger reloption
patch, but there is no facilities to do regularly check is via test suit.
Creating dummy index will create such facilities.
bloom is a user-facing extension, while src/test/modules are normally
not installed (there is an exception for MSVC anyway..), so stressing
add_enum_reloption in src/test/modules looks more appropriate to me,
except if you can define an option which is useful for the user and is
an enum.
--
Michael
В письме от среда, 20 февраля 2019 г. 15:08:32 MSK пользователь Michael
Paquier написал:
On Sun, Jan 06, 2019 at 06:28:11PM +0300, Nikolay Shaplov wrote:
Actually I am not satisfied with it too... That is why I started that
bigger reloptions refactoring work. So for now I would offer to keep
initialization as it was for other types: in initialize_reloptions using
[type]RelOpts[] arrays. And then I will offer patch that changes it for
all types and remove difference between biult-in reloptions and
reloptions in extensions.Should this be switched as waiting on author or just marked as
returned with feedback then?
Actually I would prefer "Commited" ;-)
And speaking seriously... Anyway, this test module in src/test/modules should
be a separate patch, as it would test that all types of options are properly
reaching index extension, not only enum.
From my point of view, it can be committed without any dependency with enum
reloption. Either we first commit this patch, and then that test module will
test that enum support, or first we commit that test moudule without testing
enum support, and add tests into this enum patch.
I would prefer to commit enum first, and I would promise that I will add thus
module to the top of my dev TODO list. If it is not ok for you, then please
tell me about it and set this patch as "Waiting for author" and I will do the
test patch first.
2.5 May be this src/test/modules dummy index is subject to another patch.
So I will start working on it right now, but we will do this work not
dependent to any other patches. And just add there what we need when it
is ready. I would actually add there some code that checks all types of
options, because we actually never check that option value from reloptons
successfully reaches extension code. I did this checks manually, when I
wrote that bigger reloption patch, but there is no facilities to do
regularly check is via test suit. Creating dummy index will create such
facilities.bloom is a user-facing extension, while src/test/modules are normally
not installed (there is an exception for MSVC anyway..), so stressing
add_enum_reloption in src/test/modules looks more appropriate to me,
except if you can define an option which is useful for the user and is
an enum.
Thank you for pointing me right direction. 've been waiting for it. Now I can
go on :)) So let's it be src/test/modules module...
В письме от среда, 20 февраля 2019 г. 15:08:32 MSK пользователь Michael
Paquier написал:
2.5 May be this src/test/modules dummy index is subject to another patch.
So I will start working on it right now, but we will do this work not
dependent to any other patches. And just add there what we need when it
is ready. I would actually add there some code that checks all types of
options, because we actually never check that option value from reloptons
successfully reaches extension code. I did this checks manually, when I
wrote that bigger reloption patch, but there is no facilities to do
regularly check is via test suit. Creating dummy index will create such
facilities.bloom is a user-facing extension, while src/test/modules are normally
not installed (there is an exception for MSVC anyway..), so stressing
add_enum_reloption in src/test/modules looks more appropriate to me,
except if you can define an option which is useful for the user and is
an enum.
I've created a module in src/test/modules where we would be able to add enum
support when that module is commited.
https://commitfest.postgresql.org/23/2064/
I've filed it as a separate patch (it is standalone work as I can feel it).
Sadly I did not manage to prepare it before this commitfest, so I've added it
to a next one. :-((
On 3/18/19 11:54 PM, Nikolay Shaplov wrote:
В письме от среда, 20 февраля 2019 г. 15:08:32 MSK пользователь Michael
Paquier написал:2.5 May be this src/test/modules dummy index is subject to another patch.
So I will start working on it right now, but we will do this work not
dependent to any other patches. And just add there what we need when it
is ready. I would actually add there some code that checks all types of
options, because we actually never check that option value from reloptons
successfully reaches extension code. I did this checks manually, when I
wrote that bigger reloption patch, but there is no facilities to do
regularly check is via test suit. Creating dummy index will create such
facilities.bloom is a user-facing extension, while src/test/modules are normally
not installed (there is an exception for MSVC anyway..), so stressing
add_enum_reloption in src/test/modules looks more appropriate to me,
except if you can define an option which is useful for the user and is
an enum.I've created a module in src/test/modules where we would be able to add enum
support when that module is commited.https://commitfest.postgresql.org/23/2064/
I've filed it as a separate patch (it is standalone work as I can feel it).
Sadly I did not manage to prepare it before this commitfest, so I've added it
to a next one. :-((
It's not clear to me that all of Michael's and Álvaro's issues have been
addressed, so I've marked this Waiting on Author.
Regards,
--
-David
david@pgmasters.net
On Mon, Mar 25, 2019 at 9:39 PM David Steele <david@pgmasters.net> wrote:
It's not clear to me that all of Michael's and Álvaro's issues have been
addressed, so I've marked this Waiting on Author.
Hi Nikolay,
To help reviewers for the Commitfest that is starting, could you
please rebase this patch?
--
Thomas Munro
https://enterprisedb.com
It strikes me that the way to avoid sentence construction is to have
each enum reloption declare a string that it uses to list the values it
accepts. So for example we would have
+#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 } \
+}
+
+ GistBufferingValidMsg = gettext_noop("Valid values are \"on\", \"off\", and \"auto\".");I think that's the most contentious point on this patch at this point
(though I may be misremembering).
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
В письме от понедельник, 1 июля 2019 г. 21:25:30 MSK пользователь Thomas Munro
написал:
It's not clear to me that all of Michael's and Álvaro's issues have been
addressed, so I've marked this Waiting on Author.
To help reviewers for the Commitfest that is starting, could you
please rebase this patch?
Oh, yes, sure, thank you for reminding.
Attachments:
enum-reloptions6b.difftext/x-patch; charset=UTF-8; name=enum-reloptions6b.diffDownload
diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
index 5773021..fffab3a 100644
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
@@ -433,32 +433,41 @@ static relopt_real realRelOpts[] =
{{NULL}}
};
-static relopt_string stringRelOpts[] =
+static relopt_enum_elt_def gist_buffering_enum_def[] =
+GIST_OPTION_BUFFERING_ENUM_DEF;
+static relopt_enum_elt_def view_check_option_enum_def[] =
+VIEW_OPTION_CHECK_OPTION_ENUM_DEF;
+static relopt_enum enumRelOpts[] =
{
{
{
"buffering",
- "Enables buffering build for this GiST index",
- RELOPT_KIND_GIST,
- AccessExclusiveLock
+ "Enables buffering build for this GiST index",
+ RELOPT_KIND_GIST,
+ AccessExclusiveLock
},
- 4,
- false,
- gistValidateBufferingOption,
- "auto"
+ gist_buffering_enum_def,
+ GIST_OPTION_BUFFERING_AUTO
},
{
{
"check_option",
- "View has WITH CHECK OPTION defined (local or cascaded).",
- RELOPT_KIND_VIEW,
- AccessExclusiveLock
+ "View has WITH CHECK OPTION defined (local or cascaded).",
+ 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}}
};
@@ -505,6 +514,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 +558,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 +663,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;
@@ -719,6 +745,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_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 +1278,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_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 (!parsed)
+ {
+ /*
+ * If value is not among the allowed string values, but we
+ * are not asked to validate, just use default numeric
+ * value. Otherwise raise an error.
+ */
+ if (!validate)
+ option->values.enum_val = optenum->default_val;
+ else
+ {
+ StringInfoData buf;
+
+ initStringInfo(&buf);
+ for (elt = optenum->members; elt->string_val; elt++)
+ {
+ appendStringInfo(&buf, "\"%s\"", elt->string_val);
+ if (elt[1].string_val)
+ 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;
@@ -1327,6 +1416,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 +1537,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 ecef0ff..b835779 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;
@@ -237,25 +236,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 49df056..a8857f7 100644
--- a/src/backend/access/gist/gistutil.c
+++ b/src/backend/access/gist/gistutil.c
@@ -895,7 +895,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 87ed453..1ff914d 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 f80694b..d586b04 100644
--- a/src/include/access/gist_private.h
+++ b/src/include/access/gist_private.h
@@ -379,13 +379,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 a1912f4..92c099e 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,24 @@ 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;
+ /* null-terminated array of members */
+ relopt_enum_elt_def *members;
+ int default_val;
+} relopt_enum;
+
/* validation routines for strings */
typedef void (*validate_string_relopt) (const char *value);
@@ -252,6 +272,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 13a5801..663e096 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 d7f33ab..3054bdd 100644
--- a/src/include/utils/rel.h
+++ b/src/include/utils/rel.h
@@ -325,6 +325,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
@@ -334,7 +353,7 @@ typedef struct ViewOptions
{
int32 vl_len_; /* varlena header (do not touch directly!) */
bool security_barrier;
- int check_option_offset;
+ int check_option;
} ViewOptions;
/*
@@ -353,7 +372,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
@@ -362,10 +382,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
@@ -374,11 +392,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 0a43449..dab48c9 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 86a2642..b5f3680 100644
--- a/src/test/regress/expected/updatable_views.out
+++ b/src/test/regress/expected/updatable_views.out
@@ -1734,7 +1734,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"
В письме от понедельник, 1 июля 2019 г. 14:06:28 MSK пользователь Alvaro
Herrera написал:
It strikes me that the way to avoid sentence construction is to have
each enum reloption declare a string that it uses to list the values it
accepts. So for example we would have+#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 } \ +} + + GistBufferingValidMsg = gettext_noop("Valid values are \"on\", \"off\", and \"auto\".");I think that's the most contentious point on this patch at this point
(though I may be misremembering).
I actually removed "and" from the list and let it be simple coma separated
list
ERROR: invalid value for "check_option" option
DETAIL: Valid values are: "local", "cascaded".
Now we can translate left part, and subst list to the right part
errdetail("Valid values are: %s.", buf.data)));
It is not that nice as before, but quite acceptable, as I see it.
You do not see it that way?
On 2019-Jul-03, Nikolay Shaplov wrote:
В письме от понедельник, 1 июля 2019 г. 14:06:28 MSK пользователь Alvaro
Herrera написал:
+ GistBufferingValidMsg = gettext_noop("Valid values are \"on\", \"off\",
and \"auto\".");I think that's the most contentious point on this patch at this point
(though I may be misremembering).I actually removed "and" from the list and let it be simple coma separated
listERROR: invalid value for "check_option" option
DETAIL: Valid values are: "local", "cascaded".Now we can translate left part, and subst list to the right part
Yes, I saw that, and you know what? Nobody said they liked this idea.
You do not see it that way?
I think this is easier to sell if you change that.
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
After looking closer once again, I don't like this patch.
I think the FOOBAR_ENUM_DEF defines serve no purpose, other than
source-code placement next to the enum value definitions. I think for
example check_option, living in reloptions.c, should look like this:
{
{
"check_option",
"View has WITH CHECK OPTION defined (local or cascaded).",
RELOPT_KIND_VIEW,
AccessExclusiveLock
},
{
{ "local", VIEW_OPTION_CHECK_OPTION_LOCAL },
{ "cascaded", VIEW_OPTION_CHECK_OPTION_CASCADED },
{ NULL }
},
"Valid values are \"local\" and \"cascaded\"."
},
Note the relopt_enum is pretty much the same you have, except we also
have a const char *valid_values_errdetail; and the ugly #define no
longer exists but instead we put it in enumRelOpts.
rel.h ends up like this:
/*
* Values for ViewOptions->check_option.
*/
typedef enum
{
VIEWOPTIONS_CHECK_OPTION_NOTSET,
VIEWOPTIONS_CHECK_OPTION_LOCAL,
VIEWOPTIONS_CHECK_OPTION_CASCADED
} ViewOpts_CheckOptionValues;
/*
* ViewOptions
* Contents of rd_options for views
*/
typedef struct ViewOptions
{
int32 vl_len_; /* varlena header (do not touch directly!) */
bool security_barrier;
ViewOpts_CheckOptionValues check_option;
} ViewOptions;
I'm marking this Waiting on Author.
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
В письме от четверг, 5 сентября 2019 г. 11:42:27 MSK пользователь Alvaro
Herrera from 2ndQuadrant написал:
After looking closer once again, I don't like this patch.
I think the FOOBAR_ENUM_DEF defines serve no purpose, other than
source-code placement next to the enum value definitions. I think for
example check_option, living in reloptions.c, should look like this:
This sounds as a good idea, I tried it, but did not succeed.
When I do as you suggest I get a bunch of warnings, and more over it, tests do
not pass afterwards.
reloptions.c:447:3: warning: braces around scalar initializer
{
^
reloptions.c:447:3: warning: (near initialization for
‘enumRelOpts[0].members’)
reloptions.c:448:4: warning: braces around scalar initializer
{ "on", GIST_OPTION_BUFFERING_ON },
^
reloptions.c:448:4: warning: (near initialization for
‘enumRelOpts[0].members’)
reloptions.c:448:4: warning: initialization from incompatible pointer type
reloptions.c:448:4: warning: (near initialization for
‘enumRelOpts[0].members’)
and so on, see full warning list attached.
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.
I also attached a diff of what I have done to get these warnings. It should be
applied to latest version of patch we are discussing.
Attachments:
enum-reloptions.not-a-commitfest-patchtext/x-patch; charset=UTF-8; name=enum-reloptions.not-a-commitfest-patchDownload
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 fffab3a..fcf4766 100644
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
@@ -433,8 +433,6 @@ static relopt_real realRelOpts[] =
{{NULL}}
};
-static relopt_enum_elt_def gist_buffering_enum_def[] =
-GIST_OPTION_BUFFERING_ENUM_DEF;
static relopt_enum_elt_def view_check_option_enum_def[] =
VIEW_OPTION_CHECK_OPTION_ENUM_DEF;
static relopt_enum enumRelOpts[] =
@@ -446,8 +444,13 @@ static relopt_enum enumRelOpts[] =
RELOPT_KIND_GIST,
AccessExclusiveLock
},
- gist_buffering_enum_def,
- GIST_OPTION_BUFFERING_AUTO
+ {
+ {"on", GIST_OPTION_BUFFERING_ON },
+ {"off", GIST_OPTION_BUFFERING_OFF },
+ {"auto", GIST_OPTION_BUFFERING_AUTO },
+ { NULL }
+ },
+ GIST_OPTION_BUFFERING_AUTO
},
{
{
diff --git a/src/include/access/gist_private.h b/src/include/access/gist_private.h
index d586b04..047490a 100644
--- a/src/include/access/gist_private.h
+++ b/src/include/access/gist_private.h
@@ -379,24 +379,15 @@ 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.
+ * gist_option_buffering_values defines values for buffering enum reloption
*/
-typedef enum gist_option_buffering_numeric_values
+typedef enum gist_option_buffering_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 } \
-}
+} gist_option_buffering_numeric_values;
+
/*
* Storage type for GiST's reloptions
On Fri, Sep 13, 2019 at 10:16:30AM +0300, Nikolay Shaplov wrote:
I also attached a diff of what I have done to get these warnings. It should be
applied to latest version of patch we are discussing.
If you do that I think that the CF bot is not going to appreciate and
will result in failures trying to apply the patch.
--
Michael
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.
Well, you need to change the definition of the struct correspondingly
also, which your patch doesn't show you doing.
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
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
Attachments:
enum-reloptions-7.patchtext/x-diff; charset=us-asciiDownload
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
I decided I didn't dislike that patch all that much anyway, so I cleaned
it up a little bit and here's v8.
The add_enum_reloption stuff is still broken. Please fix it and
resubmit. I'm marking this Waiting on Author now.
Thanks,
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachments:
enum-reloptions-8.patchtext/x-diff; charset=us-asciiDownload
diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
index 20f4ed3c38..89a5775229 100644
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
@@ -433,7 +433,25 @@ static relopt_real realRelOpts[] =
{{NULL}}
};
-static relopt_string stringRelOpts[] =
+/* values from GistOptBufferingMode */
+relopt_enum_elt_def gistBufferingOptValues[] =
+{
+ {"auto", GIST_OPTION_BUFFERING_AUTO},
+ {"on", GIST_OPTION_BUFFERING_ON},
+ {"off", GIST_OPTION_BUFFERING_OFF},
+ {(const char *) NULL} /* list terminator */
+};
+
+/* values from ViewOptCheckOption */
+relopt_enum_elt_def viewCheckOptValues[] =
+{
+ /* no value for NOT_SET */
+ {"local", VIEW_OPTION_CHECK_OPTION_LOCAL},
+ {"cascaded", VIEW_OPTION_CHECK_OPTION_CASCADED},
+ {(const char *) NULL} /* list terminator */
+};
+
+static relopt_enum enumRelOpts[] =
{
{
{
@@ -442,10 +460,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,15 +471,20 @@ 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\"."
},
/* list terminator */
{{NULL}}
};
+static relopt_string stringRelOpts[] =
+{
+ /* list terminator */
+ {{NULL}}
+};
+
static relopt_gen **relOpts = NULL;
static bits32 last_assigned_kind = RELOPT_KIND_LAST_DEFAULT;
@@ -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_enum_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,36 @@ 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 +1414,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 +1535,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..2f4543dee5 100644
--- a/src/backend/access/gist/gistbuild.c
+++ b/src/backend/access/gist/gistbuild.c
@@ -125,15 +125,15 @@ gistbuild(Relation heap, Relation index, IndexInfo *indexInfo)
buildstate.indexrel = index;
buildstate.heaprel = heap;
+
if (index->rd_options)
{
/* 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 +236,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..3fec06fe33 100644
--- a/src/include/access/gist_private.h
+++ b/src/include/access/gist_private.h
@@ -395,6 +395,14 @@ typedef struct GISTBuildBuffers
int rootlevel;
} GISTBuildBuffers;
+/* GiSTOptions->buffering_mode values */
+typedef enum GistOptBufferingMode
+{
+ GIST_OPTION_BUFFERING_AUTO,
+ GIST_OPTION_BUFFERING_ON,
+ GIST_OPTION_BUFFERING_OFF
+} GistOptBufferingMode;
+
/*
* Storage type for GiST's reloptions
*/
@@ -402,7 +410,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..25bbca055a 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..a5cf804f9f 100644
--- a/src/include/utils/rel.h
+++ b/src/include/utils/rel.h
@@ -327,6 +327,13 @@ typedef struct StdRdOptions
((relation)->rd_options ? \
((StdRdOptions *) (relation)->rd_options)->parallel_workers : (defaultpw))
+/* ViewOptions->check_option values */
+typedef enum ViewOptCheckOption
+{
+ VIEW_OPTION_CHECK_OPTION_NOT_SET,
+ VIEW_OPTION_CHECK_OPTION_LOCAL,
+ VIEW_OPTION_CHECK_OPTION_CASCADED
+} ViewOptCheckOption;
/*
* ViewOptions
@@ -336,7 +343,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 +362,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 +372,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 +382,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
On 2019-Sep-17, Alvaro Herrera wrote:
I decided I didn't dislike that patch all that much anyway, so I cleaned
it up a little bit and here's v8.The add_enum_reloption stuff is still broken. Please fix it and
resubmit. I'm marking this Waiting on Author now.
I finished this and pushed.
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services