Correction of RowMark Removal During Sel-Join Elimination

From 749c86bea70bed0d6b91fd656627b864e74369d3 Mon Sep 17 00:00:00 2001
From: "Andrei V. Lepikhov" <lepihov@gmail.com>
Date: Wed, 23 Jul 2025 13:46:02 +0200
Subject: [PATCH v1] Force RowMark Sanity Checking

The SJE feature eliminates incorrect RowMarks without introducing any potential
errors. This is due to a straightforward reason: the planned RowMark is used to
reference a rtable entry in later execution stages. An RTE entry for keeping
and removing relations is identical and refers to the same relation OID.

To reduce confusion and prevent future issues, this commit cleans up the code
and fixes the incorrect behaviour. Additionally, it renames variables to make
similar errors more apparent in the future. Furthermore, it includes sanity
checks in setrefs.c on existing non-null RTE and RelOptInfo entries
for each RowMark.
---
 src/backend/optimizer/plan/analyzejoins.c | 49 ++++++++++++-----------
 src/backend/optimizer/plan/setrefs.c      |  4 ++
 2 files changed, 30 insertions(+), 23 deletions(-)

diff --git a/src/backend/optimizer/plan/analyzejoins.c b/src/backend/optimizer/plan/analyzejoins.c
index 4d55c2ea591..df63f1699dc 100644
--- a/src/backend/optimizer/plan/analyzejoins.c
+++ b/src/backend/optimizer/plan/analyzejoins.c
@@ -631,6 +631,7 @@ remove_leftjoinrel_from_query(PlannerInfo *root, int relid,
 	 * remove_join_clause_from_rels will touch it.)
 	 */
 	root->simple_rel_array[relid] = NULL;
+	root->simple_rte_array[relid] = NULL;
 
 	/* And nuke the RelOptInfo, just in case there's another access path */
 	pfree(rel);
@@ -1979,10 +1980,12 @@ remove_self_join_rel(PlannerInfo *root, PlanRowMark *kmark, PlanRowMark *rmark,
 	 * remove_join_clause_from_rels will touch it.)
 	 */
 	root->simple_rel_array[toRemove->relid] = NULL;
+	root->simple_rte_array[toRemove->relid] = NULL;
 
 	/* And nuke the RelOptInfo, just in case there's another access path. */
 	pfree(toRemove);
 
+
 	/*
 	 * Now repeat construction of attr_needed bits coming from all other
 	 * sources.
@@ -2142,21 +2145,21 @@ remove_self_joins_one_group(PlannerInfo *root, Relids relids)
 
 	while ((r = bms_next_member(relids, r)) > 0)
 	{
-		RelOptInfo *inner = root->simple_rel_array[r];
+		RelOptInfo *rrel = root->simple_rel_array[r];
 
 		k = r;
 
 		while ((k = bms_next_member(relids, k)) > 0)
 		{
 			Relids		joinrelids = NULL;
-			RelOptInfo *outer = root->simple_rel_array[k];
+			RelOptInfo *krel = root->simple_rel_array[k];
 			List	   *restrictlist;
 			List	   *selfjoinquals;
 			List	   *otherjoinquals;
 			ListCell   *lc;
 			bool		jinfo_check = true;
-			PlanRowMark *omark = NULL;
-			PlanRowMark *imark = NULL;
+			PlanRowMark *kmark = NULL;
+			PlanRowMark *rmark = NULL;
 			List	   *uclauses = NIL;
 
 			/* A sanity check: the relations have the same Oid. */
@@ -2194,21 +2197,21 @@ remove_self_joins_one_group(PlannerInfo *root, Relids relids)
 			{
 				PlanRowMark *rowMark = (PlanRowMark *) lfirst(lc);
 
-				if (rowMark->rti == k)
+				if (rowMark->rti == r)
 				{
-					Assert(imark == NULL);
-					imark = rowMark;
+					Assert(rmark == NULL);
+					rmark = rowMark;
 				}
-				else if (rowMark->rti == r)
+				else if (rowMark->rti == k)
 				{
-					Assert(omark == NULL);
-					omark = rowMark;
+					Assert(kmark == NULL);
+					kmark = rowMark;
 				}
 
-				if (omark && imark)
+				if (kmark && rmark)
 					break;
 			}
-			if (omark && imark && omark->markType != imark->markType)
+			if (kmark && rmark && kmark->markType != rmark->markType)
 				continue;
 
 			/*
@@ -2229,8 +2232,8 @@ remove_self_joins_one_group(PlannerInfo *root, Relids relids)
 			 * build_joinrel_restrictlist() routine.
 			 */
 			restrictlist = generate_join_implied_equalities(root, joinrelids,
-															inner->relids,
-															outer, NULL);
+															rrel->relids,
+															krel, NULL);
 			if (restrictlist == NIL)
 				continue;
 
