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 1qrDyQ-003DfY-N7 for pgsql-hackers@arkaria.postgresql.org; Fri, 13 Oct 2023 08:56:47 +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 1qrDyO-00GDR5-Eo for pgsql-hackers@arkaria.postgresql.org; Fri, 13 Oct 2023 08:56:45 +0000 Received: from makus.postgresql.org ([2001:4800:3e1:1::229]) by malur.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1qrDyN-00GDQw-SP for pgsql-hackers@lists.postgresql.org; Fri, 13 Oct 2023 08:56:44 +0000 Received: from mail.postgrespro.ru ([93.174.131.139]) by makus.postgresql.org with esmtp (Exim 4.94.2) (envelope-from ) id 1qrDyK-000GZH-Oa for pgsql-hackers@lists.postgresql.org; Fri, 13 Oct 2023 08:56:43 +0000 Received: from [172.30.0.94] (unknown [172.30.0.94]) (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 A52C3E20EA2; Fri, 13 Oct 2023 11:56:37 +0300 (MSK) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=postgrespro.ru; s=mx2023; t=1697187398; bh=cUV2KCb1PH1MlMKEJ8JBS9+6J+WblwyD5RLuuWr6lTU=; h=Message-ID:Date:User-Agent:Subject:To:Cc:References:From: In-Reply-To:From; b=Sr9brHnHh7SRmvLp5MmM/L4VC7WfRY8unb2C+VZQVJkJodrtId4MgNUMEnRy2N/tD +QkyfD/K6L2CLKepr3PDrO6x8Y9KdIxhxAHU/IyjahrkDW3q7T70htnXl0ZWkNAf3B BYAgc3vQnHL7aqtjGuCvSLXrgimhUrVyIt4aBrr8Ihg8ZUu5aIwquUxb1kjhQNo13Y /pGBfhlyYze18SRjU9SoM88tT1uecmZd8a5fLu1J7mcCNaZtlVNb9wIyReMcBOFhGT DksQi+KxtHpAEHvfSZuE6r0FUo1TOvO8r9nKTOxbRRe4YNnkFYelHoZPG0VFbBynJl Ao4G2C732kY+w== Content-Type: multipart/alternative; boundary="------------SqlLnx0ZIx0zVa0f3VO3a0BV" Message-ID: <29ddec15-a83f-4f24-b069-6e93c189b169@postgrespro.ru> Date: Fri, 13 Oct 2023 11:56:37 +0300 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: Removing unneeded self joins Content-Language: en-US To: Andrei Lepikhov , Alexander Korotkov Cc: "Gregory Stark (as CFM)" , =?UTF-8?Q?Micha=C5=82_K=C5=82eczek?= , PostgreSQL Hackers References: <64486b0b-0404-e39e-322d-0801154901f3@postgrespro.ru> <0fd15c10-eaa5-51bd-b5c7-e635fe03a94d@postgrespro.ru> <20221206184643.zya44h5auv6f45zp@awork3.anarazel.de> <36579124-f9d9-a51e-38b3-af3511921c7e@postgrespro.ru> <88079CBD-B5BB-4C61-A68B-A8C67599E1C7@kleczek.org> <05469e80-454b-41d7-d04f-421f42bbd4a9@postgrespro.ru> <48c11b23-9c23-440e-9a3c-020e05196449@postgrespro.ru> <9eb86ca9-da3c-417f-8ce1-29c0ef5a4f50@postgrespro.ru> From: "a.rybakina" In-Reply-To: <9eb86ca9-da3c-417f-8ce1-29c0ef5a4f50@postgrespro.ru> 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. --------------SqlLnx0ZIx0zVa0f3VO3a0BV Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit >> Also I've incorporated improvements from Alena Rybakina except one for >> skipping SJ removal when no SJ quals is found.  It's not yet clear for >> me if this check fix some cases. But at least optimization got skipped >> in some useful cases (as you can see in regression tests). > > Agree. I wouldn't say I like it too. But also, I suggest skipping some > unnecessary assertions proposed in that patch: > Assert(toKeep->relid != -1); - quite strange. Why -1? Why not all the > negative numbers, at least? > Assert(is_opclause(orinfo->clause)); - above we skip clauses with > rinfo->mergeopfamilies == NIL. Each mergejoinable clause is already > checked as is_opclause. > All these changes (see in the attachment) are optional. > I don't mind about asserts, maybe I misunderstood something in the patch. About skipping SJ removal when no SJ quals is found, I assume it is about it: split_selfjoin_quals(root, restrictlist, &selfjoinquals,                                   &otherjoinquals, inner->relid, outer->relid); +            if (list_length(selfjoinquals) == 0) +             { +                 /* +                  * XXX: +                  * we would detect self-join without quals like 'x==x' if we had +                  * an foreign key constraint on some of other quals and this join +                  * haven't any columns from the outer in the target list. +                  * But it is still complex task. +                  */ +                 continue; +             } as far as I remember, this is the place where it is checked that the SJ list is empty and it is logical, in my opinion, that no transformations should be performed if no elements are found for them. As for the cases where SJ did not work, maybe this is just right if there are no elements for processing these cases. I'll try to check or come up with tests for them. If I'm wrong, write. On 11.10.2023 06:51, Andrei Lepikhov wrote: > On 11/10/2023 02:29, Alena Rybakina wrote: >> I have reviewed your patch and I noticed a few things. > > Thanks for your efforts, > >> I have looked at the latest version of the code, I assume that the >> error lies in the replace_varno_walker function, especially in the >> place where we check the node by type Var, and does not form any >> NullTest node. > > It's not a bug, it's an optimization we discussed with Alexander above. > >> Secondly, I added some code in some places to catch erroneous cases >> and added a condition when we should not try to apply the >> self-join-removal transformation due to the absence of an empty >> self-join list after searching for it and in general if there are no >> joins in the query. Besides, I added a query for testing and wrote >> about it above. I have attached my diff file. > Ok, I will look at this >> In addition, I found a comment for myself that was not clear to me. I >> would be glad if you could explain it to me. >> >> You mentioned superior outer join in the comment, unfortunately, I >> didn't find anything about it in the PostgreSQL code, and this >> explanation remained unclear to me. Could you explain in more detail >> what you meant? > I meant here that only clauses pushed by > reconsider_outer_join_clauses() can be found in the joininfo list, and > they are not relevant, as you can understand. > Having written that, I realized that it was a false statement. ;) - > joininfo can also contain results of previous SJE iterations, look: > > CREATE TABLE test (oid int PRIMARY KEY); > CREATE UNIQUE INDEX ON test((oid*oid)); > explain > SELECT count(*) > FROM test c1, test c2, test c3 > WHERE c1.oid=c2.oid AND c1.oid*c2.oid=c3.oid*c3.oid; > explain > SELECT count(*) > FROM test c1, test c2, test c3 > WHERE c1.oid=c3.oid AND c1.oid*c3.oid=c2.oid*c2.oid; > explain > SELECT count(*) > FROM test c1, test c2, test c3 > WHERE c3.oid=c2.oid AND c3.oid*c2.oid=c1.oid*c1.oid; Ok, I understood. Thank you for explanation. -- Regards, Alena Rybakina --------------SqlLnx0ZIx0zVa0f3VO3a0BV Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: 8bit
Also I've incorporated improvements from Alena Rybakina except one for
skipping SJ removal when no SJ quals is found.  It's not yet clear for
me if this check fix some cases. But at least optimization got skipped
in some useful cases (as you can see in regression tests).

Agree. I wouldn't say I like it too. But also, I suggest skipping some unnecessary assertions proposed in that patch:
Assert(toKeep->relid != -1); - quite strange. Why -1? Why not all the negative numbers, at least?
Assert(is_opclause(orinfo->clause)); - above we skip clauses with rinfo->mergeopfamilies == NIL. Each mergejoinable clause is already checked as is_opclause.
All these changes (see in the attachment) are optional.

I don't mind about asserts, maybe I misunderstood something in the patch.

About skipping SJ removal when no SJ quals is found, I assume it is about it:

split_selfjoin_quals(root, restrictlist, &selfjoinquals,
                                  &otherjoinquals, inner->relid, outer->relid);
 
+            if (list_length(selfjoinquals) == 0)
+             {
+                 /*
+                  * XXX:
+                  * we would detect self-join without quals like 'x==x' if we had
+                  * an foreign key constraint on some of other quals and this join
+                  * haven't any columns from the outer in the target list.
+                  * But it is still complex task.
+                  */
+                 continue;
+             }

as far as I remember, this is the place where it is checked that the SJ list is empty and it is logical, in my opinion, that no transformations should be performed if no elements are found for them.

As for the cases where SJ did not work, maybe this is just right if there are no elements for processing these cases. I'll try to check or come up with tests for them. If I'm wrong, write.

On 11.10.2023 06:51, Andrei Lepikhov wrote:
On 11/10/2023 02:29, Alena Rybakina wrote:
I have reviewed your patch and I noticed a few things.

Thanks for your efforts,

I have looked at the latest version of the code, I assume that the error lies in the replace_varno_walker function, especially in the place where we check the node by type Var, and does not form any NullTest node.

It's not a bug, it's an optimization we discussed with Alexander above.

Secondly, I added some code in some places to catch erroneous cases and added a condition when we should not try to apply the self-join-removal transformation due to the absence of an empty self-join list after searching for it and in general if there are no joins in the query. Besides, I added a query for testing and wrote about it above. I have attached my diff file.
Ok, I will look at this
In addition, I found a comment for myself that was not clear to me. I would be glad if you could explain it to me.

You mentioned superior outer join in the comment, unfortunately, I didn't find anything about it in the PostgreSQL code, and this explanation remained unclear to me. Could you explain in more detail what you meant?
I meant here that only clauses pushed by reconsider_outer_join_clauses() can be found in the joininfo list, and they are not relevant, as you can understand.
Having written that, I realized that it was a false statement. ;) - joininfo can also contain results of previous SJE iterations, look:

CREATE TABLE test (oid int PRIMARY KEY);
CREATE UNIQUE INDEX ON test((oid*oid));
explain
SELECT count(*)
FROM test c1, test c2, test c3
WHERE c1.oid=c2.oid AND c1.oid*c2.oid=c3.oid*c3.oid;
explain
SELECT count(*)
FROM test c1, test c2, test c3
WHERE c1.oid=c3.oid AND c1.oid*c3.oid=c2.oid*c2.oid;
explain
SELECT count(*)
FROM test c1, test c2, test c3
WHERE c3.oid=c2.oid AND c3.oid*c2.oid=c1.oid*c1.oid;

Ok, I understood. Thank you for explanation.


-- 
Regards,
Alena Rybakina

--------------SqlLnx0ZIx0zVa0f3VO3a0BV--