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(at)gmail(dot)com> wrote:
>> On Fri, Nov 29, 2024 at 7:51 AM Alena Rybakina
>> <a(dot)rybakina(at)postgrespro(dot)ru> wrote:
>>> On 29.11.2024 03:04, Alexander Korotkov wrote:
>>>> On Thu, Nov 28, 2024 at 9:33 PM Alena Rybakina
>>>> <a(dot)rybakina(at)postgrespro(dot)ru> wrote:
>>>>> On 28.11.2024 22:28, Ranier Vilela wrote:
>>>>>
>>>>> Em qui., 28 de nov. de 2024 às 16:03, Alena Rybakina <a(dot)rybakina(at)postgrespro(dot)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