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

Hi!

On 12.01.2025 21:39, Alexander Korotkov wrote:
> On Fri, Nov 29, 2024 at 9:54 AM Alexander Korotkov <aekorotkov@gmail.com> wrote:
>> On Fri, Nov 29, 2024 at 7:51 AM Alena Rybakina
>> <a.rybakina@postgrespro.ru> wrote:
>>> On 29.11.2024 03:04, Alexander Korotkov wrote:
>>>> On Thu, Nov 28, 2024 at 9:33 PM Alena Rybakina
>>>> <a.rybakina@postgrespro.ru> wrote:
>>>>> On 28.11.2024 22:28, Ranier Vilela wrote:
>>>>>
>>>>> Em qui., 28 de nov. de 2024 às 16:03, Alena Rybakina <a.rybakina@postgrespro.ru> escreveu:
>>>>>> Hi! Thank you for the case.
>>>>>>
>>>>>> On 28.11.2024 21:00, Alexander Lakhin wrote:
>>>>>>> Hello Alexander,
>>>>>>>
>>>>>>> 21.11.2024 09:34, Alexander Korotkov wrote:
>>>>>>>> I'm going to push this if no objections.
>>>>>>> Please look at the following query, which triggers an error after
>>>>>>> ae4569161:
>>>>>>> SET random_page_cost = 1;
>>>>>>> CREATE TABLE tbl(u UUID);
>>>>>>> CREATE INDEX idx ON tbl USING HASH (u);
>>>>>>> SELECT COUNT(*) FROM tbl WHERE u = '00000000000000000000000000000000' OR
>>>>>>>     u = '11111111111111111111111111111111';
>>>>>>>
>>>>>>> ERROR:  XX000: ScalarArrayOpExpr index qual found where not allowed
>>>>>>> LOCATION:  ExecIndexBuildScanKeys, nodeIndexscan.c:1625
>>>>>>>
>>>>>>>
>>>>>> I found out what the problem is index scan method was not generated. We
>>>>>> need to check this during OR clauses for SAOP transformation.
>>>>>>
>>>>>> There is a patch to fix this problem.
>>>>> Hi.
>>>>> Thanks for the quick fix.
>>>>>
>>>>> But I wonder if it is not possible to avoid all if the index is useless?
>>>>> Maybe moving your fix to the beginning of the function?
>>>>>
>>>>> diff --git a/src/backend/optimizer/path/indxpath.c b/src/backend/optimizer/path/indxpath.c
>>>>> index d827fc9f4d..5ea0b27d01 100644
>>>>> --- a/src/backend/optimizer/path/indxpath.c
>>>>> +++ b/src/backend/optimizer/path/indxpath.c
>>>>> @@ -3248,6 +3248,10 @@ match_orclause_to_indexcol(PlannerInfo *root,
>>>>>     Assert(IsA(orclause, BoolExpr));
>>>>>     Assert(orclause->boolop == OR_EXPR);
>>>>>
>>>>> + /* Ignore index if it doesn't support index scans */
>>>>> + if(!index->amsearcharray)
>>>>> + return NULL;
>>>>> +
>>>>>
>>>>> Agree. I have updated the patch
>>>>>
>>>>>     /*
>>>>>     * Try to convert a list of OR-clauses to a single SAOP expression. Each
>>>>>     * OR entry must be in the form: (indexkey operator constant) or (constant
>>>>>
>>>>> The test bug:
>>>>> EXPLAIN SELECT COUNT(*) FROM tbl WHERE u = '00000000000000000000000000000000' OR u = '11111111111111111111111111111111';
>>>>>                                                               QUERY PLAN
>>>>> ----------------------------------------------------------------------------------------------------------------------------------
>>>>>    Aggregate  (cost=12.46..12.47 rows=1 width=8)
>>>>>      ->  Bitmap Heap Scan on tbl  (cost=2.14..12.41 rows=18 width=0)
>>>>>            Recheck Cond: ((u = '00000000-0000-0000-0000-000000000000'::uuid) OR (u = '11111111-1111-1111-1111-111111111111'::uuid))
>>>>>            ->  BitmapOr  (cost=2.14..2.14 rows=18 width=0)
>>>>>                  ->  Bitmap Index Scan on idx  (cost=0.00..1.07 rows=9 width=0)
>>>>>                        Index Cond: (u = '00000000-0000-0000-0000-000000000000'::uuid)
>>>>>                  ->  Bitmap Index Scan on idx  (cost=0.00..1.07 rows=9 width=0)
>>>>>                        Index Cond: (u = '11111111-1111-1111-1111-111111111111'::uuid)
>>>>> (8 rows)
>>>> I slightly revised the fix and added similar check to
>>>> group_similar_or_args().  Could you, please, review that before
>>>> commit?
>>>>
>>> I agree with changes. Thank you!
>> Andrei, Alena, thank you for the feedback. Pushed!
> I think we should give some more attention to the patch enabling OR to
> SAOP transformation for joins (first time posted in [1]).  I think we
> tried to only work with Const and Param, because we were previously
> working during parse stage. So, at that stage if we have the clause
> like "a.x = 1 OR a.x = b.x OR b.x = 2", then we don't know if we
> should transform it into "a.x = ANY(1, b.x) OR b.x = 2" or into "a.x
> =1 OR b.x = ANY(a.x, 2)". But if we do the transformation during the
> index matching, we would actually be able to try the both and select
> the best.
>
> The revised patch is attached.  Most notably it revises
> group_similar_or_args() to have the same notion of const-ness as
> others.  In that function we split potential index key and constant
> early to save time on enumerating all possible index keys.  But it
> appears to be possible to split by relids bitmapsets: index key should
> use our relid, while const shouldn't.  Other that that, comments,
> commit message and naming are revised.
>
> Links.
> 1. https://www.postgresql.org/message-id/CAPpHfdu9QJ%3DGbua3CUUH2KKG_8urakJTen4JD47PGh9wWP%3DQxQ%40mail.gmail.com
>
I like your idea. I looked at your patch and haven't noticed any bugs 
yet, but my review is not finished.

I think we're missing tests here - I only noticed one difference in the 
regression test related to your specific improvement.

I thought it would be possible to look at cases where q1 and q2 are not 
equal to an integer constant table,
but have a more complex structure. For example, set the conditions "q1 
as select (1=1)::integer" and "q2 as select (1=0)::integer".

-- 
Regards,
Alena Rybakina
Postgres Professional