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
diff --git a/src/backend/rewrite/rewriteManip.c b/src/backend/rewrite/rewriteManip.c
index 1c61085a0a7..df96b1bba6a 100644
--- a/src/backend/rewrite/rewriteManip.c
+++ b/src/backend/rewrite/rewriteManip.c
@@ -637,41 +637,58 @@ ChangeVarNodes_walker(Node *node, ChangeVarNodes_context *context)
}
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))
+ 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, rinfo->clause_relids
+ /*bms_union(rinfo->clause_relids, rinfo->required_relids)*/))
{
- expression_tree_walker((Node *) rinfo->clause, ChangeVarNodes_walker, (void *) context);
- expression_tree_walker((Node *) rinfo->orclause, ChangeVarNodes_walker, (void *) context);
+ Relids new_clause_relids;
- rinfo->clause_relids =
- adjust_relid_set(rinfo->clause_relids, context->rt_index, context->new_index);
+ expression_tree_walker((Node *) rinfo->clause,
+ ChangeVarNodes_walker, (void *) context);
+ expression_tree_walker((Node *) rinfo->orclause,
+ ChangeVarNodes_walker, (void *) context);
+
+ 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);
+ 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->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);
+ 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 &&
+ is_multiple &&
relid == context->new_index && IsA(rinfo->clause, OpExpr))
{
Expr *leftOp;
