Thread: Some problems regarding the self-join elimination code
While working on another patch, I looked at ChangeVarNodes() and found
some issues introduced by the self-join elimination commit. I think
it'd better to fix or improve them.
* ChangeVarNodes_walker includes special handling for RestrictInfo
nodes. And it adjusts RestrictInfo.orclause only if the rt_index of
the relation to be removed is contained in RestrictInfo.clause_relids.
I don't think this is correct. Even if the to-be-removed relation is
not present in clause_relids, it can still be found somewhere within
the orclause. As an example, consider:
create table t (a int primary key, b int, c int);
explain (costs off)
select * from t t1
inner join t t2 on t1.a = t2.a
left join t t3 on t1.c = 1 and (t2.b = t3.b or t2.b = 1);
server closed the connection unexpectedly
This query triggers an Assert failure in match_orclause_to_indexcol,
and the root cause is that the removed relation is still present
in the outer_relids of the OR-clauses.
To fix, I think we may need to run ChangeVarNodes_walker on the
orclause in all cases.
* After the adjustment, the num_base_rels is recalculated as:
+ rinfo->num_base_rels = bms_num_members(rinfo->clause_relids);
I don't think this is correct either. num_base_rels is the number of
base rels in clause_relids and should not count outer-join relids.
And we have no guarantee that clause_relids does not include any
outer-join relids.
To fix, I think we should exclude all outer-join relids. Perhaps we
can leverage root->outer_join_rels to achieve this.
* I'm not sure why the self-join elimination commit removed all the
comments in ChangeVarNodes(Extended). I think these comments are very
helpful for understanding the code.
- /*
- * Must be prepared to start with a Query or a bare expression tree; if
- * it's a Query, go straight to query_tree_walker to make sure that
- * sublevels_up doesn't get incremented prematurely.
- */
if (node && IsA(node, Query))
{
Query *qry = (Query *) node;
- /*
- * If we are starting at a Query, and sublevels_up is
zero, then we
- * must also fix rangetable indexes in the Query
itself --- namely
- * resultRelation, mergeTargetRelation, exclRelIndex
and rowMarks
- * entries. sublevels_up cannot be zero when recursing into a
- * subquery, so there's no need to have the same logic inside
- * ChangeVarNodes_walker.
- */
if (sublevels_up == 0)
{
ListCell *l;
@@ -701,7 +772,6 @@ ChangeVarNodes(Node *node, int rt_index, int
new_index, int sublevels_up)
if (qry->mergeTargetRelation == rt_index)
qry->mergeTargetRelation = new_index;
- /* this is unlikely to ever be used, but ... */
* Speaking of comments, I’ve noticed that some changes are lacking
comments. For example, there are almost no comments regarding the
special handling of RestrictInfo nodes in ChangeVarNodes_walker,
except for an explanation about adding the NullTest.
BTW, there's a typo in that explanation: qual() should be equal().
This is not a thorough review of the code; I just randomly looked at
ChangeVarNodes(). It's possible that there are other issues that
haven't been discovered, but I think we should at least fix these.
I'll provide a patch for the fixes later. Apologies for being so
nitpicky.
Thanks
Richard
On Wed, Apr 2, 2025 at 10:26 PM Richard Guo <guofenglinux@gmail.com> wrote: > This is not a thorough review of the code; I just randomly looked at > ChangeVarNodes(). It's possible that there are other issues that > haven't been discovered, but I think we should at least fix these. > I'll provide a patch for the fixes later. Apologies for being so > nitpicky. With more exposure to the changes in ChangeVarNodes(), I have some more concerns. As I understand it, ChangeVarNodes() is designed to update the varno fields of Var nodes in the given tree that belong to the specific relation; neat and clear. However, now, within this function, we also create new NullTest quals for SJE, which doesn't seem like something this function should be handling. It makes this function look a bit like a kludge. Actually, while working on the reduce-NullTest-during-constant-folding patch, I tried to figure out where the new NOT NULL quals from SJE are added. I was a bit surprised to find that they are added in ChangeVarNodes. I didn't expect this function to have this extra responsibility. Additionally, I have a minor suggestion regarding the new boolean parameter, "change_RangeTblRef". AFAIU, by convention, we typically use flags to control the behavior of walker functions, like in pull_var_clause. While it's fine to use a boolean parameter here given that we currently only have one flag, for the sake of future extensibility, I think using flags might be a better choice. Any ideas on how we could improve this function? Apologies again for being nitpicky. Thanks Richard
On 4/2/25 15:26, Richard Guo wrote: > While working on another patch, I looked at ChangeVarNodes() and found > some issues introduced by the self-join elimination commit. I think > it'd better to fix or improve them. > > * ChangeVarNodes_walker includes special handling for RestrictInfo > nodes. And it adjusts RestrictInfo.orclause only if the rt_index of > the relation to be removed is contained in RestrictInfo.clause_relids. > > I don't think this is correct. Even if the to-be-removed relation is > not present in clause_relids, it can still be found somewhere within > the orclause. As an example, consider: Yeah, discovering how we tolerated such a gaffe I found that the comment on the clause_relids field is quite vague here: "The relids (varnos+varnullingrels) actually referenced in the clause:" and only in the RestrictInfo description you can find some additional information. I think we should modify the check, uniting clause_relids and required_relids before searching for the removing relid. > + rinfo->num_base_rels = bms_num_members(rinfo->clause_relids); > > I don't think this is correct either. num_base_rels is the number of > base rels in clause_relids and should not count outer-join relids. > And we have no guarantee that clause_relids does not include any > outer-join relids. It is a clear mistake, no apologies to me. > > To fix, I think we should exclude all outer-join relids. Perhaps we > can leverage root->outer_join_rels to achieve this. I think we may use a difference in size of rinfo->clause_relids before and after adjustion. If something were removed, just decrease num_base_rels. > * Speaking of comments, I’ve noticed that some changes are lacking > comments. For example, there are almost no comments regarding the > special handling of RestrictInfo nodes in ChangeVarNodes_walker, > except for an explanation about adding the NullTest. Ok, it seems that comments were removed too hastily. Not addressed it yet. I also added little code refactoring to make it more readable. See fix.diff in attachment as my proposal for future discussion. -- regards, Andrei Lepikhov
Attachment
On 4/4/25 09:37, Richard Guo wrote: > On Wed, Apr 2, 2025 at 10:26 PM Richard Guo <guofenglinux@gmail.com> wrote: > With more exposure to the changes in ChangeVarNodes(), I have some > more concerns. As I understand it, ChangeVarNodes() is designed to > update the varno fields of Var nodes in the given tree that belong to > the specific relation; neat and clear. However, now, within this > function, we also create new NullTest quals for SJE, which doesn't > seem like something this function should be handling. It makes this > function look a bit like a kludge. To be precise, inside the function we replace old clause with the NullTest, because relid replacement action made it degenerate. It seems essential to me and I think it may be ok to add a comment at the top of ChangeVarNodes, describing this minor optimisation. > Additionally, I have a minor suggestion regarding the new boolean > parameter, "change_RangeTblRef". AFAIU, by convention, we typically > use flags to control the behavior of walker functions, like in > pull_var_clause. While it's fine to use a boolean parameter here > given that we currently only have one flag, for the sake of future > extensibility, I think using flags might be a better choice. That was exactly why I wasn't happy with replacing the change_relid routine with ChangeVarNodes. But variant with a flag seems non-trivial to implement. Do you have any proposal about the code? -- regards, Andrei Lepikhov
On Tue, 8 Apr 2025 at 12:31, Andrei Lepikhov <lepihov@gmail.com> wrote: > > On 4/4/25 09:37, Richard Guo wrote: > > On Wed, Apr 2, 2025 at 10:26 PM Richard Guo <guofenglinux@gmail.com> wrote: > > With more exposure to the changes in ChangeVarNodes(), I have some > > more concerns. As I understand it, ChangeVarNodes() is designed to > > update the varno fields of Var nodes in the given tree that belong to > > the specific relation; neat and clear. However, now, within this > > function, we also create new NullTest quals for SJE, which doesn't > > seem like something this function should be handling. It makes this > > function look a bit like a kludge. > To be precise, inside the function we replace old clause with the > NullTest, because relid replacement action made it degenerate. It seems > essential to me and I think it may be ok to add a comment at the top of > ChangeVarNodes, describing this minor optimisation. > I recall raising the same concerns when I originally reviewed the patch. I don't think this code belongs in the rewriter, because it's very specific to how SJE wants to handle these kinds of nodes. Note also that RestrictInfo is defined in nodes/pathnodes.h, which describes its contents as internal data structures for the planner, suggesting that the rewriter has no business processing those kinds of nodes. I don't think simply adding a comment addresses those concerns. > > Additionally, I have a minor suggestion regarding the new boolean > > parameter, "change_RangeTblRef". AFAIU, by convention, we typically > > use flags to control the behavior of walker functions, like in > > pull_var_clause. While it's fine to use a boolean parameter here > > given that we currently only have one flag, for the sake of future > > extensibility, I think using flags might be a better choice. > That was exactly why I wasn't happy with replacing the change_relid > routine with ChangeVarNodes. > But variant with a flag seems non-trivial to implement. Do you have any > proposal about the code? > Perhaps the way to do it is to make ChangeVarNodesExtended() take a callback function to be invoked for each node visited. The callback (which would then be in the planner, with the other SJE code) would do any special handling it needed (for RangeTblRef and RestrictInfo nodes), and call ChangeVarNodes_walker() for any other types of node, to get the default behaviour. Regards, Dean
On Tue, Apr 8, 2025 at 11:12 PM Dean Rasheed <dean.a.rasheed@gmail.com> wrote: > On Tue, 8 Apr 2025 at 12:31, Andrei Lepikhov <lepihov@gmail.com> wrote: > > On 4/4/25 09:37, Richard Guo wrote: > > > With more exposure to the changes in ChangeVarNodes(), I have some > > > more concerns. As I understand it, ChangeVarNodes() is designed to > > > update the varno fields of Var nodes in the given tree that belong to > > > the specific relation; neat and clear. However, now, within this > > > function, we also create new NullTest quals for SJE, which doesn't > > > seem like something this function should be handling. It makes this > > > function look a bit like a kludge. > > To be precise, inside the function we replace old clause with the > > NullTest, because relid replacement action made it degenerate. It seems > > essential to me and I think it may be ok to add a comment at the top of > > ChangeVarNodes, describing this minor optimisation. > I recall raising the same concerns when I originally reviewed the > patch. I don't think this code belongs in the rewriter, because it's > very specific to how SJE wants to handle these kinds of nodes. Correct. And I don't think it's this function's responsibility to create new NullTest quals for SJE. > Note also that RestrictInfo is defined in nodes/pathnodes.h, which > describes its contents as internal data structures for the planner, > suggesting that the rewriter has no business processing those kinds of > nodes. > I don't think simply adding a comment addresses those concerns. Agreed. We may need more than just a comment change. > > > Additionally, I have a minor suggestion regarding the new boolean > > > parameter, "change_RangeTblRef". AFAIU, by convention, we typically > > > use flags to control the behavior of walker functions, like in > > > pull_var_clause. While it's fine to use a boolean parameter here > > > given that we currently only have one flag, for the sake of future > > > extensibility, I think using flags might be a better choice. > > That was exactly why I wasn't happy with replacing the change_relid > > routine with ChangeVarNodes. > > But variant with a flag seems non-trivial to implement. Do you have any > > proposal about the code? > Perhaps the way to do it is to make ChangeVarNodesExtended() take a > callback function to be invoked for each node visited. The callback > (which would then be in the planner, with the other SJE code) would do > any special handling it needed (for RangeTblRef and RestrictInfo > nodes), and call ChangeVarNodes_walker() for any other types of node, > to get the default behaviour. Yeah, this might be a better approach. Perhaps we can borrow some ideas from replace_rte_variables. Thanks Richard
On 4/9/25 04:05, Richard Guo wrote: > On Tue, Apr 8, 2025 at 11:12 PM Dean Rasheed <dean.a.rasheed@gmail.com> wrote: >> On Tue, 8 Apr 2025 at 12:31, Andrei Lepikhov <lepihov@gmail.com> wrote: >> Perhaps the way to do it is to make ChangeVarNodesExtended() take a >> callback function to be invoked for each node visited. The callback >> (which would then be in the planner, with the other SJE code) would do >> any special handling it needed (for RangeTblRef and RestrictInfo >> nodes), and call ChangeVarNodes_walker() for any other types of node, >> to get the default behaviour. > > Yeah, this might be a better approach. Perhaps we can borrow some > ideas from replace_rte_variables. 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? [1] https://www.postgresql.org/message-id/3622801.1715010885%40sss.pgh.pa.us -- regards, Andrei Lepikhov
On Wed, Apr 9, 2025 at 10:39 AM Andrei Lepikhov <lepihov@gmail.com> wrote: > On 4/9/25 04:05, Richard Guo wrote: > > On Tue, Apr 8, 2025 at 11:12 PM Dean Rasheed <dean.a.rasheed@gmail.com> wrote: > >> On Tue, 8 Apr 2025 at 12:31, Andrei Lepikhov <lepihov@gmail.com> wrote: > >> Perhaps the way to do it is to make ChangeVarNodesExtended() take a > >> callback function to be invoked for each node visited. The callback > >> (which would then be in the planner, with the other SJE code) would do > >> any special handling it needed (for RangeTblRef and RestrictInfo > >> nodes), and call ChangeVarNodes_walker() for any other types of node, > >> to get the default behaviour. > > > > Yeah, this might be a better approach. Perhaps we can borrow some > > ideas from replace_rte_variables. > 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. ------ Regards, Alexander Korotkov Supabase
On 4/10/25 13:36, Alexander Korotkov wrote: > On Wed, Apr 9, 2025 at 10:39 AM Andrei Lepikhov <lepihov@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. -- regards, Andrei Lepikhov
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 <lepihov@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