Subject: Remove vardata parameters from eqjoinsel_inner

From: Ilia Evdokimov <[email-masked]>
Date: Fri, 21 Feb 2025 13:04:21 +0300

Hi hackers,

When calculating selectivity for an inner equijoin, we call 
eqjoinsel_inner, which uses unused parameters vardata1 and vardata2. 
These parameters might have been left behind accidentally when we moved 
getting sslots out of the function. I suggest removing them, as they can 
be added back at any time if needed. I attached patch with fixes.

--
Best regards,
Ilia Evdokimov,
Tantor Labs LLC.
From 1bf58c5725ab9a906f57117a2c2e1ee7f287f5d5 Mon Sep 17 00:00:00 2001
From: Ilia Evdokimov <[email-masked]>
Date: Fri, 21 Feb 2025 12:38:13 +0300
Subject: [PATCH v1] Remove unused vardata parameters in eqjoinsel_inner.

The vardata parameters in eqjoinsel_inner are unused, so remove them.
This is a static function, so the parameters can easily be added again if
they are ever needed.
---
 src/backend/utils/adt/selfuncs.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/src/backend/utils/adt/selfuncs.c b/src/backend/utils/adt/selfuncs.c
index c2918c9c83..63b408bdcc 100644
--- a/src/backend/utils/adt/selfuncs.c
+++ b/src/backend/utils/adt/selfuncs.c
@@ -150,7 +150,6 @@ get_index_stats_hook_type get_index_stats_hook = NULL;
 
 static double eqsel_internal(PG_FUNCTION_ARGS, bool negate);
 static double eqjoinsel_inner(Oid opfuncoid, Oid collation,
-							  VariableStatData *vardata1, VariableStatData *vardata2,
 							  double nd1, double nd2,
 							  bool isdefault1, bool isdefault2,
 							  AttStatsSlot *sslot1, AttStatsSlot *sslot2,
@@ -2348,7 +2347,6 @@ eqjoinsel(PG_FUNCTION_ARGS)
 
 	/* We need to compute the inner-join selectivity in all cases */
 	selec_inner = eqjoinsel_inner(opfuncoid, collation,
-								  &vardata1, &vardata2,
 								  nd1, nd2,
 								  isdefault1, isdefault2,
 								  &sslot1, &sslot2,
@@ -2436,7 +2434,6 @@ eqjoinsel(PG_FUNCTION_ARGS)
  */
 static double
 eqjoinsel_inner(Oid opfuncoid, Oid collation,
-				VariableStatData *vardata1, VariableStatData *vardata2,
 				double nd1, double nd2,
 				bool isdefault1, bool isdefault2,
 				AttStatsSlot *sslot1, AttStatsSlot *sslot2,
-- 
2.34.1

---

From: Richard Guo <[email-masked]>
Date: Thu, 27 Mar 2025 16:48:03 +0900

> be added back at any time if needed. I attached patch with fixes.

Yeah, these parameters haven't been used since a314c3407, when we
moved get_variable_numdistinct and get_attstatsslot out of
eqjoinsel_inner and eqjoinsel_semi to avoid repetitive information
lookup when we call both eqjoinsel_inner and eqjoinsel_semi.

I'm wondering whether we should also remove parameter vardata1 from
eqjoinsel_semi.  vardata2 is still needed though to clamp nd2 to be
not more than the rel's row estimate.

Thanks
Richard

---

From: Ilia Evdokimov <[email-masked]>
Date: Thu, 27 Mar 2025 14:00:30 +0300

> not more than the rel's row estimate.

Indeed, the parameter vardata1 in eqjoinsel_semi() is currently unused 
and could logically be removed. However, simply leaving a single 
parameter named vardata2 would appear strange and unintuitive, as it 
implicitly suggests the existence of a corresponding "first" parameter. 
I suggest renaming vardata2 to something more descriptive, such as 
rhs_vardata, clearly indicating its role related specifically to the 
right side of the join condition.

I attached v2 patch with changes.

Any thoughts?

--
Best regards,
Ilia Evdokimov,
Tantor Labs LLC.
From 6ec99d712d7fda3cc18a8700318158f54bae4c55 Mon Sep 17 00:00:00 2001
From: Evdokimov Ilia <[email-masked]>
Date: Thu, 27 Mar 2025 13:59:02 +0300
Subject: [PATCH v2] Remove unused vardata parameters in eqjoinsel_inner and
 eqjoinsel_semi

---
 src/backend/utils/adt/selfuncs.c | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

diff --git a/src/backend/utils/adt/selfuncs.c b/src/backend/utils/adt/selfuncs.c
index 5b35debc8ff..6d428d65d64 100644
--- a/src/backend/utils/adt/selfuncs.c
+++ b/src/backend/utils/adt/selfuncs.c
@@ -150,14 +150,13 @@ get_index_stats_hook_type get_index_stats_hook = NULL;
 
 static double eqsel_internal(PG_FUNCTION_ARGS, bool negate);
 static double eqjoinsel_inner(Oid opfuncoid, Oid collation,
-							  VariableStatData *vardata1, VariableStatData *vardata2,
 							  double nd1, double nd2,
 							  bool isdefault1, bool isdefault2,
 							  AttStatsSlot *sslot1, AttStatsSlot *sslot2,
 							  Form_pg_statistic stats1, Form_pg_statistic stats2,
 							  bool have_mcvs1, bool have_mcvs2);
 static double eqjoinsel_semi(Oid opfuncoid, Oid collation,
-							 VariableStatData *vardata1, VariableStatData *vardata2,
+							 VariableStatData *rhs_vardata,
 							 double nd1, double nd2,
 							 bool isdefault1, bool isdefault2,
 							 AttStatsSlot *sslot1, AttStatsSlot *sslot2,
@@ -2348,7 +2347,6 @@ eqjoinsel(PG_FUNCTION_ARGS)
 
 	/* We need to compute the inner-join selectivity in all cases */
 	selec_inner = eqjoinsel_inner(opfuncoid, collation,
-								  &vardata1, &vardata2,
 								  nd1, nd2,
 								  isdefault1, isdefault2,
 								  &sslot1, &sslot2,
@@ -2375,7 +2373,7 @@ eqjoinsel(PG_FUNCTION_ARGS)
 
 			if (!join_is_reversed)
 				selec = eqjoinsel_semi(opfuncoid, collation,
-									   &vardata1, &vardata2,
+									   &vardata2,
 									   nd1, nd2,
 									   isdefault1, isdefault2,
 									   &sslot1, &sslot2,
@@ -2388,7 +2386,7 @@ eqjoinsel(PG_FUNCTION_ARGS)
 				Oid			commopfuncoid = OidIsValid(commop) ? get_opcode(commop) : InvalidOid;
 
 				selec = eqjoinsel_semi(commopfuncoid, collation,
-									   &vardata2, &vardata1,
+									   &vardata1,
 									   nd2, nd1,
 									   isdefault2, isdefault1,
 									   &sslot2, &sslot1,
@@ -2436,7 +2434,6 @@ eqjoinsel(PG_FUNCTION_ARGS)
  */
 static double
 eqjoinsel_inner(Oid opfuncoid, Oid collation,
-				VariableStatData *vardata1, VariableStatData *vardata2,
 				double nd1, double nd2,
 				bool isdefault1, bool isdefault2,
 				AttStatsSlot *sslot1, AttStatsSlot *sslot2,
@@ -2633,7 +2630,7 @@ eqjoinsel_inner(Oid opfuncoid, Oid collation,
  */
 static double
 eqjoinsel_semi(Oid opfuncoid, Oid collation,
-			   VariableStatData *vardata1, VariableStatData *vardata2,
+			   VariableStatData *rhs_vardata,
 			   double nd1, double nd2,
 			   bool isdefault1, bool isdefault2,
 			   AttStatsSlot *sslot1, AttStatsSlot *sslot2,
@@ -2662,11 +2659,11 @@ eqjoinsel_semi(Oid opfuncoid, Oid collation,
 	 * great, maybe, but it didn't come out of nowhere either.  This is most
 	 * helpful when the inner relation is empty and consequently has no stats.
 	 */
-	if (vardata2->rel)
+	if (rhs_vardata->rel)
 	{
-		if (nd2 >= vardata2->rel->rows)
+		if (nd2 >= rhs_vardata->rel->rows)
 		{
-			nd2 = vardata2->rel->rows;
+			nd2 = rhs_vardata->rel->rows;
 			isdefault2 = false;
 		}
 	}
-- 
2.34.1

---

From: Tom Lane <[email-masked]>
Date: Thu, 27 Mar 2025 09:40:16 -0400

> not more than the rel's row estimate.

I do not believe this change is worth the code churn.  In the first
place, we may well need those values again someday.  In the second
place, the savings would be negligible.  (In fact, since
eqjoinsel_inner probably gets inlined at its sole call site, the
savings would likely be completely nonexistent.)  If the caller
could avoid calculating the vardata info at all, that would be
worth thinking about, but I think it can't.

			regards, tom lane