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: Mon, 24 Jun 2024 23:51:56 +0300
Message-ID: <9736220.CDJkKcVGEf@thinkpad-pgpro>
Organization: Postgres Professional
In-Reply-To: <196b44d5-f4e5-41a0-9a8f-9fbf26a13760@postgrespro.ru>
References:
 <567ED6CA.2040504@sigaev.ru>
 <CAPpHfdv087igFrMiU3yghtG9SmmYoWk0jP8WFJ9yR301CHit+w@mail.gmail.com>
 <196b44d5-f4e5-41a0-9a8f-9fbf26a13760@postgrespro.ru>
MIME-Version: 1.0
Content-Type: multipart/signed; boundary="nextPart3668543.5fSG56mABF";
 micalg="pgp-sha512"; protocol="application/pgp-signature"
Archived-At: <https://www.postgresql.org/message-id/9736220.CDJkKcVGEf%40thinkpad-pgpro>
Precedence: bulk

--nextPart3668543.5fSG56mABF
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: Mon, 24 Jun 2024 23:51:56 +0300
Message-ID: <9736220.CDJkKcVGEf@thinkpad-pgpro>
Organization: Postgres Professional
In-Reply-To: <196b44d5-f4e5-41a0-9a8f-9fbf26a13760@postgrespro.ru>
MIME-Version: 1.0

Hi!

Let me join the review process.

I am no expert in execution plans, so there would not be much help in doing=
=20
even better optimization. But I can read the code, as a person who is not=20
familiar=20
with this area and help making it clear even to a person like me.

So, I am reading v25-0001-Transform-OR-clauses-to-ANY-expression.patch that=
=20
have been posted some time ago, and especially transform_or_to_any function.

> @@ -38,7 +45,6 @@
>  int			from_collapse_limit;
>  int			join_collapse_limit;
>
> -=20
>  /*
>   * deconstruct_jointree requires multiple passes over the join tree, =20
because we=20
>   * need to finish computing JoinDomains before we start  distributing qu=
als.

Do not think that removing empty line should be part of the patch

> +			/*
> +			 * If the const node's (right side of operator=20
expression) type
> +			 * don't have =C3=A2=E2=82=AC=C5=93true=C3=A2=E2=82=AC=C2=9D array ty=
pe, then we cannnot=20
do the
> +			 * transformation. We simply concatenate the=20
expression node.
> +			 */
Guess using unicode double quotes is not the best idea here...=20
  =20
Now to the first part of  `transform_or_to_any`:

> +	List	   *entries =3D NIL;
I guess the idea of entries should be explained from the start. What kind o=
f=20
entries are accomulated there... I see they are added there all around the=
=20
code, but what is the purpose of that is not quite clear when you read it.

At the first part of `transform_or_to_any` function, you costanly repeat tw=
o=20
lines, like a mantra:

> +			entries =3D lappend(entries, rinfo);
> +			continue;

"If something is wrong -- do that mantra"

=46rom my perspective, if you have to repeat same code again and again, the=
n=20
most probably you have some issues with architecture of the code. If you=20
repeat some code again and again, you need to try to rewrite the code, the=
=20
way, that part is repeated only once.

In that case I would try to move the most of the first loop of=20
`transform_or_to_any`  to a separate function (let's say its name is=20
prepare_single_or), that will do all the checks, if this or is good for us;=
=20
return NULL if it does not suits our purposes (and in this case we do "entr=
ies=20
=3D lappend(entries, rinfo); continue" in the main code, but only once) or=
=20
return pointer to some useful data if this or clause is good for our purpos=
es.=20

This, I guess will make that part more clear and easy to read, without=20
repeating same "lappend mantra" again and again.

Will continue digging into the code tomorrow.

P.S. Sorry for sending partly finished email. Pressed Ctrl+Enter=20
accidentally... With no way to make it back :-(((=20
--nextPart3668543.5fSG56mABF
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+sk3ebqQKlezKOi8PMbfuIHAGpgFAmZ53GwACgkQPMbfuIHA
Gpi2nggAi2w9Ps+7ZqDBZJMehPSd18kJQpIvDX8nc4fQErXjclaJVKrf5mG8oglQ
HosZDZ1O0vjC/8DR6NtNPjx2BlHaZ1P+ONQzyopBJPi31ONgVUIJESX4mO4k95k8
ZGIvATARCIliQ26EebgnhKxlv9e3NwqLLF0O09AEDIOBnGKk+MlxC5mbwneb7leX
PsAwJRXkq2xw1a35gu9yRWDRk+d750Efy4xRZ/+RYdhFDWSXJIjr4qw6E391Fs1d
kK0HxCKkpYQj8dvYstIKtiB2QEPufqZGampWMyZDOi00ZiME5FJ/V/U9lN9vPbR0
TKTcoYFRyIxa9wqWwLH6EYIDSrCNEg==
=+Dvw
-----END PGP SIGNATURE-----

--nextPart3668543.5fSG56mABF--