Clean up match_orclause_to_indexcol().HEADmaster

Remove bogus stripping of RelabelTypes: that can result in building
an output SAOP tree with incorrect exposed exprType for the operands,
which might confuse polymorphic operators.  Moreover it demonstrably
prevents folding some OR-trees to SAOPs when the RHS expressions
have different base types that were coerced to the same type by
RelabelTypes.

Reduce prohibition on type_is_rowtype to just disallow type RECORD.
We need that because otherwise we would happily fold multiple RECORD
Consts into a RECORDARRAY Const even if they aren't the same record
type.  (We could allow that perhaps, if we checked that they all have
the same typmod, but the case doesn't seem worth that much effort.)
However, there is no reason at all to disallow the transformation
for named composite types, nor domains over them: as long as we can
find a suitable array type we're good.

Remove some assertions that seem rather out of place (it's not
this code's duty to verify that the RestrictInfo structure is
sane).  Rewrite some comments.

The issues with RelabelType stripping seem severe enough to
back-patch this into v18 where the code was introduced.

Author: Tender Wang <[email protected]>
Co-authored-by: Tom Lane <[email protected]>
Discussion: https://postgr.es/m/CAHewXN=aH7GQBk4fXU-WaEeVmQWUmBAeNyBfJ3VKzPphyPKUkQ@mail.gmail.com
Backpatch-through: 18

1 files changed, 45 insertions, 70 deletions

diff --git a/src/backend/optimizer/path/indxpath.c b/src/backend/optimizer/path/indxpath.c
index edc6d2ac1d3..c62e3f87724 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))
+		/* If it's not a RestrictInfo (i.e. it's a sub-AND), we can't use it */
+		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);
@@ -3407,22 +3381,6 @@ match_orclause_to_indexcol(PlannerInfo *root,
 		}
 
 		/*
-		 * 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
 		 * first.
@@ -3439,54 +3397,71 @@ 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;
+
+			/*
+			 * Disallow if either type is RECORD, mainly because we can't be
+			 * positive that all the RHS expressions are the same record type.
+			 */
+			if (consttype == RECORDOID || exprType(indexExpr) == RECORDOID)
+				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;