0004-Minor-fixes-20250401.patch

From ff721aeed52f15e9aad22f03777e37225ff4183f Mon Sep 17 00:00:00 2001
From: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
Date: Tue, 1 Apr 2025 22:13:08 +0530
Subject: [PATCH 4/4] Minor fixes

1. Assert movement
2. grammar fixes
3. removed duplicate comments
4. comment rephrased

Author: Ashutosh Bapat
---
 src/backend/optimizer/path/equivclass.c | 68 +++++++++++--------------
 1 file changed, 30 insertions(+), 38 deletions(-)

diff --git a/src/backend/optimizer/path/equivclass.c b/src/backend/optimizer/path/equivclass.c
index 12155c0c957..3249f966573 100644
--- a/src/backend/optimizer/path/equivclass.c
+++ b/src/backend/optimizer/path/equivclass.c
@@ -90,7 +90,8 @@ static RestrictInfo *ec_search_derived_clause_for_ems(PlannerInfo *root,
 /*
  * Hash key identifying a derived clause.
  *
- * See fill_ec_derives_key() for details on canonical key construction.
+ * The structure is not supposed to be filled manually. Please use
+ * fill_ec_derives_key() to setup key in canonical form.
  */
 typedef struct
 {
@@ -3478,10 +3479,24 @@ ec_build_derives_hash(PlannerInfo *root, EquivalenceClass *ec)
  *		Add a clause to the set of derived clauses for the given
  *		EquivalenceClass. Always appends to ec_derives_list; also adds
  *		to ec_derives_hash if it exists.
+ *
+ * While at it, also check some derived clause invariants.
  */
 static void
 ec_add_derived_clause(EquivalenceClass *ec, RestrictInfo *clause)
 {
+	/*
+	 * Constant, if present, is always placed on the RHS; see
+	 * generate_base_implied_equalities_const(). LHS is never a constant.
+	 */
+	Assert(!clause->left_em->em_is_const);
+
+	/*
+	 * Clauses containing a constant are never considered redundant, so
+	 * parent_ec is not set.
+	 */
+	Assert(!clause->parent_ec || !clause->right_em->em_is_const);
+
 	ec->ec_derives_list = lappend(ec->ec_derives_list, clause);
 	if (ec->ec_derives_hash)
 		ec_add_clause_to_derives_hash(ec, clause);
@@ -3492,8 +3507,10 @@ ec_add_derived_clause(EquivalenceClass *ec, RestrictInfo *clause)
  *		Add a list of clauses to the set of clauses derived from the given
  *		EquivalenceClass; adding to the list and hash table if needed.
  *
- * This function is similar to above function but optimized for adding multiple
- * clauses at a time to the ec_derives_list.
+ * This function is similar to ec_add_derived_clause() but optimized for adding
+ * multiple clauses at a time to the ec_derives_list. However we don't need to
+ * repeat the Asserts in that function since they must have been applied when
+ * the clauses were inserted in the derived clause list from where they came.
  */
 static void
 ec_add_derived_clauses(EquivalenceClass *ec, List *clauses)
@@ -3506,16 +3523,16 @@ ec_add_derived_clauses(EquivalenceClass *ec, List *clauses)
 
 /*
  * fill_ec_derives_key
- *		Compute a canonical key for ec_derives_hash lookup or insertion.
+ *		Setup canonical key for ec_derives_hash lookup or insertion.
  *
  * Derived clauses are looked up using a pair of EquivalenceMembers and a
  * parent EquivalenceClass. To avoid storing or searching for both EM orderings,
  * we canonicalize the key:
  *
- * - For clauses involving two non-constant EMs, we order the EMs by address
- *   and place the lower one first.
+ * - For clauses involving two non-constant EMs, em1 is set to the EM with lower
+ *   memory address and em2 is set to the other one.
  * - For clauses involving a constant EM, the caller must pass the non-constant
- *   EM as leftem; we then set em1 = NULL and em2 = leftem.
+ *   EM as leftem and NULL as rightem; we then set em1 = NULL and em2 = leftem.
  */
 static inline void
 fill_ec_derives_key(ECDerivesKey *key,
@@ -3525,10 +3542,6 @@ fill_ec_derives_key(ECDerivesKey *key,
 {
 	Assert(leftem);				/* Always required for lookup or insertion */
 
-	/*
-	 * Clauses with a constant EM are always stored and looked up using only
-	 * the non-constant EM, with the other key slot set to NULL.
-	 */
 	if (rightem == NULL)
 	{
 		key->em1 = NULL;
@@ -3549,9 +3562,9 @@ fill_ec_derives_key(ECDerivesKey *key,
 
 /*
  * ec_add_clause_to_derives_hash
- *		Add a derived clause to ec_derives_hash for the given EquivalenceClass.
+ *		Add a derived clause to ec_derives_hash in the given EquivalenceClass.
  *
- * Each clause is inserted under a canonicalized key. For constant-containing
+ * Each clause is associated with a canonicalized key. For constant-containing
  * clauses, only the non-constant EM is used for lookup; see comments in
  * fill_ec_derives_key().
  */
@@ -3562,23 +3575,6 @@ ec_add_clause_to_derives_hash(EquivalenceClass *ec, RestrictInfo *rinfo)
 	ECDerivesEntry *entry;
 	bool		found;
 
-	/*
-	 * Constants are always placed on the RHS; see
-	 * generate_base_implied_equalities_const().
-	 */
-	Assert(!rinfo->left_em->em_is_const);
-
-	/*
-	 * Clauses containing a constant are never considered redundant, so
-	 * parent_ec is not set.
-	 */
-	Assert(!rinfo->parent_ec || !rinfo->right_em->em_is_const);
-
-	/*
-	 * See fill_ec_derives_key() for details: we use a canonicalized key to
-	 * avoid storing both EM orderings. For constant EMs, only the
-	 * non-constant EM is included in the key.
-	 */
 	fill_ec_derives_key(&key,
 						rinfo->left_em,
 						rinfo->right_em->em_is_const ? NULL : rinfo->right_em,
@@ -3654,11 +3650,12 @@ ec_search_clause_for_ems(PlannerInfo *root, EquivalenceClass *ec,
  * lookup; otherwise, scan ec_derives_list linearly.
  *
  * Clauses involving constants are looked up using only the non-constant EM
- * (leftem), with rightem set to NULL. In that case, we expect to find a
+ * passed as leftem and rightem set to NULL. In that case, we expect to find a
  * clause with a constant on the RHS.
  *
- * We do not attempt a second lookup with EMs swapped, because the key is
- * canonicalized during both insertion and lookup.
+ * While looking up the derived clause in the list, we match each given EM with
+ * both sides of the clause respectively.  But hash table look up is carried out
+ * only once using a canonicalized key.
  */
 static RestrictInfo *
 ec_search_derived_clause_for_ems(PlannerInfo *root, EquivalenceClass *ec,
@@ -3686,11 +3683,6 @@ ec_search_derived_clause_for_ems(PlannerInfo *root, EquivalenceClass *ec,
 		{
 			rinfo = entry->rinfo;
 			Assert(rinfo);
-
-			/*
-			 * If this is a lookup in a const-containing EC, the RHS must be a
-			 * constant. The caller signals this by passing NULL for rightem.
-			 */
 			Assert(rightem || rinfo->right_em->em_is_const);
 			return rinfo;
 		}
-- 
2.34.1