v1-0002-Some-fixes.patch

From 9d8c408b3379faab6269421f67d75530f1c4cde2 Mon Sep 17 00:00:00 2001
From: Amit Langote <amitlan@postgresql.org>
Date: Fri, 22 Mar 2024 16:51:48 +0900
Subject: [PATCH v1 2/2] Some fixes

* Revert (IMO) unnecessary modifications of build_child_join_sjinfo().
For example, hunks to rename sjinfo to child_sjinfo seem unnecessary,
because it's clear from the context that they are "child" sjinfos.

* Rename free_child_sjinfo_members() to free_child_join_sjinfo() to
be symmetric with build_child_join_sjinfo().  Note that the function
is charged with freeing the sjinfo itself too.  Like
build_child_join_sjinfo(), name the argument sjinfo, not
child_sjinfo.

* Don't set the child sjinfo pointer to NULL after freeing it.  While
a good convention in general, it's not really common in the planner
code, so doing it here seems a bit overkill

* Rename make_dummy_sjinfo() to init_dummy_sjinfo() because the
function is not really making it per se, only initializing fields
in the SpecialJoinInfo struct made available by the caller.

* Make init_dummy_sjinfo() take Relids instead of RelOptInfos because
that's what's needed.  Also allows to reduce changes to
build_child_join_sjinfo()'s signature.

* Update the reason mentioned in a comment in free_child_join_sjinfo()
about why semi_rhs_expr is not freed -- it may be freed, but there's
no way to "deep" free it, so we leave it alone.

* Update a comment somewhere about why SpecialJoinInfo can be
"transient" sometimes.
---
 src/backend/optimizer/path/costsize.c |   6 +-
 src/backend/optimizer/path/joinrels.c | 125 +++++++++++++-------------
 src/backend/optimizer/util/pathnode.c |   5 +-
 src/include/optimizer/paths.h         |   4 +-
 4 files changed, 70 insertions(+), 70 deletions(-)

