From: Andrei Lepikhov <a.lepikhov@postgrespro.ru>
To: Alexander Korotkov <aekorotkov@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>
Cc: a.rybakina <a.rybakina@postgrespro.ru>
Subject: Re: Removing unneeded self joins
Date: Fri, 13 Oct 2023 09:55:13 +0700
Message-ID: <9eb86ca9-da3c-417f-8ce1-29c0ef5a4f50@postgrespro.ru> (raw)
In-Reply-To: <CAPpHfdsQkY0Gu7bQ9Ekdyzo_5Xxz74TYKbwmk1on1WAASpxXcQ@mail.gmail.com>
References: <64486b0b-0404-e39e-322d-0801154901f3@postgrespro.ru>
<CALNJ-vTyL-LpvSOPZxpC63Et3LJLUAFZSfRqGEhT5Rj7_EEj7w@mail.gmail.com>
<0fd15c10-eaa5-51bd-b5c7-e635fe03a94d@postgrespro.ru>
<d9493d5a-b950-465e-f09b-62c92a23fba1@postgrespro.ru>
<20221206184643.zya44h5auv6f45zp@awork3.anarazel.de>
<36579124-f9d9-a51e-38b3-af3511921c7e@postgrespro.ru>
<88079CBD-B5BB-4C61-A68B-A8C67599E1C7@kleczek.org>
<CAM-w4HMZXW+hKc8C+WLQ+gdpEkoh9378a2UGKyZRz7mZVerG+Q@mail.gmail.com>
<c97574c4-13e6-9107-26ab-af2bd626fc85@postgrespro.ru>
<bcf2f3c5-fbdc-f85b-af3e-ebec616160f6@postgrespro.ru>
<05469e80-454b-41d7-d04f-421f42bbd4a9@postgrespro.ru>
<CAPpHfdt9G3EU+vXuqexhweEQoMF-acPUUFuB6hV2Kg0gCy7AjA@mail.gmail.com>
<ebe66358-428c-439f-85dd-da72c9b2b0c2@postgrespro.ru>
<CAPpHfdt-0kVV7O==aJEbjY2iGYBu+XBzTHEbPv_6sVNeC7fffQ@mail.gmail.com>
<48c11b23-9c23-440e-9a3c-020e05196449@postgrespro.ru>
<CAPpHfdsQkY0Gu7bQ9Ekdyzo_5Xxz74TYKbwmk1on1WAASpxXcQ@mail.gmail.com>
On 12/10/2023 18:32, Alexander Korotkov wrote:
> On Thu, Oct 5, 2023 at 12:17 PM Andrei Lepikhov
>> We have almost the results we wanted to have. But in the last explain
>> you can see that nothing happened with the OR clause. We should use the
>> expression mutator instead of walker to handle such clauses. But It
>> doesn't process the RestrictInfo node ... I'm inclined to put a solution
>> of this issue off for a while.
>
> OK. I think it doesn't worth to eliminate IS NULL quals with this
> complexity (at least at this stage of work).
Yeah. I think It would be meaningful in the case of replacing also
nested x IS NOT NULL with nothing. But it requires using a mutator
instead of the walker and may be done more accurately next time.
> I made improvements over the code. Mostly new comments, grammar
> corrections of existing comments and small refactoring.
Great!
> Also, I found that the suggestion from David Rowley [1] to qsort
> array of relations to faster find duplicates is still unaddressed.
> I've implemented it. That helps to evade quadratic complexity with
> large number of relations.
I see. The thread is too long so far, thanks for the catch.
> 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.
--
regards,
Andrey Lepikhov
Postgres Professional
diff --git a/src/backend/optimizer/plan/analyzejoins.c b/src/backend/optimizer/plan/analyzejoins.cindex f0746f35a3..7b8dc7a2b7 100644--- a/src/backend/optimizer/plan/analyzejoins.c+++ b/src/backend/optimizer/plan/analyzejoins.c@@ -1710,8 +1710,6 @@ remove_self_join_rel(PlannerInfo *root, PlanRowMark *kmark, PlanRowMark *rmark,
List *binfo_candidates = NIL;
ReplaceVarnoContext ctx = {.from = toRemove->relid,.to = toKeep->relid};
- Assert(toKeep->relid != -1);-
/*
* Replace index of removing table with the keeping one. The technique of
* removing/distributing restrictinfo is used here to attach just appeared
@@ -2017,8 +2015,6 @@ match_unique_clauses(PlannerInfo *root, RelOptInfo *outer, List *uclauses,
/* Don't consider clauses which aren't similar to 'F(X)=G(Y)' */
continue;
- Assert(is_opclause(orinfo->clause));-
oclause = bms_is_empty(orinfo->left_relids) ?
get_rightop(orinfo->clause) : get_leftop(orinfo->clause);
c2 = (bms_is_empty(orinfo->left_relids) ?
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.listindex 2ff4881fdf..96ebd6eed3 100644--- a/src/tools/pgindent/typedefs.list+++ b/src/tools/pgindent/typedefs.list@@ -367,7 +367,6 @@ CatalogId
CatalogIdMapEntry
CatalogIndexState
ChangeVarNodes_context
-ReplaceVarnoContext
CheckPoint
CheckPointStmt
CheckpointStatsData
@@ -2341,6 +2340,7 @@ ReorderBufferUpdateProgressTxnCB
ReorderTuple
RepOriginId
ReparameterizeForeignPathByChild_function
+ReplaceVarnoContext
ReplaceVarsFromTargetList_context
ReplaceVarsNoMatchOption
ReplicaIdentityStmt
@@ -2474,6 +2474,7 @@ SeenRelsEntry
SelectLimit
SelectStmt
Selectivity
+SelfJoinCandidate
SemTPadded
SemiAntiJoinFactors
SeqScan
Attachments:minor-changes-v44.diff (text/plain, 1.7K) downloaddiff --git a/src/backend/optimizer/plan/analyzejoins.c b/src/backend/optimizer/plan/analyzejoins.cindex f0746f35a3..7b8dc7a2b7 100644--- a/src/backend/optimizer/plan/analyzejoins.c+++ b/src/backend/optimizer/plan/analyzejoins.c@@ -1710,8 +1710,6 @@ remove_self_join_rel(PlannerInfo *root, PlanRowMark *kmark, PlanRowMark *rmark,
List *binfo_candidates = NIL;
ReplaceVarnoContext ctx = {.from = toRemove->relid,.to = toKeep->relid};
- Assert(toKeep->relid != -1);-
/*
* Replace index of removing table with the keeping one. The technique of
* removing/distributing restrictinfo is used here to attach just appeared
@@ -2017,8 +2015,6 @@ match_unique_clauses(PlannerInfo *root, RelOptInfo *outer, List *uclauses,
/* Don't consider clauses which aren't similar to 'F(X)=G(Y)' */
continue;
- Assert(is_opclause(orinfo->clause));-
oclause = bms_is_empty(orinfo->left_relids) ?
get_rightop(orinfo->clause) : get_leftop(orinfo->clause);
c2 = (bms_is_empty(orinfo->left_relids) ?
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.listindex 2ff4881fdf..96ebd6eed3 100644--- a/src/tools/pgindent/typedefs.list+++ b/src/tools/pgindent/typedefs.list@@ -367,7 +367,6 @@ CatalogId
CatalogIdMapEntry
CatalogIndexState
ChangeVarNodes_context
-ReplaceVarnoContext
CheckPoint
CheckPointStmt
CheckpointStatsData
@@ -2341,6 +2340,7 @@ ReorderBufferUpdateProgressTxnCB
ReorderTuple
RepOriginId
ReparameterizeForeignPathByChild_function
+ReplaceVarnoContext
ReplaceVarsFromTargetList_context
ReplaceVarsNoMatchOption
ReplicaIdentityStmt
@@ -2474,6 +2474,7 @@ SeenRelsEntry
SelectLimit
SelectStmt
Selectivity
+SelfJoinCandidate
SemTPadded
SemiAntiJoinFactors
SeqScan
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: a.lepikhov@postgrespro.ru, aekorotkov@gmail.com, stark.cfm@gmail.com, michal@kleczek.org, pgsql-hackers@lists.postgresql.org, a.rybakina@postgrespro.ru
Subject: Re: Removing unneeded self joins
In-Reply-To: <9eb86ca9-da3c-417f-8ce1-29c0ef5a4f50@postgrespro.ru>
* 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