From: Nikolay Shaplov <dhyan@nataraj.su>
To: Alvaro Herrera <alvherre@alvh.no-ip.org>
Cc: pgsql-hackers@lists.postgresql.org, pgsql-hackers@postgresql.org
Subject: Re: [PATCH][PROPOSAL] Add enum releation option type
Date: Thu, 15 Feb 2018 19:29:55 +0300
Message-ID: <7548428.vkkzRLI03f@x200m>
User-Agent: KMail/4.14.1 (Linux/3.16.0-4-amd64; KDE/4.14.2; x86_64; ; )
In-Reply-To: <20180215155327.52suiqdcf3f4z3ir@alvherre.pgsql>
References: <20180215155327.52suiqdcf3f4z3ir@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: multipart/signed; boundary="nextPart1844521.Jb0TgimK9Y";
 micalg="pgp-sha256"; protocol="application/pgp-signature"
Precedence: bulk


--nextPart1844521.Jb0TgimK9Y
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 15 =D1=84=D0=B5=
=D0=B2=D1=80=D0=B0=D0=BB=D1=8F 2018 12:53:27 =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:

> > > Maybe we need some in-core user to verify the string case still w=
orks.
> > > A new module in src/test/modules perhaps?
> >=20
> > I've looked attentively in src/test/modules... To properly test all=

> > reloptions hooks for modules wee need to create an extension with s=
ome
> > dummy index in it. This way we can properly test everything. Creati=
ng
> > 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
>=20
> Do we really need a dummy index?  I was thinking in something that ju=
st
> 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 t=
o it=20
using add_string_reloption, and then call fillRelOptions with some dumm=
y data=20
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=20=

altogether, not just some part of it.

Moreover when my whole patch is commited these all should be rewritten,=
 as I=20
changed some options internals for some good reasons.=20

So I would prefer to keep it untested while we are done with reloptions=
, and=20
then test it in a good way, with creating dummy index and so on. I thin=
k it=20
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 repr=
esents
> > > everything about each value (string name and C symbol.  Maybe the=

> > > numerical value too if that's needed, but is it?  I suppose all c=
ode
> > > should use the C symbol only, so why do we care about the numeric=
al
> > > value?).
> >=20
> > One more reason to keep numeric value, that came to my mind, is tha=
t it
> > seems to be logical to keep enum value in postgres internals repres=
ented
> > as C-enum. And C-enum is actually an int value (can be easily caste=
d both
> > ways). It is not strictly necessary, but it seems to be a bit logic=
al...
>=20
> 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 f=
ine
> 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 go=
od to=20
keep them, they can be removed now...
(I would prefer to keep them for further debugging, but if it is not ri=
ght, I=20
can easily remove them, I do not need them right now)

=2D-=20
Do code for fun.
--nextPart1844521.Jb0TgimK9Y
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

iQEcBAABCAAGBQJahbWDAAoJEKc13HowK+H4IPcIAI8qhAgStFjwpN+y5SlQosAy
JY+qKbtW9pNRMSi0oXXctKss2pXorUHxKrGHJcupsf14qFZiwJLeXP2xQoe2ODE9
X+gNQ+UFYdMfD0RwwylVq6OymxGaOIvYEnZ7gxXIiOkoasQiOS0MiEcvXyD89+av
6TMzHBj2pMSWh8kfBruILY3a6GQ5QWMSZpLVIQ0GPWvYJVSiz14V1ASj3N2mh5VP
SpEveY2vILDJVzztbk2ZqsCRacAGgO67qmCsYAArM615g9MUVgwIIYxcXCf9hPqg
l60BNxszwfBwLV6BbbzFlrui1xOd+ZLDa/Breqje03ETK1aJO866BrimnUS0fTw=
=Nvna
-----END PGP SIGNATURE-----

--nextPart1844521.Jb0TgimK9Y--