From: Nikolay Shaplov <dhyan@nataraj.su>
To: Alexander Korotkov <aekorotkov@gmail.com>,
 pgsql-hackers@lists.postgresql.org
Cc: Andrei Lepikhov <a.lepikhov@postgrespro.ru>,
 jian he <jian.universality@gmail.com>, Robert Haas <robertmhaas@gmail.com>,
 pgsql-hackers@postgresql.org, Peter Geoghegan <pg@bowt.ie>,
 "Finnerty, Jim" <jfinnert@amazon.com>, Marcos Pegoraro <marcos@f10.com.br>,
 teodor@sigaev.ru, Tomas Vondra <tomas.vondra@enterprisedb.com>,
 Peter Eisentraut <peter@eisentraut.org>, Ranier Vilela <ranier.vf@gmail.com>,
 Alena Rybakina <a.rybakina@postgrespro.ru>
Subject: Re: POC, WIP: OR-clause support for indexes
Date: Wed, 26 Jun 2024 20:41:13 +0300
Message-ID: <2193851.QkHrqEjB74@thinkpad-pgpro>
Organization: Postgres Professional
In-Reply-To: <9736220.CDJkKcVGEf@thinkpad-pgpro>
References:
 <567ED6CA.2040504@sigaev.ru>
 <196b44d5-f4e5-41a0-9a8f-9fbf26a13760@postgrespro.ru>
 <9736220.CDJkKcVGEf@thinkpad-pgpro>
MIME-Version: 1.0
Content-Type: multipart/signed; boundary="nextPart4991650.8F6SAcFxjW";
 micalg="pgp-sha512"; protocol="application/pgp-signature"
Archived-At: <https://www.postgresql.org/message-id/2193851.QkHrqEjB74%40thinkpad-pgpro>
Precedence: bulk

--nextPart4991650.8F6SAcFxjW
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset="UTF-8"; protected-headers="v1"
From: Nikolay Shaplov <dhyan@nataraj.su>
Subject: Re: POC, WIP: OR-clause support for indexes
Date: Wed, 26 Jun 2024 20:41:13 +0300
Message-ID: <2193851.QkHrqEjB74@thinkpad-pgpro>
Organization: Postgres Professional
In-Reply-To: <9736220.CDJkKcVGEf@thinkpad-pgpro>
MIME-Version: 1.0

=D0=92 =D0=BF=D0=B8=D1=81=D1=8C=D0=BC=D0=B5 =D0=BE=D1=82 =D0=BF=D0=BE=D0=BD=
=D0=B5=D0=B4=D0=B5=D0=BB=D1=8C=D0=BD=D0=B8=D0=BA, 24 =D0=B8=D1=8E=D0=BD=D1=
=8F 2024 =D0=B3. 23:51:56 MSK =D0=BF=D0=BE=D0=BB=D1=8C=D0=B7=D0=BE=D0=B2=D0=
=B0=D1=82=D0=B5=D0=BB=D1=8C Nikolay=20
Shaplov =D0=BD=D0=B0=D0=BF=D0=B8=D1=81=D0=B0=D0=BB:

So,  I continue reading the patch.

I see there is `entries` variable in the code, that is the list of=20
`RestrictInfo` objects and `entry` that is `OrClauseGroup` object.

This naming is quite misguiding (at least for me).

 `entries` variable name can be used, as we deal only with RestrictInfo=20
entries here. It is kind of "generic" type. Though naming it=20
`restric_info_entry` might make te code more readable.

But when we come to an `entry` variable, it is very specific entry, it shou=
ld=20
be `OrClauseGroup` entry, not just any entry. So I would suggest to name th=
is=20
variable `or_clause_group_entry`, or even `or_clause_group` , so when we me=
et=20
this variable in the middle of the code, we can get the idea what we are=20
dealing with, without scrolling code up.

Variable naming is very important thing. It can drastically improve (or rui=
n)=20
code readability.

=3D=3D=3D=3D=3D=3D=3D=3D

Also I see some changes in the tests int this patch. There are should be te=
sts=20
that check that this new feature works well. And there are test whose behav=
ior=20
have been just accidentally affected.

I whould suggest to split these tests into two patches, as they should be=20
reviewed in different ways. Functionality tests should be thoroughly checke=
d=20
that all stuff we added is properly tested, and affected tests should be ch=
ecked=20
that nothing important is not broken. It would be more easy to check if the=
se=20
are two different patches.

I would also suggest to add to the commit message of affected tests changes=
=20
some explanation why this changes does not really breaks anything. This wil=
l=20
do the checking more simple.

To be continued.

=2D-=20
Nikolay Shaplov aka Nataraj
=46uzzing Engineer at Postgres Professional
Matrix IM: @dhyan:nataraj.su

--nextPart4991650.8F6SAcFxjW
Content-Type: application/pgp-signature; name="signature.asc"
Content-Description: This is a digitally signed message part.
Content-Transfer-Encoding: 7Bit

-----BEGIN PGP SIGNATURE-----

iQEzBAABCgAdFiEE+sk3ebqQKlezKOi8PMbfuIHAGpgFAmZ8UrkACgkQPMbfuIHA
GpjLmQf/Uhzh7eFZTY2IsYCe3d4ZuZjcSXGSXhbuUcDu0dvFpdgitWwb6OLa8ruW
6Dej62nK0Fq5cxJctsAyi5G3EtnPqLxuvkYxPKD8xxtYX9Cc4naYdSy/9u/AT8vQ
iYGc7f9j+ga6yl39mFX+WaRRmMNMePMHD7JihYI2yHe3dqrogTZSXIVlMBEmDDtj
ITN6Kz9qnll6b5ShZENDdHU3Wg2zV1SVvTaYnIWAMbLmRfKAeswb6225RvW3uFt4
oV8UEH8l3Ljn79wtQ8pQ0webLqhKB/Pxa99h68I2oaGdvSPJBQgQeIESJyq8uUEm
Ke1yO9nemP1yDgtCRmv97pjzGejybA==
=MzEt
-----END PGP SIGNATURE-----

--nextPart4991650.8F6SAcFxjW--