[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 Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

В письме от 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.

В письме от 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.

В письме от 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.

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 Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

В письме от 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.

Nikolay Shaplov wrote:

В письме от 15 февраля 2018 12:53:27 пользователь Alvaro Herrera написал:

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.

No, I don't, but let's make sure that there really is a test module
closer to the end of the patch series.

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)

I'd like to give this deeper review to have a better opinion on this.

--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

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 Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

В письме от 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.

В письме от 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.

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 Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

В письме от 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.

В письме от 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.

On Wed, Mar 7, 2018 at 10:23 AM Nikolay Shaplov <dhyan@nataraj.su> 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

Robert Haas <robertmhaas@gmail.com> writes:

On Wed, Mar 7, 2018 at 10:23 AM Nikolay Shaplov <dhyan@nataraj.su> 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

В письме от 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.

В письме от 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.