v23-0006-Introduce-update_clause_relids-for-updating-Rest.patch

From 6bcce92df6dc597e050b7161e5555f3bfd7da841 Mon Sep 17 00:00:00 2001
From: Yuya Watari <watari.yuya@gmail.com>
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