From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Nikolay Shaplov <dhyan@nataraj.su>
Cc: pgsql-hackers@postgresql.org
Subject: Re: [PATCH][PROPOSAL] Add enum releation option type
Date: Fri, 9 Feb 2018 18:45:29 -0300
Message-ID: <20180209214529.mbz6iqji7v436xde@alvherre.pgsql> (raw)
In-Reply-To: <43332102.S2V5pIjXRx@x200m>
Nikolay Shaplov wrote:
> I found out, that all relation options of string type in current postgres, are
> actually behaving as "enum" type.
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.
Maybe we need some in-core user to verify the string case still works.
A new module in src/test/modules perhaps? 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.
> [...] But each time this behavior is implemented as validate function
> plus strcmp to compare option value against one of the possible
> values.
>
> I think it is not the best practice. It is better to have enum type
> where it is technically enum, and keep sting type for further usage
> (for example for some kind of index patterns or something like this).
Agreed with the goal, for code simplicity and hopefully reducing
code duplication.
> 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?
> 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.
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?).
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
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: alvherre@alvh.no-ip.org, dhyan@nataraj.su
Subject: Re: [PATCH][PROPOSAL] Add enum releation option type
In-Reply-To: <20180209214529.mbz6iqji7v436xde@alvherre.pgsql>
* 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