From: Nikolay Shaplov <dhyan@nataraj.su>
To: pgsql-hackers@lists.postgresql.org
Cc: Nikita Glukhov <n.gluhov@postgrespro.ru>,
 PostgreSQL mailing lists <pgsql-hackers@postgresql.org>, obartunov@gmail.com
Subject: Re: [PATCH] Opclass parameters
Date: Tue, 20 Nov 2018 14:15:45 +0300
Message-ID: <12473406.MrkCg5crFA@x200m>
User-Agent: KMail/4.14.1 (Linux/3.16.0-4-amd64; KDE/4.14.2; x86_64; ; )
In-Reply-To: <71c6164e-59ad-e0c4-76c5-b1dfdd68eee3@postgrespro.ru>
References: <d22c3a18-31c7-1879-fc11-4c1ce2f5e5af@postgrespro.ru>
 <6382cb29-83d8-90f2-1dbb-b4ac606cbf1f@postgrespro.ru>
 <71c6164e-59ad-e0c4-76c5-b1dfdd68eee3@postgrespro.ru>
MIME-Version: 1.0
Content-Type: multipart/signed; boundary="nextPart3611555.vxChgF5EnQ";
 micalg="pgp-sha256"; protocol="application/pgp-signature"
Precedence: bulk


--nextPart3611555.vxChgF5EnQ
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 =D0=BD=D0=BE=
=D1=8F=D0=B1=D1=80=D1=8F 2018 18:26:43 =D0=BF=D0=BE=D0=BB=D1=8C=D0=B7=D0=
=BE=D0=B2=D0=B0=D1=82=D0=B5=D0=BB=D1=8C Nikita Glukhov =D0=BD=D0=B0=D0=BF=
=D0=B8=D1=81=D0=B0=D0=BB:

> Attached 2nd version of the patches. Nothing has changed since March,=

> this is just a rebased version.
>=20
> CREATE INDEX syntax and parameters storage method still need discussi=
on.
I've played around a bit with you patch and come to some conclusions, I=
'd like=20
to share. They are almost same as those before, but now there are more=20=

details.

Again some issues about storing opclass options in pg_inedx:

1. Having both indoption and indoptions column in pg_index will make so=
meone's=20
brain explode for sure. If not, it will bring troubles when people star=
t=20
confusing them.

2. Now I found out how do you store option values for each opclass: tex=
t[] of=20
indoptions in pg_index is not the same as text[] in=20
reloptions in pg_catalog (and it brings more confusion). In reloption e=
ach=20
member of the array is a single option.=20

reloptions          | {fillfactor=3D90,autovacuum_enabled=3Dfalse}

In indoptions, is a whole string of options for one of the indexed attr=
ibutes,=20
each array item has all options for one indexed attribute. And this str=
ing=20
needs furthermore parsing, that differs from reloption parsing.

indoptions     | {"{numranges=3D150}","{numranges=3D160}"}


This brings us to the following issues:

2a. pg_index stores properties of index in general. Properties of each =
indexed=20
attributes is stored in pg_attribute table. If we follow this philosoph=
y
it is wrong to add any kind of per-attribute array values into pg_index=
. These=20
values should be added to pg_attribute one per each pg_attribute entry.=


2b. Since you've chosen method of storage that differs from one that is=
 used=20
in reloptions, that will lead to two verstions of code that processes t=
he=20
attributes. And from now on, if we accept this, we should support both =
of them=20
and keep them in sync. (I see that you tried to reuse as much code as=20=

possible, but still you added some more that duplicate current reloptio=
ns=20
functionality.)

I know that relotions code is not really suitable for reusing. This was=
 the=20
reason why I started solving oplcass option task with rewriting relopti=
ons=20
code,to make it 100% reusable for any kind of options. So I would offer=
 you=20
again to join me as a reviewer of that code. This will make opclass cod=
e more=20
simple and more sensible, if my option code is used...=20

3. Speaking of sensible code

