From: Andrei Lepikhov <lepihov@gmail.com>
To: jian he <jian.universality@gmail.com>
To: Alexander Korotkov <aekorotkov@gmail.com>
Cc: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Robert Haas <robertmhaas@gmail.com>
Cc: Andrei Lepikhov <a.lepikhov@postgrespro.ru>
Cc: Richard Guo <guofenglinux@gmail.com>
Cc: Alexander Lakhin <exclusion@gmail.com>
Cc: Gregory Stark (as CFM) <stark.cfm@gmail.com>
Cc: Michał Kłeczek <michal@kleczek.org>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: Removing unneeded self joins
Date: Fri, 12 Jul 2024 10:05:15 +0700
Message-ID: <5b49501c-9cb3-4c5d-9d56-49704ff08143@gmail.com> (raw)
In-Reply-To: <CACJufxF-ubw739W+33_rFSA+YTdRTDcKwVidKwXAS8NCy4nx+Q@mail.gmail.com>
References: <64486b0b-0404-e39e-322d-0801154901f3@postgrespro.ru>
<CAPpHfdsHWbdPVzUdmoKv3dpo8RHJOa=LQR-ZAT+89uvktMs9XA@mail.gmail.com>
<735b5cd4-568d-fbf7-ad8c-dcfb4c3af86e@gmail.com>
<CAPpHfdssga9CBQQ6s32t1Xb2Js2WuPfiporbAse334CJiyBb1Q@mail.gmail.com>
<CAPpHfdvGVcud2i00pEVH2_GS09c8UsDh6P-SEOLd6YMGOxiNEw@mail.gmail.com>
<CAMbWs49Q8g1LPVCeNHYv-Y-gFo826ertrg6nYNC9LL=8=zHP3g@mail.gmail.com>
<CAPpHfdsUiwbGLbBwtdL+UCT5GCUVEU+SqQaaZjtcje3znU2JgQ@mail.gmail.com>
<2422119.1714691974@sss.pgh.pa.us>
<CAPpHfdv6B8HCLdj8WidBryRrX0+X3F1rrR8uAuMQmp6rvPdscg@mail.gmail.com>
<24e93efd-eeac-43a6-a75a-ad68eacb32ee@postgrespro.ru>
<CA+TgmoZ66x=uWasH+O40a=G=fpbXN1ANEdGVunvR8BKuekqa2g@mail.gmail.com>
<3622801.1715010885@sss.pgh.pa.us>
<CACJufxG3sqJKe1OskHhn7OCdtrEeeRFcD8R4TTQE+LGJEQaL9w@mail.gmail.com>
<CACJufxFNDukHU2WG7T61orXdgFJ5gEkSVWcgsiGbb8tRL5=DTQ@mail.gmail.com>
<96250a42-20e3-40f0-9d45-f53ae852f8ed@gmail.com>
<CACJufxF-ubw739W+33_rFSA+YTdRTDcKwVidKwXAS8NCy4nx+Q@mail.gmail.com>
On 7/11/24 14:43, jian he wrote:
> On Tue, Jul 9, 2024 at 2:06 PM Andrei Lepikhov <lepihov@gmail.com> wrote:
>>
>> On 7/2/24 07:25, jian he wrote:
>>> to make sure it's correct, I have added a lot of tests,
>>> Some of this may be contrived, maybe some of the tests are redundant.
>> Thanks for your job!
>> I passed through the patches and have some notes:
>> 1. Patch 0001 has not been applied anymore since the previous week's
>> changes in the core. Also, there is one place with trailing whitespace.
>
> thanks.
> because the previous thread mentioned the EPQ problem.
> in remove_useless_self_joins, i make it can only process CMD_SELECT query.
I would like to oppose here: IMO, it is just a mishap which we made
because of a long history of patch transformations. There we lost the
case where RowMark exists for only one of candidate relations.
Also, after review I think we don't need so many new tests. Specifically
for DML we already have one:
EXPLAIN (COSTS OFF)
UPDATE sj sq SET b = 1 FROM sj as sz WHERE sq.a = sz.a;
And we should just add something to elaborate it a bit.
See the patch in attachment containing my proposal to improve v4-0001
main SJE patch. I think it resolved the issue with EPQ assertion as well
as problems with returning value.
--
regards, Andrei Lepikhov
Attachments:v4-0002-Apply-SJE-to-DML-queries-2.patch (text/x-patch, 6.9K) download
From c04add30999ecd64c51bde7db56a6e5637c16c74 Mon Sep 17 00:00:00 2001
From: "Andrei V. Lepikhov" <lepihov@gmail.com>
Date: Tue, 9 Jul 2024 12:25:23 +0700
Subject: [PATCH] Apply SJE to DML queries: Just don't include result relation
to the set of SJE candidates.
Also, fix the subtle bug with RowMarks.
---
src/backend/optimizer/plan/analyzejoins.c | 24 +++------
src/test/regress/expected/join.out | 61 +++++++++++++++++++++++
src/test/regress/sql/join.sql | 17 ++++++-
3 files changed, 84 insertions(+), 18 deletions(-)
diff --git a/src/backend/optimizer/plan/analyzejoins.c b/src/backend/optimizer/plan/analyzejoins.cindex bb14597762..d2b9ba7c08 100644--- a/src/backend/optimizer/plan/analyzejoins.c+++ b/src/backend/optimizer/plan/analyzejoins.c@@ -1860,10 +1860,6 @@ remove_self_join_rel(PlannerInfo *root, PlanRowMark *kmark, PlanRowMark *rmark,
/* restore the rangetblref in a proper order. */
restore_rangetblref((Node *) root->parse, toKeep->relid, toRemove->relid, 0, 0);
- /* See remove_self_joins_one_group() */- Assert(root->parse->resultRelation != toRemove->relid);- Assert(root->parse->resultRelation != toKeep->relid);-
/* Replace links in the planner info */
remove_rel_from_query(root, toRemove, toKeep->relid, NULL, NULL);
@@ -2046,14 +2042,6 @@ remove_self_joins_one_group(PlannerInfo *root, Relids relids)
{
RelOptInfo *inner = root->simple_rel_array[r];
- /*- * We don't accept result relation as either source or target relation- * of SJE, because result relation has different behavior in- * EvalPlanQual() and RETURNING clause.- */- if (root->parse->resultRelation == r)- continue;-
k = r;
while ((k = bms_next_member(relids, k)) > 0)
@@ -2069,9 +2057,6 @@ remove_self_joins_one_group(PlannerInfo *root, Relids relids)
PlanRowMark *imark = NULL;
List *uclauses = NIL;
- if (root->parse->resultRelation == k)- continue;-
/* A sanity check: the relations have the same Oid. */
Assert(root->simple_rte_array[k]->relid ==
root->simple_rte_array[r]->relid);
@@ -2121,7 +2106,8 @@ remove_self_joins_one_group(PlannerInfo *root, Relids relids)
if (omark && imark)
break;
}
- if (omark && imark && omark->markType != imark->markType)+ if (((omark == NULL) ^ (imark == NULL)) ||+ (omark && omark->markType != imark->markType))
continue;
/*
@@ -2231,7 +2217,8 @@ remove_self_joins_recurse(PlannerInfo *root, List *joinlist, Relids toRemove)
*/
if (rte->rtekind == RTE_RELATION &&
rte->relkind == RELKIND_RELATION &&
- rte->tablesample == NULL)+ rte->tablesample == NULL &&+ varno != root->parse->resultRelation)
{
Assert(!bms_is_member(varno, relids));
relids = bms_add_member(relids, varno);
@@ -2300,6 +2287,9 @@ remove_self_joins_recurse(PlannerInfo *root, List *joinlist, Relids toRemove)
relids = bms_del_members(relids, group);
+ /* Don't apply SJE to result relation */+ Assert(!bms_is_member(root->parse->resultRelation, group));+
/*
* Try to remove self-joins from a group of identical entries.
* Make the next attempt iteratively - if something is deleted
diff --git a/src/test/regress/expected/join.out b/src/test/regress/expected/join.outindex 4e4cec633a..78dfcd4866 100644--- a/src/test/regress/expected/join.out+++ b/src/test/regress/expected/join.out@@ -7068,6 +7068,18 @@ UPDATE sj sq SET b = 1 FROM sj as sz WHERE sq.a = sz.a;
-> Seq Scan on sj sz
(6 rows)
+EXPLAIN (COSTS OFF)+UPDATE sj sq SET b = sz.b FROM sj as sz WHERE sq.a = sz.a;+ QUERY PLAN +-------------------------------------+ Update on sj sq+ -> Nested Loop+ Join Filter: (sq.a = sz.a)+ -> Seq Scan on sj sq+ -> Materialize+ -> Seq Scan on sj sz+(6 rows)+
CREATE RULE sj_del_rule AS ON DELETE TO sj
DO INSTEAD
UPDATE sj SET a = 1 WHERE a = old.a;
@@ -7083,6 +7095,55 @@ EXPLAIN (COSTS OFF) DELETE FROM sj;
(6 rows)
DROP RULE sj_del_rule ON sj CASCADE;
+-- Allow SJE, remove (s2 JOIN s3).+EXPLAIN (COSTS OFF)+UPDATE sj s1 SET b = s2.b, a = s3.a FROM sj AS s2, sj AS s3+WHERE s1.a = s2.a AND s1.a=s3.a RETURNING s1.*,s2.*,s3.*;+ QUERY PLAN +---------------------------------------------+ Update on sj s1+ -> Nested Loop+ Join Filter: (s1.a = s3.a)+ -> Seq Scan on sj s1+ -> Materialize+ -> Seq Scan on sj s3+ Filter: (a IS NOT NULL)+(7 rows)++-- Allow SJE, but it is just impossible+EXPLAIN (COSTS OFF)+UPDATE sj s1 SET b = s2.b, a = s3.a FROM sj AS s2, sj AS s3+WHERE s1.a = s2.a AND s1.b=s3.b;+ QUERY PLAN +-------------------------------------------------+ Update on sj s1+ -> Nested Loop+ Join Filter: (s1.b = s3.b)+ -> Seq Scan on sj s3+ -> Materialize+ -> Nested Loop+ Join Filter: (s1.a = s2.a)+ -> Seq Scan on sj s1+ -> Materialize+ -> Seq Scan on sj s2+(10 rows)++-- Allow SJE. One more shot for the case of subquery+EXPLAIN (COSTS OFF)+UPDATE sj s1 SET b = q.b, a = q.a FROM (+ SELECT s2.a AS a, s3.b AS b FROM sj AS s2, sj AS s3+ WHERE s2.a=s3.a) AS q WHERE s1.b=q.a;+ QUERY PLAN +---------------------------------------------+ Update on sj s1+ -> Nested Loop+ Join Filter: (s3.a = s1.b)+ -> Seq Scan on sj s1+ -> Materialize+ -> Seq Scan on sj s3+ Filter: (a IS NOT NULL)+(7 rows)+
-- Check that SJE does not mistakenly omit qual clauses (bug #18187)
insert into emp1 values (1, 1);
explain (costs off)
diff --git a/src/test/regress/sql/join.sql b/src/test/regress/sql/join.sqlindex 3e94e0af53..7b32a9bb95 100644--- a/src/test/regress/sql/join.sql+++ b/src/test/regress/sql/join.sql@@ -2724,13 +2724,28 @@ TRUNCATE emp1;
EXPLAIN (COSTS OFF)
UPDATE sj sq SET b = 1 FROM sj as sz WHERE sq.a = sz.a;
-+EXPLAIN (COSTS OFF)+UPDATE sj sq SET b = sz.b FROM sj as sz WHERE sq.a = sz.a;
CREATE RULE sj_del_rule AS ON DELETE TO sj
DO INSTEAD
UPDATE sj SET a = 1 WHERE a = old.a;
EXPLAIN (COSTS OFF) DELETE FROM sj;
DROP RULE sj_del_rule ON sj CASCADE;
+-- Allow SJE, remove (s2 JOIN s3).+EXPLAIN (COSTS OFF)+UPDATE sj s1 SET b = s2.b, a = s3.a FROM sj AS s2, sj AS s3+WHERE s1.a = s2.a AND s1.a=s3.a RETURNING s1.*,s2.*,s3.*;+-- Allow SJE, but it is just impossible+EXPLAIN (COSTS OFF)+UPDATE sj s1 SET b = s2.b, a = s3.a FROM sj AS s2, sj AS s3+WHERE s1.a = s2.a AND s1.b=s3.b;+-- Allow SJE. One more shot for the case of subquery+EXPLAIN (COSTS OFF)+UPDATE sj s1 SET b = q.b, a = q.a FROM (+ SELECT s2.a AS a, s3.b AS b FROM sj AS s2, sj AS s3+ WHERE s2.a=s3.a) AS q WHERE s1.b=q.a;+
-- Check that SJE does not mistakenly omit qual clauses (bug #18187)
insert into emp1 values (1, 1);
explain (costs off)
--
2.39.2
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: lepihov@gmail.com, jian.universality@gmail.com, aekorotkov@gmail.com, tgl@sss.pgh.pa.us, robertmhaas@gmail.com, a.lepikhov@postgrespro.ru, guofenglinux@gmail.com, exclusion@gmail.com, stark.cfm@gmail.com, michal@kleczek.org, pgsql-hackers@lists.postgresql.org
Subject: Re: Removing unneeded self joins
In-Reply-To: <5b49501c-9cb3-4c5d-9d56-49704ff08143@gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox