Message-ID: <a805a3f3-558a-42ef-a6b2-ad0c8af36a64@postgrespro.ru>
Date: Sun, 12 Jan 2025 23:38:35 +0300
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: POC, WIP: OR-clause support for indexes
To: Alexander Korotkov <aekorotkov@gmail.com>
Cc: Ranier Vilela <ranier.vf@gmail.com>,
 Alexander Lakhin <exclusion@gmail.com>, Andrei Lepikhov <lepihov@gmail.com>,
 jian he <jian.universality@gmail.com>, Robert Haas <robertmhaas@gmail.com>,
 Nikolay Shaplov <dhyan@nataraj.su>, pgsql-hackers@lists.postgresql.org,
 pgsql-hackers@postgresql.org, Peter Geoghegan <pg@bowt.ie>,
 Marcos Pegoraro <marcos@f10.com.br>, teodor@sigaev.ru,
 Peter Eisentraut <peter@eisentraut.org>
References: <567ED6CA.2040504@sigaev.ru>
 <CAPpHfdvxF1OZUoJr2bg8cmAnty-KyRkswPa3hCPSmR5gSx7-Yg@mail.gmail.com>
 <CACJufxGS_MKqkfnw3BMhfi+=xuf2SAFvwf0Eq3e12XqAQaKdZg@mail.gmail.com>
 <0026f562-d403-4f97-b9dc-3fe3a279e8c9@postgrespro.ru>
 <CAPpHfduzBgV3AecMU0jFqOSjK9iP86HiHEzj2Hv6hLqWu7JJFQ@mail.gmail.com>
 <CAPpHfduqOuvh6i=jYDPWnAUg325hsOLUAW9r_awdirQRA7uzHA@mail.gmail.com>
 <37bf870a-22e0-4aba-b845-549482118f8d@gmail.com>
 <CAPpHfdtPkA3XGM+FOanXfFNm45YEMkuqpfL0ZSL7uOP1bHPPEg@mail.gmail.com>
 <8174de69-9e1a-0827-0e81-ef97f56a5939@gmail.com>
 <ed2b909c-35d4-4a39-ac38-8a15634a281b@postgrespro.ru>
 <CAEudQApBzH9JNkDpXARrjFG0WGM2PcU02f6Ga9sLVS2H6z9YAQ@mail.gmail.com>
 <3b717020-14d9-425f-8c60-ce336b5a23b3@postgrespro.ru>
 <CAPpHfdvM4E-wPCRbYkr9eGH2sSxgOYKZ1iOUyjKYqTEyurFRLQ@mail.gmail.com>
 <43db6a40-5c5f-4909-ad62-3060b297e686@postgrespro.ru>
 <CAPpHfdtgcye9ELaLHw4ZOUxWQ9t1P=oBDJX8KwwOHqasESdXQQ@mail.gmail.com>
 <CAPpHfdv+jtNwofg-p5z86jLYZUTt6tR17Wy00ta0dL=wHQN3ZA@mail.gmail.com>
Content-Language: en-US
From: Alena Rybakina <a.rybakina@postgrespro.ru>
In-Reply-To: <CAPpHfdv+jtNwofg-p5z86jLYZUTt6tR17Wy00ta0dL=wHQN3ZA@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Archived-At: <https://www.postgresql.org/message-id/a805a3f3-558a-42ef-a6b2-ad0c8af36a64%40postgrespro.ru>
Precedence: bulk

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