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

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