From f9e07e53c7c7c4088f0e0c3850968be3d1178981 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Wed, 26 Aug 2026 13:10:50 -0400 Subject: [PATCH v6 2/2] De-klugify remove_rels_from_query_tree's removal of OJ relids. To remove nullingrel bits referring to an outer join we're removing, the previous patch did this: ChangeVarNodes((Node *) root->parse, relid, -1, 0); That's at best a cowboy hack. For one thing, it's indistinguishable from other calls that exist in the tree today that replace a given relid with INNER_VAR, intending to update Var.varno fields not nullingrels. For another thing, if there were any Vars still referencing the deleted relation, it would silently change them to invalid INNER_VAR references rather than raising an alarm. Instead, let's tweak ChangeVarNodes' API slightly to have an explicit representation of the case that we're using it to remove nullingrel references: invent a new special varno INVALID_VAR and pass that value not -1. Then within ChangeVarNodes, add Asserts that we don't attempt to assign INVALID_VAR to any field that's an actual varno. In testing this, those Asserts fired on existing regression test queries. That turned out to be because ChangeVarNodes descended into stale subquery trees that were left behind by remove_useless_outer_joins, in the case where it detects that a left join to a subquery RTE can be removed. We don't need or want that to happen, so fix remove_useless_outer_joins to set those RTEs' subquery fields to NULL, as pull_up_simple_subquery has long done. (Thanks to Richard Guo for finding that bit.) Bug: #19560 Reported-by: Orestis Markou Author: Tom Lane Reviewed-by: Richard Guo Discussion: https://postgr.es/m/1186816.1784573544@sss.pgh.pa.us Backpatch-through: 16 --- src/backend/optimizer/plan/analyzejoins.c | 25 ++++++++---- src/backend/rewrite/rewriteManip.c | 49 +++++++++++++++++++++-- src/include/nodes/primnodes.h | 4 ++ 3 files changed, 68 insertions(+), 10 deletions(-) diff --git a/src/backend/optimizer/plan/analyzejoins.c b/src/backend/optimizer/plan/analyzejoins.c index 7c77118c76a..b1bbd2fd6b7 100644 --- a/src/backend/optimizer/plan/analyzejoins.c +++ b/src/backend/optimizer/plan/analyzejoins.c @@ -155,6 +155,17 @@ remove_useless_outer_joins(PlannerInfo *root) removed_relids = bms_add_member(removed_relids, innerrelid); removed_relids = bms_add_member(removed_relids, sjinfo->ojrelid); + /* + * As in pull_up_simple_subquery, discard no-longer-needed subqueries. + * This is not just an optimization, but is necessary to prevent + * subsequent processing from descending into stale subtrees and + * seeing inconsistent data. (Although simple_rte_array[] will be + * rebuilt shortly, we can still use it to access the correct RTE in + * the parse tree.) + */ + if (root->simple_rte_array[innerrelid]->rtekind == RTE_SUBQUERY) + root->simple_rte_array[innerrelid]->subquery = NULL; + /* * It's okay to keep scanning join_info_list for more removable joins, * even though the data that join_is_removable consults is now @@ -413,10 +424,11 @@ remove_join_from_jointree(Node *jtnode, int ojrelid, int *nremoved) * * Having removed some relations and outer joins from the jointree, we must * get rid of any references to them that are left behind elsewhere. There - * should be no ordinary Vars of a removed relation left, but the relids can + * should be no ordinary Vars of a removed relation left, but OJ relids can * still appear in the nullingrels sets of surviving Vars and PlaceHolderVars, - * and in the phrels sets of PlaceHolderVars. ChangeVarNodes knows how to - * strip a relid out of all of those. + * and both regular and OJ relids can appear in the phrels sets of + * PlaceHolderVars. ChangeVarNodes knows how to strip a relid out of all of + * those. */ static void remove_rels_from_query_tree(PlannerInfo *root, Relids removed_relids) @@ -425,8 +437,7 @@ remove_rels_from_query_tree(PlannerInfo *root, Relids removed_relids) while ((relid = bms_next_member(removed_relids, relid)) >= 0) { - /* Pass -1 for new_index to get the removal behavior */ - ChangeVarNodes((Node *) root->parse, relid, -1, 0); + ChangeVarNodes((Node *) root->parse, relid, INVALID_VAR, 0); /* * processed_tlist shares some but not all of its nodes with @@ -434,11 +445,11 @@ remove_rels_from_query_tree(PlannerInfo *root, Relids removed_relids) * harmless: ChangeVarNodes works in-place, and removing a relid that * isn't there is idempotent.) */ - ChangeVarNodes((Node *) root->processed_tlist, relid, -1, 0); + ChangeVarNodes((Node *) root->processed_tlist, relid, INVALID_VAR, 0); /* There could be references in the append_rel_list, too */ if (root->append_rel_list != NIL) - ChangeVarNodes((Node *) root->append_rel_list, relid, -1, 0); + ChangeVarNodes((Node *) root->append_rel_list, relid, INVALID_VAR, 0); } } diff --git a/src/backend/rewrite/rewriteManip.c b/src/backend/rewrite/rewriteManip.c index 6334765ba05..52157d980b5 100644 --- a/src/backend/rewrite/rewriteManip.c +++ b/src/backend/rewrite/rewriteManip.c @@ -529,9 +529,13 @@ OffsetVarNodes(Node *node, int offset, int sublevels_up) * * Find all Var nodes in the given tree belonging to a specific relation * (identified by sublevels_up and rt_index), and change their varno fields - * to 'new_index' (see adjust_relid_set for the exact change behavior). - * The varnosyn fields are changed too. Also adjust other nodes that - * contain rangetable indexes, such as RangeTblRef and JoinExpr. + * to 'new_index', and update varnosyn and varnullingrels fields similarly. + * Also adjust other nodes that contain rangetable indexes, such as + * RangeTblRef and JoinExpr. + * + * Also, new_index can be INVALID_VAR to indicate that we are deleting the + * given relid from the tree. In this case we should only find rt_index + * in nullingrels sets, never in any varno field. * * NOTE: although this has the form of a walker, we cheat and modify the * nodes in-place. The given expression tree should have been copied @@ -557,12 +561,18 @@ ChangeVarNodes_walker(Node *node, ChangeVarNodes_context *context) if (var->varlevelsup == context->sublevels_up) { if (var->varno == context->rt_index) + { + Assert(context->new_index != INVALID_VAR); var->varno = context->new_index; + } var->varnullingrels = adjust_relid_set(var->varnullingrels, context->rt_index, context->new_index); if (var->varnosyn == context->rt_index) + { + Assert(context->new_index != INVALID_VAR); var->varnosyn = context->new_index; + } } return false; } @@ -572,7 +582,10 @@ ChangeVarNodes_walker(Node *node, ChangeVarNodes_context *context) if (context->sublevels_up == 0 && cexpr->cvarno == context->rt_index) + { + Assert(context->new_index != INVALID_VAR); cexpr->cvarno = context->new_index; + } return false; } if (IsA(node, RangeTblRef)) @@ -581,7 +594,10 @@ ChangeVarNodes_walker(Node *node, ChangeVarNodes_context *context) if (context->sublevels_up == 0 && rtr->rtindex == context->rt_index) + { + Assert(context->new_index != INVALID_VAR); rtr->rtindex = context->new_index; + } /* the subquery itself is visited separately */ return false; } @@ -591,7 +607,10 @@ ChangeVarNodes_walker(Node *node, ChangeVarNodes_context *context) if (context->sublevels_up == 0 && j->rtindex == context->rt_index) + { + Assert(context->new_index != INVALID_VAR); j->rtindex = context->new_index; + } /* fall through to examine children */ } if (IsA(node, PlaceHolderVar)) @@ -616,9 +635,15 @@ ChangeVarNodes_walker(Node *node, ChangeVarNodes_context *context) if (context->sublevels_up == 0) { if (rowmark->rti == context->rt_index) + { + Assert(context->new_index != INVALID_VAR); rowmark->rti = context->new_index; + } if (rowmark->prti == context->rt_index) + { + Assert(context->new_index != INVALID_VAR); rowmark->prti = context->new_index; + } } return false; } @@ -629,9 +654,15 @@ ChangeVarNodes_walker(Node *node, ChangeVarNodes_context *context) if (context->sublevels_up == 0) { if (appinfo->parent_relid == context->rt_index) + { + Assert(context->new_index != INVALID_VAR); appinfo->parent_relid = context->new_index; + } if (appinfo->child_relid == context->rt_index) + { + Assert(context->new_index != INVALID_VAR); appinfo->child_relid = context->new_index; + } } /* fall through to examine children */ } @@ -685,21 +716,33 @@ ChangeVarNodes(Node *node, int rt_index, int new_index, int sublevels_up) ListCell *l; if (qry->resultRelation == rt_index) + { + Assert(new_index != INVALID_VAR); qry->resultRelation = new_index; + } if (qry->mergeTargetRelation == rt_index) + { + Assert(new_index != INVALID_VAR); qry->mergeTargetRelation = new_index; + } /* this is unlikely to ever be used, but ... */ if (qry->onConflict && qry->onConflict->exclRelIndex == rt_index) + { + Assert(new_index != INVALID_VAR); qry->onConflict->exclRelIndex = new_index; + } foreach(l, qry->rowMarks) { RowMarkClause *rc = (RowMarkClause *) lfirst(l); if (rc->rti == rt_index) + { + Assert(new_index != INVALID_VAR); rc->rti = new_index; + } } } query_tree_walker(qry, ChangeVarNodes_walker, &context, 0); diff --git a/src/include/nodes/primnodes.h b/src/include/nodes/primnodes.h index 44f828cbb37..5a636d1f179 100644 --- a/src/include/nodes/primnodes.h +++ b/src/include/nodes/primnodes.h @@ -217,6 +217,9 @@ typedef struct Expr * row identity information during UPDATE/DELETE/MERGE. This value should * never be seen outside the planner. * + * INVALID_VAR should never appear as anything's varno. We use it in a + * few APIs to denote removal of an RTE. + * * varnullingrels is the set of RT indexes of outer joins that can force * the Var's value to null (at the point where it appears in the query). * See optimizer/README for discussion of that. @@ -244,6 +247,7 @@ typedef struct Expr #define OUTER_VAR (-2) /* reference to outer subplan */ #define INDEX_VAR (-3) /* reference to index column */ #define ROWID_VAR (-4) /* row identity column during planning */ +#define INVALID_VAR (-5) /* this is not a valid varno! */ #define IS_SPECIAL_VARNO(varno) ((int) (varno) < 0) -- 2.52.0