Re: POC, WIP: OR-clause support for indexes

From: Nikolay Shaplov <dhyan(at)nataraj(dot)su>
To: Alexander Korotkov <aekorotkov(at)gmail(dot)com>, pgsql-hackers(at)lists(dot)postgresql(dot)org
Cc: Andrei Lepikhov <a(dot)lepikhov(at)postgrespro(dot)ru>, jian he <jian(dot)universality(at)gmail(dot)com>, Robert Haas <robertmhaas(at)gmail(dot)com>, pgsql-hackers(at)postgresql(dot)org, Peter Geoghegan <pg(at)bowt(dot)ie>, "Finnerty, Jim" <jfinnert(at)amazon(dot)com>, Marcos Pegoraro <marcos(at)f10(dot)com(dot)br>, teodor(at)sigaev(dot)ru, Tomas Vondra <tomas(dot)vondra(at)enterprisedb(dot)com>, Peter Eisentraut <peter(at)eisentraut(dot)org>, Ranier Vilela <ranier(dot)vf(at)gmail(dot)com>, Alena Rybakina <a(dot)rybakina(at)postgrespro(dot)ru>
Subject: Re: POC, WIP: OR-clause support for indexes
Date: 2024-06-26 17:41:13
Message-ID: 2193851.QkHrqEjB74@thinkpad-pgpro
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

В письме от понедельник, 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

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Tom Lane 2024-06-26 18:09:28 JIT causes core dump during error recovery
Previous Message Tom Lane 2024-06-26 17:39:22 Re: Reg: Alternate way of hashing database role passwords