Skip to content

Commit 9c4f389

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 580e998 commit 9c4f389

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
@@ -1595,7 +1595,7 @@ ExplainNode(PlanState *planstate, List *ancestors,
15951595
case T_IndexOnlyScan:
15961596
show_scan_qual(((IndexOnlyScan *) plan)->indexqual,
15971597
"Index Cond", planstate, ancestors, es);
1598-
if (((IndexOnlyScan *) plan)->indexqual)
1598+
if (((IndexOnlyScan *) plan)->recheckqual)
15991599
show_instrumentation_count("Rows Removed by Index Recheck", 2,
16001600
planstate, es);
16011601
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
@@ -512,6 +512,7 @@ _copyIndexOnlyScan(const IndexOnlyScan *from)
512512
*/
513513
COPY_SCALAR_FIELD(indexid);
514514
COPY_NODE_FIELD(indexqual);
515+
COPY_NODE_FIELD(recheckqual);
515516
COPY_NODE_FIELD(indexorderby);
516517
COPY_NODE_FIELD(indextlist);
517518
COPY_SCALAR_FIELD(indexorderdir);

‎src/backend/nodes/outfuncs.c

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

571571
WRITE_OID_FIELD(indexid);
572572
WRITE_NODE_FIELD(indexqual);
573+
WRITE_NODE_FIELD(recheckqual);
573574
WRITE_NODE_FIELD(indexorderby);
574575
WRITE_NODE_FIELD(indextlist);
575576
WRITE_ENUM_FIELD(indexorderdir, ScanDirection);

‎src/backend/nodes/readfuncs.c

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

18041804
READ_OID_FIELD(indexid);
18051805
READ_NODE_FIELD(indexqual);
1806+
READ_NODE_FIELD(recheckqual);
18061807
READ_NODE_FIELD(indexorderby);
18071808
READ_NODE_FIELD(indextlist);
18081809
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"
@@ -179,10 +178,10 @@ static IndexScan *make_indexscan(List *qptlist, List *qpqual, Index scanrelid,
179178
ScanDirection indexscandir);
180179
static IndexOnlyScan *make_indexonlyscan(List *qptlist, List *qpqual,
181180
Index scanrelid, Oid indexid,
182-
List *indexqual, List *indexorderby,
181+
List *indexqual, List *recheckqual,
182+
List *indexorderby,
183183
List *indextlist,
184184
ScanDirection indexscandir);
185-
static List *make_indexonly_tlist(IndexOptInfo *indexinfo);
186185
static BitmapIndexScan *make_bitmap_indexscan(Index scanrelid, Oid indexid,
187186
List *indexqual,
188187
List *indexqualorig);
@@ -592,7 +591,7 @@ create_scan_plan(PlannerInfo *root, Path *best_path, int flags)
592591
if (best_path->pathtype == T_IndexOnlyScan)
593592
{
594593
/* For index-only scan, the preferred tlist is the index's */
595-
tlist = copyObject(make_indexonly_tlist(((IndexPath *) best_path)->indexinfo));
594+
tlist = copyObject(((IndexPath *) best_path)->indexinfo->indextlist);
596595

597596
/*
598597
* Transfer sortgroupref data to the replacement tlist, if
@@ -2773,7 +2772,8 @@ create_indexscan_plan(PlannerInfo *root,
27732772
List *indexclauses = best_path->indexclauses;
27742773
List *indexorderbys = best_path->indexorderbys;
27752774
Index baserelid = best_path->path.parent->relid;
2776-
Oid indexoid = best_path->indexinfo->indexoid;
2775+
IndexOptInfo *indexinfo = best_path->indexinfo;
2776+
Oid indexoid = indexinfo->indexoid;
27772777
List *qpqual;
27782778
List *stripped_indexquals;
27792779
List *fixed_indexquals;
@@ -2903,15 +2903,34 @@ create_indexscan_plan(PlannerInfo *root,
29032903
}
29042904
}
29052905

2906+
/*
2907+
* For an index-only scan, we must mark indextlist entries as resjunk if
2908+
* they are columns that the index AM can't return; this cues setrefs.c to
2909+
* not generate references to those columns.
2910+
*/
2911+
if (indexonly)
2912+
{
2913+
int i = 0;
2914+
2915+
foreach(l, indexinfo->indextlist)
2916+
{
2917+
TargetEntry *indextle = (TargetEntry *) lfirst(l);
2918+
2919+
indextle->resjunk = !indexinfo->canreturn[i];
2920+
i++;
2921+
}
2922+
}
2923+
29062924
/* Finally ready to build the plan node */
29072925
if (indexonly)
29082926
scan_plan = (Scan *) make_indexonlyscan(tlist,
29092927
qpqual,
29102928
baserelid,
29112929
indexoid,
29122930
fixed_indexquals,
2931+
stripped_indexquals,
29132932
fixed_indexorderbys,
2914-
make_indexonly_tlist(best_path->indexinfo),
2933+
indexinfo->indextlist,
29152934
best_path->indexscandir);
29162935
else
29172936
scan_plan = (Scan *) make_indexscan(tlist,
@@ -5213,6 +5232,7 @@ make_indexonlyscan(List *qptlist,
52135232
Index scanrelid,
52145233
Oid indexid,
52155234
List *indexqual,
5235+
List *recheckqual,
52165236
List *indexorderby,
52175237
List *indextlist,
52185238
ScanDirection indexscandir)
@@ -5227,60 +5247,14 @@ make_indexonlyscan(List *qptlist,
52275247
node->scan.scanrelid = scanrelid;
52285248
node->indexid = indexid;
52295249
node->indexqual = indexqual;
5250+
node->recheckqual = recheckqual;
52305251
node->indexorderby = indexorderby;
52315252
node->indextlist = indextlist;
52325253
node->indexorderdir = indexscandir;
52335254

52345255
return node;
52355256
}
52365257

5237-
/*
5238-
* make_indexonly_tlist
5239-
*
5240-
* Construct the indextlist for an IndexOnlyScan plan node.
5241-
* We must replace any column that can't be returned by the index AM
5242-
* with a null Const of the appropriate datatype. This is necessary
5243-
* to prevent setrefs.c from trying to use the value of such a column,
5244-
* and anyway it makes the indextlist a better representative of what
5245-
* the indexscan will really return. (We do this here, not where the
5246-
* IndexOptInfo is originally constructed, because earlier planner
5247-
* steps need to know what is in such columns.)
5248-
*/
5249-
static List *
5250-
make_indexonly_tlist(IndexOptInfo *indexinfo)
5251-
{
5252-
List *result;
5253-
int i;
5254-
ListCell *lc;
5255-
5256-
/* We needn't work hard for the common case of btrees. */
5257-
if (indexinfo->relam == BTREE_AM_OID)
5258-
return indexinfo->indextlist;
5259-
5260-
result = NIL;
5261-
i = 0;
5262-
foreach(lc, indexinfo->indextlist)
5263-
{
5264-
TargetEntry *indextle = (TargetEntry *) lfirst(lc);
5265-
5266-
if (indexinfo->canreturn[i])
5267-
result = lappend(result, indextle);
5268-
else
5269-
{
5270-
TargetEntry *newtle = makeNode(TargetEntry);
5271-
Node *texpr = (Node *) indextle->expr;
5272-
5273-
memcpy(newtle, indextle, sizeof(TargetEntry));
5274-
newtle->expr = (Expr *) makeNullConst(exprType(texpr),
5275-
exprTypmod(texpr),
5276-
exprCollation(texpr));
5277-
result = lappend(result, newtle);
5278-
}
5279-
i++;
5280-
}
5281-
return result;
5282-
}
5283-
52845258
static BitmapIndexScan *
52855259
make_bitmap_indexscan(Index scanrelid,
52865260
Oid indexid,

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

Lines changed: 25 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -990,8 +990,26 @@ set_indexonlyscan_references(PlannerInfo *root,
990990
int rtoffset)
991991
{
992992
indexed_tlist *index_itlist;
993+
List *stripped_indextlist;
994+
ListCell *lc;
995+
996+
/*
997+
* Vars in the plan node's targetlist, qual, and recheckqual must only
998+
* reference columns that the index AM can actually return. To ensure
999+
* this, remove non-returnable columns (which are marked as resjunk) from
1000+
* the indexed tlist. We can just drop them because the indexed_tlist
1001+
* machinery pays attention to TLE resnos, not physical list position.
1002+
*/
1003+
stripped_indextlist = NIL;
1004+
foreach(lc, plan->indextlist)
1005+
{
1006+
TargetEntry *indextle = (TargetEntry *) lfirst(lc);
9931007

994-
index_itlist = build_tlist_index(plan->indextlist);
1008+
if (!indextle->resjunk)
1009+
stripped_indextlist = lappend(stripped_indextlist, indextle);
1010+
}
1011+
1012+
index_itlist = build_tlist_index(stripped_indextlist);
9951013

9961014
plan->scan.scanrelid += rtoffset;
9971015
plan->scan.plan.targetlist = (List *)
@@ -1006,6 +1024,12 @@ set_indexonlyscan_references(PlannerInfo *root,
10061024
index_itlist,
10071025
INDEX_VAR,
10081026
rtoffset);
1027+
plan->recheckqual = (List *)
1028+
fix_upper_expr(root,
1029+
(Node *) plan->recheckqual,
1030+
index_itlist,
1031+
INDEX_VAR,
1032+
rtoffset);
10091033
/* indexqual is already transformed to reference index columns */
10101034
plan->indexqual = fix_scan_list(root, plan->indexqual, rtoffset);
10111035
/* 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
@@ -2306,6 +2306,8 @@ finalize_plan(PlannerInfo *root, Plan *plan,
23062306
case T_IndexOnlyScan:
23072307
finalize_primnode((Node *) ((IndexOnlyScan *) plan)->indexqual,
23082308
&context);
2309+
finalize_primnode((Node *) ((IndexOnlyScan *) plan)->recheckqual,
2310+
&context);
23092311
finalize_primnode((Node *) ((IndexOnlyScan *) plan)->indexorderby,
23102312
&context);
23112313

‎src/include/nodes/execnodes.h

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

‎src/include/nodes/plannodes.h

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

438452
/* ----------------

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

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

267+
-- Similarly, test that index rechecks involving a non-returnable column
268+
-- are done correctly.
269+
explain (verbose, costs off)
270+
select p from gist_tbl where circle(p,1) @> circle(point(0,0),0.95);
271+
QUERY PLAN
272+
---------------------------------------------------------------------------------------
273+
Index Only Scan using gist_tbl_multi_index on public.gist_tbl
274+
Output: p
275+
Index Cond: ((circle(gist_tbl.p, '1'::double precision)) @> '<(0,0),0.95>'::circle)
276+
(3 rows)
277+
278+
select p from gist_tbl where circle(p,1) @> circle(point(0,0),0.95);
279+
p
280+
-------
281+
(0,0)
282+
(1 row)
283+
284+
-- This case isn't supported, but it should at least EXPLAIN correctly.
285+
explain (verbose, costs off)
286+
select p from gist_tbl order by circle(p,1) <-> point(0,0) limit 1;
287+
QUERY PLAN
288+
------------------------------------------------------------------------------------
289+
Limit
290+
Output: p, ((circle(p, '1'::double precision) <-> '(0,0)'::point))
291+
-> Index Only Scan using gist_tbl_multi_index on public.gist_tbl
292+
Output: p, (circle(p, '1'::double precision) <-> '(0,0)'::point)
293+
Order By: ((circle(gist_tbl.p, '1'::double precision)) <-> '(0,0)'::point)
294+
(5 rows)
295+
296+
select p from gist_tbl order by circle(p,1) <-> point(0,0) limit 1;
297+
ERROR: lossy distance functions are not supported in index-only scans
267298
-- Clean up
268299
reset enable_seqscan;
269300
reset enable_bitmapscan;

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

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

140+
-- Similarly, test that index rechecks involving a non-returnable column
141+
-- are done correctly.
142+
explain (verbose, costs off)
143+
select p from gist_tbl where circle(p,1) @> circle(point(0,0),0.95);
144+
select p from gist_tbl where circle(p,1) @> circle(point(0,0),0.95);
145+
146+
-- This case isn't supported, but it should at least EXPLAIN correctly.
147+
explain (verbose, costs off)
148+
select p from gist_tbl order by circle(p,1) <-> point(0,0) limit 1;
149+
select p from gist_tbl order by circle(p,1) <-> point(0,0) limit 1;
150+
140151
-- Clean up
141152
reset enable_seqscan;
142153
reset enable_bitmapscan;

0 commit comments

Comments
 (0)