On 4/10/25 14:39, Andrei Lepikhov wrote:
On 4/10/25 13:36, Alexander Korotkov wrote:
On Wed, Apr 9, 2025 at 10:39 AM Andrei Lepikhov <lepi...@gmail.com>
wrote:
It seems we are coming to the conclusion that join removal optimisation
may do something out of ChangeVarNodes resposibility. Before further
complicating of this function code I would like to know opinion of Tom,
who initially proposed [1] to use this routine. May be better a) return
to more specialised change_relid / sje_walker machinery or b) move
ChangeVarNodes out of rewriteManip and make it multi-purpose routine,
allowing to transform expression that may happen after a Var node
change?
What about adding a callback to ChangeVarNodes_context that would
called for each RestrictInfo after changing varnodes itself? SJE
could use a callback that replaces OpExpr with NullTest when needed.
I think it is doable, of course. Just looking forward a little, it may
need more complication in the future (SJE definitely should be widened
to partitioned tables) and it may be simpler to have two different
routines for two different stages of planning.
To provide some food for thought, here is a draft in attachment which
addresses both issues: RestrictInfo relid replacement and move
SJE-specific code out of the ChangeVarNodes routine (callback approach).
--
regards, Andrei Lepikhov
From 6b68703d7b38326393da58e42618e4915a0e4590 Mon Sep 17 00:00:00 2001
From: "Andrei V. Lepikhov" <lepi...@gmail.com>
Date: Fri, 11 Apr 2025 14:30:33 +0200
Subject: [PATCH v0] Switch the approach to ChangeVarNodes's extensibility.
---
src/backend/optimizer/plan/analyzejoins.c | 149 +++++++++++++++++++---
src/backend/rewrite/rewriteManip.c | 95 ++------------
src/include/rewrite/rewriteManip.h | 14 +-
3 files changed, 161 insertions(+), 97 deletions(-)
diff --git a/src/backend/optimizer/plan/analyzejoins.c b/src/backend/optimizer/plan/analyzejoins.c
index 6b58567f511..0f0ed1785e6 100644
--- a/src/backend/optimizer/plan/analyzejoins.c
+++ b/src/backend/optimizer/plan/analyzejoins.c
@@ -74,6 +74,7 @@ static bool is_innerrel_unique_for(PlannerInfo *root,
List *restrictlist,
List **extra_clauses);
static int self_join_candidates_cmp(const void *a, const void *b);
+static bool ChangeVarNodes_callback(Node *node, void *arg);
/*
@@ -397,7 +398,8 @@ remove_rel_from_query(PlannerInfo *root, RelOptInfo *rel,
{
Assert(subst > 0);
- ChangeVarNodes((Node *) sjinf->semi_rhs_exprs, relid, subst, 0);
+ ChangeVarNodesExtended((Node *) sjinf->semi_rhs_exprs, relid, subst,
+ 0, ChangeVarNodes_callback);
}
}
@@ -458,7 +460,8 @@ remove_rel_from_query(PlannerInfo *root, RelOptInfo *rel,
sjinfo->ojrelid, subst);
Assert(!bms_is_empty(phv->phrels));
- ChangeVarNodes((Node *) phv->phexpr, relid, subst, 0);
+ ChangeVarNodesExtended((Node *) phv->phexpr, relid, subst, 0,
+ ChangeVarNodes_callback);
Assert(phv->phnullingrels == NULL); /* no need to adjust */
}
@@ -512,7 +515,8 @@ remove_rel_from_query(PlannerInfo *root, RelOptInfo *rel,
}
if (subst > 0)
- ChangeVarNodes((Node *) otherrel->lateral_vars, relid, subst, 0);
+ ChangeVarNodesExtended((Node *) otherrel->lateral_vars, relid,
+ subst, 0, ChangeVarNodes_callback);
}
}
@@ -746,7 +750,8 @@ remove_rel_from_eclass(EquivalenceClass *ec, SpecialJoinInfo *sjinfo,
RestrictInfo *rinfo = (RestrictInfo *) lfirst(lc);
if (sjinfo == NULL)
- ChangeVarNodes((Node *) rinfo, relid, subst, 0);
+ ChangeVarNodesExtended((Node *) rinfo, relid, subst, 0,
+ ChangeVarNodes_callback);
else
remove_rel_from_restrictinfo(rinfo, relid, sjinfo->ojrelid);
}
@@ -1537,7 +1542,8 @@ update_eclasses(EquivalenceClass *ec, int from, int to)
em->em_jdomain->jd_relids = adjust_relid_set(em->em_jdomain->jd_relids, from, to);
/* We only process inner joins */
- ChangeVarNodes((Node *) em->em_expr, from, to, 0);
+ ChangeVarNodesExtended((Node *) em->em_expr, from, to, 0,
+ ChangeVarNodes_callback);
foreach_node(EquivalenceMember, other, new_members)
{
@@ -1571,7 +1577,8 @@ update_eclasses(EquivalenceClass *ec, int from, int to)
continue;
}
- ChangeVarNodes((Node *) rinfo, from, to, 0);
+ ChangeVarNodesExtended((Node *) rinfo, from, to, 0,
+ ChangeVarNodes_callback);
/*
* After switching the clause to the remaining relation, check it for
@@ -1666,6 +1673,108 @@ add_non_redundant_clauses(PlannerInfo *root,
}
}
+static bool
+ChangeVarNodes_callback(Node *node, void *arg)
+{
+ ChangeVarNodes_context *context = (ChangeVarNodes_context *) arg;
+
+ if (node == NULL)
+ return false;
+
+ if (IsA(node, RangeTblRef))
+ {
+ return false;
+ }
+ else if (IsA(node, RestrictInfo))
+ {
+ RestrictInfo *rinfo = (RestrictInfo *) node;
+ int relid = -1;
+ bool is_req_equal =
+ (rinfo->required_relids == rinfo->clause_relids);
+ bool is_multiple =
+ (bms_membership(rinfo->clause_relids) == BMS_MULTIPLE);
+
+ if (bms_is_member(context->rt_index,
+ bms_union(rinfo->clause_relids, rinfo->required_relids)))
+ {
+ Relids new_clause_relids;
+
+ expression_tree_walker((Node *) rinfo->clause,
+ ChangeVarNodes_callback, arg);
+ expression_tree_walker((Node *) rinfo->orclause,
+ ChangeVarNodes_callback, arg);
+
+ new_clause_relids = adjust_relid_set(rinfo->clause_relids,
+ context->rt_index,
+ context->new_index);
+
+ /* This operation is legal until we remove only baserels */
+ rinfo->num_base_rels -= bms_num_members(rinfo->clause_relids) -
+ bms_num_members(new_clause_relids);
+
+ rinfo->clause_relids = new_clause_relids;
+ rinfo->num_base_rels = bms_num_members(rinfo->clause_relids);
+ rinfo->left_relids = adjust_relid_set(rinfo->left_relids,
+ context->rt_index,
+ context->new_index);
+ rinfo->right_relids = adjust_relid_set(rinfo->right_relids,
+ context->rt_index,
+ context->new_index);
+ }
+
+ if (is_req_equal)
+ rinfo->required_relids = rinfo->clause_relids;
+ else
+ rinfo->required_relids = adjust_relid_set(rinfo->required_relids,
+ context->rt_index,
+ context->new_index);
+
+ rinfo->outer_relids = adjust_relid_set(rinfo->outer_relids,
+ context->rt_index,
+ context->new_index);
+ rinfo->incompatible_relids = adjust_relid_set(rinfo->incompatible_relids,
+ context->rt_index,
+ context->new_index);
+
+ if (rinfo->mergeopfamilies &&
+ bms_get_singleton_member(rinfo->clause_relids, &relid) &&
+ is_multiple &&
+ relid == context->new_index && IsA(rinfo->clause, OpExpr))
+ {
+ Expr *leftOp;
+ Expr *rightOp;
+
+ leftOp = (Expr *) get_leftop(rinfo->clause);
+ rightOp = (Expr *) get_rightop(rinfo->clause);
+
+ /*
+ * For self-join elimination, changing varnos could transform
+ * "t1.a = t2.a" into "t1.a = t1.a". That is always true as long
+ * as "t1.a" is not null. We use qual() to check for such a case,
+ * and then we replace the qual for a check for not null
+ * (NullTest).
+ */
+ if (leftOp != NULL && equal(leftOp, rightOp))
+ {
+ NullTest *ntest = makeNode(NullTest);
+
+ ntest->arg = leftOp;
+ ntest->nulltesttype = IS_NOT_NULL;
+ ntest->argisrow = false;
+ ntest->location = -1;
+ rinfo->clause = (Expr *) ntest;
+ rinfo->mergeopfamilies = NIL;
+ rinfo->left_em = NULL;
+ rinfo->right_em = NULL;
+ }
+ Assert(rinfo->orclause == NULL);
+ }
+ return false;
+ }
+ else
+ return standard_ChangeVarNodes_walker(node, context);
+}
+
/*
* Remove a relation after we have proven that it participates only in an
* unneeded unique self-join.
@@ -1714,7 +1823,8 @@ remove_self_join_rel(PlannerInfo *root, PlanRowMark *kmark, PlanRowMark *rmark,
foreach_node(RestrictInfo, rinfo, joininfos)
{
remove_join_clause_from_rels(root, rinfo, rinfo->required_relids);
- ChangeVarNodes((Node *) rinfo, toRemove->relid, toKeep->relid, 0);
+ ChangeVarNodesExtended((Node *) rinfo, toRemove->relid, toKeep->relid,
+ 0, ChangeVarNodes_callback);
if (bms_membership(rinfo->required_relids) == BMS_MULTIPLE)
jinfo_candidates = lappend(jinfo_candidates, rinfo);
@@ -1732,7 +1842,8 @@ remove_self_join_rel(PlannerInfo *root, PlanRowMark *kmark, PlanRowMark *rmark,
restrictlist);
foreach_node(RestrictInfo, rinfo, toRemove->baserestrictinfo)
{
- ChangeVarNodes((Node *) rinfo, toRemove->relid, toKeep->relid, 0);
+ ChangeVarNodesExtended((Node *) rinfo, toRemove->relid, toKeep->relid,
+ 0, ChangeVarNodes_callback);
if (bms_membership(rinfo->required_relids) == BMS_MULTIPLE)
jinfo_candidates = lappend(jinfo_candidates, rinfo);
@@ -1774,7 +1885,8 @@ remove_self_join_rel(PlannerInfo *root, PlanRowMark *kmark, PlanRowMark *rmark,
{
Node *node = lfirst(lc);
- ChangeVarNodes(node, toRemove->relid, toKeep->relid, 0);
+ ChangeVarNodesExtended(node, toRemove->relid, toKeep->relid, 0,
+ ChangeVarNodes_callback);
if (!list_member(toKeep->reltarget->exprs, node))
toKeep->reltarget->exprs = lappend(toKeep->reltarget->exprs, node);
}
@@ -1818,14 +1930,18 @@ remove_self_join_rel(PlannerInfo *root, PlanRowMark *kmark, PlanRowMark *rmark,
* Replace varno in all the query structures, except nodes RangeTblRef
* otherwise later remove_rel_from_joinlist will yield errors.
*/
- ChangeVarNodesExtended((Node *) root->parse, toRemove->relid, toKeep->relid, 0, false);
+ ChangeVarNodesExtended((Node *) root->parse, toRemove->relid, toKeep->relid,
+ 0, ChangeVarNodes_callback);
/* Replace links in the planner info */
remove_rel_from_query(root, toRemove, toKeep->relid, NULL, NULL);
/* At last, replace varno in root targetlist and HAVING clause */
- ChangeVarNodes((Node *) root->processed_tlist, toRemove->relid, toKeep->relid, 0);
- ChangeVarNodes((Node *) root->processed_groupClause, toRemove->relid, toKeep->relid, 0);
+ ChangeVarNodesExtended((Node *) root->processed_tlist, toRemove->relid,
+ toKeep->relid, 0, ChangeVarNodes_callback);
+ ChangeVarNodesExtended((Node *) root->processed_groupClause,
+ toRemove->relid, toKeep->relid, 0,
+ ChangeVarNodes_callback);
adjust_relid_set(root->all_result_relids, toRemove->relid, toKeep->relid);
adjust_relid_set(root->leaf_result_relids, toRemove->relid, toKeep->relid);
@@ -1908,8 +2024,10 @@ split_selfjoin_quals(PlannerInfo *root, List *joinquals, List **selfjoinquals,
* when we have cast of the same var to different (but compatible)
* types.
*/
- ChangeVarNodes(rightexpr, bms_singleton_member(rinfo->right_relids),
- bms_singleton_member(rinfo->left_relids), 0);
+ ChangeVarNodesExtended(rightexpr,
+ bms_singleton_member(rinfo->right_relids),
+ bms_singleton_member(rinfo->left_relids), 0,
+ ChangeVarNodes_callback);
if (equal(leftexpr, rightexpr))
sjoinquals = lappend(sjoinquals, rinfo);
@@ -1945,7 +2063,8 @@ match_unique_clauses(PlannerInfo *root, RelOptInfo *outer, List *uclauses,
bms_is_empty(rinfo->right_relids));
clause = (Expr *) copyObject(rinfo->clause);
- ChangeVarNodes((Node *) clause, relid, outer->relid, 0);
+ ChangeVarNodesExtended((Node *) clause, relid, outer->relid, 0,
+ ChangeVarNodes_callback);
iclause = bms_is_empty(rinfo->left_relids) ? get_rightop(clause) :
get_leftop(clause);
diff --git a/src/backend/rewrite/rewriteManip.c b/src/backend/rewrite/rewriteManip.c
index 1c61085a0a7..790cd738f9e 100644
--- a/src/backend/rewrite/rewriteManip.c
+++ b/src/backend/rewrite/rewriteManip.c
@@ -550,16 +550,18 @@ offset_relid_set(Relids relids, int offset)
* earlier to ensure that no unwanted side-effects occur!
*/
-typedef struct
-{
- int rt_index;
- int new_index;
- int sublevels_up;
- bool change_RangeTblRef;
-} ChangeVarNodes_context;
-
static bool
ChangeVarNodes_walker(Node *node, ChangeVarNodes_context *context)
+{
+ /* if caller has defined a callback it may need to alter default behaviour */
+ if (context->callback)
+ return context->callback(node, (void *) context);
+ else
+ return standard_ChangeVarNodes_walker(node, context);
+}
+
+bool
+standard_ChangeVarNodes_walker(Node *node, ChangeVarNodes_context *context)
{
if (node == NULL)
return false;
@@ -588,7 +590,7 @@ ChangeVarNodes_walker(Node *node, ChangeVarNodes_context *context)
cexpr->cvarno = context->new_index;
return false;
}
- if (IsA(node, RangeTblRef) && context->change_RangeTblRef)
+ if (IsA(node, RangeTblRef))
{
RangeTblRef *rtr = (RangeTblRef *) node;
@@ -635,75 +637,6 @@ ChangeVarNodes_walker(Node *node, ChangeVarNodes_context *context)
}
return false;
}
- if (IsA(node, RestrictInfo))
- {
- RestrictInfo *rinfo = (RestrictInfo *) node;
- int relid = -1;
- bool is_req_equal =
- (rinfo->required_relids == rinfo->clause_relids);
- bool clause_relids_is_multiple =
- (bms_membership(rinfo->clause_relids) == BMS_MULTIPLE);
-
- if (bms_is_member(context->rt_index, rinfo->clause_relids))
- {
- expression_tree_walker((Node *) rinfo->clause, ChangeVarNodes_walker, (void *) context);
- expression_tree_walker((Node *) rinfo->orclause, ChangeVarNodes_walker, (void *) context);
-
- rinfo->clause_relids =
- adjust_relid_set(rinfo->clause_relids, context->rt_index, context->new_index);
- rinfo->num_base_rels = bms_num_members(rinfo->clause_relids);
- rinfo->left_relids =
- adjust_relid_set(rinfo->left_relids, context->rt_index, context->new_index);
- rinfo->right_relids =
- adjust_relid_set(rinfo->right_relids, context->rt_index, context->new_index);
- }
-
- if (is_req_equal)
- rinfo->required_relids = rinfo->clause_relids;
- else
- rinfo->required_relids =
- adjust_relid_set(rinfo->required_relids, context->rt_index, context->new_index);
-
- rinfo->outer_relids =
- adjust_relid_set(rinfo->outer_relids, context->rt_index, context->new_index);
- rinfo->incompatible_relids =
- adjust_relid_set(rinfo->incompatible_relids, context->rt_index, context->new_index);
-
- if (rinfo->mergeopfamilies &&
- bms_get_singleton_member(rinfo->clause_relids, &relid) &&
- clause_relids_is_multiple &&
- relid == context->new_index && IsA(rinfo->clause, OpExpr))
- {
- Expr *leftOp;
- Expr *rightOp;
-
- leftOp = (Expr *) get_leftop(rinfo->clause);
- rightOp = (Expr *) get_rightop(rinfo->clause);
-
- /*
- * For self-join elimination, changing varnos could transform
- * "t1.a = t2.a" into "t1.a = t1.a". That is always true as long
- * as "t1.a" is not null. We use qual() to check for such a case,
- * and then we replace the qual for a check for not null
- * (NullTest).
- */
- if (leftOp != NULL && equal(leftOp, rightOp))
- {
- NullTest *ntest = makeNode(NullTest);
-
- ntest->arg = leftOp;
- ntest->nulltesttype = IS_NOT_NULL;
- ntest->argisrow = false;
- ntest->location = -1;
- rinfo->clause = (Expr *) ntest;
- rinfo->mergeopfamilies = NIL;
- rinfo->left_em = NULL;
- rinfo->right_em = NULL;
- }
- Assert(rinfo->orclause == NULL);
- }
- return false;
- }
if (IsA(node, AppendRelInfo))
{
AppendRelInfo *appinfo = (AppendRelInfo *) node;
@@ -749,14 +682,14 @@ ChangeVarNodes_walker(Node *node, ChangeVarNodes_context *context)
*/
void
ChangeVarNodesExtended(Node *node, int rt_index, int new_index,
- int sublevels_up, bool change_RangeTblRef)
+ int sublevels_up, ChangeVarNodes_callback_type callback)
{
ChangeVarNodes_context context;
context.rt_index = rt_index;
context.new_index = new_index;
context.sublevels_up = sublevels_up;
- context.change_RangeTblRef = change_RangeTblRef;
+ context.callback = callback;
if (node && IsA(node, Query))
{
@@ -792,7 +725,7 @@ ChangeVarNodesExtended(Node *node, int rt_index, int new_index,
void
ChangeVarNodes(Node *node, int rt_index, int new_index, int sublevels_up)
{
- ChangeVarNodesExtended(node, rt_index, new_index, sublevels_up, true);
+ ChangeVarNodesExtended(node, rt_index, new_index, sublevels_up, NULL);
}
/*
diff --git a/src/include/rewrite/rewriteManip.h b/src/include/rewrite/rewriteManip.h
index ea3908739c6..6d46274f1ee 100644
--- a/src/include/rewrite/rewriteManip.h
+++ b/src/include/rewrite/rewriteManip.h
@@ -41,7 +41,18 @@ typedef enum ReplaceVarsNoMatchOption
REPLACEVARS_SUBSTITUTE_NULL, /* replace with a NULL Const */
} ReplaceVarsNoMatchOption;
+typedef bool (*ChangeVarNodes_callback_type) (Node *node, void *arg);
+typedef struct
+{
+ int rt_index;
+ int new_index;
+ int sublevels_up;
+ ChangeVarNodes_callback_type callback;
+} ChangeVarNodes_context;
+
+extern bool standard_ChangeVarNodes_walker(Node *node,
+ ChangeVarNodes_context *context);
extern Relids adjust_relid_set(Relids relids, int oldrelid, int newrelid);
extern void CombineRangeTables(List **dst_rtable, List **dst_perminfos,
List *src_rtable, List *src_perminfos);
@@ -49,7 +60,8 @@ extern void OffsetVarNodes(Node *node, int offset, int sublevels_up);
extern void ChangeVarNodes(Node *node, int rt_index, int new_index,
int sublevels_up);
extern void ChangeVarNodesExtended(Node *node, int rt_index, int new_index,
- int sublevels_up, bool change_RangeTblRef);
+ int sublevels_up,
+ ChangeVarNodes_callback_type callback);
extern void IncrementVarSublevelsUp(Node *node, int delta_sublevels_up,
int min_sublevels_up);
extern void IncrementVarSublevelsUp_rtable(List *rtable,
--
2.39.5
