From 6bcce92df6dc597e050b7161e5555f3bfd7da841 Mon Sep 17 00:00:00 2001 From: Yuya Watari Date: Thu, 11 Jan 2024 16:18:38 +0900 Subject: [PATCH v23 6/6] Introduce update_clause_relids() for updating RestrictInfo->clause_relids Originally, the update for RestrictInfo->clause_relids was done by directly setting the field. However, this failed to update the corresponding indexes, such as eclass_source_indexes and eclass_derive_indexes. This commit provides a helper function to update the field with fixing the indexes. --- src/backend/optimizer/path/equivclass.c | 120 ++++++++++++++++++++++ src/backend/optimizer/plan/analyzejoins.c | 81 +++++++++------ src/backend/optimizer/util/appendinfo.c | 2 + src/backend/optimizer/util/restrictinfo.c | 5 + src/include/nodes/pathnodes.h | 11 +- src/include/optimizer/paths.h | 2 + 6 files changed, 191 insertions(+), 30 deletions(-) diff --git a/src/backend/optimizer/path/equivclass.c b/src/backend/optimizer/path/equivclass.c index d1019315ea..3f4df1a8c7 100644 --- a/src/backend/optimizer/path/equivclass.c +++ b/src/backend/optimizer/path/equivclass.c @@ -623,6 +623,8 @@ add_eq_source(PlannerInfo *root, EquivalenceClass *ec, RestrictInfo *rinfo) ec->ec_source_indexes = bms_add_member(ec->ec_source_indexes, source_idx); root->eq_sources = lappend(root->eq_sources, rinfo); + Assert(rinfo->eq_sources_index == -1); + rinfo->eq_sources_index = source_idx; i = -1; while ((i = bms_next_member(rinfo->clause_relids, i)) >= 0) @@ -645,6 +647,8 @@ add_eq_derive(PlannerInfo *root, EquivalenceClass *ec, RestrictInfo *rinfo) ec->ec_derive_indexes = bms_add_member(ec->ec_derive_indexes, derive_idx); root->eq_derives = lappend(root->eq_derives, rinfo); + Assert(rinfo->eq_derives_index == -1); + rinfo->eq_derives_index = derive_idx; i = -1; while ((i = bms_next_member(rinfo->clause_relids, i)) >= 0) @@ -656,6 +660,122 @@ add_eq_derive(PlannerInfo *root, EquivalenceClass *ec, RestrictInfo *rinfo) } } +/* + * update_clause_relids + * Update RestrictInfo->clause_relids with adjusting the relevant indexes. + * The clause_relids field must be updated through this function, not by + * setting the field directly. + */ +void +update_clause_relids(PlannerInfo *root, RestrictInfo *rinfo, + Relids new_clause_relids) +{ + int i; + Relids removing_relids; + Relids adding_relids; +#ifdef USE_ASSERT_CHECKING + Relids common_relids; +#endif + + /* + * If there is nothing to do, we return immediately after setting the + * clause_relids field. + */ + if (rinfo->eq_sources_index == -1 && rinfo->eq_derives_index == -1) + { + rinfo->clause_relids = new_clause_relids; + return; + } + + /* + * Remove any references between this RestrictInfo and RangeTblEntries + * that are no longer relevant. + */ + removing_relids = bms_difference(rinfo->clause_relids, new_clause_relids); + i = -1; + while ((i = bms_next_member(removing_relids, i)) >= 0) + { + RangeTblEntry *rte = root->simple_rte_array[i]; + + if (rinfo->eq_sources_index != -1) + { + Assert(bms_is_member(rinfo->eq_sources_index, + rte->eclass_source_indexes)); + rte->eclass_source_indexes = + bms_del_member(rte->eclass_source_indexes, + rinfo->eq_sources_index); + } + if (rinfo->eq_derives_index != -1) + { + Assert(bms_is_member(rinfo->eq_derives_index, + rte->eclass_derive_indexes)); + rte->eclass_derive_indexes = + bms_del_member(rte->eclass_derive_indexes, + rinfo->eq_derives_index); + } + } + bms_free(removing_relids); + + /* + * Add references between this RestrictInfo and RangeTblEntries that will + * be relevant. + */ + adding_relids = bms_difference(new_clause_relids, rinfo->clause_relids); + i = -1; + while ((i = bms_next_member(adding_relids, i)) >= 0) + { + RangeTblEntry *rte = root->simple_rte_array[i]; + + if (rinfo->eq_sources_index != -1) + { + Assert(!bms_is_member(rinfo->eq_sources_index, + rte->eclass_source_indexes)); + rte->eclass_source_indexes = + bms_add_member(rte->eclass_source_indexes, + rinfo->eq_sources_index); + } + if (rinfo->eq_derives_index != -1) + { + Assert(!bms_is_member(rinfo->eq_derives_index, + rte->eclass_derive_indexes)); + rte->eclass_derive_indexes = + bms_add_member(rte->eclass_derive_indexes, + rinfo->eq_derives_index); + } + } + bms_free(adding_relids); + +#ifdef USE_ASSERT_CHECKING + /* + * Verify that all indexes are set for common Relids. + */ + common_relids = bms_intersect(rinfo->clause_relids, new_clause_relids); + i = -1; + while ((i = bms_next_member(common_relids, i)) >= 0) + { + RangeTblEntry *rte = root->simple_rte_array[i]; + + if (rinfo->eq_sources_index != -1) + { + Assert(bms_is_member(rinfo->eq_sources_index, + rte->eclass_source_indexes)); + } + if (rinfo->eq_derives_index != -1) + { + Assert(bms_is_member(rinfo->eq_derives_index, + rte->eclass_derive_indexes)); + } + } + bms_free(common_relids); +#endif + + /* + * Since we have done everything to adjust the indexes, we can set the + * clause_relids field. + */ + rinfo->clause_relids = new_clause_relids; +} + /* * get_eclass_for_sort_expr diff --git a/src/backend/optimizer/plan/analyzejoins.c b/src/backend/optimizer/plan/analyzejoins.c index 4081827e3a..2da2a6c14e 100644 --- a/src/backend/optimizer/plan/analyzejoins.c +++ b/src/backend/optimizer/plan/analyzejoins.c @@ -39,6 +39,7 @@ */ typedef struct { + PlannerInfo *root; int from; int to; } ReplaceVarnoContext; @@ -59,7 +60,8 @@ static bool join_is_removable(PlannerInfo *root, SpecialJoinInfo *sjinfo); static void remove_leftjoinrel_from_query(PlannerInfo *root, int relid, SpecialJoinInfo *sjinfo); -static void remove_rel_from_restrictinfo(RestrictInfo *rinfo, +static void remove_rel_from_restrictinfo(PlannerInfo *root, + RestrictInfo *rinfo, int relid, int ojrelid); static void remove_rel_from_eclass(PlannerInfo *root, EquivalenceClass *ec, int relid, int ojrelid); @@ -76,7 +78,7 @@ static bool is_innerrel_unique_for(PlannerInfo *root, List *restrictlist, List **extra_clauses); static Bitmapset *replace_relid(Relids relids, int oldId, int newId); -static void replace_varno(Node *node, int from, int to); +static void replace_varno(PlannerInfo *root, Node *node, int from, int to); static bool replace_varno_walker(Node *node, ReplaceVarnoContext *ctx); static int self_join_candidates_cmp(const void *a, const void *b); @@ -395,7 +397,7 @@ remove_rel_from_query(PlannerInfo *root, RelOptInfo *rel, } /* Update lateral references. */ - replace_varno((Node *) otherrel->lateral_vars, relid, subst); + replace_varno(root, (Node *) otherrel->lateral_vars, relid, subst); } /* @@ -432,7 +434,7 @@ remove_rel_from_query(PlannerInfo *root, RelOptInfo *rel, sjinf->commute_below_l = replace_relid(sjinf->commute_below_l, ojrelid, subst); sjinf->commute_below_r = replace_relid(sjinf->commute_below_r, ojrelid, subst); - replace_varno((Node *) sjinf->semi_rhs_exprs, relid, subst); + replace_varno(root, (Node *) sjinf->semi_rhs_exprs, relid, subst); } /* @@ -477,7 +479,7 @@ remove_rel_from_query(PlannerInfo *root, RelOptInfo *rel, phv->phrels = replace_relid(phv->phrels, relid, subst); phv->phrels = replace_relid(phv->phrels, ojrelid, subst); Assert(!bms_is_empty(phv->phrels)); - replace_varno((Node *) phv->phexpr, relid, subst); + replace_varno(root, (Node *) phv->phexpr, relid, subst); Assert(phv->phnullingrels == NULL); /* no need to adjust */ } } @@ -551,7 +553,7 @@ remove_leftjoinrel_from_query(PlannerInfo *root, int relid, * that any such PHV is safe (and updated its ph_eval_at), so we * can just drop those references. */ - remove_rel_from_restrictinfo(rinfo, relid, ojrelid); + remove_rel_from_restrictinfo(root, rinfo, relid, ojrelid); /* * Cross-check that the clause itself does not reference the @@ -608,16 +610,24 @@ remove_leftjoinrel_from_query(PlannerInfo *root, int relid, * we have to also clean up the sub-clauses. */ static void -remove_rel_from_restrictinfo(RestrictInfo *rinfo, int relid, int ojrelid) +remove_rel_from_restrictinfo(PlannerInfo *root, RestrictInfo *rinfo, int relid, + int ojrelid) { + Relids new_clause_relids; + + /* + * The 'new_clause_relids' passed to update_clause_relids() must be a + * different instance from the current rinfo->clause_relids, so we make a + * copy of them. + */ + new_clause_relids = bms_copy(rinfo->clause_relids); + new_clause_relids = bms_del_member(new_clause_relids, relid); + new_clause_relids = bms_del_member(new_clause_relids, ojrelid); + update_clause_relids(root, rinfo, new_clause_relids); /* - * The clause_relids probably aren't shared with anything else, but let's + * The required_relids probably aren't shared with anything else, but let's * copy them just to be sure. */ - rinfo->clause_relids = bms_copy(rinfo->clause_relids); - rinfo->clause_relids = bms_del_member(rinfo->clause_relids, relid); - rinfo->clause_relids = bms_del_member(rinfo->clause_relids, ojrelid); - /* Likewise for required_relids */ rinfo->required_relids = bms_copy(rinfo->required_relids); rinfo->required_relids = bms_del_member(rinfo->required_relids, relid); rinfo->required_relids = bms_del_member(rinfo->required_relids, ojrelid); @@ -642,14 +652,14 @@ remove_rel_from_restrictinfo(RestrictInfo *rinfo, int relid, int ojrelid) { RestrictInfo *rinfo2 = lfirst_node(RestrictInfo, lc2); - remove_rel_from_restrictinfo(rinfo2, relid, ojrelid); + remove_rel_from_restrictinfo(root, rinfo2, relid, ojrelid); } } else { RestrictInfo *rinfo2 = castNode(RestrictInfo, orarg); - remove_rel_from_restrictinfo(rinfo2, relid, ojrelid); + remove_rel_from_restrictinfo(root, rinfo2, relid, ojrelid); } } } @@ -708,7 +718,7 @@ remove_rel_from_eclass(PlannerInfo *root, EquivalenceClass *ec, int relid, { RestrictInfo *rinfo = list_nth_node(RestrictInfo, root->eq_sources, i); - remove_rel_from_restrictinfo(rinfo, relid, ojrelid); + remove_rel_from_restrictinfo(root, rinfo, relid, ojrelid); } /* @@ -1448,13 +1458,14 @@ is_innerrel_unique_for(PlannerInfo *root, * Replace each occurrence of removing relid with the keeping one */ static void -replace_varno(Node *node, int from, int to) +replace_varno(PlannerInfo *root, Node *node, int from, int to) { ReplaceVarnoContext ctx; if (to <= 0) return; + ctx.root = root; ctx.from = from; ctx.to = to; replace_varno_walker(node, &ctx); @@ -1498,9 +1509,9 @@ replace_varno_walker(Node *node, ReplaceVarnoContext *ctx) if (bms_is_member(ctx->from, rinfo->clause_relids)) { - replace_varno((Node *) rinfo->clause, ctx->from, ctx->to); - replace_varno((Node *) rinfo->orclause, ctx->from, ctx->to); - rinfo->clause_relids = replace_relid(rinfo->clause_relids, ctx->from, ctx->to); + replace_varno(ctx->root, (Node *) rinfo->clause, ctx->from, ctx->to); + replace_varno(ctx->root, (Node *) rinfo->orclause, ctx->from, ctx->to); + update_clause_relids(ctx->root, rinfo, replace_relid(rinfo->clause_relids, ctx->from, ctx->to)); rinfo->left_relids = replace_relid(rinfo->left_relids, ctx->from, ctx->to); rinfo->right_relids = replace_relid(rinfo->right_relids, ctx->from, ctx->to); } @@ -1613,7 +1624,7 @@ update_eclasses(PlannerInfo *root, EquivalenceClass *ec, int from, int to) em->em_jdomain->jd_relids = replace_relid(em->em_jdomain->jd_relids, from, to); /* We only process inner joins */ - replace_varno((Node *) em->em_expr, from, to); + replace_varno(root, (Node *) em->em_expr, from, to); foreach(lc1, new_members) { @@ -1660,6 +1671,12 @@ update_eclasses(PlannerInfo *root, EquivalenceClass *ec, int from, int to) * the eq_derives indexes. But set a null to detect potential problems. */ list_nth_cell(root->eq_derives, i)->ptr_value = NULL; + + /* + * Since this RestrictInfo no longer exists in root->eq_derives, we + * must reset the stored index. + */ + rinfo->eq_derives_index = -1; } bms_free(ec->ec_derive_indexes); ec->ec_derive_indexes = NULL; @@ -1677,7 +1694,7 @@ update_eclasses(PlannerInfo *root, EquivalenceClass *ec, int from, int to) continue; } - replace_varno((Node *) rinfo, from, to); + replace_varno(root, (Node *) rinfo, from, to); /* * After switching the clause to the remaining relation, check it for @@ -1720,6 +1737,12 @@ update_eclasses(PlannerInfo *root, EquivalenceClass *ec, int from, int to) * the eq_sources indexes. But set a null to detect potential problems. */ list_nth_cell(root->eq_sources, i)->ptr_value = NULL; + + /* + * Since this RestrictInfo no longer exists in root->eq_sources, we + * must reset the stored index. + */ + rinfo->eq_sources_index = -1; } else new_source_indexes = bms_add_member(new_source_indexes, i); @@ -1782,7 +1805,7 @@ remove_self_join_rel(PlannerInfo *root, PlanRowMark *kmark, PlanRowMark *rmark, int i; List *jinfo_candidates = NIL; List *binfo_candidates = NIL; - ReplaceVarnoContext ctx = {.from = toRemove->relid,.to = toKeep->relid}; + ReplaceVarnoContext ctx = {.root = root,.from = toRemove->relid,.to = toKeep->relid}; Assert(toKeep->relid != -1); @@ -1798,7 +1821,7 @@ remove_self_join_rel(PlannerInfo *root, PlanRowMark *kmark, PlanRowMark *rmark, RestrictInfo *rinfo = lfirst_node(RestrictInfo, lc); remove_join_clause_from_rels(root, rinfo, rinfo->required_relids); - replace_varno((Node *) rinfo, toRemove->relid, toKeep->relid); + replace_varno(root, (Node *) rinfo, toRemove->relid, toKeep->relid); if (bms_membership(rinfo->required_relids) == BMS_MULTIPLE) jinfo_candidates = lappend(jinfo_candidates, rinfo); @@ -1818,7 +1841,7 @@ remove_self_join_rel(PlannerInfo *root, PlanRowMark *kmark, PlanRowMark *rmark, { RestrictInfo *rinfo = lfirst_node(RestrictInfo, lc); - replace_varno((Node *) rinfo, toRemove->relid, toKeep->relid); + replace_varno(root, (Node *) rinfo, toRemove->relid, toKeep->relid); if (bms_membership(rinfo->required_relids) == BMS_MULTIPLE) jinfo_candidates = lappend(jinfo_candidates, rinfo); @@ -1918,7 +1941,7 @@ remove_self_join_rel(PlannerInfo *root, PlanRowMark *kmark, PlanRowMark *rmark, { Node *node = lfirst(lc); - replace_varno(node, toRemove->relid, toKeep->relid); + replace_varno(root, node, toRemove->relid, toKeep->relid); if (!list_member(toKeep->reltarget->exprs, node)) toKeep->reltarget->exprs = lappend(toKeep->reltarget->exprs, node); } @@ -1970,9 +1993,9 @@ remove_self_join_rel(PlannerInfo *root, PlanRowMark *kmark, PlanRowMark *rmark, remove_rel_from_query(root, toRemove, toKeep->relid, NULL, NULL); /* At last, replace varno in root targetlist and HAVING clause */ - replace_varno((Node *) root->processed_tlist, + replace_varno(root, (Node *) root->processed_tlist, toRemove->relid, toKeep->relid); - replace_varno((Node *) root->processed_groupClause, + replace_varno(root, (Node *) root->processed_groupClause, toRemove->relid, toKeep->relid); replace_relid(root->all_result_relids, toRemove->relid, toKeep->relid); replace_relid(root->leaf_result_relids, toRemove->relid, toKeep->relid); @@ -2049,7 +2072,7 @@ split_selfjoin_quals(PlannerInfo *root, List *joinquals, List **selfjoinquals, * when we have cast of the same var to different (but compatible) * types. */ - replace_varno(rightexpr, bms_singleton_member(rinfo->right_relids), + replace_varno(root, rightexpr, bms_singleton_member(rinfo->right_relids), bms_singleton_member(rinfo->left_relids)); if (equal(leftexpr, rightexpr)) @@ -2090,7 +2113,7 @@ match_unique_clauses(PlannerInfo *root, RelOptInfo *outer, List *uclauses, bms_is_empty(rinfo->right_relids)); clause = (Expr *) copyObject(rinfo->clause); - replace_varno((Node *) clause, relid, outer->relid); + replace_varno(root, (Node *) clause, relid, outer->relid); iclause = bms_is_empty(rinfo->left_relids) ? get_rightop(clause) : get_leftop(clause); diff --git a/src/backend/optimizer/util/appendinfo.c b/src/backend/optimizer/util/appendinfo.c index 51fdeace7d..27e4b093d7 100644 --- a/src/backend/optimizer/util/appendinfo.c +++ b/src/backend/optimizer/util/appendinfo.c @@ -491,6 +491,8 @@ adjust_appendrel_attrs_mutator(Node *node, newinfo->right_bucketsize = -1; newinfo->left_mcvfreq = -1; newinfo->right_mcvfreq = -1; + newinfo->eq_sources_index = -1; + newinfo->eq_derives_index = -1; return (Node *) newinfo; } diff --git a/src/backend/optimizer/util/restrictinfo.c b/src/backend/optimizer/util/restrictinfo.c index 0b406e9334..59ba503ecb 100644 --- a/src/backend/optimizer/util/restrictinfo.c +++ b/src/backend/optimizer/util/restrictinfo.c @@ -247,6 +247,9 @@ make_restrictinfo_internal(PlannerInfo *root, restrictinfo->left_hasheqoperator = InvalidOid; restrictinfo->right_hasheqoperator = InvalidOid; + restrictinfo->eq_sources_index = -1; + restrictinfo->eq_derives_index = -1; + return restrictinfo; } @@ -403,6 +406,8 @@ commute_restrictinfo(RestrictInfo *rinfo, Oid comm_op) result->right_mcvfreq = rinfo->left_mcvfreq; result->left_hasheqoperator = InvalidOid; result->right_hasheqoperator = InvalidOid; + result->eq_sources_index = -1; + result->eq_derives_index = -1; return result; } diff --git a/src/include/nodes/pathnodes.h b/src/include/nodes/pathnodes.h index 1c5d7bc0c1..b856722533 100644 --- a/src/include/nodes/pathnodes.h +++ b/src/include/nodes/pathnodes.h @@ -2622,7 +2622,12 @@ typedef struct RestrictInfo /* number of base rels in clause_relids */ int num_base_rels pg_node_attr(equal_ignore); - /* The relids (varnos+varnullingrels) actually referenced in the clause: */ + /* + * The relids (varnos+varnullingrels) actually referenced in the clause. + * + * NOTE: This field must be updated through update_clause_relids(), not by + * setting the field directly. + */ Relids clause_relids pg_node_attr(equal_ignore); /* The set of relids required to evaluate the clause: */ @@ -2737,6 +2742,10 @@ typedef struct RestrictInfo /* hash equality operators used for memoize nodes, else InvalidOid */ Oid left_hasheqoperator pg_node_attr(equal_ignore); Oid right_hasheqoperator pg_node_attr(equal_ignore); + + /* the index within root->eq_sources and root->eq_derives */ + int eq_sources_index pg_node_attr(equal_ignore); + int eq_derives_index pg_node_attr(equal_ignore); } RestrictInfo; /* diff --git a/src/include/optimizer/paths.h b/src/include/optimizer/paths.h index d071f6d0fc..c23b177f65 100644 --- a/src/include/optimizer/paths.h +++ b/src/include/optimizer/paths.h @@ -128,6 +128,8 @@ extern bool process_equivalence(PlannerInfo *root, extern Expr *canonicalize_ec_expression(Expr *expr, Oid req_type, Oid req_collation); extern void reconsider_outer_join_clauses(PlannerInfo *root); +extern void update_clause_relids(PlannerInfo *root, RestrictInfo *rinfo, + Relids new_clause_relids); extern EquivalenceClass *get_eclass_for_sort_expr(PlannerInfo *root, Expr *expr, List *opfamilies, -- 2.42.0.windows.2