Skip to content

Commit 20d08b2

Browse files
tglsfdctglsfdc
committed
Fix index-only scan plans, take 2.
Commit 4ace456 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 4ace456 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
1 parent 6c81108 commit 20d08b2

File tree

12 files changed

+123
-66
lines changed

12 files changed

+123
-66
lines changed

‎src/backend/commands/explain.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1706,7 +1706,7 @@ ExplainNode(PlanState *planstate, List *ancestors,
17061706
case T_IndexOnlyScan:
17071707
show_scan_qual(((IndexOnlyScan *) plan)->indexqual,
17081708
"Index Cond", planstate, ancestors, es);
1709-
if (((IndexOnlyScan *) plan)->indexqual)
1709+
if (((IndexOnlyScan *) plan)->recheckqual)
17101710
show_instrumentation_count("Rows Removed by Index Recheck", 2,
17111711
planstate, es);
17121712
show_scan_qual(((IndexOnlyScan *) plan)->indexorderby,

‎src/backend/executor/nodeIndexonlyscan.c

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -214,13 +214,11 @@ IndexOnlyNext(IndexOnlyScanState *node)
214214

215215
/*
216216
* If the index was lossy, we have to recheck the index quals.
217-
* (Currently, this can never happen, but we should support the case
218-
* for possible future use, eg with GiST indexes.)
219217
*/
220218
if (scandesc->xs_recheck)
221219
{
222220
econtext->ecxt_scantuple = slot;
223-
if (!ExecQualAndReset(node->indexqual, econtext))
221+
if (!ExecQualAndReset(node->recheckqual, econtext))
224222
{
225223
/* Fails recheck, so drop it and loop back for another */
226224
InstrCountFiltered2(node, 1);
@@ -555,8 +553,8 @@ ExecInitIndexOnlyScan(IndexOnlyScan *node, EState *estate, int eflags)
555553
*/
556554
indexstate->ss.ps.qual =
557555
ExecInitQual(node->scan.plan.qual, (PlanState *) indexstate);
558-
indexstate->indexqual =
559-
ExecInitQual(node->indexqual, (PlanState *) indexstate);
556+
indexstate->recheckqual =
557+
ExecInitQual(node->recheckqual, (PlanState *) indexstate);
560558

561559
/*
562560
* If we are just doing EXPLAIN (ie, aren't going to run the plan), stop

‎src/backend/nodes/copyfuncs.c

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -515,6 +515,7 @@ _copyIndexOnlyScan(const IndexOnlyScan *from)
515515
*/
516516
COPY_SCALAR_FIELD(indexid);
517517
COPY_NODE_FIELD(indexqual);
518+
COPY_NODE_FIELD(recheckqual);
518519
COPY_NODE_FIELD(indexorderby);
519520
COPY_NODE_FIELD(indextlist);
520521
COPY_SCALAR_FIELD(indexorderdir);

‎src/backend/nodes/outfuncs.c

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -573,6 +573,7 @@ _outIndexOnlyScan(StringInfo str, const IndexOnlyScan *node)
573573

574574
WRITE_OID_FIELD(indexid);
575575
WRITE_NODE_FIELD(indexqual);
576+
WRITE_NODE_FIELD(recheckqual);
576577
WRITE_NODE_FIELD(indexorderby);
577578
WRITE_NODE_FIELD(indextlist);
578579
WRITE_ENUM_FIELD(indexorderdir, ScanDirection);

‎src/backend/nodes/readfuncs.c

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1837,6 +1837,7 @@ _readIndexOnlyScan(void)
18371837

18381838
READ_OID_FIELD(indexid);
18391839
READ_NODE_FIELD(indexqual);
1840+
READ_NODE_FIELD(recheckqual);
18401841
READ_NODE_FIELD(indexorderby);
18411842
READ_NODE_FIELD(indextlist);
18421843
READ_ENUM_FIELD(indexorderdir, ScanDirection);

‎src/backend/optimizer/plan/createplan.c

Lines changed: 27 additions & 53 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,6 @@
2020
#include <math.h>
2121

2222
#include "access/sysattr.h"
23-
#include "catalog/pg_am.h"
2423
#include "catalog/pg_class.h"
2524
#include "foreign/fdwapi.h"
2625
#include "miscadmin.h"
@@ -181,10 +180,10 @@ static IndexScan *make_indexscan(List *qptlist, List *qpqual, Index scanrelid,
181180
ScanDirection indexscandir);
182181
static IndexOnlyScan *make_indexonlyscan(List *qptlist, List *qpqual,
183182
Index scanrelid, Oid indexid,
184-
List *indexqual, List *indexorderby,
183+
List *indexqual, List *recheckqual,
184+
List *indexorderby,
185185
List *indextlist,
186186
ScanDirection indexscandir);
187-
static List *make_indexonly_tlist(IndexOptInfo *indexinfo);
188187
static BitmapIndexScan *make_bitmap_indexscan(Index scanrelid, Oid indexid,
189188
List *indexqual,
190189
List *indexqualorig);
@@ -602,7 +601,7 @@ create_scan_plan(PlannerInfo *root, Path *best_path, int flags)
602601
if (best_path->pathtype == T_IndexOnlyScan)
603602
{
604603
/* For index-only scan, the preferred tlist is the index's */
605-
tlist = copyObject(make_indexonly_tlist(((IndexPath *) best_path)->indexinfo));
604+
tlist = copyObject(((IndexPath *) best_path)->indexinfo->indextlist);
606605

607606
/*
608607
* Transfer sortgroupref data to the replacement tlist, if
@@ -2846,7 +2845,8 @@ create_indexscan_plan(PlannerInfo *root,
28462845
List *indexclauses = best_path->indexclauses;
28472846
List *indexorderbys = best_path->indexorderbys;
28482847
Index baserelid = best_path->path.parent->relid;
2849-
Oid indexoid = best_path->indexinfo->indexoid;
2848+
IndexOptInfo *indexinfo = best_path->indexinfo;
2849+
Oid indexoid = indexinfo->indexoid;
28502850
List *qpqual;
28512851
List *stripped_indexquals;
28522852
List *fixed_indexquals;
@@ -2976,15 +2976,34 @@ create_indexscan_plan(PlannerInfo *root,
29762976
}
29772977
}
29782978

2979+
/*
2980+
* For an index-only scan, we must mark indextlist entries as resjunk if
2981+
* they are columns that the index AM can't return; this cues setrefs.c to
2982+
* not generate references to those columns.
2983+
*/
2984+
if (indexonly)
2985+
{
2986+
int i = 0;
2987+
2988+
foreach(l, indexinfo->indextlist)
2989+
{
2990+
TargetEntry *indextle = (TargetEntry *) lfirst(l);
2991+
2992+
indextle->resjunk = !indexinfo->canreturn[i];
2993+
i++;
2994+
}
2995+
}
2996+
29792997
/* Finally ready to build the plan node */
29802998
if (indexonly)
29812999
scan_plan = (Scan *) make_indexonlyscan(tlist,
29823000
qpqual,
29833001
baserelid,
29843002
indexoid,
29853003
fixed_indexquals,
3004+
stripped_indexquals,
29863005
fixed_indexorderbys,
2987-
make_indexonly_tlist(best_path->indexinfo),
3006+
indexinfo->indextlist,
29883007
best_path->indexscandir);
29893008
else
29903009
scan_plan = (Scan *) make_indexscan(tlist,
@@ -5290,6 +5309,7 @@ make_indexonlyscan(List *qptlist,
52905309
Index scanrelid,
52915310
Oid indexid,
52925311
List *indexqual,
5312+
List *recheckqual,
52935313
List *indexorderby,
52945314
List *indextlist,
52955315
ScanDirection indexscandir)
@@ -5304,60 +5324,14 @@ make_indexonlyscan(List *qptlist,
53045324
node->scan.scanrelid = scanrelid;
53055325
node->indexid = indexid;
53065326
node->indexqual = indexqual;
5327+
node->recheckqual = recheckqual;
53075328
node->indexorderby = indexorderby;
53085329
node->indextlist = indextlist;
53095330
node->indexorderdir = indexscandir;
53105331

53115332
return node;
53125333
}
53135334

5314-
/*
5315-
* make_indexonly_tlist
5316-
*
5317-
* Construct the indextlist for an IndexOnlyScan plan node.
5318-
* We must replace any column that can't be returned by the index AM
5319-
* with a null Const of the appropriate datatype. This is necessary
5320-
* to prevent setrefs.c from trying to use the value of such a column,
5321-
* and anyway it makes the indextlist a better representative of what
5322-
* the indexscan will really return. (We do this here, not where the
5323-
* IndexOptInfo is originally constructed, because earlier planner
5324-
* steps need to know what is in such columns.)
5325-
*/
5326-
static List *
5327-
make_indexonly_tlist(IndexOptInfo *indexinfo)
5328-
{
5329-
List *result;
5330-
int i;
5331-
ListCell *lc;
5332-
5333-
/* We needn't work hard for the common case of btrees. */
5334-
if (indexinfo->relam == BTREE_AM_OID)
5335-
return indexinfo->indextlist;
5336-
5337-
result = NIL;
5338-
i = 0;
5339-
foreach(lc, indexinfo->indextlist)
5340-
{
5341-
TargetEntry *indextle = (TargetEntry *) lfirst(lc);
5342-
5343-
if (indexinfo->canreturn[i])
5344-
result = lappend(result, indextle);
5345-
else
5346-
{
5347-
TargetEntry *newtle = makeNode(TargetEntry);
5348-
Node *texpr = (Node *) indextle->expr;
5349-
5350-
memcpy(newtle, indextle, sizeof(TargetEntry));
5351-
newtle->expr = (Expr *) makeNullConst(exprType(texpr),
5352-
exprTypmod(texpr),
5353-
exprCollation(texpr));
5354-
result = lappend(result, newtle);
5355-
}
5356-
i++;
5357-
}
5358-
return result;
5359-
}
5360-
53615335
static BitmapIndexScan *
53625336
make_bitmap_indexscan(Index scanrelid,
53635337
Oid indexid,

‎src/backend/optimizer/plan/setrefs.c

Lines changed: 25 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1017,8 +1017,26 @@ set_indexonlyscan_references(PlannerInfo *root,
10171017
int rtoffset)
10181018
{
10191019
indexed_tlist *index_itlist;
1020+
List *stripped_indextlist;
1021+
ListCell *lc;
1022+
1023+
/*
1024+
* Vars in the plan node's targetlist, qual, and recheckqual must only
1025+
* reference columns that the index AM can actually return. To ensure
1026+
* this, remove non-returnable columns (which are marked as resjunk) from
1027+
* the indexed tlist. We can just drop them because the indexed_tlist
1028+
* machinery pays attention to TLE resnos, not physical list position.
1029+
*/
1030+
stripped_indextlist = NIL;
1031+
foreach(lc, plan->indextlist)
1032+
{
1033+
TargetEntry *indextle = (TargetEntry *) lfirst(lc);
10201034

1021-
index_itlist = build_tlist_index(plan->indextlist);
1035+
if (!indextle->resjunk)
1036+
stripped_indextlist = lappend(stripped_indextlist, indextle);
1037+
}
1038+
1039+
index_itlist = build_tlist_index(stripped_indextlist);
10221040

10231041
plan->scan.scanrelid += rtoffset;
10241042
plan->scan.plan.targetlist = (List *)
@@ -1033,6 +1051,12 @@ set_indexonlyscan_references(PlannerInfo *root,
10331051
index_itlist,
10341052
INDEX_VAR,
10351053
rtoffset);
1054+
plan->recheckqual = (List *)
1055+
fix_upper_expr(root,
1056+
(Node *) plan->recheckqual,
1057+
index_itlist,
1058+
INDEX_VAR,
1059+
rtoffset);
10361060
/* indexqual is already transformed to reference index columns */
10371061
plan->indexqual = fix_scan_list(root, plan->indexqual, rtoffset);
10381062
/* indexorderby is already transformed to reference index columns */

‎src/backend/optimizer/plan/subselect.c

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2308,6 +2308,8 @@ finalize_plan(PlannerInfo *root, Plan *plan,
23082308
case T_IndexOnlyScan:
23092309
finalize_primnode((Node *) ((IndexOnlyScan *) plan)->indexqual,
23102310
&context);
2311+
finalize_primnode((Node *) ((IndexOnlyScan *) plan)->recheckqual,
2312+
&context);
23112313
finalize_primnode((Node *) ((IndexOnlyScan *) plan)->indexorderby,
23122314
&context);
23132315

‎src/include/nodes/execnodes.h

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1454,7 +1454,7 @@ typedef struct IndexScanState
14541454
/* ----------------
14551455
* IndexOnlyScanState information
14561456
*
1457-
* indexqual execution state for indexqual expressions
1457+
* recheckqual execution state for recheckqual expressions
14581458
* ScanKeys Skey structures for index quals
14591459
* NumScanKeys number of ScanKeys
14601460
* OrderByKeys Skey structures for index ordering operators
@@ -1473,7 +1473,7 @@ typedef struct IndexScanState
14731473
typedef struct IndexOnlyScanState
14741474
{
14751475
ScanState ss; /* its first field is NodeTag */
1476-
ExprState *indexqual;
1476+
ExprState *recheckqual;
14771477
struct ScanKeyData *ioss_ScanKeys;
14781478
int ioss_NumScanKeys;
14791479
struct ScanKeyData *ioss_OrderByKeys;

‎src/include/nodes/plannodes.h

Lines changed: 18 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -418,15 +418,28 @@ typedef struct IndexScan
418418
* index-only scan, in which the data comes from the index not the heap.
419419
* Because of this, *all* Vars in the plan node's targetlist, qual, and
420420
* index expressions reference index columns and have varno = INDEX_VAR.
421-
* Hence we do not need separate indexqualorig and indexorderbyorig lists,
422-
* since their contents would be equivalent to indexqual and indexorderby.
421+
*
422+
* We could almost use indexqual directly against the index's output tuple
423+
* when rechecking lossy index operators, but that won't work for quals on
424+
* index columns that are not retrievable. Hence, recheckqual is needed
425+
* for rechecks: it expresses the same condition as indexqual, but using
426+
* only index columns that are retrievable. (We will not generate an
427+
* index-only scan if this is not possible. An example is that if an
428+
* index has table column "x" in a retrievable index column "ind1", plus
429+
* an expression f(x) in a non-retrievable column "ind2", an indexable
430+
* query on f(x) will use "ind2" in indexqual and f(ind1) in recheckqual.
431+
* Without the "ind1" column, an index-only scan would be disallowed.)
432+
*
433+
* We don't currently need a recheckable equivalent of indexorderby,
434+
* because we don't support lossy operators in index ORDER BY.
423435
*
424436
* To help EXPLAIN interpret the index Vars for display, we provide
425437
* indextlist, which represents the contents of the index as a targetlist
426438
* with one TLE per index column. Vars appearing in this list reference
427439
* the base table, and this is the only field in the plan node that may
428-
* contain such Vars. Note however that index columns that the AM can't
429-
* reconstruct are replaced by null Consts in indextlist.
440+
* contain such Vars. Also, for the convenience of setrefs.c, TLEs in
441+
* indextlist are marked as resjunk if they correspond to columns that
442+
* the index AM cannot reconstruct.
430443
* ----------------
431444
*/
432445
typedef struct IndexOnlyScan
@@ -437,6 +450,7 @@ typedef struct IndexOnlyScan
437450
List *indexorderby; /* list of index ORDER BY exprs */
438451
List *indextlist; /* TargetEntry list describing index's cols */
439452
ScanDirection indexorderdir; /* forward or backward or don't care */
453+
List *recheckqual; /* index quals in recheckable form */
440454
} IndexOnlyScan;
441455

442456
/* ----------------

‎src/test/regress/expected/gist.out

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -340,6 +340,37 @@ where p <@ box(point(5, 5), point(5.3, 5.3));
340340
<(5.3,5.3),1>
341341
(7 rows)
342342

343+
-- Similarly, test that index rechecks involving a non-returnable column
344+
-- are done correctly.
345+
explain (verbose, costs off)
346+
select p from gist_tbl where circle(p,1) @> circle(point(0,0),0.95);
347+
QUERY PLAN
348+
---------------------------------------------------------------------------------------
349+
Index Only Scan using gist_tbl_multi_index on public.gist_tbl
350+
Output: p
351+
Index Cond: ((circle(gist_tbl.p, '1'::double precision)) @> '<(0,0),0.95>'::circle)
352+
(3 rows)
353+
354+
select p from gist_tbl where circle(p,1) @> circle(point(0,0),0.95);
355+
p
356+
-------
357+
(0,0)
358+
(1 row)
359+
360+
-- This case isn't supported, but it should at least EXPLAIN correctly.
361+
explain (verbose, costs off)
362+
select p from gist_tbl order by circle(p,1) <-> point(0,0) limit 1;
363+
QUERY PLAN
364+
------------------------------------------------------------------------------------
365+
Limit
366+
Output: p, ((circle(p, '1'::double precision) <-> '(0,0)'::point))
367+
-> Index Only Scan using gist_tbl_multi_index on public.gist_tbl
368+
Output: p, (circle(p, '1'::double precision) <-> '(0,0)'::point)
369+
Order By: ((circle(gist_tbl.p, '1'::double precision)) <-> '(0,0)'::point)
370+
(5 rows)
371+
372+
select p from gist_tbl order by circle(p,1) <-> point(0,0) limit 1;
373+
ERROR: lossy distance functions are not supported in index-only scans
343374
-- Clean up
344375
reset enable_seqscan;
345376
reset enable_bitmapscan;

‎src/test/regress/sql/gist.sql

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -153,6 +153,17 @@ where p <@ box(point(5, 5), point(5.3, 5.3));
153153
select circle(p,1) from gist_tbl
154154
where p <@ box(point(5, 5), point(5.3, 5.3));
155155

156+
-- Similarly, test that index rechecks involving a non-returnable column
157+
-- are done correctly.
158+
explain (verbose, costs off)
159+
select p from gist_tbl where circle(p,1) @> circle(point(0,0),0.95);
160+
select p from gist_tbl where circle(p,1) @> circle(point(0,0),0.95);
161+
162+
-- This case isn't supported, but it should at least EXPLAIN correctly.
163+
explain (verbose, costs off)
164+
select p from gist_tbl order by circle(p,1) <-> point(0,0) limit 1;
165+
select p from gist_tbl order by circle(p,1) <-> point(0,0) limit 1;
166+
156167
-- Clean up
157168
reset enable_seqscan;
158169
reset enable_bitmapscan;

0 commit comments

Comments
 (0)