Thread: [PATCH][PROPOSAL] Add enum releation option type

Hi!
While working with my big reloption patch, 
https://www.postgresql.org/message-id/flat/2146419.veIEZdk4E4@x200m#2146419.veIEZdk4E4@x200m
I found out, that all relation options of string type in current postgres, are 
actually behaving as "enum" type. 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).

Now strcmp in this case does not really slow down postgres, as both string 
options are checked when index is created. One check there will not really 
slow down. But if in future somebody would want to have such option checked on 
regular basis, it is better to have it as enum type: once "strcmp" it while 
parsing, and then just "==" when one need to check option value in runtime.

The patch is in attachment. I hope the code is quite clear.

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.

2. Also about the string with the list of acceptable values: the code that 
creates this string is inside parse_one_reloption function now. It is a bit 
too complex. May be there is already exists some helper function that creates 
a string "XXX", "YYY", "ZZZ" from the list of XXX YYY ZZZ values, I do not 
know of? Or may be it is reason to create such a function. If so where to 
create it? Inside "reloptions.c"? Or it can be reused and I'd better put it 
somewhere else?

I hope there would be not further difficulties with this patch. Hope it can be 
committed in proper time.

-- 
Do code for fun.

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.

В письме от пятница, 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.

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

В письме от четверг, 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.

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.

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

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

В письме от среда, 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. :-((

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
david@pgmasters.net

On Mon, Mar 25, 2019 at 9:39 PM David Steele <david@pgmasters.net> 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

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

В письме от понедельник, 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.

В письме от понедельник, 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?

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

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!) */
    bool        security_barrier;
    ViewOpts_CheckOptionValues check_option;
} ViewOptions;

I'm marking this Waiting on Author.

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

В письме от четверг, 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.

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

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

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

I decided I didn't dislike that patch all that much anyway, so I cleaned
it up a little bit and here's v8.

The add_enum_reloption stuff is still broken.  Please fix it and
resubmit.  I'm marking this Waiting on Author now.

Thanks,

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

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