Re: [PATCH][PROPOSAL] Add enum releation option type
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 Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: [PATCH][PROPOSAL] Add enum releation option type
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 Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
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] = [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] = [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;
Re: [PATCH][PROPOSAL] Add enum releation option type
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 Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
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] = [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] = [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 @@
Re: [PATCH][PROPOSAL] Add enum releation option type
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 Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: [PATCH][PROPOSAL] Add enum releation option type
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 signature.asc Description: PGP signature
Re: [PATCH][PROPOSAL] Add enum releation option type
В письме от четверг, 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.
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’)
reloptions.c:448:4: warning: excess elements in scalar initializer
reloptions.c:448:4: warning: (near initialization for ‘enumRelOpts[0].members’)
reloptions.c:449:4: warning: braces around scalar initializer
{"off", GIST_OPTION_BUFFERING_OFF },
^
reloptions.c:449:4: warning: (near initialization for ‘enumRelOpts[0].members’)
reloptions.c:449:4: warning: initialization from incompatible pointer type
reloptions.c:449:4: warning: (near initialization for ‘enumRelOpts[0].members’)
reloptions.c:449:4: warning: excess elements in scalar initializer
reloptions.c:449:4: warning: (near initialization for ‘enumRelOpts[0].members’)
reloptions.c:449:4: warning: excess elements in scalar initializer
reloptions.c:449:4: warning: (near initialization for ‘enumRelOpts[0].members’)
reloptions.c:450:4: warning: braces around scalar initializer
{"auto", GIST_OPTION_BUFFERING_AUTO },
^
reloptions.c:450:4: warning: (near initialization for ‘enumRelOpts[0].members’)
reloptions.c:450:4: warning: initialization from incompatible pointer type
reloptions.c:450:4: warning: (near initialization for ‘enumRelOpts[0].members’)
reloptions.c:450:4: warning: excess elements in scalar initializer
reloptions.c:450:4: warning: (near initialization for ‘enumRelOpts[0].members’)
reloptions.c:450:4: warning: excess elements in scalar initializer
reloptions.c:450:4: warning: (near initialization for ‘enumRelOpts[0].members’)
reloptions.c:451:4: warning: braces around scalar initializer
{ NULL }
^
reloptions.c:451:4: warning: (near initialization for ‘enumRelOpts[0].members’)
reloptions.c:451:4: warning: excess elements in scalar initializer
reloptions.c:451:4: warning: (near initialization for ‘enumRelOpts[0].members’)
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
Re: [PATCH][PROPOSAL] Add enum releation option type
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!) */
boolsecurity_barrier;
ViewOpts_CheckOptionValues check_option;
} ViewOptions;
I'm marking this Waiting on Author.
--
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: [PATCH][PROPOSAL] Add enum releation option type
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
> 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
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 Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: [PATCH][PROPOSAL] Add enum releation option type
В письме от понедельник, 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?
Re: [PATCH][PROPOSAL] Add enum releation option type
В письме от понедельник, 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.
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] = [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] = [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();
+ for (elt = optenum->members; elt->string_val; elt++)
+ {
+ appendStringInfo(, "\"%s\"", elt->string_val);
+ if (elt[1].string_val)
+appendStringInfo(, ", ");
+ }
+ 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;
Re: [PATCH][PROPOSAL] Add enum releation option type
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 Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Re: [PATCH][PROPOSAL] Add enum releation option type
On Mon, Mar 25, 2019 at 9:39 PM David Steele 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
Re: Re: [PATCH][PROPOSAL] Add enum releation option type
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 da...@pgmasters.net
Re: [PATCH][PROPOSAL] Add enum releation option type
В письме от среда, 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. :-((
Re: [PATCH][PROPOSAL] Add enum releation option type
В письме от среда, 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...
Re: [PATCH][PROPOSAL] Add enum releation option type
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 signature.asc Description: PGP signature
Re: [PATCH][PROPOSAL] Add enum releation option type
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.
Re: [PATCH][PROPOSAL] Add enum releation option type
В письме от четверг, 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.
Re: [PATCH][PROPOSAL] Add enum releation option type
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 Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
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] = [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] = [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();
+ for (elt = optenum->members; elt->string_val; elt++)
+ {
+ appendStringInfo(, "\"%s\"", elt->string_val);
+ if (elt[1].string_val)
+
Re: [PATCH][PROPOSAL] Add enum releation option type
В письме от пятница, 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.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] = [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] = [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();
+ for(el_def = optenum->enum_def; el_def->text_value;
+ el_def++)
+
Re: [PATCH][PROPOSAL] Add enum releation option type
В письме от 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.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] = [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] = [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();
+ for(el_def = optenum->enum_def; el_def->text_value;
+ el_def++)
+ {
+ appendStringInfo(,"\"%s\"",el_def->text_value);
+ if (el_def[1].text_value)
+appendStringInfo(,", ");
+ }
+ 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:
+
Re: [PATCH][PROPOSAL] Add enum releation option type
В письме от 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. See > > https://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. signature.asc Description: This is a digitally signed message part.
Re: [PATCH][PROPOSAL] Add enum releation option type
Robert Haas writes: > On Wed, Mar 7, 2018 at 10:23 AM Nikolay Shaplov 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
Re: [PATCH][PROPOSAL] Add enum releation option type
On Wed, Mar 7, 2018 at 10:23 AM Nikolay Shaplov 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
Re: [PATCH][PROPOSAL] Add enum releation option type
В письме от 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. signature.asc Description: This is a digitally signed message part.
Re: [PATCH][PROPOSAL] Add enum releation option type
В письме от 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.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] = [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] = [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 =
Re: [PATCH][PROPOSAL] Add enum releation option type
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
Re: [PATCH][PROPOSAL] Add enum releation option type
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 Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: [PATCH][PROPOSAL] Add enum releation option type
В письме от 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. signature.asc Description: This is a digitally signed message part.
Re: [PATCH][PROPOSAL] Add enum releation option type
В письме от 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.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] = [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] = [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;
+
Re: [PATCH][PROPOSAL] Add enum releation option type
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 Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: [PATCH][PROPOSAL] Add enum releation option type
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
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] = [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] = [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();
+
+ for (elem = opt_enum->allowed_values; elem->name; elem++)
+ {
+ if (elem->name == RELOPT_ENUM_DEFAULT)
+ continue; /* should be the first in the list */
+
+ appendStringInfo(, "\"%s\"", elem->name);
+
+ if (elem[1].name)
+ appendStringInfo(, 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;
+
Re: [PATCH][PROPOSAL] Add enum releation option type
В письме от 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 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. 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. signature.asc Description: This is a digitally signed message part.
Re: [PATCH][PROPOSAL] Add enum releation option type
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 Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: [PATCH][PROPOSAL] Add enum releation option type
В письме от 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. signature.asc Description: This is a digitally signed message part.
Re: [PATCH][PROPOSAL] Add enum releation option type
В письме от 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.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] = [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] = [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();
+ i = 0;
+ while (opt_enum->allowed_values[i])
+ {
+ appendStringInfo(,"\"%s\"",
+ opt_enum->allowed_values[i]);
+ if (opt_enum->allowed_values[i + 1])
+ {
+ if (opt_enum->allowed_values[i + 2])
+appendStringInfo(,", ");
+ else
+appendStringInfo(," 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,
Re: [PATCH][PROPOSAL] Add enum releation option type
В письме от 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. signature.asc Description: This is a digitally signed message part.
Re: [PATCH][PROPOSAL] Add enum releation option type
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 Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
