0001-Comments-for-the-0001-patch.patch

text/x-patch

Filename: 0001-Comments-for-the-0001-patch.patch
Type: text/x-patch
Part: 0
Message: Re: POC, WIP: OR-clause support for indexes

Patch

Same data as JSON: GET /api/v1/attachments/:id/patch the parsed metadata as JSON — format, series position, per-file stats; never the diff bytes. API reference →
Format: format-patch
Series: patch 0001
Subject: Comments for the 0001 patch
File+
src/backend/optimizer/path/indxpath.c 31 21
From ae279c2bef8b0b11739f79b49d68319894c63aa9 Mon Sep 17 00:00:00 2001
From: "Andrei V. Lepikhov" <lepihov@gmail.com>
Date: Wed, 9 Oct 2024 14:05:25 +0700
Subject: [PATCH 1/3] Comments for the 0001 patch

---
 src/backend/optimizer/path/indxpath.c | 52 ++++++++++++++++-----------
 1 file changed, 31 insertions(+), 21 deletions(-)

diff --git a/src/backend/optimizer/path/indxpath.c b/src/backend/optimizer/path/indxpath.c
index 46d576a0c2..602141911d 100644
--- a/src/backend/optimizer/path/indxpath.c
+++ b/src/backend/optimizer/path/indxpath.c
@@ -2557,6 +2557,10 @@ match_clause_to_index(PlannerInfo *root,
  *	  It is also possible to match ScalarArrayOpExpr clauses to indexes, when
  *	  the clause is of the form "indexkey op ANY (arrayconst)".
  *
+ *	  It is also possible to match a list of OR clauses if it may be transformed
+ *	  into single ScalarArrayOpExpr clause. On success, returning index clause
+ *	  will contain trasformed clause.
+ *
  *	  For boolean indexes, it is also possible to match the clause directly
  *	  to the indexkey; or perhaps the clause is (NOT indexkey).
  *
@@ -3157,6 +3161,10 @@ match_rowcompare_to_indexcol(PlannerInfo *root,
  * match_orclause_to_indexcol()
  *	  Handles the OR-expr case for match_clause_to_indexcol() in the case
  *	  when it could be transformed to ScalarArrayOpExpr.
+ *
+ *	  Given a list of an OR-clause args, attempts to transform this BoolExpr
+ *	  into single SAOP expression. On success, returns IndexClause, containing
+ *	  transformed expression or NULL, if failed.
  */
 static IndexClause *
 match_orclause_to_indexcol(PlannerInfo *root,
@@ -3184,11 +3192,12 @@ match_orclause_to_indexcol(PlannerInfo *root,
 	Assert(orclause->boolop == OR_EXPR);
 
 	/*
-	 * Iterate over OR entries.  Check that each OR entry is of the form:
-	 * (indexkey operator constant) or (constant operator indexkey). Operators
-	 * of all the entries must match.  Constant might be either Const or
-	 * Param.  Exit with NULL on first non-matching entry.  Exit is
-	 * implemented as a break from the loop, which is catched afterwards.
+	 * Try to convert list of OR-clauses to single SAOP expression. Each OR
+	 * entry must be in the form: (indexkey operator constant) or (constant
+	 * operator indexkey). Operators of all the entries must match. Constant
+	 * might be either Const or Param. To be effective, give up on first
+	 * non-matching entry. Exit is implemented as a break from the loop, which
+	 * is catched afterwards.
 	 */
 	foreach(lc, orclause->args)
 	{
@@ -3286,8 +3295,8 @@ match_orclause_to_indexcol(PlannerInfo *root,
 			break;
 
 		/*
-		 * For the first matching qual, save information about operator, type
-		 * and collation.  For the other quals just check the match with the
+		 * Save information about the operator, type, and collation for the
+		 * first matching qual. Then, check that subsequent quals match the
 		 * first.
 		 */
 		if (firstTime)
@@ -3298,9 +3307,9 @@ match_orclause_to_indexcol(PlannerInfo *root,
 			inputcollid = subClause->inputcollid;
 
 			/*
-			 * Check operator is present in the opfamily, expression collation
-			 * matches index collation.  Also, there must be an array type in
-			 * order to construct an array later.
+			 * Check operator is presented in the opfamily and expression
+			 * collation matches index collation. Also, there must be an array
+			 * type to construct an array later.
 			 */
 			if (!IndexCollMatchesExprColl(index->indexcollations[indexcol], inputcollid) ||
 				!op_in_opfamily(matchOpno, index->opfamily[indexcol]) ||
@@ -3332,12 +3341,14 @@ match_orclause_to_indexcol(PlannerInfo *root,
 		return NULL;
 	}
 
+	/*
+	 * Assemble an array from the list of constants. It seems more profitable to
+	 * build a const array. But in presence of parameters we don't have specific
+	 * value here and must employ an ArrayExpr instead.
+	 */
+
 	if (have_param)
 	{
-		/*
-		 * We need to construct an ArrayExpr given we have Param's not just
-		 * Const's.
-		 */
 		ArrayExpr  *arrayExpr = makeNode(ArrayExpr);
 
 		/* array_collid will be set by parse_collate.c */
@@ -3351,10 +3362,6 @@ match_orclause_to_indexcol(PlannerInfo *root,
 	}
 	else
 	{
-		/*
-		 * We have only Const's.  In this case we can construct an array
-		 * directly.
-		 */
 		int16		typlen;
 		bool		typbyval;
 		char		typalign;
@@ -3383,8 +3390,7 @@ match_orclause_to_indexcol(PlannerInfo *root,
 	}
 
 	/* Lookup for operator to fetch necessary information for the SAOP node */
-	opertup = SearchSysCache1(OPEROID,
-							  ObjectIdGetDatum(matchOpno));
+	opertup = SearchSysCache1(OPEROID, ObjectIdGetDatum(matchOpno));
 	if (!HeapTupleIsValid(opertup))
 		elog(ERROR, "cache lookup failed for operator %u", matchOpno);
 
@@ -3404,7 +3410,11 @@ match_orclause_to_indexcol(PlannerInfo *root,
 	ReleaseSysCache(opertup);
 
 	/*
-	 * Finally build an IndexClause based on the SAOP node.
+	 * Finally build an IndexClause based on the SAOP node. Use
+	 * make_simple_restrictinfo to get RestrictInfo with clean selectivity
+	 * estimations because it may differ from estimation made for an OR clause.
+	 * Although it is not a lossy expression, keep old version of rinfo in
+	 * iclause->rinfo to detect duplicates and recheck original clause.
 	 */
 	iclause = makeNode(IndexClause);
 	iclause->rinfo = rinfo;
-- 
2.39.5