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

From: Nikolay Shaplov <dhyan(at)nataraj(dot)su>
To: pgsql-hackers(at)lists(dot)postgresql(dot)org
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
Date: 2018-03-07 15:08:49
Message-ID: 3831139.TYuODbpbJx@x200m
Views: Raw Message | Whole Thread | Download mbox
Thread:
Lists: 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.

Attachment Content-Type Size
enum-reloptions4.diff text/x-patch 15.2 KB

In response to

Browse pgsql-hackers by date

  From Date Subject
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