Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1iAGTA-0004Pr-Tx for pgsql-hackers@arkaria.postgresql.org; Tue, 17 Sep 2019 16:36:49 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.89) (envelope-from ) id 1iAGT9-00061a-By for pgsql-hackers@arkaria.postgresql.org; Tue, 17 Sep 2019 16:36:47 +0000 Received: from makus.postgresql.org ([2001:4800:3e1:1::229]) by malur.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1iAGT8-00061O-MQ for pgsql-hackers@lists.postgresql.org; Tue, 17 Sep 2019 16:36:47 +0000 Received: from mail-qt1-x841.google.com ([2607:f8b0:4864:20::841]) by makus.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.92) (envelope-from ) id 1iAGT4-0007xx-Cd for pgsql-hackers@postgresql.org; Tue, 17 Sep 2019 16:36:45 +0000 Received: by mail-qt1-x841.google.com with SMTP id x4so5179315qtq.8 for ; Tue, 17 Sep 2019 09:36:42 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=2ndquadrant-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:mime-version:content-disposition :content-transfer-encoding:in-reply-to:user-agent; bh=bwDMFdUwapdWVxa85tUxOyAw4T+BDfZ1uAxU7FFqj5U=; b=vcYM7iswHFf1m6kBPX9cpCfzQg6An/uTZ79Tw9ZQcl03sCUKYwSl2XWchwCtY5h2Ah b7ZI6owql6VDP7HSj2/Cc5e9plJSMmXjR6rREL31Eta4ZxOdmZ/+GaVdmqXHi91J0qZF GMMGqDOG56O/we8tenWRjfts0sRWj/ruIRM9rAodo2qez1FPJySOLUv/k5LBrQseJ3lT AqjIS8PFFpY061R8FzhqsbrcvbXy4NKfgI/+5kxXFeqRTPi1vZIXW9sTeutcCoAysjDS B0s/H8ZgaJDUKhIfo48GAoWLy0j++u8nEzqB/khnmyOf+agGaVPa69W7JLAscPo4bP6y zATA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:mime-version :content-disposition:content-transfer-encoding:in-reply-to :user-agent; bh=bwDMFdUwapdWVxa85tUxOyAw4T+BDfZ1uAxU7FFqj5U=; b=jC+sgAm3UcVMICFzQeRqmCpTWp3W+lKzyOEUSd/9uL9H65aAFGCDR5YwKvEq6E8bPG FIwd8F9GGrkqswDQ1UVbhuIzTPEfb3uqH8mlPStM8/oYB/Usj0Ps3Qd2WZZREGgZ3iFS 0sBGzqQjWEnfzKCdNai1vQSKpCH3Je4vzGWphlQHNWVWicVrVR9n0uM7AenqJWasCEYW wG/H/8I2yfmeNnuFKIP/HlHmzwx/0B6nmuWMT1iiqsMnvLqVGRVOO09T9UvN8pc1rC3U f1sJyts6bGSgtiw8mVPnPTbO47sOQHTq3yxfcbm+EJ0NJGOu6kpuz4BUBJdkN4slK36/ I9hQ== X-Gm-Message-State: APjAAAWdZyKk2LzYR2eqzka1IrkW3rtYjl6sDjKUY+nr5zt/M4qtXkHQ ZPd2zMtBWMlcbnQVvCrw3KXIEA== X-Google-Smtp-Source: APXvYqzDkSVQedPbrnOCr7tG0uRELo0STCjUXbWkNfSwm7VJ//cUWk3y8DwAuv7dI2u1yEUD7j7gJg== X-Received: by 2002:ad4:55b2:: with SMTP id f18mr4098905qvx.16.1568738201309; Tue, 17 Sep 2019 09:36:41 -0700 (PDT) Received: from nimloth.alvh.no-ip.org ([177.154.139.199]) by smtp.gmail.com with ESMTPSA id s16sm1219090qkg.40.2019.09.17.09.36.39 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 17 Sep 2019 09:36:39 -0700 (PDT) Received: by nimloth.alvh.no-ip.org (Postfix, from userid 1000) id 3F085120EE1; Tue, 17 Sep 2019 13:36:36 -0300 (-03) Date: Tue, 17 Sep 2019 13:36:36 -0300 From: Alvaro Herrera To: Nikolay Shaplov Cc: pgsql-hackers@lists.postgresql.org, Tom Lane , Robert Haas , Nikita Glukhov , "pgsql-hackers@postgresql.org" Subject: Re: [PATCH][PROPOSAL] Add enum releation option type Message-ID: <20190917163636.GA1947@alvherre.pgsql> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="u3/rZRmxL6MmkK24" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20190916214531.GA15390@alvherre.pgsql> User-Agent: Mutt/1.9.4 (2018-02-28) List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk --u3/rZRmxL6MmkK24 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit 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 --u3/rZRmxL6MmkK24 Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="enum-reloptions-8.patch" 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 --u3/rZRmxL6MmkK24--