Re: [PATCH][PROPOSAL] Add enum releation option type - Mailing list pgsql-hackers

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