diff --git a/src/backend/optimizer/path/costsize.c b/src/backend/optimizer/path/costsize.c
index 1b76523e79..ee23ed7835 100644
--- a/src/backend/optimizer/path/costsize.c
+++ b/src/backend/optimizer/path/costsize.c
@@ -5050,7 +5050,7 @@ compute_semi_anti_join_factors(PlannerInfo *root,
 	/*
 	 * Also get the normal inner-join selectivity of the join clauses.
 	 */
-	make_dummy_sjinfo(&norm_sjinfo, outerrel, innerrel);
+	init_dummy_sjinfo(&norm_sjinfo, outerrel->relids, innerrel->relids);
 
 	nselec = clauselist_selectivity(root,
 									joinquals,
@@ -5203,8 +5203,8 @@ approx_tuple_count(PlannerInfo *root, JoinPath *path, List *quals)
 	/*
 	 * Make up a SpecialJoinInfo for JOIN_INNER semantics.
 	 */
-	make_dummy_sjinfo(&sjinfo, path->outerjoinpath->parent,
-					  path->innerjoinpath->parent);
+	init_dummy_sjinfo(&sjinfo, path->outerjoinpath->parent->relids,
+					  path->innerjoinpath->parent->relids);
 
 	/* Get the approximate selectivity */
 	foreach(l, quals)
diff --git a/src/backend/optimizer/path/joinrels.c b/src/backend/optimizer/path/joinrels.c
index 465dcf7d5d..bef5b5f213 100644
--- a/src/backend/optimizer/path/joinrels.c
+++ b/src/backend/optimizer/path/joinrels.c
@@ -44,9 +44,8 @@ static void try_partitionwise_join(PlannerInfo *root, RelOptInfo *rel1,
 								   List *parent_restrictlist);
 static SpecialJoinInfo *build_child_join_sjinfo(PlannerInfo *root,
 												SpecialJoinInfo *parent_sjinfo,
-												RelOptInfo *left_child,
-												RelOptInfo *right_child);
-static void free_child_sjinfo_members(SpecialJoinInfo **child_sjinfo);
+												Relids left_relids, Relids right_relids);
+static void free_child_join_sjinfo(SpecialJoinInfo *child_sjinfo);
 static void compute_partition_bounds(PlannerInfo *root, RelOptInfo *rel1,
 									 RelOptInfo *rel2, RelOptInfo *joinrel,
 									 SpecialJoinInfo *parent_sjinfo,
@@ -656,18 +655,24 @@ join_is_legal(PlannerInfo *root, RelOptInfo *rel1, RelOptInfo *rel2,
 }
 
 /*
- * make_dummy_sjinfo
- *    Populate the given SpecialJoinInfo for a plain inner join, between rel1
- *    and rel2, which does not have a SpecialJoinInfo associated with it.
+ * init_dummy_sjinfo
+ *    Populate the given SpecialJoinInfo for a plain inner join between rel1
+ *    and rel2
+ *
+ * Inner joins do not normally have an associated SpecialJoinInfo node.  But
+ * some functions involved in join planning require one to be passed,
+ * containing at least the information of which relations are being joined,
+ * so we initialize that information here.
  */
 void
-make_dummy_sjinfo(SpecialJoinInfo *sjinfo, RelOptInfo *rel1, RelOptInfo *rel2)
+init_dummy_sjinfo(SpecialJoinInfo *sjinfo, Relids left_relids,
+				  Relids right_relids)
 {
 	sjinfo->type = T_SpecialJoinInfo;
-	sjinfo->min_lefthand = rel1->relids;
-	sjinfo->min_righthand = rel2->relids;
-	sjinfo->syn_lefthand = rel1->relids;
-	sjinfo->syn_righthand = rel2->relids;
+	sjinfo->min_lefthand = left_relids;
+	sjinfo->min_righthand = right_relids;
+	sjinfo->syn_lefthand = left_relids;
+	sjinfo->syn_righthand = right_relids;
 	sjinfo->jointype = JOIN_INNER;
 	sjinfo->ojrelid = 0;
 	sjinfo->commute_above_l = NULL;
@@ -744,7 +749,7 @@ make_join_rel(PlannerInfo *root, RelOptInfo *rel1, RelOptInfo *rel2)
 	if (sjinfo == NULL)
 	{
 		sjinfo = &sjinfo_data;
-		make_dummy_sjinfo(sjinfo, rel1, rel2);
+		init_dummy_sjinfo(sjinfo, rel1->relids, rel2->relids);
 	}
 
 	/*
@@ -1628,8 +1633,9 @@ try_partitionwise_join(PlannerInfo *root, RelOptInfo *rel1, RelOptInfo *rel2,
 		 * Construct SpecialJoinInfo from parent join relations's
 		 * SpecialJoinInfo.
 		 */
-		child_sjinfo = build_child_join_sjinfo(root, parent_sjinfo, child_rel1,
-											   child_rel2);
+		child_sjinfo = build_child_join_sjinfo(root, parent_sjinfo,
+											   child_rel1->relids,
+											   child_rel2->relids);
 
 		/* Find the AppendRelInfo structures */
 		appinfos = find_appinfos_by_relids(root,
@@ -1670,7 +1676,7 @@ try_partitionwise_join(PlannerInfo *root, RelOptInfo *rel1, RelOptInfo *rel2,
 									child_restrictlist);
 
 		pfree(appinfos);
-		free_child_sjinfo_members(&child_sjinfo);
+		free_child_join_sjinfo(child_sjinfo);
 	}
 }
 
@@ -1679,91 +1685,84 @@ try_partitionwise_join(PlannerInfo *root, RelOptInfo *rel1, RelOptInfo *rel2,
  * SpecialJoinInfo for the join between parents. left_relids and right_relids
  * are the relids of left and right side of the join respectively.
  *
- * If translations are added to or removed from this function, consider freeing
- * the translated objects in free_child_sjinfo_members() appropriately.
+ * If translations are added to or removed from this function, consider
+ * updating free_child_join_sjinfo() appropriately.
  */
 static SpecialJoinInfo *
 build_child_join_sjinfo(PlannerInfo *root, SpecialJoinInfo *parent_sjinfo,
-						RelOptInfo *left_child, RelOptInfo *right_child)
+						Relids left_relids, Relids right_relids)
 {
+	SpecialJoinInfo *sjinfo = makeNode(SpecialJoinInfo);
 	AppendRelInfo **left_appinfos;
 	int			left_nappinfos;
 	AppendRelInfo **right_appinfos;
 	int			right_nappinfos;
-	SpecialJoinInfo *child_sjinfo = makeNode(SpecialJoinInfo);
 
 	/* Dummy SpecialJoinInfos can be created without any translation. */
 	if (parent_sjinfo->jointype == JOIN_INNER)
 	{
 		Assert(parent_sjinfo->ojrelid == 0);
-		make_dummy_sjinfo(child_sjinfo, left_child, right_child);
-		return child_sjinfo;
+		init_dummy_sjinfo(sjinfo, left_relids, right_relids);
+		return sjinfo;
 	}
 
-	memcpy(child_sjinfo, parent_sjinfo, sizeof(SpecialJoinInfo));
-	left_appinfos = find_appinfos_by_relids(root, left_child->relids,
+	memcpy(sjinfo, parent_sjinfo, sizeof(SpecialJoinInfo));
+	left_appinfos = find_appinfos_by_relids(root, left_relids,
 											&left_nappinfos);
-	right_appinfos = find_appinfos_by_relids(root, right_child->relids,
+	right_appinfos = find_appinfos_by_relids(root, right_relids,
 											 &right_nappinfos);
 
-	child_sjinfo->min_lefthand =
-		adjust_child_relids(child_sjinfo->min_lefthand,
-							left_nappinfos,
-							left_appinfos);
-	child_sjinfo->min_righthand =
-		adjust_child_relids(child_sjinfo->min_righthand,
-							right_nappinfos,
-							right_appinfos);
-	child_sjinfo->syn_lefthand = adjust_child_relids(child_sjinfo->syn_lefthand,
-													 left_nappinfos,
-													 left_appinfos);
-	child_sjinfo->syn_righthand = adjust_child_relids(child_sjinfo->syn_righthand,
-													  right_nappinfos,
-													  right_appinfos);
-
+	sjinfo->min_lefthand = adjust_child_relids(sjinfo->min_lefthand,
+											   left_nappinfos, left_appinfos);
+	sjinfo->min_righthand = adjust_child_relids(sjinfo->min_righthand,
+												right_nappinfos,
+												right_appinfos);
+	sjinfo->syn_lefthand = adjust_child_relids(sjinfo->syn_lefthand,
+											   left_nappinfos, left_appinfos);
+	sjinfo->syn_righthand = adjust_child_relids(sjinfo->syn_righthand,
+												right_nappinfos,
+												right_appinfos);
 	/* outer-join relids need no adjustment */
-	child_sjinfo->semi_rhs_exprs =
-		(List *) adjust_appendrel_attrs(root,
-										(Node *) child_sjinfo->semi_rhs_exprs,
-										right_nappinfos, right_appinfos);
+	sjinfo->semi_rhs_exprs = (List *) adjust_appendrel_attrs(root,
+															 (Node *) sjinfo->semi_rhs_exprs,
+															 right_nappinfos,
+															 right_appinfos);
 
 	pfree(left_appinfos);
 	pfree(right_appinfos);
 
-	return child_sjinfo;
+	return sjinfo;
 }
 
 /*
- * free_child_sjinfo_members
- *		Free memory consumed by a child SpecialJoinInfo.
+ * free_child_join_sjinfo
+ *		Free memory consumed by a SpecialJoinInfo created by
+ *		build_child_join_sjinfo()
  *
  * Only members that are translated copies of their counterpart in the parent
- * SpecialJoinInfo are freed here.  However, members that could be referenced
- * elsewhere are not freed.
+ * SpecialJoinInfo are freed here; see build_child_join_sjinfo().
  */
 static void
-free_child_sjinfo_members(SpecialJoinInfo **child_sjinfo_p)
+free_child_join_sjinfo(SpecialJoinInfo *sjinfo)
 {
-	SpecialJoinInfo *child_sjinfo = *child_sjinfo_p;
-
 	/*
-	 * Dummy SpecialJoinInfos do not have any translated fields and hence have
-	 * nothing to free.
+	 * Dummy SpecialJoinInfos of inner joins do not have any translated fields
+	 * and hence have nothing to be freed; see init_dummy_sjinfo().
 	 */
-	if (child_sjinfo->jointype != JOIN_INNER)
+	if (sjinfo->jointype != JOIN_INNER)
 	{
-		bms_free(child_sjinfo->min_lefthand);
-		bms_free(child_sjinfo->min_righthand);
-		bms_free(child_sjinfo->syn_lefthand);
-		bms_free(child_sjinfo->syn_righthand);
+		bms_free(sjinfo->min_lefthand);
+		bms_free(sjinfo->min_righthand);
+		bms_free(sjinfo->syn_lefthand);
+		bms_free(sjinfo->syn_righthand);
 
-		/* semi_rhs_exprs may be referenced, so don't free. */
+		/*
+		 * semi_rhs_exprs may in principle be freed, but a simple pfree() does
+		 * not suffice, so we leave it alone.
+		 */
 	}
 
-	/* Avoid dangling pointer. */
-	*child_sjinfo_p = NULL;
-
-	pfree(child_sjinfo);
+	pfree(sjinfo);
 }
 
 /*
diff --git a/src/backend/optimizer/util/pathnode.c b/src/backend/optimizer/util/pathnode.c
index 0a7e5c2678..c29ca5a0da 100644
--- a/src/backend/optimizer/util/pathnode.c
+++ b/src/backend/optimizer/util/pathnode.c
@@ -1707,8 +1707,9 @@ create_unique_path(PlannerInfo *root, RelOptInfo *rel, Path *subpath,
 	pathnode->subpath = subpath;
 
 	/*
-	 * Under GEQO, the sjinfo might be short-lived, so we'd better make copies
-	 * of data structures we extract from it.
+	 * Under GEQO and when planning child joins, the sjinfo might be
+	 * short-lived, so we'd better make copies of data structures we extract
+	 * from it.
 	 */
 	pathnode->in_operators = copyObject(sjinfo->semi_operators);
 	pathnode->uniq_exprs = copyObject(sjinfo->semi_rhs_exprs);
diff --git a/src/include/optimizer/paths.h b/src/include/optimizer/paths.h
index 1e340a6335..125676b5bf 100644
--- a/src/include/optimizer/paths.h
+++ b/src/include/optimizer/paths.h
@@ -112,8 +112,8 @@ extern bool have_join_order_restriction(PlannerInfo *root,
 extern bool have_dangerous_phv(PlannerInfo *root,
 							   Relids outer_relids, Relids inner_params);
 extern void mark_dummy_rel(RelOptInfo *rel);
-extern void make_dummy_sjinfo(SpecialJoinInfo *sjinfo, RelOptInfo *rel1,
-							  RelOptInfo *rel2);
+extern void init_dummy_sjinfo(SpecialJoinInfo *sjinfo, Relids left_relids,
+							  Relids right_relids);
 
 /*
  * equivclass.c
-- 
2.43.0