From: Nikolay Shaplov <dhyan@nataraj.su>
To: pgsql-hackers@lists.postgresql.org
Cc: Nikita Glukhov <n.gluhov@postgrespro.ru>
Cc: Alvaro Herrera <alvherre@alvh.no-ip.org>
Cc: PostgreSQL mailing lists <pgsql-hackers@postgresql.org>
Subject: Re: [PATCH][PROPOSAL] Add enum releation option type
Date: Wed, 07 Mar 2018 18:08:49 +0300
Message-ID: <3831139.TYuODbpbJx@x200m> (raw)
In-Reply-To: <68976929-aeeb-336d-869c-94e53c9b1c87@postgrespro.ru>
References: <20180215213518.5btgsjhbjxlolife@alvherre.pgsql>
<68976929-aeeb-336d-869c-94e53c9b1c87@postgrespro.ru>
В письме от 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:
[text/x-patch] enum-reloptions4.diff (15.2K, ../3831139.TYuODbpbJx@x200m/2-enum-reloptions4.diff)
download | inline diff:diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.cindex 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.cindex 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.cindex 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.cindex 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.hindex 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.hindex 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.hindex 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.hindex 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.outindex 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".
[application/pgp-signature] signature.asc (472B, ../3831139.TYuODbpbJx@x200m/3-signature.asc)
download
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: dhyan@nataraj.su, pgsql-hackers@lists.postgresql.org, n.gluhov@postgrespro.ru, alvherre@alvh.no-ip.org
Subject: Re: [PATCH][PROPOSAL] Add enum releation option type
In-Reply-To: <3831139.TYuODbpbJx@x200m>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox