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

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