Datum
g_int_options(PG_FUNCTION_ARGS)
{
   Datum       raw_options =3D PG_GETARG_DATUM(0);
   bool        validate =3D PG_GETARG_BOOL(1);
   relopt_int  siglen =3D
       { {"numranges", "number of ranges for compression", 0, 0, 9,=20
RELOPT_TYPE_INT },
           G_INT_NUMRANGES_DEFAULT, 1, G_INT_NUMRANGES_MAX };
   relopt_gen *optgen[] =3D { &siglen.gen };
   int         offsets[] =3D { offsetof(IntArrayOptions, num_ranges) };=

   IntArrayOptions *options =3D
       parseAndFillLocalRelOptions(raw_options, optgen, offsets, 1,
                                   sizeof(IntArrayOptions), validate);

   PG_RETURN_POINTER(options);
}

It seems to be not a very nice hack.
What would you do if you would like to have both int, real and boolean =
options=20
for one opclass? I do not know how to implement it using this code.
We have only int opclass options for now, but more will come and we sho=
uld be=20
ready for it.

4. Now getting back to not adding opclass options wherever we can, just=
=20
because we can:

4a. For inrarray there were no opclass options tests added. I am sure t=
here=20
should be one, at least just to make sure it still does not segfault wh=
en you=20
try to set one. And in some cases more tests can be needed. To add and =
review=20
them one should be familiar with this opclass internals. So it is good =
when=20
different people do it for different opclasses

4b. When you add opclass options instead of hardcoded values, it comes =
to=20
setting minimum and maximum value. Why do you choose 1000 as maximum=20=

for num_ranges in gist_int_ops in intarray? Why 1 as minimum? All these=
=20
decisions needs careful considerations and can't be done for bunch of=20=

opclasses just in one review.=20

4c. Patch usually take a long path from prototype to final commit. Do y=
ou=20
really want to update all these opclasses code each time when some chan=
ges=20
in the main opclass option code is made? ;-)

So I would suggest to work only with intarray and add other opclasses l=
ater.

5. You've been asking about SQL grammar

> CREATE INDEX idx ON tab USING am (
>    {expr {opclass | DEFAULT} ({name=3Dvalue} [,...])} [,...]
> );

As for me I do not really care about it. For me all the solutions is=20=

acceptable. But looking at is i came to one notion:

I've never seen before DEFAULT keyword to be used in this way. There is=
 logic=20
in such usage, but I did not remember any practical usage case.
If there are such usages (I can easily missed it) or if it is somehow=20=

recommended in SQL standard -- let it be. But if none above, I would su=
ggest=20
to use WITH keyword instead. As it is already used for reloptions. As f=
ar as I=20
remember in my prototype I used "WITH OPTIONS" but did if just because =
did not=20
find my way through yac with single "WITH". So ideal way for me would b=
e=20

create index ON test USING GIST (i WITH (sig_len_int=3D22));=20

But as I said it is not thing of importance for me. Just an observation=
.

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

iQEcBAABCAAGBQJb8+ziAAoJEKc13HowK+H408EH/jgklJhCtFC6f0SY3PvP/Ppd
RgPN5OCqnbUtkQHOLUp+j1Ho0An2qgs6LWYLmbeSuPxkCRsMAfMIWFzXNM/RsiJT
ithkTRt9BucgytqUDoSJVmg3xqpsQ9bR1RRnZlKUPuu9kEJWvI4/qoSMfH+Valag
R0OndMHnYM2XYvTy7hC2QttgbRFrm/HQvDu1BvTJ3kkSWVSI57HDJjVbV9QXrfxX
ctvteiZEK5h69xw3rEfChI/jZuKvphJCaakWaOLCiUc4xN/msDwsGKVhriVOiTaq
wR6KUTn5OomfbVmuTyq1Rlrya2xvMfRcJS1JwhOxD6VSvjbHrnshxA/aVa1MPiI=
=UlJU
-----END PGP SIGNATURE-----

--nextPart3611555.vxChgF5EnQ--