From ff721aeed52f15e9aad22f03777e37225ff4183f Mon Sep 17 00:00:00 2001 From: Ashutosh Bapat 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