@@ -2240,7 +2243,7 @@ remove_self_joins_one_group(PlannerInfo *root, Relids relids)
 			 * otherjoinquals.
 			 */
 			split_selfjoin_quals(root, restrictlist, &selfjoinquals,
-								 &otherjoinquals, inner->relid, outer->relid);
+								 &otherjoinquals, rrel->relid, krel->relid);
 
 			Assert(list_length(restrictlist) ==
 				   (list_length(selfjoinquals) + list_length(otherjoinquals)));
@@ -2251,17 +2254,17 @@ remove_self_joins_one_group(PlannerInfo *root, Relids relids)
 			 * degenerate case works only if both sides have the same clause.
 			 * So doesn't matter which side to add.
 			 */
-			selfjoinquals = list_concat(selfjoinquals, outer->baserestrictinfo);
+			selfjoinquals = list_concat(selfjoinquals, krel->baserestrictinfo);
 
 			/*
-			 * Determine if the inner table can duplicate outer rows.  We must
+			 * Determine if the rrel can duplicate outer rows. We must
 			 * bypass the unique rel cache here since we're possibly using a
 			 * subset of join quals. We can use 'force_cache' == true when all
 			 * join quals are self-join quals.  Otherwise, we could end up
 			 * putting false negatives in the cache.
 			 */
