Message-ID: <7aed2a84-41a5-450f-9630-514338172017@postgrespro.ru>
Date: Fri, 23 Aug 2024 17:06:04 +0300
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: POC, WIP: OR-clause support for indexes
To: Alexander Korotkov <aekorotkov@gmail.com>,
 Andrei Lepikhov <lepihov@gmail.com>
Cc: Nikolay Shaplov <dhyan@nataraj.su>, pgsql-hackers@lists.postgresql.org,
 jian he <jian.universality@gmail.com>, Robert Haas <robertmhaas@gmail.com>,
 pgsql-hackers@postgresql.org, Peter Geoghegan <pg@bowt.ie>,
 Marcos Pegoraro <marcos@f10.com.br>, teodor@sigaev.ru,
 Peter Eisentraut <peter@eisentraut.org>, Ranier Vilela <ranier.vf@gmail.com>
References: <567ED6CA.2040504@sigaev.ru>
 <CAPpHfdsrtAB=0B-qYuQMPgyyy=-CycFJfzTWO_30nx27vXc3dA@mail.gmail.com>
 <759292d5-cb51-4b12-89fa-576c1d9b374d@postgrespro.ru>
 <CAPpHfdvUEjkuFNUbg7GuLkXotwvpDgBYz4S8YcE=LRf9aBQh-Q@mail.gmail.com>
 <531fc0ab-371e-4235-97e3-dd2d077b6995@postgrespro.ru>
 <CAPpHfdu5iQOjF93vGbjidsQkhHvY2NSm29duENYH_cbhC6x+Mg@mail.gmail.com>
 <CAPpHfdv9GrXfCOdPsMszBrORig+nW2+JBuMD-np_keVuKJ7bdQ@mail.gmail.com>
 <cf2c669f-93ec-4e35-a521-cecac67def73@postgrespro.ru>
 <CAPpHfduTM2W3kmiN1=5fvgcu6yA5=ghq82OkvLvZC8Mh9PpEuA@mail.gmail.com>
 <c3c490e2-1f04-420b-bd5c-f4840d6a7198@postgrespro.ru>
 <e657e536-f10b-4131-a3f2-667c73c86588@postgrespro.ru>
 <CAPpHfduFJHTNGCTFWt4GFs-E6Ts8xS5mCsKOYKBsZAmoOr8ZSA@mail.gmail.com>
 <65bfdcb6-0f0c-4d4c-a721-cb38a0ba91c7@postgrespro.ru>
 <CAPpHfdtR3cBKCSj_7dd7EDF_pa-rUUQBx3-guK6kZZ=_hSeMfg@mail.gmail.com>
 <670d1dc2-a280-4cf6-bc29-e2eafb107081@gmail.com>
 <CAPpHfdvF864n=Lzmjd2XBi9TwboZvrhRtLSt2hCP+JVUv6XKzg@mail.gmail.com>
Content-Language: en-US
From: Alena Rybakina <a.rybakina@postgrespro.ru>
In-Reply-To: <CAPpHfdvF864n=Lzmjd2XBi9TwboZvrhRtLSt2hCP+JVUv6XKzg@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Archived-At: <https://www.postgresql.org/message-id/7aed2a84-41a5-450f-9630-514338172017%40postgrespro.ru>
Precedence: bulk

Hi!

To be fair, I fixed this before [0] by selecting the appropriate group 
of "or" expressions to transform them to "ANY" expression and then 
checking for compatibility with the index column. maybe we should try 
this too? I can think about it.

[0] 
https://www.postgresql.org/message-id/531fc0ab-371e-4235-97e3-dd2d077b6995%40postgrespro.ru

On 23.08.2024 15:58, Alexander Korotkov wrote:
> Hi!
>
> Thank you for your feedback.
>
> On Fri, Aug 23, 2024 at 1:23 PM Andrei Lepikhov <lepihov@gmail.com> wrote:
>> On 21/8/2024 16:52, Alexander Korotkov wrote:
>>>> /* Only operator clauses scan match */
>>>> Should it be:
>>>> /* Only operator clauses can match */
>>>> ?
>>> Corrected, thanks.
>> I found one more: /* Only operator clauses scan match  */ - in the
>> second patch.
>> Also I propose:
>> - “might match to the index as whole” -> “might match the index as a whole“
>> - Group similar OR-arguments intro dedicated RestrictInfos -> ‘into’
> Fixed.
>
>>>> The second one:
>>>> When creating IndexClause, we assign the original and derived clauses to
>>>> the new, containing transformed array. But logically, we should set the
>>>> clause with a list of ORs as the original. Why did you do so?
>>> I actually didn't notice that.  Corrected to set the OR clause as the
>>> original.  That change turned recheck to use original OR clauses,
>>> probably better this way.  Also, that change spotted misuse of
>>> RestrictInfo.clause and RestrictInfo.orclause in the second patch.
>>> Corrected this too.
>> New findings:
>> =============
>>
>> 1)
>> if (list_length(clause->args) != 2)
>>          return NULL;
>> I guess, above we can 'continue' the process.
>>
>> 2) Calling the match_index_to_operand in three nested cycles you could
>> break the search on first successful match, couldn't it? At least, the
>> comment "just stop with first matching index key" say so.
> Fixed.
>
>> 3) I finally found the limit of this feature: the case of two partial
>> indexes on the same column. Look at the example below:
>>
>> SET enable_indexscan = 'off';
>> SET enable_seqscan = 'off';
>> DROP TABLE IF EXISTS test CASCADE;
>> CREATE TABLE test (x int);
>> INSERT INTO test (x) SELECT * FROM generate_series(1,100);
>> CREATE INDEX ON test (x) WHERE x < 80;
>> CREATE INDEX ON test (x) WHERE x > 80;
>> VACUUM ANALYZE test;
>> EXPLAIN (ANALYZE, COSTS OFF, TIMING OFF)
>> SELECT * FROM test WHERE x=1 OR x = 79;
>> EXPLAIN (ANALYZE, COSTS OFF, TIMING OFF)
>> SELECT * FROM test WHERE x=91 OR x = 81;
>> EXPLAIN (ANALYZE, COSTS OFF, TIMING OFF)
>> SELECT * FROM test WHERE x=1 OR x = 81 OR x = 83;
>>
>> The last query doesn't group clauses into two indexes. The reason is in
>> match_index_to_operand which classifies all 'x=' to one class. I'm not
>> sure because of overhead, but it may be resolved by using
>> predicate_implied_by to partial indexes.
> Yes, this is the conscious limitation of my patch: to consider similar
> OR arguments altogether and one-by-one, not in arbitrary groups.  The
> important thing here is that we still generating BitmapOR patch as we
> do without the patch.  So, there is no regression.  I would leave this
> as is to not make this feature too complicated. This could be improved
> in future though.
>
> ------
> Regards,
> Alexander Korotkov
> Supabase

-- 
Regards,
Alena Rybakina
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company