]> git.ipfire.org Git - thirdparty/postgresql.git/commitdiff
Fix index-only scan plans, take 2.
authorTom Lane <tgl@sss.pgh.pa.us>
Mon, 3 Jan 2022 20:42:27 +0000 (15:42 -0500)
committerTom Lane <tgl@sss.pgh.pa.us>
Mon, 3 Jan 2022 20:42:27 +0000 (15:42 -0500)
Commit 4ace45677 failed to fix the problem fully, because the
same issue of attempting to fetch a non-returnable index column
can occur when rechecking the indexqual after using a lossy index
operator.  Moreover, it broke EXPLAIN for such indexquals (which
indicates a gap in our test cases :-().

Revert the code changes of 4ace45677 in favor of adding a new field
to struct IndexOnlyScan, containing a version of the indexqual that
can be executed against the index-returned tuple without using any
non-returnable columns.  (The restrictions imposed by check_index_only
guarantee this is possible, although we may have to recompute indexed
expressions.)  Support construction of that during setrefs.c
processing by marking IndexOnlyScan.indextlist entries as resjunk
if they can't be returned, rather than removing them entirely.
(We could alternatively require setrefs.c to look up the IndexOptInfo
again, but abusing resjunk this way seems like a reasonably safe way
to avoid needing to do that.)

This solution isn't great from an API-stability standpoint: if there
are any extensions out there that build IndexOnlyScan structs directly,
they'll be broken in the next minor releases.  However, only a very
invasive extension would be likely to do such a thing.  There's no
change in the Path representation, so typical planner extensions
shouldn't have a problem.

As before, back-patch to all supported branches.

Discussion: https://postgr.es/m/3179992.1641150853@sss.pgh.pa.us
Discussion: https://postgr.es/m/17350-b5bdcf476e5badbb@postgresql.org

12 files changed:
src/backend/commands/explain.c
src/backend/executor/nodeIndexonlyscan.c
src/backend/nodes/copyfuncs.c
src/backend/nodes/outfuncs.c
src/backend/nodes/readfuncs.c
src/backend/optimizer/plan/createplan.c
src/backend/optimizer/plan/setrefs.c
src/backend/optimizer/plan/subselect.c
src/include/nodes/execnodes.h
src/include/nodes/plannodes.h
src/test/regress/expected/gist.out
src/test/regress/sql/gist.sql

index e9ba140808e17b8d670c2320c578dd3227ef254f..e57cf3ad3511a97eea81e92eef4de0fe74639ae1 100644 (file)
@@ -1524,7 +1524,7 @@ ExplainNode(PlanState *planstate, List *ancestors,
                case T_IndexOnlyScan:
                        show_scan_qual(((IndexOnlyScan *) plan)->indexqual,
                                                   "Index Cond", planstate, ancestors, es);
-                       if (((IndexOnlyScan *) plan)->indexqual)
+                       if (((IndexOnlyScan *) plan)->recheckqual)
                                show_instrumentation_count("Rows Removed by Index Recheck", 2,
                                                                                   planstate, es);
                        show_scan_qual(((IndexOnlyScan *) plan)->indexorderby,
index 7d2d50691e8b300e476e2c5bd2731e1bc39d8c0f..c7d18bd0c37477ca7cd014bb426fd7aba2a2bd02 100644 (file)
@@ -208,13 +208,11 @@ IndexOnlyNext(IndexOnlyScanState *node)
 
                /*
                 * If the index was lossy, we have to recheck the index quals.
-                * (Currently, this can never happen, but we should support the case
-                * for possible future use, eg with GiST indexes.)
                 */
                if (scandesc->xs_recheck)
                {
                        econtext->ecxt_scantuple = slot;
-                       if (!ExecQualAndReset(node->indexqual, econtext))
+                       if (!ExecQualAndReset(node->recheckqual, econtext))
                        {
                                /* Fails recheck, so drop it and loop back for another */
                                InstrCountFiltered2(node, 1);
@@ -548,8 +546,8 @@ ExecInitIndexOnlyScan(IndexOnlyScan *node, EState *estate, int eflags)
         */
        indexstate->ss.ps.qual =
                ExecInitQual(node->scan.plan.qual, (PlanState *) indexstate);
-       indexstate->indexqual =
-               ExecInitQual(node->indexqual, (PlanState *) indexstate);
+       indexstate->recheckqual =
+               ExecInitQual(node->recheckqual, (PlanState *) indexstate);
 
        /*
         * If we are just doing EXPLAIN (ie, aren't going to run the plan), stop
index a38d6e4db154b48ddb72bbb72c226400deb86ddc..21c57e42fcc19fe17c78c28d3645ce6495e66ca4 100644 (file)
@@ -513,6 +513,7 @@ _copyIndexOnlyScan(const IndexOnlyScan *from)
         */
        COPY_SCALAR_FIELD(indexid);
        COPY_NODE_FIELD(indexqual);
+       COPY_NODE_FIELD(recheckqual);
        COPY_NODE_FIELD(indexorderby);
        COPY_NODE_FIELD(indextlist);
        COPY_SCALAR_FIELD(indexorderdir);
index fcc0fbd703edfa1bdda17dd90c8d101b08998606..aca20907888e5c33e974580861e57e9a4c9c152d 100644 (file)
@@ -582,6 +582,7 @@ _outIndexOnlyScan(StringInfo str, const IndexOnlyScan *node)
 
        WRITE_OID_FIELD(indexid);
        WRITE_NODE_FIELD(indexqual);
+       WRITE_NODE_FIELD(recheckqual);
        WRITE_NODE_FIELD(indexorderby);
        WRITE_NODE_FIELD(indextlist);
        WRITE_ENUM_FIELD(indexorderdir, ScanDirection);
index 1a72dd3be39acc9083f3d3344743c55757d9114c..c46fb924cfada63ededeaddfe18bce73aa8973a6 100644 (file)
@@ -1786,6 +1786,7 @@ _readIndexOnlyScan(void)
 
        READ_OID_FIELD(indexid);
        READ_NODE_FIELD(indexqual);
+       READ_NODE_FIELD(recheckqual);
        READ_NODE_FIELD(indexorderby);
        READ_NODE_FIELD(indextlist);
        READ_ENUM_FIELD(indexorderdir, ScanDirection);
index 9f935b08f97faeafd4f571b5495c2c130b996540..839f20a7f6f56b0ffd1a393363c8b20e47ceacdd 100644 (file)
@@ -20,7 +20,6 @@
 #include <math.h>
 
 #include "access/sysattr.h"
-#include "catalog/pg_am.h"
 #include "catalog/pg_class.h"
 #include "foreign/fdwapi.h"
 #include "miscadmin.h"
@@ -172,10 +171,10 @@ static IndexScan *make_indexscan(List *qptlist, List *qpqual, Index scanrelid,
                           ScanDirection indexscandir);
 static IndexOnlyScan *make_indexonlyscan(List *qptlist, List *qpqual,
                                   Index scanrelid, Oid indexid,
-                                  List *indexqual, List *indexorderby,
+                                  List *indexqual, List *recheckqual,
+                                  List *indexorderby,
                                   List *indextlist,
                                   ScanDirection indexscandir);
-static List *make_indexonly_tlist(IndexOptInfo *indexinfo);
 static BitmapIndexScan *make_bitmap_indexscan(Index scanrelid, Oid indexid,
                                          List *indexqual,
                                          List *indexqualorig);
@@ -579,7 +578,7 @@ create_scan_plan(PlannerInfo *root, Path *best_path, int flags)
                if (best_path->pathtype == T_IndexOnlyScan)
                {
                        /* For index-only scan, the preferred tlist is the index's */
-                       tlist = copyObject(make_indexonly_tlist(((IndexPath *) best_path)->indexinfo));
+                       tlist = copyObject(((IndexPath *) best_path)->indexinfo->indextlist);
 
                        /*
                         * Transfer sortgroupref data to the replacement tlist, if
@@ -2585,7 +2584,8 @@ create_indexscan_plan(PlannerInfo *root,
        List       *indexquals = best_path->indexquals;
        List       *indexorderbys = best_path->indexorderbys;
        Index           baserelid = best_path->path.parent->relid;
-       Oid                     indexoid = best_path->indexinfo->indexoid;
+       IndexOptInfo *indexinfo = best_path->indexinfo;
+       Oid                     indexoid = indexinfo->indexoid;
        List       *qpqual;
        List       *stripped_indexquals;
        List       *fixed_indexquals;
@@ -2718,6 +2718,24 @@ create_indexscan_plan(PlannerInfo *root,
                }
        }
 
+       /*
+        * For an index-only scan, we must mark indextlist entries as resjunk if
+        * they are columns that the index AM can't return; this cues setrefs.c to
+        * not generate references to those columns.
+        */
+       if (indexonly)
+       {
+               int                     i = 0;
+
+               foreach(l, indexinfo->indextlist)
+               {
+                       TargetEntry *indextle = (TargetEntry *) lfirst(l);
+
+                       indextle->resjunk = !indexinfo->canreturn[i];
+                       i++;
+               }
+       }
+
        /* Finally ready to build the plan node */
        if (indexonly)
                scan_plan = (Scan *) make_indexonlyscan(tlist,
@@ -2725,8 +2743,9 @@ create_indexscan_plan(PlannerInfo *root,
                                                                                                baserelid,
                                                                                                indexoid,
                                                                                                fixed_indexquals,
+                                                                                               stripped_indexquals,
                                                                                                fixed_indexorderbys,
-                                                                                               make_indexonly_tlist(best_path->indexinfo),
+                                                                                               indexinfo->indextlist,
                                                                                                best_path->indexscandir);
        else
                scan_plan = (Scan *) make_indexscan(tlist,
@@ -4975,6 +4994,7 @@ make_indexonlyscan(List *qptlist,
                                   Index scanrelid,
                                   Oid indexid,
                                   List *indexqual,
+                                  List *recheckqual,
                                   List *indexorderby,
                                   List *indextlist,
                                   ScanDirection indexscandir)
@@ -4989,6 +5009,7 @@ make_indexonlyscan(List *qptlist,
        node->scan.scanrelid = scanrelid;
        node->indexid = indexid;
        node->indexqual = indexqual;
+       node->recheckqual = recheckqual;
        node->indexorderby = indexorderby;
        node->indextlist = indextlist;
        node->indexorderdir = indexscandir;
@@ -4996,53 +5017,6 @@ make_indexonlyscan(List *qptlist,
        return node;
 }
 
-/*
- * make_indexonly_tlist
- *
- * Construct the indextlist for an IndexOnlyScan plan node.
- * We must replace any column that can't be returned by the index AM
- * with a null Const of the appropriate datatype.  This is necessary
- * to prevent setrefs.c from trying to use the value of such a column,
- * and anyway it makes the indextlist a better representative of what
- * the indexscan will really return.  (We do this here, not where the
- * IndexOptInfo is originally constructed, because earlier planner
- * steps need to know what is in such columns.)
- */
-static List *
-make_indexonly_tlist(IndexOptInfo *indexinfo)
-{
-       List       *result;
-       int                     i;
-       ListCell   *lc;
-
-       /* We needn't work hard for the common case of btrees. */
-       if (indexinfo->relam == BTREE_AM_OID)
-               return indexinfo->indextlist;
-
-       result = NIL;
-       i = 0;
-       foreach(lc, indexinfo->indextlist)
-       {
-               TargetEntry *indextle = (TargetEntry *) lfirst(lc);
-
-               if (indexinfo->canreturn[i])
-                       result = lappend(result, indextle);
-               else
-               {
-                       TargetEntry *newtle = makeNode(TargetEntry);
-                       Node       *texpr = (Node *) indextle->expr;
-
-                       memcpy(newtle, indextle, sizeof(TargetEntry));
-                       newtle->expr = (Expr *) makeNullConst(exprType(texpr),
-                                                                                                 exprTypmod(texpr),
-                                                                                                 exprCollation(texpr));
-                       result = lappend(result, newtle);
-               }
-               i++;
-       }
-       return result;
-}
-
 static BitmapIndexScan *
 make_bitmap_indexscan(Index scanrelid,
                                          Oid indexid,
index 80e6e0da0d6f428b1524a5ee86d86e26787e8ef3..a6ad28570ace53b74a9c99099f4749b43d5eba63 100644 (file)
@@ -1019,8 +1019,26 @@ set_indexonlyscan_references(PlannerInfo *root,
                                                         int rtoffset)
 {
        indexed_tlist *index_itlist;
+       List       *stripped_indextlist;
+       ListCell   *lc;
+
+       /*
+        * Vars in the plan node's targetlist, qual, and recheckqual must only
+        * reference columns that the index AM can actually return.  To ensure
+        * this, remove non-returnable columns (which are marked as resjunk) from
+        * the indexed tlist.  We can just drop them because the indexed_tlist
+        * machinery pays attention to TLE resnos, not physical list position.
+        */
+       stripped_indextlist = NIL;
+       foreach(lc, plan->indextlist)
+       {
+               TargetEntry *indextle = (TargetEntry *) lfirst(lc);
 
-       index_itlist = build_tlist_index(plan->indextlist);
+               if (!indextle->resjunk)
+                       stripped_indextlist = lappend(stripped_indextlist, indextle);
+       }
+
+       index_itlist = build_tlist_index(stripped_indextlist);
 
        plan->scan.scanrelid += rtoffset;
        plan->scan.plan.targetlist = (List *)
@@ -1035,6 +1053,12 @@ set_indexonlyscan_references(PlannerInfo *root,
                                           index_itlist,
                                           INDEX_VAR,
                                           rtoffset);
+       plan->recheckqual = (List *)
+               fix_upper_expr(root,
+                                          (Node *) plan->recheckqual,
+                                          index_itlist,
+                                          INDEX_VAR,
+                                          rtoffset);
        /* indexqual is already transformed to reference index columns */
        plan->indexqual = fix_scan_list(root, plan->indexqual, rtoffset);
        /* indexorderby is already transformed to reference index columns */
index a4f54e3fe312b478f68d0a8ba94ba3bd47543543..09e5c58e4edeae3f2ce5dd17a5dde5981bf4e3d0 100644 (file)
@@ -2084,6 +2084,8 @@ finalize_plan(PlannerInfo *root, Plan *plan,
                case T_IndexOnlyScan:
                        finalize_primnode((Node *) ((IndexOnlyScan *) plan)->indexqual,
                                                          &context);
+                       finalize_primnode((Node *) ((IndexOnlyScan *) plan)->recheckqual,
+                                                         &context);
                        finalize_primnode((Node *) ((IndexOnlyScan *) plan)->indexorderby,
                                                          &context);
 
index 77ada4377f3fb6aefdda82238782e7ab87c0b889..0ae7ac88d607f1589e54312f5573de2966be35a0 100644 (file)
@@ -1317,7 +1317,7 @@ typedef struct IndexScanState
 /* ----------------
  *      IndexOnlyScanState information
  *
- *             indexqual                  execution state for indexqual expressions
+ *             recheckqual                execution state for recheckqual expressions
  *             ScanKeys                   Skey structures for index quals
  *             NumScanKeys                number of ScanKeys
  *             OrderByKeys                Skey structures for index ordering operators
@@ -1335,7 +1335,7 @@ typedef struct IndexScanState
 typedef struct IndexOnlyScanState
 {
        ScanState       ss;                             /* its first field is NodeTag */
-       ExprState  *indexqual;
+       ExprState  *recheckqual;
        ScanKey         ioss_ScanKeys;
        int                     ioss_NumScanKeys;
        ScanKey         ioss_OrderByKeys;
index a090ed67acf850b84632e11e05d6bb4ae25d533f..6a161b2df97e4ecb5e8fdb0885475f750ee86150 100644 (file)
@@ -418,15 +418,28 @@ typedef struct IndexScan
  * index-only scan, in which the data comes from the index not the heap.
  * Because of this, *all* Vars in the plan node's targetlist, qual, and
  * index expressions reference index columns and have varno = INDEX_VAR.
- * Hence we do not need separate indexqualorig and indexorderbyorig lists,
- * since their contents would be equivalent to indexqual and indexorderby.
+ *
+ * We could almost use indexqual directly against the index's output tuple
+ * when rechecking lossy index operators, but that won't work for quals on
+ * index columns that are not retrievable.  Hence, recheckqual is needed
+ * for rechecks: it expresses the same condition as indexqual, but using
+ * only index columns that are retrievable.  (We will not generate an
+ * index-only scan if this is not possible.  An example is that if an
+ * index has table column "x" in a retrievable index column "ind1", plus
+ * an expression f(x) in a non-retrievable column "ind2", an indexable
+ * query on f(x) will use "ind2" in indexqual and f(ind1) in recheckqual.
+ * Without the "ind1" column, an index-only scan would be disallowed.)
+ *
+ * We don't currently need a recheckable equivalent of indexorderby,
+ * because we don't support lossy operators in index ORDER BY.
  *
  * To help EXPLAIN interpret the index Vars for display, we provide
  * indextlist, which represents the contents of the index as a targetlist
  * with one TLE per index column.  Vars appearing in this list reference
  * the base table, and this is the only field in the plan node that may
- * contain such Vars.  Note however that index columns that the AM can't
- * reconstruct are replaced by null Consts in indextlist.
+ * contain such Vars.  Also, for the convenience of setrefs.c, TLEs in
+ * indextlist are marked as resjunk if they correspond to columns that
+ * the index AM cannot reconstruct.
  * ----------------
  */
 typedef struct IndexOnlyScan
@@ -437,6 +450,7 @@ typedef struct IndexOnlyScan
        List       *indexorderby;       /* list of index ORDER BY exprs */
        List       *indextlist;         /* TargetEntry list describing index's cols */
        ScanDirection indexorderdir;    /* forward or backward or don't care */
+       List       *recheckqual;        /* index quals in recheckable form */
 } IndexOnlyScan;
 
 /* ----------------
index 823c2aaa771f3c63f4a1a07c223fde9b1c3edb6a..6a9e33eb430aba96c8052e871242b221811b95a2 100644 (file)
@@ -266,6 +266,37 @@ where p <@ box(point(5, 5), point(5.3, 5.3));
  <(5.3,5.3),1>
 (7 rows)
 
+-- Similarly, test that index rechecks involving a non-returnable column
+-- are done correctly.
+explain (verbose, costs off)
+select p from gist_tbl where circle(p,1) @> circle(point(0,0),0.95);
+                                      QUERY PLAN                                       
+---------------------------------------------------------------------------------------
+ Index Only Scan using gist_tbl_multi_index on public.gist_tbl
+   Output: p
+   Index Cond: ((circle(gist_tbl.p, '1'::double precision)) @> '<(0,0),0.95>'::circle)
+(3 rows)
+
+select p from gist_tbl where circle(p,1) @> circle(point(0,0),0.95);
+   p   
+-------
+ (0,0)
+(1 row)
+
+-- This case isn't supported, but it should at least EXPLAIN correctly.
+explain (verbose, costs off)
+select p from gist_tbl order by circle(p,1) <-> point(0,0) limit 1;
+                                     QUERY PLAN                                     
+------------------------------------------------------------------------------------
+ Limit
+   Output: p, ((circle(p, '1'::double precision) <-> '(0,0)'::point))
+   ->  Index Only Scan using gist_tbl_multi_index on public.gist_tbl
+         Output: p, (circle(p, '1'::double precision) <-> '(0,0)'::point)
+         Order By: ((circle(gist_tbl.p, '1'::double precision)) <-> '(0,0)'::point)
+(5 rows)
+
+select p from gist_tbl order by circle(p,1) <-> point(0,0) limit 1;
+ERROR:  lossy distance functions are not supported in index-only scans
 -- Clean up
 reset enable_seqscan;
 reset enable_bitmapscan;
index c7933d02f0af15fa9b275954ddfb612f3e8a79cf..ab77dc2721846a260ad3f61450d4b59cff7e2cbf 100644 (file)
@@ -139,6 +139,17 @@ where p <@ box(point(5, 5), point(5.3, 5.3));
 select circle(p,1) from gist_tbl
 where p <@ box(point(5, 5), point(5.3, 5.3));
 
+-- Similarly, test that index rechecks involving a non-returnable column
+-- are done correctly.
+explain (verbose, costs off)
+select p from gist_tbl where circle(p,1) @> circle(point(0,0),0.95);
+select p from gist_tbl where circle(p,1) @> circle(point(0,0),0.95);
+
+-- This case isn't supported, but it should at least EXPLAIN correctly.
+explain (verbose, costs off)
+select p from gist_tbl order by circle(p,1) <-> point(0,0) limit 1;
+select p from gist_tbl order by circle(p,1) <-> point(0,0) limit 1;
+
 -- Clean up
 reset enable_seqscan;
 reset enable_bitmapscan;