-			if (!innerrel_is_unique_ext(root, joinrelids, inner->relids,
-										outer, JOIN_INNER, selfjoinquals,
+			if (!innerrel_is_unique_ext(root, joinrelids, rrel->relids,
+										krel, JOIN_INNER, selfjoinquals,
 										list_length(otherjoinquals) == 0,
 										&uclauses))
 				continue;
@@ -2277,14 +2280,14 @@ remove_self_joins_one_group(PlannerInfo *root, Relids relids)
 			 * expressions, or we won't match the same row on each side of the
 			 * join.
 			 */
-			if (!match_unique_clauses(root, inner, uclauses, outer->relid))
+			if (!match_unique_clauses(root, rrel, uclauses, krel->relid))
 				continue;
 
 			/*
-			 * We can remove either relation, so remove the inner one in order
-			 * to simplify this loop.
+			 * Remove rrel ReloptInfo from the planner structures and the
+			 * corresponding row mark.
 			 */
-			remove_self_join_rel(root, omark, imark, outer, inner, restrictlist);
+			remove_self_join_rel(root, kmark, rmark, krel, rrel, restrictlist);
 
 			result = bms_add_member(result, r);
 
diff --git a/src/backend/optimizer/plan/setrefs.c b/src/backend/optimizer/plan/setrefs.c
index 846e44186c3..d706546f332 100644
--- a/src/backend/optimizer/plan/setrefs.c
+++ b/src/backend/optimizer/plan/setrefs.c
@@ -307,6 +307,10 @@ set_plan_references(PlannerInfo *root, Plan *plan)
 		PlanRowMark *rc = lfirst_node(PlanRowMark, lc);
 		PlanRowMark *newrc;
 
+		/* sanity check on existing row marks */
+		Assert(root->simple_rel_array[rc->rti] != NULL &&
+			   root->simple_rte_array[rc->rti] != NULL);
+
 		/* flat copy is enough since all fields are scalars */
 		newrc = (PlanRowMark *) palloc(sizeof(PlanRowMark));
 		memcpy(newrc, rc, sizeof(PlanRowMark));
-- 
2.50.1

From c9d414b1a46d654deace1bc7a390683483dba5ec Mon Sep 17 00:00:00 2001
From: Alexander Korotkov <akorotkov@postgresql.org>
Date: Sun, 24 Aug 2025 03:26:11 +0300
Subject: [PATCH v2 1/2] Refactor variable names in
 remove_self_joins_one_group()

Rename inner and outer to rrel and krel, respectively, to highlight their
connection to r and k indexes.  For the same reason, rename imark and omark
to rmark and kmark.

Discussion: https://postgr.es/m/18c6bd6c-6d2a-419a-b0da-dfedef34b585%40gmail.com
Author: Andrei Lepikhov <lepihov@gmail.com>
Reviewed-by: Greg Sabino Mullane <htamfids@gmail.com>
Backpatch-through: 18
---
 src/backend/optimizer/plan/analyzejoins.c | 50 +++++++++++------------
 1 file changed, 25 insertions(+), 25 deletions(-)

diff --git a/src/backend/optimizer/plan/analyzejoins.c b/src/backend/optimizer/plan/analyzejoins.c
index da92d8ee414..15e82351639 100644
--- a/src/backend/optimizer/plan/analyzejoins.c
+++ b/src/backend/optimizer/plan/analyzejoins.c
@@ -2141,21 +2141,21 @@ remove_self_joins_one_group(PlannerInfo *root, Relids relids)
 
 	while ((r = bms_next_member(relids, r)) > 0)
 	{
-		RelOptInfo *inner = root->simple_rel_array[r];
+		RelOptInfo *rrel = root->simple_rel_array[r];
 
 		k = r;
 
 		while ((k = bms_next_member(relids, k)) > 0)
 		{
 			Relids		joinrelids = NULL;
-			RelOptInfo *outer = root->simple_rel_array[k];
+			RelOptInfo *krel = root->simple_rel_array[k];
 			List	   *restrictlist;
 			List	   *selfjoinquals;
 			List	   *otherjoinquals;
 			ListCell   *lc;
 			bool		jinfo_check = true;
-			PlanRowMark *omark = NULL;
-			PlanRowMark *imark = NULL;
+			PlanRowMark *kmark = NULL;
+			PlanRowMark *rmark = NULL;
 			List	   *uclauses = NIL;
 
 			/* A sanity check: the relations have the same Oid. */
@@ -2195,19 +2195,19 @@ remove_self_joins_one_group(PlannerInfo *root, Relids relids)
 
 				if (rowMark->rti == k)
 				{
-					Assert(imark == NULL);
-					imark = rowMark;
+					Assert(rmark == NULL);
+					rmark = rowMark;
 				}
 				else if (rowMark->rti == r)
 				{
-					Assert(omark == NULL);
-					omark = rowMark;
+					Assert(kmark == NULL);
+					kmark = rowMark;
 				}
 
-				if (omark && imark)
+				if (kmark && rmark)
 					break;
 			}
-			if (omark && imark && omark->markType != imark->markType)
+			if (kmark && rmark && kmark->markType != rmark->markType)
 				continue;
 
 			/*
@@ -2228,8 +2228,8 @@ remove_self_joins_one_group(PlannerInfo *root, Relids relids)
 			 * build_joinrel_restrictlist() routine.
 			 */
 			restrictlist = generate_join_implied_equalities(root, joinrelids,
-															inner->relids,
-															outer, NULL);
+															rrel->relids,
+															krel, NULL);
 			if (restrictlist == NIL)
 				continue;
 
@@ -2239,7 +2239,7 @@ remove_self_joins_one_group(PlannerInfo *root, Relids relids)
 			 * otherjoinquals.
 			 */
 			split_selfjoin_quals(root, restrictlist, &selfjoinquals,
-								 &otherjoinquals, inner->relid, outer->relid);
+								 &otherjoinquals, rrel->relid, krel->relid);
 
 			Assert(list_length(restrictlist) ==
 				   (list_length(selfjoinquals) + list_length(otherjoinquals)));
@@ -2250,17 +2250,17 @@ remove_self_joins_one_group(PlannerInfo *root, Relids relids)
 			 * degenerate case works only if both sides have the same clause.
 			 * So doesn't matter which side to add.
 			 */
-			selfjoinquals = list_concat(selfjoinquals, outer->baserestrictinfo);
+			selfjoinquals = list_concat(selfjoinquals, krel->baserestrictinfo);
 
 			/*
-			 * Determine if the inner table can duplicate outer rows.  We must
-			 * bypass the unique rel cache here since we're possibly using a
-			 * subset of join quals. We can use 'force_cache' == true when all
-			 * join quals are self-join quals.  Otherwise, we could end up
-			 * putting false negatives in the cache.
+			 * Determine if the rrel can duplicate outer rows.  We must bypass
+			 * the unique rel cache here since we're possibly using a subset
+			 * of join quals. We can use 'force_cache' == true when all join
+			 * quals are self-join quals.  Otherwise, we could end up putting
+			 * false negatives in the cache.
 			 */
-			if (!innerrel_is_unique_ext(root, joinrelids, inner->relids,
-										outer, JOIN_INNER, selfjoinquals,
+			if (!innerrel_is_unique_ext(root, joinrelids, rrel->relids,
+										krel, JOIN_INNER, selfjoinquals,
 										list_length(otherjoinquals) == 0,
 										&uclauses))
 				continue;
@@ -2276,14 +2276,14 @@ remove_self_joins_one_group(PlannerInfo *root, Relids relids)
 			 * expressions, or we won't match the same row on each side of the
 			 * join.
 			 */
-			if (!match_unique_clauses(root, inner, uclauses, outer->relid))
+			if (!match_unique_clauses(root, rrel, uclauses, krel->relid))
 				continue;
 
 			/*
-			 * We can remove either relation, so remove the inner one in order
-			 * to simplify this loop.
+			 * Remove rrel ReloptInfo from the planner structures and the
+			 * corresponding row mark.
 			 */
-			remove_self_join_rel(root, omark, imark, outer, inner, restrictlist);
+			remove_self_join_rel(root, kmark, rmark, krel, rrel, restrictlist);
 
 			result = bms_add_member(result, r);
 
-- 
2.39.5 (Apple Git-154)

From 81b5c3e8ddf58e239892ef792672b192c9c72a5f Mon Sep 17 00:00:00 2001
From: Alexander Korotkov <akorotkov@postgresql.org>
Date: Sun, 24 Aug 2025 03:34:22 +0300
Subject: [PATCH v2 2/2] Improve RowMark handling during Self-Join Elimination

The Self-Join Elimination SJE feature messes up keeping and removing RowMark's
in remove_self_joins_one_group().  That didn't lead to user-level error,
because the planned RowMark is only used to reference a rtable entry in later
execution stages.  An RTE entry for keeping and removing relations is
identical and refers to the same relation OID.

To reduce confusion and prevent future issues, this commit cleans up the code
and fixes the incorrect behaviour.  Furthermore, it includes sanity checks in
setrefs.c on existing non-null RTE and RelOptInfo entries for each RowMark.

Discussion: https://postgr.es/m/18c6bd6c-6d2a-419a-b0da-dfedef34b585%40gmail.com
Author: Andrei Lepikhov <lepihov@gmail.com>
Reviewed-by: Greg Sabino Mullane <htamfids@gmail.com>
Backpatch-through: 18
---
 src/backend/optimizer/plan/analyzejoins.c | 17 ++++++++++-------
 src/backend/optimizer/plan/setrefs.c      |  4 ++++
 2 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/src/backend/optimizer/plan/analyzejoins.c b/src/backend/optimizer/plan/analyzejoins.c
index 15e82351639..a0468353b4b 100644
--- a/src/backend/optimizer/plan/analyzejoins.c
+++ b/src/backend/optimizer/plan/analyzejoins.c
@@ -631,6 +631,7 @@ remove_leftjoinrel_from_query(PlannerInfo *root, int relid,
 	 * remove_join_clause_from_rels will touch it.)
 	 */
 	root->simple_rel_array[relid] = NULL;
+	root->simple_rte_array[relid] = NULL;
 
 	/* And nuke the RelOptInfo, just in case there's another access path */
 	pfree(rel);
@@ -1978,10 +1979,12 @@ remove_self_join_rel(PlannerInfo *root, PlanRowMark *kmark, PlanRowMark *rmark,
 	 * remove_join_clause_from_rels will touch it.)
 	 */
 	root->simple_rel_array[toRemove->relid] = NULL;
+	root->simple_rte_array[toRemove->relid] = NULL;
 
 	/* And nuke the RelOptInfo, just in case there's another access path. */
 	pfree(toRemove);
 
+
 	/*
 	 * Now repeat construction of attr_needed bits coming from all other
 	 * sources.
@@ -2193,12 +2196,12 @@ remove_self_joins_one_group(PlannerInfo *root, Relids relids)
 			{
 				PlanRowMark *rowMark = (PlanRowMark *) lfirst(lc);
 
-				if (rowMark->rti == k)
+				if (rowMark->rti == r)
 				{
 					Assert(rmark == NULL);
 					rmark = rowMark;
 				}
-				else if (rowMark->rti == r)
+				else if (rowMark->rti == k)
 				{
 					Assert(kmark == NULL);
 					kmark = rowMark;
@@ -2253,11 +2256,11 @@ remove_self_joins_one_group(PlannerInfo *root, Relids relids)
 			selfjoinquals = list_concat(selfjoinquals, krel->baserestrictinfo);
 
 			/*
-			 * Determine if the rrel can duplicate outer rows.  We must bypass
-			 * the unique rel cache here since we're possibly using a subset
-			 * of join quals. We can use 'force_cache' == true when all join
-			 * quals are self-join quals.  Otherwise, we could end up putting
-			 * false negatives in the cache.
+			 * Determine if the rrel can duplicate outer rows. We must
+			 * bypass the unique rel cache here since we're possibly using a
+			 * subset of join quals. We can use 'force_cache' == true when all
+			 * join quals are self-join quals.  Otherwise, we could end up
+			 * putting false negatives in the cache.
 			 */
 			if (!innerrel_is_unique_ext(root, joinrelids, rrel->relids,
 										krel, JOIN_INNER, selfjoinquals,
diff --git a/src/backend/optimizer/plan/setrefs.c b/src/backend/optimizer/plan/setrefs.c
index 846e44186c3..d706546f332 100644
--- a/src/backend/optimizer/plan/setrefs.c
+++ b/src/backend/optimizer/plan/setrefs.c
@@ -307,6 +307,10 @@ set_plan_references(PlannerInfo *root, Plan *plan)
 		PlanRowMark *rc = lfirst_node(PlanRowMark, lc);
 		PlanRowMark *newrc;
 
+		/* sanity check on existing row marks */
+		Assert(root->simple_rel_array[rc->rti] != NULL &&
+			   root->simple_rte_array[rc->rti] != NULL);
+
 		/* flat copy is enough since all fields are scalars */
 		newrc = (PlanRowMark *) palloc(sizeof(PlanRowMark));
 		memcpy(newrc, rc, sizeof(PlanRowMark));
-- 
2.39.5 (Apple Git-154)

diff --git a/src/backend/optimizer/util/plancat.c b/src/backend/optimizer/util/plancat.c
index 6ce4efea118..cd704987347 100644
--- a/src/backend/optimizer/util/plancat.c
+++ b/src/backend/optimizer/util/plancat.c
@@ -1406,6 +1406,10 @@ get_relation_constraints(PlannerInfo *root,
 
 			cexpr = stringToNode(constr->check[i].ccbin);
 
+			/* Fix Vars to have the desired varno */
+			if (varno != 1)
+				ChangeVarNodes(cexpr, 1, varno, 0);
+
 			/*
 			 * Run each expression through const-simplification and
 			 * canonicalization.  This is not just an optimization, but is
@@ -1420,10 +1424,6 @@ get_relation_constraints(PlannerInfo *root,
 
 			cexpr = (Node *) canonicalize_qual((Expr *) cexpr, true);
 
-			/* Fix Vars to have the desired varno */
-			if (varno != 1)
-				ChangeVarNodes(cexpr, 1, varno, 0);
-
 			/*
 			 * Finally, convert to implicit-AND format (that is, a List) and
 			 * append the resulting item(s) to our output list.

From fa9fe13e903b8df099d30fd166e9e389063ed110 Mon Sep 17 00:00:00 2001
From: Alexander Korotkov <akorotkov@postgresql.org>
Date: Sun, 31 Aug 2025 02:08:45 +0300
Subject: [PATCH v2] Fix the operation order in get_relation_constraints()

Must do ChangeVarNodes() before eval_const_expressions(), which which might
access the corresponding RTEs by their varno's.

Reported-by: Alexander Lakhin <exclusion@gmail.com>
Discussion: https://postgr.es/m/9b10b847-067e-45cf-bbf7-7802b6303fc0%40gmail.com
Backpatch-through: 13
---
 src/backend/optimizer/util/plancat.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/src/backend/optimizer/util/plancat.c b/src/backend/optimizer/util/plancat.c
index 6ce4efea118..73fd57a316b 100644
--- a/src/backend/optimizer/util/plancat.c
+++ b/src/backend/optimizer/util/plancat.c
@@ -1406,6 +1406,14 @@ get_relation_constraints(PlannerInfo *root,
 
 			cexpr = stringToNode(constr->check[i].ccbin);
 
+			/*
+			 * Fix Vars to have the desired varno.  Must do this before
+			 * eval_const_expressions(), which might access the corresponding
+			 * RTEs.
+			 */
+			if (varno != 1)
+				ChangeVarNodes(cexpr, 1, varno, 0);
+
 			/*
 			 * Run each expression through const-simplification and
 			 * canonicalization.  This is not just an optimization, but is
@@ -1420,10 +1428,6 @@ get_relation_constraints(PlannerInfo *root,
 
 			cexpr = (Node *) canonicalize_qual((Expr *) cexpr, true);
 
-			/* Fix Vars to have the desired varno */
-			if (varno != 1)
-				ChangeVarNodes(cexpr, 1, varno, 0);
-
 			/*
 			 * Finally, convert to implicit-AND format (that is, a List) and
 			 * append the resulting item(s) to our output list.
-- 
2.39.5 (Apple Git-154)