|From:||Nikolay Shaplov <dhyan(at)nataraj(dot)su>|
|Cc:||Nikita Glukhov <n(dot)gluhov(at)postgrespro(dot)ru>, Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>, PostgreSQL mailing lists <pgsql-hackers(at)postgresql(dot)org>|
|Subject:||Re: [PATCH][PROPOSAL] Add enum releation option type|
|Views:||Raw Message | Whole Thread | Download mbox | Resend email|
В письме от 1 марта 2018 19:11:05 пользователь Nikita Glukhov написал:
> 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
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
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
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,
will do, and then we can assign one enum to another, keeping the traditional
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.
|Next Message||David Fetter||2018-03-07 15:10:44||Re: Implementing SQL ASSERTION|
|Previous Message||Stephen Frost||2018-03-07 15:05:59||Re: public schema default ACL|