Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1rV8UW-00F4bt-LH for pgsql-hackers@arkaria.postgresql.org; Wed, 31 Jan 2024 11:10:53 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.94.2) (envelope-from ) id 1rV8UV-00DUxm-RB for pgsql-hackers@arkaria.postgresql.org; Wed, 31 Jan 2024 11:10:51 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1rV8UV-00DUx0-BO for pgsql-hackers@lists.postgresql.org; Wed, 31 Jan 2024 11:10:51 +0000 Received: from mail.postgrespro.ru ([93.174.131.139]) by magus.postgresql.org with esmtps (TLS1.2) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1rV8UQ-004hMy-Ta for pgsql-hackers@postgresql.org; Wed, 31 Jan 2024 11:10:50 +0000 Received: from [10.4.13.208] (unknown [93.174.131.141]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client did not present a certificate) (Authenticated sender: a.rybakina@postgrespro.ru) by mail.postgrespro.ru (Postfix/587) with ESMTPSA id AB062E21097; Wed, 31 Jan 2024 14:10:44 +0300 (MSK) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=postgrespro.ru; s=mx2023; t=1706699445; bh=hIKOBxfcc+F11Uf2b/WAaYNKN9T/KS96Lb9m/+dVVho=; h=Message-ID:Date:User-Agent:Subject:To:Cc:References:From: In-Reply-To:From; b=EShfyUctknw9XGG5R65Osy3khZdjBoDhpCVkcOSE+azoe4cEU+gJbIjftX5BVTHny 6hu9AUJ4F3FK3tPnAyY+38n7bJSRQwFPBroSmGidJlXQxln9c/GMQD1ZBmOW99QxPC spzLWUOd1cg6JY1HZAOANe/WTEh4mogJT8VYA+B7Q9zMfzcHNy7Nth8RWSd1HE7Enk MEG3W0V4xJ6J9BqxxTVpastktbV5uFVi96QOvHSZvC/GTQWa7QCi68ockiPLz37plU Vbiitn7R3EYFtH7DGvyJyLt0XuReUAp3wyFEErrrRLIniCmLMHaYf8Ua98CIlkvMdm bjFLLu92RauCA== Content-Type: multipart/alternative; boundary="------------hKpcqsFlRdlVa0c0xA8N8bg1" Message-ID: Date: Wed, 31 Jan 2024 14:10:44 +0300 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: POC, WIP: OR-clause support for indexes Content-Language: en-US To: jian he Cc: Andrei Lepikhov , Robert Haas , pgsql-hackers@postgresql.org, Alexander Korotkov , Peter Geoghegan , "Finnerty, Jim" , Marcos Pegoraro , teodor@sigaev.ru, Ranier Vilela , Tomas Vondra , Peter Eisentraut References: <567ED6CA.2040504@sigaev.ru> <4c1c2c0b-802f-45ae-a7cd-007bfd8957cf@postgrespro.ru> <52f0d302-7963-452f-8c5a-f66d261514eb@postgrespro.ru> <43ad8a48-b980-410d-a83c-5beebf82a4ed@postgrespro.ru> <04ef16eb-46c1-4ce7-9f68-d1c80ef0be81@postgrespro.ru> <59e67a40-95a8-4d74-ae4e-027ea0f59084@postgrespro.ru> <59b77a8a-29d6-4727-bf26-4a30a6a9719a@postgrespro.ru> <32d23d97-d07f-4d44-9933-ba3295fb3ed8@postgrespro.ru> From: Alena Rybakina In-Reply-To: List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk This is a multi-part message in MIME format. --------------hKpcqsFlRdlVa0c0xA8N8bg1 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Hi, thank you for your review and interest in this subject. On 31.01.2024 13:15, jian he wrote: > On Wed, Jan 31, 2024 at 10:55 AM jian he wrote: >> based on my understanding of >> https://www.postgresql.org/docs/current/xoper-optimization.html#XOPER-COMMUTATOR >> I think you need move commutator check right after the `if >> (get_op_rettype(opno) != BOOLOID)` branch >> > I was wrong about this part. sorry for the noise. > > > I have made some changes (attachment). > * if the operator expression left or right side type category is > {array | domain | composite}, then don't do the transformation. > (i am not 10% sure with composite) To be honest, I'm not sure about this check, because we check the type of variable there: if (!IsA(orqual, OpExpr))         {             or_list = lappend(or_list, orqual);             continue;         } And below: if (IsA(leftop, Const))         {             opno = get_commutator(opno);             if (!OidIsValid(opno))             {                 /* Commuter doesn't exist, we can't reverse the order */                 or_list = lappend(or_list, orqual);                 continue;             }             nconst_expr = get_rightop(orqual);             const_expr = get_leftop(orqual);         }         else if (IsA(rightop, Const))         {             const_expr = get_rightop(orqual);             nconst_expr = get_leftop(orqual);         }         else         {             or_list = lappend(or_list, orqual);             continue;         } Isn't that enough? Besides, some of examples (with ARRAY) works fine: postgres=# CREATE TABLE sal_emp (     pay_by_quarter  integer[],     pay_by_quater1 integer[] ); CREATE TABLE postgres=# INSERT INTO sal_emp     VALUES (     '{10000, 10000, 10000, 10000}',     '{1,2,3,4}'); INSERT 0 1 postgres=# select * from sal_emp where pay_by_quarter[1] = 10000 or pay_by_quarter[1]=2;       pay_by_quarter       | pay_by_quater1 ---------------------------+----------------  {10000,10000,10000,10000} | {1,2,3,4} (1 row) postgres=# explain select * from sal_emp where pay_by_quarter[1] = 10000 or pay_by_quarter[1]=2;                           QUERY PLAN --------------------------------------------------------------  Seq Scan on sal_emp  (cost=0.00..21.00 rows=9 width=64)    Filter: (pay_by_quarter[1] = ANY ('{10000,2}'::integer[])) (2 rows) > * if the left side of the operator expression node contains volatile > functions, then don't do the transformation. I'm also not sure about the volatility check function, because we perform such a conversion at the parsing stage, and at this stage we don't have a RelOptInfo variable and especially a RestictInfo such as PathTarget. Speaking of NextValueExpr, I couldn't find any examples where the current patch wouldn't work. I wrote one of them below: postgres=# create table foo (f1 int, f2 int generated always as identity); CREATE TABLE postgres=# insert into foo values(1); INSERT 0 1 postgres=# explain verbose update foo set f1 = 2 where f1=1 or f1=2 ;                             QUERY PLAN -------------------------------------------------------------------  Update on public.foo  (cost=0.00..38.25 rows=0 width=0)    ->  Seq Scan on public.foo  (cost=0.00..38.25 rows=23 width=10)          Output: 2, ctid          Filter: (foo.f1 = ANY ('{1,2}'::integer[])) (4 rows) Maybe I missed something. Do you have any examples? > * some other minor cosmetic changes. Thank you, I agree with them. -- Regards, Alena Rybakina Postgres Professional:http://www.postgrespro.com The Russian Postgres Company --------------hKpcqsFlRdlVa0c0xA8N8bg1 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: 8bit
Hi, thank you for your review and interest in this subject.

