On Mon, Oct 28, 2019 at 05:16:54PM +0900, Amit Langote wrote: > On Sat, Oct 26, 2019 at 11:45 AM Michael Paquier <mich...@paquier.xyz> wrote: >> On Fri, Oct 25, 2019 at 04:42:24PM +0900, Amit Langote wrote: >> > Hmm, if we're inventing a new API to replace the old one, why not use >> > that opportunity to be consistent with our general style, which >> > predominantly seems to be either words_separated_by_underscore() or >> > UpperCamelCase(). Thoughts? >> >> Not wrong. Using small-case characters separated with underscores >> would be more consistent with the rest perhaps? We use that for the >> initialization of custom variables and for all the relkind-related >> interfaces. > > OK, I went with build_reloptions(), which looks very similar to nearby > exported functions.
Thanks. >> + * Parses reloptions provided by the caller in text array format and >> + * fills and returns a struct containing the parsed option values >> The sentence structure is weird, perhaps: >> This routine parses "reloptions" provided by the caller in text-array >> format. The parsing is done with a table describing the allowed >> options, defined by "relopt_elems" of length "num_relopt_elems". The >> returned result is a structure containing all the parsed option >> values. > > Thanks, I have expanded the header comment based on your text. Looks fine. I have done some refinements as per the attached. Running the regression tests of dummy_index_am has proved to be able to break the assertion you have added. I don't have a good idea of how to make that more simple and reliable, but there is one thing outstanding here: the number of options parsed by parseRelOptions should never be higher than num_relopt_elems. So let's at least be safer about that. Also if some options are parsed options will never be NULL, so there is no need to check for it before pfree()-ing it, no? Any comments from others? Alvaro perhaps? -- Michael
diff --git a/contrib/bloom/blutils.c b/contrib/bloom/blutils.c
index 3d44616adc..e2063bac62 100644
--- a/contrib/bloom/blutils.c
+++ b/contrib/bloom/blutils.c
@@ -475,18 +475,18 @@ BloomInitMetapage(Relation index)
bytea *
bloptions(Datum reloptions, bool validate)
{
- relopt_value *options;
- int numoptions;
BloomOptions *rdopts;
/* Parse the user-given reloptions */
- options = parseRelOptions(reloptions, validate, bl_relopt_kind, &numoptions);
- rdopts = allocateReloptStruct(sizeof(BloomOptions), options, numoptions);
- fillRelOptions((void *) rdopts, sizeof(BloomOptions), options, numoptions,
- validate, bl_relopt_tab, lengthof(bl_relopt_tab));
+ rdopts = (BloomOptions *) build_reloptions(reloptions, validate,
+ bl_relopt_kind,
+ sizeof(BloomOptions),
+ bl_relopt_tab,
+ lengthof(bl_relopt_tab));
/* Convert signature length from # of bits to # to words, rounding up */
- rdopts->bloomLength = (rdopts->bloomLength + SIGNWORDBITS - 1) / SIGNWORDBITS;
+ if (rdopts)
+ rdopts->bloomLength = (rdopts->bloomLength + SIGNWORDBITS - 1) / SIGNWORDBITS;
return (bytea *) rdopts;
}
diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c
index ae7b729edd..b4f681a79e 100644
--- a/src/backend/access/brin/brin.c
+++ b/src/backend/access/brin/brin.c
@@ -820,29 +820,15 @@ brinvacuumcleanup(IndexVacuumInfo *info, IndexBulkDeleteResult *stats)
bytea *
brinoptions(Datum reloptions, bool validate)
{
- relopt_value *options;
- BrinOptions *rdopts;
- int numoptions;
static const relopt_parse_elt tab[] = {
{"pages_per_range", RELOPT_TYPE_INT, offsetof(BrinOptions, pagesPerRange)},
{"autosummarize", RELOPT_TYPE_BOOL, offsetof(BrinOptions, autosummarize)}
};
- options = parseRelOptions(reloptions, validate, RELOPT_KIND_BRIN,
- &numoptions);
-
- /* if none set, we're done */
- if (numoptions == 0)
- return NULL;
-
- rdopts = allocateReloptStruct(sizeof(BrinOptions), options, numoptions);
-
- fillRelOptions((void *) rdopts, sizeof(BrinOptions), options, numoptions,
- validate, tab, lengthof(tab));
-
- pfree(options);
-
- return (bytea *) rdopts;
+ return (bytea *) build_reloptions(reloptions, validate,
+ RELOPT_KIND_BRIN,
+ sizeof(BrinOptions),
+ tab, lengthof(tab));
}
/*
diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
index b5072c00fe..94c0029a0a 100644
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
@@ -495,6 +495,14 @@ static bool need_initialization = true;
static void initialize_reloptions(void);
static void parse_one_reloption(relopt_value *option, char *text_str,
int text_len, bool validate);
+static relopt_value *parseRelOptions(Datum options, bool validate,
+ relopt_kind kind, int *numrelopts);
+static void *allocateReloptStruct(Size base, relopt_value *options,
+ int numoptions);
+static void fillRelOptions(void *rdopts, Size basesize,
+ relopt_value *options, int numoptions,
+ bool validate,
+ const relopt_parse_elt *elems, int nelems);
/*
* initialize_reloptions
@@ -1121,6 +1129,55 @@ extractRelOptions(HeapTuple tuple, TupleDesc tupdesc,
return options;
}
+/*
+ * build_reloptions
+ *
+ * Parses "reloptions" provided by the caller, returning them in a
+ * structure containing the parsed options. The parsing is done with
+ * the help of a "parsing table" describing the allowed options, defined
+ * by "relopt_elems" of length "num_relopt_elems".
+ *
+ * "validate" must be true if reloptions value is freshly built by
+ * transformRelOptions(), as opposed to being read from the catalog, in which
+ * case the values contained in it must already be valid.
+ *
+ * The result is a structure containing all the parsed option values in
+ * text-array format. NULL is returned if the passed-in options did not
+ * match any of the options in the parsing table, unless validate is true
+ * in which case an error would be reported.
+ */
+void *
+build_reloptions(Datum reloptions, bool validate,
+ relopt_kind kind,
+ Size relopt_struct_size,
+ const relopt_parse_elt *relopt_elems,
+ int num_relopt_elems)
+{
+ int numoptions;
+ relopt_value *options;
+ void *rdopts;
+
+ /* Parse options specific to given relopt_kind. */
+ options = parseRelOptions(reloptions, validate, kind, &numoptions);
+ Assert(numoptions <= num_relopt_elems);
+
+ /* If none set, we're done. */
+ if (numoptions == 0)
+ {
+ Assert(options == NULL);
+ return NULL;
+ }
+
+ /* Allocate and fill the structure. */
+ rdopts = allocateReloptStruct(relopt_struct_size, options, numoptions);
+ fillRelOptions(rdopts, relopt_struct_size, options, numoptions,
+ validate, relopt_elems, num_relopt_elems);
+
+ pfree(options);
+
+ return rdopts;
+}
+
/*
* Interpret reloptions that are given in text-array format.
*
@@ -1140,7 +1197,7 @@ extractRelOptions(HeapTuple tuple, TupleDesc tupdesc,
* returned array. Values of type string are allocated separately and must
* be freed by the caller.
*/
-relopt_value *
+static relopt_value *
parseRelOptions(Datum options, bool validate, relopt_kind kind,
int *numrelopts)
{
@@ -1365,7 +1422,7 @@ parse_one_reloption(relopt_value *option, char *text_str, int text_len,
* "base" should be sizeof(struct) of the reloptions struct (StdRdOptions or
* equivalent).
*/
-void *
+static void *
allocateReloptStruct(Size base, relopt_value *options, int numoptions)
{
Size size = base;
@@ -1389,7 +1446,7 @@ allocateReloptStruct(Size base, relopt_value *options, int numoptions)
* elems, of length numelems, is the table describing the allowed options.
* When validate is true, it is expected that all options appear in elems.
*/
-void
+static void
fillRelOptions(void *rdopts, Size basesize,
relopt_value *options, int numoptions,
bool validate,
@@ -1474,9 +1531,6 @@ fillRelOptions(void *rdopts, Size basesize,
bytea *
default_reloptions(Datum reloptions, bool validate, relopt_kind kind)
{
- relopt_value *options;
- StdRdOptions *rdopts;
- int numoptions;
static const relopt_parse_elt tab[] = {
{"fillfactor", RELOPT_TYPE_INT, offsetof(StdRdOptions, fillfactor)},
{"autovacuum_enabled", RELOPT_TYPE_BOOL,
@@ -1521,20 +1575,9 @@ default_reloptions(Datum reloptions, bool validate, relopt_kind kind)
offsetof(StdRdOptions, vacuum_truncate)}
};
- options = parseRelOptions(reloptions, validate, kind, &numoptions);
-
- /* if none set, we're done */
- if (numoptions == 0)
- return NULL;
-
- rdopts = allocateReloptStruct(sizeof(StdRdOptions), options, numoptions);
-
- fillRelOptions((void *) rdopts, sizeof(StdRdOptions), options, numoptions,
- validate, tab, lengthof(tab));
-
- pfree(options);
-
- return (bytea *) rdopts;
+ return (bytea *) build_reloptions(reloptions, validate, kind,
+ sizeof(StdRdOptions),
+ tab, lengthof(tab));
}
/*
@@ -1543,9 +1586,6 @@ default_reloptions(Datum reloptions, bool validate, relopt_kind kind)
bytea *
view_reloptions(Datum reloptions, bool validate)
{
- relopt_value *options;
- ViewOptions *vopts;
- int numoptions;
static const relopt_parse_elt tab[] = {
{"security_barrier", RELOPT_TYPE_BOOL,
offsetof(ViewOptions, security_barrier)},
@@ -1553,20 +1593,10 @@ view_reloptions(Datum reloptions, bool validate)
offsetof(ViewOptions, check_option)}
};
- options = parseRelOptions(reloptions, validate, RELOPT_KIND_VIEW, &numoptions);
-
- /* if none set, we're done */
- if (numoptions == 0)
- return NULL;
-
- vopts = allocateReloptStruct(sizeof(ViewOptions), options, numoptions);
-
- fillRelOptions((void *) vopts, sizeof(ViewOptions), options, numoptions,
- validate, tab, lengthof(tab));
-
- pfree(options);
-
- return (bytea *) vopts;
+ return (bytea *) build_reloptions(reloptions, validate,
+ RELOPT_KIND_VIEW,
+ sizeof(ViewOptions),
+ tab, lengthof(tab));
}
/*
@@ -1628,29 +1658,15 @@ index_reloptions(amoptions_function amoptions, Datum reloptions, bool validate)
bytea *
attribute_reloptions(Datum reloptions, bool validate)
{
- relopt_value *options;
- AttributeOpts *aopts;
- int numoptions;
static const relopt_parse_elt tab[] = {
{"n_distinct", RELOPT_TYPE_REAL, offsetof(AttributeOpts, n_distinct)},
{"n_distinct_inherited", RELOPT_TYPE_REAL, offsetof(AttributeOpts, n_distinct_inherited)}
};
- options = parseRelOptions(reloptions, validate, RELOPT_KIND_ATTRIBUTE,
- &numoptions);
-
- /* if none set, we're done */
- if (numoptions == 0)
- return NULL;
-
- aopts = allocateReloptStruct(sizeof(AttributeOpts), options, numoptions);
-
- fillRelOptions((void *) aopts, sizeof(AttributeOpts), options, numoptions,
- validate, tab, lengthof(tab));
-
- pfree(options);
-
- return (bytea *) aopts;
+ return (bytea *) build_reloptions(reloptions, validate,
+ RELOPT_KIND_ATTRIBUTE,
+ sizeof(AttributeOpts),
+ tab, lengthof(tab));
}
/*
@@ -1659,30 +1675,16 @@ attribute_reloptions(Datum reloptions, bool validate)
bytea *
tablespace_reloptions(Datum reloptions, bool validate)
{
- relopt_value *options;
- TableSpaceOpts *tsopts;
- int numoptions;
static const relopt_parse_elt tab[] = {
{"random_page_cost", RELOPT_TYPE_REAL, offsetof(TableSpaceOpts, random_page_cost)},
{"seq_page_cost", RELOPT_TYPE_REAL, offsetof(TableSpaceOpts, seq_page_cost)},
{"effective_io_concurrency", RELOPT_TYPE_INT, offsetof(TableSpaceOpts, effective_io_concurrency)}
};
- options = parseRelOptions(reloptions, validate, RELOPT_KIND_TABLESPACE,
- &numoptions);
-
- /* if none set, we're done */
- if (numoptions == 0)
- return NULL;
-
- tsopts = allocateReloptStruct(sizeof(TableSpaceOpts), options, numoptions);
-
- fillRelOptions((void *) tsopts, sizeof(TableSpaceOpts), options, numoptions,
- validate, tab, lengthof(tab));
-
- pfree(options);
-
- return (bytea *) tsopts;
+ return (bytea *) build_reloptions(reloptions, validate,
+ RELOPT_KIND_TABLESPACE,
+ sizeof(TableSpaceOpts),
+ tab, lengthof(tab));
}
/*
diff --git a/src/backend/access/gin/ginutil.c b/src/backend/access/gin/ginutil.c
index cf9699ad18..38593554f0 100644
--- a/src/backend/access/gin/ginutil.c
+++ b/src/backend/access/gin/ginutil.c
@@ -602,30 +602,16 @@ ginExtractEntries(GinState *ginstate, OffsetNumber attnum,
bytea *
ginoptions(Datum reloptions, bool validate)
{
- relopt_value *options;
- GinOptions *rdopts;
- int numoptions;
static const relopt_parse_elt tab[] = {
{"fastupdate", RELOPT_TYPE_BOOL, offsetof(GinOptions, useFastUpdate)},
{"gin_pending_list_limit", RELOPT_TYPE_INT, offsetof(GinOptions,
pendingListCleanupSize)}
};
- options = parseRelOptions(reloptions, validate, RELOPT_KIND_GIN,
- &numoptions);
-
- /* if none set, we're done */
- if (numoptions == 0)
- return NULL;
-
- rdopts = allocateReloptStruct(sizeof(GinOptions), options, numoptions);
-
- fillRelOptions((void *) rdopts, sizeof(GinOptions), options, numoptions,
- validate, tab, lengthof(tab));
-
- pfree(options);
-
- return (bytea *) rdopts;
+ return (bytea *) build_reloptions(reloptions, validate,
+ RELOPT_KIND_GIN,
+ sizeof(GinOptions),
+ tab, lengthof(tab));
}
/*
diff --git a/src/backend/access/gist/gistutil.c b/src/backend/access/gist/gistutil.c
index 45804d7a91..a23dec76a2 100644
--- a/src/backend/access/gist/gistutil.c
+++ b/src/backend/access/gist/gistutil.c
@@ -908,29 +908,15 @@ gistPageRecyclable(Page page)
bytea *
gistoptions(Datum reloptions, bool validate)
{
- relopt_value *options;
- GiSTOptions *rdopts;
- int numoptions;
static const relopt_parse_elt tab[] = {
{"fillfactor", RELOPT_TYPE_INT, offsetof(GiSTOptions, fillfactor)},
{"buffering", RELOPT_TYPE_ENUM, offsetof(GiSTOptions, buffering_mode)}
};
- options = parseRelOptions(reloptions, validate, RELOPT_KIND_GIST,
- &numoptions);
-
- /* if none set, we're done */
- if (numoptions == 0)
- return NULL;
-
- rdopts = allocateReloptStruct(sizeof(GiSTOptions), options, numoptions);
-
- fillRelOptions((void *) rdopts, sizeof(GiSTOptions), options, numoptions,
- validate, tab, lengthof(tab));
-
- pfree(options);
-
- return (bytea *) rdopts;
+ return (bytea *) build_reloptions(reloptions, validate,
+ RELOPT_KIND_GIST,
+ sizeof(GiSTOptions),
+ tab, lengthof(tab));
}
/*
diff --git a/src/include/access/reloptions.h b/src/include/access/reloptions.h
index 6bde2093d6..d4f6a168bd 100644
--- a/src/include/access/reloptions.h
+++ b/src/include/access/reloptions.h
@@ -25,7 +25,10 @@
#include "nodes/pg_list.h"
#include "storage/lock.h"
-/* types supported by reloptions */
+/*
+ * Types supported by reloptions. NB: when changing this, see
+ * BuildRelOptions().
+ */
typedef enum relopt_type
{
RELOPT_TYPE_BOOL,
@@ -288,14 +291,12 @@ extern Datum transformRelOptions(Datum oldOptions, List *defList,
extern List *untransformRelOptions(Datum options);
extern bytea *extractRelOptions(HeapTuple tuple, TupleDesc tupdesc,
amoptions_function amoptions);
-extern relopt_value *parseRelOptions(Datum options, bool validate,
- relopt_kind kind, int *numrelopts);
-extern void *allocateReloptStruct(Size base, relopt_value *options,
- int numoptions);
-extern void fillRelOptions(void *rdopts, Size basesize,
- relopt_value *options, int numoptions,
- bool validate,
- const relopt_parse_elt *elems, int nelems);
+
+extern void *build_reloptions(Datum reloptions, bool validate,
+ relopt_kind kind,
+ Size relopt_struct_size,
+ const relopt_parse_elt *relopt_elems,
+ int num_relopt_elems);
extern bytea *default_reloptions(Datum reloptions, bool validate,
relopt_kind kind);
diff --git a/src/test/modules/dummy_index_am/dummy_index_am.c b/src/test/modules/dummy_index_am/dummy_index_am.c
index bc68767f3a..053636e4b4 100644
--- a/src/test/modules/dummy_index_am/dummy_index_am.c
+++ b/src/test/modules/dummy_index_am/dummy_index_am.c
@@ -222,17 +222,10 @@ dicostestimate(PlannerInfo *root, IndexPath *path, double loop_count,
static bytea *
dioptions(Datum reloptions, bool validate)
{
- relopt_value *options;
- int numoptions;
- DummyIndexOptions *rdopts;
-
- /* Parse the user-given reloptions */
- options = parseRelOptions(reloptions, validate, di_relopt_kind, &numoptions);
- rdopts = allocateReloptStruct(sizeof(DummyIndexOptions), options, numoptions);
- fillRelOptions((void *) rdopts, sizeof(DummyIndexOptions), options, numoptions,
- validate, di_relopt_tab, lengthof(di_relopt_tab));
-
- return (bytea *) rdopts;
+ return (bytea *) build_reloptions(reloptions, validate,
+ di_relopt_kind,
+ sizeof(DummyIndexOptions),
+ di_relopt_tab, lengthof(di_relopt_tab));
}
/*
signature.asc
Description: PGP signature
