Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA384:256) (Exim 4.89) (envelope-from ) id 1etagf-0000jQ-GK for pgsql-hackers@arkaria.postgresql.org; Wed, 07 Mar 2018 15:09:01 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.89) (envelope-from ) id 1etage-0006N2-Dy for pgsql-hackers@arkaria.postgresql.org; Wed, 07 Mar 2018 15:09:00 +0000 Received: from makus.postgresql.org ([2001:4800:1501:1::229]) by malur.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA384:256) (Exim 4.89) (envelope-from ) id 1etagd-0006Mi-QK for pgsql-hackers@lists.postgresql.org; Wed, 07 Mar 2018 15:09:00 +0000 Received: from enot.nataraj.su ([195.54.209.170]) by makus.postgresql.org with esmtp (Exim 4.89) (envelope-from ) id 1etagZ-0000gE-BN for pgsql-hackers@postgresql.org; Wed, 07 Mar 2018 15:08:58 +0000 Received: from x200m.localnet (unknown [91.203.190.3]) by enot.nataraj.su (Postfix) with ESMTPSA id 5BFBF511E1B7; Wed, 7 Mar 2018 18:08:52 +0300 (MSK) From: Nikolay Shaplov To: pgsql-hackers@lists.postgresql.org Cc: Nikita Glukhov , Alvaro Herrera , PostgreSQL mailing lists Subject: Re: [PATCH][PROPOSAL] Add enum releation option type Date: Wed, 07 Mar 2018 18:08:49 +0300 Message-ID: <3831139.TYuODbpbJx@x200m> User-Agent: KMail/4.14.1 (Linux/3.16.0-4-amd64; KDE/4.14.2; x86_64; ; ) In-Reply-To: <68976929-aeeb-336d-869c-94e53c9b1c87@postgrespro.ru> References: <20180215213518.5btgsjhbjxlolife@alvherre.pgsql> <68976929-aeeb-336d-869c-94e53c9b1c87@postgrespro.ru> MIME-Version: 1.0 Content-Type: multipart/signed; boundary="nextPart2443440.UqCUBl66L0"; micalg="pgp-sha256"; protocol="application/pgp-signature" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk --nextPart2443440.UqCUBl66L0 Content-Type: multipart/mixed; boundary="nextPart5640513.TidDvOKHov" Content-Transfer-Encoding: quoted-printable This is a multi-part message in MIME format. =2D-nextPart5640513.TidDvOKHov Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset="utf-8" =D0=92 =D0=BF=D0=B8=D1=81=D1=8C=D0=BC=D0=B5 =D0=BE=D1=82 1 =D0=BC=D0=B0= =D1=80=D1=82=D0=B0 2018 19:11:05 =D0=BF=D0=BE=D0=BB=D1=8C=D0=B7=D0=BE=D0= =B2=D0=B0=D1=82=D0=B5=D0=BB=D1=8C Nikita Glukhov =D0=BD=D0=B0=D0=BF=D0=B8= =D1=81=D0=B0=D0=BB: > Hi. >=20 > 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.=20 Hi! I've imported yours relopt_enum_element solution. Since Alvaro like= d it,=20 and I like it to. But I called it relopt_enum_element_definition, as it= is not=20 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)=09/* pseudo-name for = default > value */ I've discussed this solution with my C-experienced friends, and each of= them=20 said, that it will work, but it is better not to use ((const char *) -1= ) as it=20 will stop working without any warning, because it is not standard C sol= ution=20 and newer C-compiler can stop accepting such thing without further noti= ce. I would avoid using of such thing if possible. > Also default option value should be placed now in the first element o= f > allowed_values[]. This helps not to expose default values definition= s (like > GIST_BUFFERING_AUTO defined in gistbuild.c). For all other reloption types, default value is a part of relopt_* stru= cture.=20 I see no reason to do otherwise for enum. As for exposing GIST_BUFFERING_AUTO. We do the same for default fillfac= tor=20 value. I see no reason why we should do otherwise here... And if we keep default value on relopt_enum, we will not need=20 RELOPT_ENUM_DEFAULT that, as I said above, I found dubious. > for (elem =3D 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 =3D=3D fal= se 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=20 brocken.=20 May be I later should write an additional patch, that throw WARNING if=20= reloptions from pg_class can't be parsed. DB admin should know about it= , I=20 think... The rest I've kept as I do before. If you think that something else sho= uld be=20 changed, I'd like to see, not only the changes, but also some explanati= ons. I=20 am not sure, for example that we should use same enum for reloptions an= d=20 metapage buffering mode representation for example. This seems to be lo= gical,=20 but it may also confuse. I wan to hear all opinions before doing it, fo= r=20 example.=20 May be=20 typedef enum gist_option_buffering_numeric_values { =09GIST_OPTION_BUFFERING_ON =3D GIST_BUFFERING_STATS, =09GIST_OPTION_BUFFERING_OFF =3D GIST_BUFFERING_DISABLED, =09GIST_OPTION_BUFFERING_AUTO =3D GIST_BUFFERING_AUTO, } gist_option_buffering_value_numbers; will do, and then we can assign one enum to another, keeping the tradit= ional=20 variable naming? I also would prefer to keep all enum definition in an .h file, not enum= part=20 in .h file, and array part in .c. =2D-=20 Do code for fun. =2D-nextPart5640513.TidDvOKHov Content-Disposition: attachment; filename="enum-reloptions4.diff" Content-Transfer-Encoding: 7Bit Content-Type: text/x-patch; charset="UTF-8"; name="enum-reloptions4.diff" diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c index 46276ce..4b775ab 100644 =2D-- a/src/backend/access/common/reloptions.c +++ b/src/backend/access/common/reloptions.c @@ -404,7 +404,11 @@ static relopt_real realRelOpts[] = {{NULL}} }; =2Dstatic 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 }, =2D 4, =2D false, =2D gistValidateBufferingOption, =2D "auto" + gist_buffering_enum_def, + GIST_OPTION_BUFFERING_AUTO }, { { @@ -425,11 +427,14 @@ static relopt_string stringRelOpts[] = RELOPT_KIND_VIEW, AccessExclusiveLock }, =2D 0, =2D true, =2D validateWithCheckOption, =2D 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)}, =2D {"check_option", RELOPT_TYPE_STRING, =2D 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 =2D-- 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; =2D char *bufferingMode = (char *) options + options->bufferingModeOffset; =2D if (strcmp(bufferingMode, "on") == 0) + if (options->buffering_mode == GIST_OPTION_BUFFERING_ON) buildstate.bufferingMode = GIST_BUFFERING_STATS; =2D 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) } /* =2D * Validator for "buffering" reloption on GiST indexes. Allows "on", "off" =2D * and "auto" values. =2D */ =2Dvoid =2DgistValidateBufferingOption(const char *value) =2D{ =2D if (value == NULL || =2D (strcmp(value, "on") != 0 && =2D strcmp(value, "off") != 0 && =2D strcmp(value, "auto") != 0)) =2D { =2D ereport(ERROR, =2D (errcode(ERRCODE_INVALID_PARAMETER_VALUE), =2D errmsg("invalid value for \"buffering\" option"), =2D errdetail("Valid values are \"on\", \"off\", and \"auto\"."))); =2D } =2D} =2D =2D/* * 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 =2D-- 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)}, =2D {"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 =2D-- a/src/backend/commands/view.c +++ b/src/backend/commands/view.c @@ -39,24 +39,6 @@ static void checkViewTupleDesc(TupleDesc newdesc, TupleDesc olddesc); /*--------------------------------------------------------------------- =2D * Validator for "check_option" reloption on views. The allowed values =2D * are "local" and "cascaded". =2D */ =2Dvoid =2DvalidateWithCheckOption(const char *value) =2D{ =2D if (value == NULL || =2D (strcmp(value, "local") != 0 && =2D strcmp(value, "cascaded") != 0)) =2D { =2D ereport(ERROR, =2D (errcode(ERRCODE_INVALID_PARAMETER_VALUE), =2D errmsg("invalid value for \"check_option\" option"), =2D errdetail("Valid values are \"local\" and \"cascaded\"."))); =2D } =2D} =2D =2D/*--------------------------------------------------------------------- * 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 =2D-- 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) */ =2D 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 =2D-- 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 =2D-- a/src/include/commands/view.h +++ b/src/include/commands/view.h @@ -17,8 +17,6 @@ #include "catalog/objectaddress.h" #include "nodes/parsenodes.h" =2Dextern void validateWithCheckOption(const char *value); =2D 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 =2D-- 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; =2D int check_option_offset; + int check_option; } ViewOptions; /* @@ -364,7 +383,8 @@ typedef struct ViewOptions */ #define RelationHasCheckOption(relation) \ ((relation)->rd_options && \ =2D ((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 && \ =2D ((ViewOptions *) (relation)->rd_options)->check_option_offset != 0 ? \ =2D strcmp((char *) (relation)->rd_options + \ =2D ((ViewOptions *) (relation)->rd_options)->check_option_offset, \ =2D "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 && \ =2D ((ViewOptions *) (relation)->rd_options)->check_option_offset != 0 ? \ =2D strcmp((char *) (relation)->rd_options + \ =2D ((ViewOptions *) (relation)->rd_options)->check_option_offset, \ =2D "cascaded") == 0 : false) =2D + ((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 =2D-- 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 =2DDETAIL: 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". =2D-nextPart5640513.TidDvOKHov-- This is a multi-part message in MIME format. --nextPart5640513.TidDvOKHov Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset="utf-8" =D0=92 =D0=BF=D0=B8=D1=81=D1=8C=D0=BC=D0=B5 =D0=BE=D1=82 1 =D0=BC=D0=B0= =D1=80=D1=82=D0=B0 2018 19:11:05 =D0=BF=D0=BE=D0=BB=D1=8C=D0=B7=D0=BE=D0= =B2=D0=B0=D1=82=D0=B5=D0=BB=D1=8C Nikita Glukhov =D0=BD=D0=B0=D0=BF=D0=B8= =D1=81=D0=B0=D0=BB: > Hi. >=20 > 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.=20 Hi! I've imported yours relopt_enum_element solution. Since Alvaro like= d it,=20 and I like it to. But I called it relopt_enum_element_definition, as it= is not=20 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)=09/* pseudo-name for = default > value */ I've discussed this solution with my C-experienced friends, and each of= them=20 said, that it will work, but it is better not to use ((const char *) -1= ) as it=20 will stop working without any warning, because it is not standard C sol= ution=20 and newer C-compiler can stop accepting such thing without further noti= ce. I would avoid using of such thing if possible. > Also default option value should be placed now in the first element o= f > allowed_values[]. This helps not to expose default values definition= s (like > GIST_BUFFERING_AUTO defined in gistbuild.c). For all other reloption types, default value is a part of relopt_* stru= cture.=20 I see no reason to do otherwise for enum. As for exposing GIST_BUFFERING_AUTO. We do the same for default fillfac= tor=20 value. I see no reason why we should do otherwise here... And if we keep default value on relopt_enum, we will not need=20 RELOPT_ENUM_DEFAULT that, as I said above, I found dubious. > for (elem =3D 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 =3D=3D fal= se 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=20 brocken.=20 May be I later should write an additional patch, that throw WARNING if=20= reloptions from pg_class can't be parsed. DB admin should know about it= , I=20 think... The rest I've kept as I do before. If you think that something else sho= uld be=20 changed, I'd like to see, not only the changes, but also some explanati= ons. I=20 am not sure, for example that we should use same enum for reloptions an= d=20 metapage buffering mode representation for example. This seems to be lo= gical,=20 but it may also confuse. I wan to hear all opinions before doing it, fo= r=20 example.=20 May be=20 typedef enum gist_option_buffering_numeric_values { =09GIST_OPTION_BUFFERING_ON =3D GIST_BUFFERING_STATS, =09GIST_OPTION_BUFFERING_OFF =3D GIST_BUFFERING_DISABLED, =09GIST_OPTION_BUFFERING_AUTO =3D GIST_BUFFERING_AUTO, } gist_option_buffering_value_numbers; will do, and then we can assign one enum to another, keeping the tradit= ional=20 variable naming? I also would prefer to keep all enum definition in an .h file, not enum= part=20 in .h file, and array part in .c. --=20 Do code for fun. --nextPart5640513.TidDvOKHov Content-Disposition: attachment; filename="enum-reloptions4.diff" Content-Transfer-Encoding: 7Bit Content-Type: text/x-patch; charset="UTF-8"; name="enum-reloptions4.diff" 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". --nextPart5640513.TidDvOKHov-- --nextPart2443440.UqCUBl66L0 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: This is a digitally signed message part. Content-Transfer-Encoding: 7Bit -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQEcBAABCAAGBQJaoACCAAoJEKc13HowK+H4YywIAICkNPKN0U1JIo3OklLG6gbB u85xuFejBJoe1+DGx8QmYHtlJ3uQzlhI7pyIryS67oFMWK6eR/LGjqAQWd87Nnq2 LaHMJvBIwGIABid9kjPC/GSgpRCqEor9P/8APxCYYFquQxuE0L77kb5egl9+DqbN k7uR6hl/qippdFAiZ4lmjESXrKmqzMp5TIsLV2GjXZq/EpLQ8X/plLxnylQ34dqs 8a4b5H39VaNJCEl/hYqXFMx2AjOf4sBsiBgldungvRpROJC60HCyFTY/uuFkM7ry fFR1vDZgTSnLi5EfOwywpwLDJ4l16jX2/m4nIrl39ceCdBTBKO4HtWYYbVMRodI= =b51E -----END PGP SIGNATURE----- --nextPart2443440.UqCUBl66L0--