On 31.01.2024 13:15, jian he wrote:
On Wed, Jan 31, 2024 at 10:55 AM jian he <jian.universality@gmail.com> wrote:

based on my understanding of
https://www.postgresql.org/docs/current/xoper-optimization.html#XOPER-COMMUTATOR
I think you need move commutator check right after the `if
(get_op_rettype(opno) != BOOLOID)` branch


I was wrong about this part. sorry for the noise.


I have made some changes (attachment).
* if the operator expression left or right side type category is
{array | domain | composite}, then don't do the transformation.
(i am not 10% sure with composite)

To be honest, I'm not sure about this check, because we check the type of variable there:

if (!IsA(orqual, OpExpr))
        {
            or_list = lappend(or_list, orqual);
            continue;
        }
And below:
if (IsA(leftop, Const))
        {
            opno = get_commutator(opno);

            if (!OidIsValid(opno))
            {
                /* Commuter doesn't exist, we can't reverse the order */
                or_list = lappend(or_list, orqual);
                continue;
            }

            nconst_expr = get_rightop(orqual);
            const_expr = get_leftop(orqual);
        }
        else if (IsA(rightop, Const))
        {
            const_expr = get_rightop(orqual);
            nconst_expr = get_leftop(orqual);
        }
        else
        {
            or_list = lappend(or_list, orqual);
            continue;
        }

Isn't that enough?

Besides, some of examples (with ARRAY) works fine:

postgres=# CREATE TABLE sal_emp (
    pay_by_quarter  integer[],
    pay_by_quater1 integer[]
);
CREATE TABLE
postgres=# INSERT INTO sal_emp
    VALUES (       
    '{10000, 10000, 10000, 10000}',
    '{1,2,3,4}');
INSERT 0 1
postgres=# select * from sal_emp where pay_by_quarter[1] = 10000 or pay_by_quarter[1]=2;
      pay_by_quarter       | pay_by_quater1
---------------------------+----------------
 {10000,10000,10000,10000} | {1,2,3,4}
(1 row)

postgres=# explain select * from sal_emp where pay_by_quarter[1] = 10000 or pay_by_quarter[1]=2;
                          QUERY PLAN                          
--------------------------------------------------------------
 Seq Scan on sal_emp  (cost=0.00..21.00 rows=9 width=64)
   Filter: (pay_by_quarter[1] = ANY ('{10000,2}'::integer[]))
(2 rows)

* if the left side of the operator expression node contains volatile
functions, then don't do the transformation.

I'm also not sure about the volatility check function, because we perform such a conversion at the parsing stage, and at this stage we don't have a RelOptInfo variable and especially a RestictInfo such as PathTarget.

Speaking of NextValueExpr, I couldn't find any examples where the current patch wouldn't work. I wrote one of them below:

postgres=# create table foo (f1 int, f2 int generated always as identity);
CREATE TABLE
postgres=# insert into foo values(1);
INSERT 0 1
postgres=# explain verbose update foo set f1 = 2 where f1=1 or f1=2 ;
                            QUERY PLAN                             
-------------------------------------------------------------------
 Update on public.foo  (cost=0.00..38.25 rows=0 width=0)
   ->  Seq Scan on public.foo  (cost=0.00..38.25 rows=23 width=10)
         Output: 2, ctid
         Filter: (foo.f1 = ANY ('{1,2}'::integer[]))
(4 rows)

Maybe I missed something. Do you have any examples?

* some other minor  cosmetic changes.
Thank you, I agree with them.
-- 
Regards,
Alena Rybakina
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
--------------hKpcqsFlRdlVa0c0xA8N8bg1--