public inbox for [email protected]
help / color / mirror / Atom feedFrom: Nikolay Shaplov <[email protected]>
To: Alexander Korotkov <[email protected]>
To: [email protected]
Cc: Andrei Lepikhov <[email protected]>
Cc: jian he <[email protected]>
Cc: Robert Haas <[email protected]>
Cc: [email protected]
Cc: Peter Geoghegan <[email protected]>
Cc: Finnerty, Jim <[email protected]>
Cc: Marcos Pegoraro <[email protected]>
Cc: [email protected]
Cc: Tomas Vondra <[email protected]>
Cc: Peter Eisentraut <[email protected]>
Cc: Ranier Vilela <[email protected]>
Cc: Alena Rybakina <[email protected]>
Subject: Re: POC, WIP: OR-clause support for indexes
Date: Wed, 26 Jun 2024 20:41:13 +0300
Message-ID: <2193851.QkHrqEjB74@thinkpad-pgpro> (raw)
In-Reply-To: <9736220.CDJkKcVGEf@thinkpad-pgpro>
References: <[email protected]>
<[email protected]>
<9736220.CDJkKcVGEf@thinkpad-pgpro>
В письме от понедельник, 24 июня 2024 г. 23:51:56 MSK пользователь Nikolay
Shaplov написал:
So, I continue reading the patch.
I see there is `entries` variable in the code, that is the list of
`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
entries here. It is kind of "generic" type. Though naming it
`restric_info_entry` might make te code more readable.
But when we come to an `entry` variable, it is very specific entry, it should
be `OrClauseGroup` entry, not just any entry. So I would suggest to name this
variable `or_clause_group_entry`, or even `or_clause_group` , so when we meet
this variable in the middle of the code, we can get the idea what we are
dealing with, without scrolling code up.
Variable naming is very important thing. It can drastically improve (or ruin)
code readability.
========
Also I see some changes in the tests int this patch. There are should be tests
that check that this new feature works well. And there are test whose behavior
have been just accidentally affected.
I whould suggest to split these tests into two patches, as they should be
reviewed in different ways. Functionality tests should be thoroughly checked
that all stuff we added is properly tested, and affected tests should be checked
that nothing important is not broken. It would be more easy to check if these
are two different patches.
I would also suggest to add to the commit message of affected tests changes
some explanation why this changes does not really breaks anything. This will
do the checking more simple.
To be continued.
--
Nikolay Shaplov aka Nataraj
Fuzzing Engineer at Postgres Professional
Matrix IM: @dhyan:nataraj.su
Attachments:
[application/pgp-signature] signature.asc (488B, ../2193851.QkHrqEjB74@thinkpad-pgpro/2-signature.asc)
download
view thread (13+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: POC, WIP: OR-clause support for indexes
In-Reply-To: <2193851.QkHrqEjB74@thinkpad-pgpro>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox