From: Nikolay Shaplov <dhyan@nataraj.su>
To: Alvaro Herrera <alvherre@alvh.no-ip.org>
Cc: pgsql-hackers@postgresql.org
Subject: Re: [PATCH][PROPOSAL] Add enum releation option type
Date: Sun, 11 Feb 2018 16:51:49 +0300
Message-ID: <2629977.Ep8BxGGJ5p@x200m> (raw)
In-Reply-To: <20180209214529.mbz6iqji7v436xde@alvherre.pgsql>
References: <20180209214529.mbz6iqji7v436xde@alvherre.pgsql>
В письме от 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.
Attachments:
[application/pgp-signature] signature.asc (472B, ../2629977.Ep8BxGGJ5p@x200m/2-signature.asc)
download
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: dhyan@nataraj.su, alvherre@alvh.no-ip.org
Subject: Re: [PATCH][PROPOSAL] Add enum releation option type
In-Reply-To: <2629977.Ep8BxGGJ5p@x200m>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox