Re: Use opresulttype instead of calling SearchSysCache1() in match_orclause_to_indexcol() - Mailing list pgsql-hackers

Tender Wang <tndrwang@gmail.com> writes:
> Tom Lane <tgl@sss.pgh.pa.us> 于2025年9月20日周六 00:16写道：
>> I don't understand what purpose this check serves at all.
>> We are looking at an arm of an OR clause, so it had better
>> yield boolean.

> Yeah, this check doesn't need any more. I removed this check in the
> attached patch.

> In match_index_to_operand(), the indexExpr has been ignored, so I removed
> below check, too.
> - if (IsA(indexExpr, RelabelType))
> - indexExpr = (Node *) ((RelabelType *) indexExpr)->arg;

Yeah.  In fact, I think it's outright wrong to do that here.
It'd result in building a SAOP expression that lacks the RelabelType,
which seems incorrect since the operator is one that expects the
relabeled type.

The RelabelType-stripping logic for the constExpr seems unnecessary as
well, if not outright wrong.  It won't matter for an actual Const,
because eval_const_expressions would have flattened the relabeled type
into the Const node.  However, if we have some non-Const right-hand
sides, the effect of stripping RelabelTypes could easily be to fail the
transformation unnecessarily.  That'd happen if the parser had coerced
all the RHS values to be the same type for application of the operator,
but then we stripped some RelabelTypes and mistakenly decided that
the resulting RHSes didn't match in type.

So I removed both of those in v2, attached.

>> In general, this code looks like a mess.  There are a lot of
>> Asserts that might be okay in development but probably should
>> not have got committed, and the comments need work.

> These assertions were removed by me, too.
> I didn’t modify these code comments since English isn’t my native language,
> and I’d appreciate your help with them.

Here's a v2 with some further cleanup work.  One notable item
is that I moved the type_is_rowtype checks, which don't seem
to need to be done more than once.

I'm not very convinced that the type_is_rowtype checks are correct
either.  I can see that we'd better forbid RECORD, because we've got
no way to be sure that all the RHSes are actually the same record
type.  But I don't see why it's necessary or appropriate to forbid
named composite types.  I didn't change that here; maybe we should
look into the discussion leading up to d4378c000.

            regards, tom lane

diff --git a/src/backend/optimizer/path/indxpath.c b/src/backend/optimizer/path/indxpath.c
index edc6d2ac1d3..c25a573adf5 100644
--- a/src/backend/optimizer/path/indxpath.c
+++ b/src/backend/optimizer/path/indxpath.c
@@ -3290,8 +3290,8 @@ match_rowcompare_to_indexcol(PlannerInfo *root,
  *
  * In this routine, we attempt to transform a list of OR-clause args into a
  * single SAOP expression matching the target index column.  On success,
- * return an IndexClause, containing the transformed expression or NULL,
- * if failed.
+ * return an IndexClause containing the transformed expression.
+ * Return NULL if the transformation fails.
  */
 static IndexClause *
 match_orclause_to_indexcol(PlannerInfo *root,
@@ -3299,24 +3299,21 @@ match_orclause_to_indexcol(PlannerInfo *root,
                            int indexcol,
                            IndexOptInfo *index)
 {
-    ListCell   *lc;
     BoolExpr   *orclause = (BoolExpr *) rinfo->orclause;
-    Node       *indexExpr = NULL;
     List       *consts = NIL;
-    ScalarArrayOpExpr *saopexpr = NULL;
+    Node       *indexExpr = NULL;
     Oid            matchOpno = InvalidOid;
-    IndexClause *iclause;
     Oid            consttype = InvalidOid;
     Oid            arraytype = InvalidOid;
     Oid            inputcollid = InvalidOid;
     bool        firstTime = true;
     bool        haveNonConst = false;
     Index        indexRelid = index->rel->relid;
+    ScalarArrayOpExpr *saopexpr;
+    IndexClause *iclause;
+    ListCell   *lc;

-    Assert(IsA(orclause, BoolExpr));
-    Assert(orclause->boolop == OR_EXPR);
-
-    /* Ignore index if it doesn't support SAOP clauses */
+    /* Forget it if index doesn't support SAOP clauses */
     if (!index->amsearcharray)
         return NULL;

@@ -3329,55 +3326,32 @@ match_orclause_to_indexcol(PlannerInfo *root,
      */
     foreach(lc, orclause->args)
     {
-        RestrictInfo *subRinfo;
+        RestrictInfo *subRinfo = (RestrictInfo *) lfirst(lc);
         OpExpr       *subClause;
         Oid            opno;
         Node       *leftop,
                    *rightop;
         Node       *constExpr;

-        if (!IsA(lfirst(lc), RestrictInfo))
+        /* The OR arms are typically RestrictInfos, but not always */
+        if (!IsA(subRinfo, RestrictInfo))
             break;

-        subRinfo = (RestrictInfo *) lfirst(lc);
-
-        /* Only operator clauses can match  */
+        /* Only operator clauses can match */
         if (!IsA(subRinfo->clause, OpExpr))
             break;

         subClause = (OpExpr *) subRinfo->clause;
         opno = subClause->opno;

-        /* Only binary operators can match  */
+        /* Only binary operators can match */
         if (list_length(subClause->args) != 2)
             break;

-        /*
-         * The parameters below must match between sub-rinfo and its parent as
-         * make_restrictinfo() fills them with the same values, and further
-         * modifications are also the same for the whole subtree.  However,
-         * still make a sanity check.
-         */
-        Assert(subRinfo->is_pushed_down == rinfo->is_pushed_down);
-        Assert(subRinfo->is_clone == rinfo->is_clone);
-        Assert(subRinfo->security_level == rinfo->security_level);
-        Assert(bms_equal(subRinfo->incompatible_relids, rinfo->incompatible_relids));
-        Assert(bms_equal(subRinfo->outer_relids, rinfo->outer_relids));
-
-        /*
-         * Also, check that required_relids in sub-rinfo is subset of parent's
-         * required_relids.
-         */
-        Assert(bms_is_subset(subRinfo->required_relids, rinfo->required_relids));
-
-        /* Only the operator returning a boolean suit the transformation. */
-        if (get_op_rettype(opno) != BOOLOID)
-            break;
-
         /*
          * Check for clauses of the form: (indexkey operator constant) or
-         * (constant operator indexkey).  See match_clause_to_indexcol's notes
-         * about const-ness.
+         * (constant operator indexkey).  These tests should agree with
+         * match_opclause_to_indexcol.
          */
         leftop = (Node *) linitial(subClause->args);
         rightop = (Node *) lsecond(subClause->args);
@@ -3406,22 +3380,6 @@ match_orclause_to_indexcol(PlannerInfo *root,
             break;
         }

-        /*
-         * Ignore any RelabelType node above the operands.  This is needed to
-         * be able to apply indexscanning in binary-compatible-operator cases.
-         * Note: we can assume there is at most one RelabelType node;
-         * eval_const_expressions() will have simplified if more than one.
-         */
-        if (IsA(constExpr, RelabelType))
-            constExpr = (Node *) ((RelabelType *) constExpr)->arg;
-        if (IsA(indexExpr, RelabelType))
-            indexExpr = (Node *) ((RelabelType *) indexExpr)->arg;
-
-        /* Forbid transformation for composite types, records. */
-        if (type_is_rowtype(exprType(constExpr)) ||
-            type_is_rowtype(exprType(indexExpr)))
-            break;
-
         /*
          * Save information about the operator, type, and collation for the
          * first matching qual.  Then, check that subsequent quals match the
@@ -3439,54 +3397,69 @@ match_orclause_to_indexcol(PlannerInfo *root,
              * the expression collation matches the index collation.  Also,
              * there must be an array type to construct an array later.
              */
-            if (!IndexCollMatchesExprColl(index->indexcollations[indexcol], inputcollid) ||
+            if (!IndexCollMatchesExprColl(index->indexcollations[indexcol],
+                                          inputcollid) ||
                 !op_in_opfamily(matchOpno, index->opfamily[indexcol]) ||
                 !OidIsValid(arraytype))
                 break;
+
+            /* Forbid this transformation for composite types.  XXX why? */
+            if (type_is_rowtype(consttype) ||
+                type_is_rowtype(exprType(indexExpr)))
+                break;
+
             firstTime = false;
         }
         else
         {
-            if (opno != matchOpno ||
+            if (matchOpno != opno ||
                 inputcollid != subClause->inputcollid ||
                 consttype != exprType(constExpr))
                 break;
         }

         /*
-         * Check if our list of constants in match_clause_to_indexcol's
-         * understanding of const-ness have something other than Const.
+         * The righthand inputs don't necessarily have to be plain Consts, but
+         * make_SAOP_expr needs to know if any are not.
          */
         if (!IsA(constExpr, Const))
             haveNonConst = true;
+
         consts = lappend(consts, constExpr);
     }

     /*
      * Handle failed conversion from breaking out of the loop because of an
-     * unsupported qual.  Free the consts list and return NULL to indicate the
-     * conversion failed.
+     * unsupported qual.  Also check that we have an indexExpr, just in case
+     * the OR list was somehow empty (it shouldn't be).  Return NULL to
+     * indicate the conversion failed.
      */
-    if (lc != NULL)
+    if (lc != NULL || indexExpr == NULL)
     {
-        list_free(consts);
+        list_free(consts);        /* might as well */
         return NULL;
     }

+    /*
+     * Build the new SAOP node.  We use the indexExpr from the last OR arm;
+     * since all the arms passed match_index_to_operand, it shouldn't matter
+     * which one we use.  But using "inputcollid" twice is a bit of a cheat:
+     * we might end up with an array Const node that is labeled with a
+     * collation despite its elements being of a noncollatable type.  But
+     * nothing is likely to complain about that, so we don't bother being more
+     * accurate.
+     */
     saopexpr = make_SAOP_expr(matchOpno, indexExpr, consttype, inputcollid,
                               inputcollid, consts, haveNonConst);
+    Assert(saopexpr != NULL);

     /*
-     * Finally, build an IndexClause based on the SAOP node.  Use
-     * make_simple_restrictinfo() to get RestrictInfo with clean selectivity
-     * estimations, because they may differ from the estimation made for an OR
-     * clause.  Although it is not a lossy expression, keep the original rinfo
-     * in iclause->rinfo as prescribed.
+     * Finally, build an IndexClause based on the SAOP node.  It's not lossy.
      */
     iclause = makeNode(IndexClause);
     iclause->rinfo = rinfo;
     iclause->indexquals = list_make1(make_simple_restrictinfo(root,
-                                                              &saopexpr->xpr));
+                                                              (Expr *) saopexpr));
     iclause->lossy = false;
     iclause->indexcol = indexcol;
     iclause->indexcols = NIL;