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>
User-Agent: KMail/4.14.1 (Linux/3.16.0-4-amd64; KDE/4.14.2; x86_64; ; )
In-Reply-To: <20180209214529.mbz6iqji7v436xde@alvherre.pgsql>
References: <20180209214529.mbz6iqji7v436xde@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: multipart/signed; boundary="nextPart5008271.ZZEcIP3EZh";
 micalg="pgp-sha256"; protocol="application/pgp-signature"
Precedence: bulk


--nextPart5008271.ZZEcIP3EZh
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset="utf-8"

=D0=92 =D0=BF=D0=B8=D1=81=D1=8C=D0=BC=D0=B5 =D0=BE=D1=82 9 =D1=84=D0=B5=
=D0=B2=D1=80=D0=B0=D0=BB=D1=8F 2018 18:45:29 =D0=BF=D0=BE=D0=BB=D1=8C=D0=
=B7=D0=BE=D0=B2=D0=B0=D1=82=D0=B5=D0=BB=D1=8C Alvaro Herrera =D0=BD=D0=B0=
=D0=BF=D0=B8=D1=81=D0=B0=D0=BB:

> If this patch gets in, I wonder if there are any external modules tha=
t
> 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 string=
s.
I did not remove string option support from the code. It might be neede=
d=20
later.

> Maybe we need some in-core user to verify the string case still works=
.
> A new module in src/test/modules perhaps? =20
This sound as a good idea. I am too do not feel really comfortable with=
 that=20
this string options possibility exists, but is not tested. I'll have a =
look=20
there, it it will not require a great job, I will add tests for string =
options=20
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 need=
ed=20
later.

> > Possible flaws:
> >=20
> > 1. I've changed error message from 'Valid values are "XXX", "YYY" a=
nd
> > "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 "a=
nd" to
> > the string.
> I don't think we care about this, but is this still the case if you u=
se
> a stringinfo?
May be not. I'll try to do it better.

> > 2. Also about the string with the list of acceptable values: the co=
de that
> > creates this string is inside parse_one_reloption function now.
>=20
> I think you could save most of that mess by using appendStringInfo an=
d
> friends.
Thanks. I will rewrite this part using these functions. That was really=
=20
helpful.

> I don't much like the way you've represented the list of possible val=
ues
> for each enum.  I think it'd be better to have a struct that represen=
ts
> everything about each value (string name and C symbol.
I actually do not like it this way too. I would prefer one structure, n=
ot two=20
lists. But I did not find way how to do it in one struct. How to gave h=
ave=20
string value and C symbol in one structure, without defining C symbols=20=

elsewhere. Otherwise it will be two lists again.
I do not have a lot of hardcore C development experience, so I can miss=
=20
something. Can you provide an example of the structure you are talking =
about?=20

> 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 wr=
ite=20
something like

elog(WARNING,"buffering_mode=3D%i",opt.buffering_mode);

to check that everything works as expected. Such cases is the only reas=
on to=20
keep numerical value.


=2D-=20
Do code for fun.
--nextPart5008271.ZZEcIP3EZh
Content-Type: application/pgp-signature; name="signature.asc"
Content-Description: This is a digitally signed message part.
Content-Transfer-Encoding: 7Bit

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQEcBAABCAAGBQJagEp1AAoJEKc13HowK+H4McMH/3jWCvxkpYSnFcb2TyLJpDY4
lbV7K7/zf5P7iL93UUU0gqCkGZM4BW4NqH6GGiCU1SEsF4R3r4NVxiVOacxrOiuI
xv2cIyrYpJfhg6kS5pKbHlO0Nsv4BKDZEMXyrT1DODUNisCyhZZbB5nMCw01iQsA
5Oe0xUElMMBAVUeB763x3QbmKXBoGGU3ltcD59yDTZXUElXIuwvLvcN+oLppk9ej
WbPJfIwuISfYKCXzphQahmxsP/cW9NCdp/68NSta6393bNgl85fWR1GeF5CIaIio
bh/v6YZ9xwL7pvuWAMn+DwKIlh4A0yUmOCK3jQXzz8ksBOOlL5qsiJr2bYL3+bc=
=4aDt
-----END PGP SIGNATURE-----

--nextPart5008271.ZZEcIP3EZh--