Skip to content

Commit d228af7

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 52d5026 commit d228af7

File tree

12 files changed

+124
-66
lines changed

12 files changed

+124
-66
lines changed

‎src/backend/commands/explain.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1746,7 +1746,7 @@ ExplainNode(PlanState *planstate, List *ancestors,
17461746
case T_IndexOnlyScan:
17471747
show_scan_qual(((IndexOnlyScan *) plan)->indexqual,
17481748
"Index Cond", planstate, ancestors, es);
1749-
if (((IndexOnlyScan *) plan)->indexqual)
1749+
if (((IndexOnlyScan *) plan)->recheckqual)
17501750
show_instrumentation_count("Rows Removed by Index Recheck", 2,
17511751
planstate, es);
17521752
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
@@ -1885,6 +1885,7 @@ _readIndexOnlyScan(void)
18851885

18861886
READ_OID_FIELD(indexid);
18871887
READ_NODE_FIELD(indexqual);
1888+
READ_NODE_FIELD(recheckqual);
18881889
READ_NODE_FIELD(indexorderby);
18891890
READ_NODE_FIELD(indextlist);
18901891
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"
@@ -189,10 +188,10 @@ static IndexScan *make_indexscan(List *qptlist, List *qpqual, Index scanrelid,
189188
ScanDirection indexscandir);
190189
static IndexOnlyScan *make_indexonlyscan(List *qptlist, List *qpqual,
191190
Index scanrelid, Oid indexid,
192-
List *indexqual, List *indexorderby,
191+
List *indexqual, List *recheckqual,
192+
List *indexorderby,
193193
List *indextlist,
194194
ScanDirection indexscandir);
195-
static List *make_indexonly_tlist(IndexOptInfo *indexinfo);
196195
static BitmapIndexScan *make_bitmap_indexscan(Index scanrelid, Oid indexid,
197196
List *indexqual,
198197
List *indexqualorig);
@@ -623,7 +622,7 @@ create_scan_plan(PlannerInfo *root, Path *best_path, int flags)
623622
if (best_path->pathtype == T_IndexOnlyScan)
624623
{
625624
/* For index-only scan, the preferred tlist is the index's */
626-
tlist = copyObject(make_indexonly_tlist(((IndexPath *) best_path)->indexinfo));
625+
tlist = copyObject(((IndexPath *) best_path)->indexinfo->indextlist);
627626

628627
/*
629628
* Transfer sortgroupref data to the replacement tlist, if
@@ -2934,7 +2933,8 @@ create_indexscan_plan(PlannerInfo *root,
29342933
List *indexclauses = best_path->indexclauses;
29352934
List *indexorderbys = best_path->indexorderbys;
29362935
Index baserelid = best_path->path.parent->relid;
2937-
Oid indexoid = best_path->indexinfo->indexoid;
2936+
IndexOptInfo *indexinfo = best_path->indexinfo;
2937+
Oid indexoid = indexinfo->indexoid;
29382938
List *qpqual;
29392939
List *stripped_indexquals;
29402940
List *fixed_indexquals;
@@ -3064,15 +3064,34 @@ create_indexscan_plan(PlannerInfo *root,
30643064
}
30653065
}
30663066

3067+
/*
3068+
* For an index-only scan, we must mark indextlist entries as resjunk if
3069+
* they are columns that the index AM can't return; this cues setrefs.c to
3070+
* not generate references to those columns.
3071+
*/
3072+
if (indexonly)
3073+
{
3074+
int i = 0;
3075+
3076+
foreach(l, indexinfo->indextlist)
3077+
{
3078+
TargetEntry *indextle = (TargetEntry *) lfirst(l);
3079+
3080+
indextle->resjunk = !indexinfo->canreturn[i];
3081+
i++;
3082+
}
3083+
}
3084+
30673085
/* Finally ready to build the plan node */
30683086
if (indexonly)
30693087
scan_plan = (Scan *) make_indexonlyscan(tlist,
30703088
qpqual,
30713089
baserelid,
30723090
indexoid,
30733091
fixed_indexquals,
3092+
stripped_indexquals,
30743093
fixed_indexorderbys,
3075-
make_indexonly_tlist(best_path->indexinfo),
3094+
indexinfo->indextlist,
30763095
best_path->indexscandir);
30773096
else
30783097
scan_plan = (Scan *) make_indexscan(tlist,
@@ -5443,6 +5462,7 @@ make_indexonlyscan(List *qptlist,
54435462
Index scanrelid,
54445463
Oid indexid,
54455464
List *indexqual,
5465+
List *recheckqual,
54465466
List *indexorderby,
54475467
List *indextlist,
54485468
ScanDirection indexscandir)
@@ -5457,60 +5477,14 @@ make_indexonlyscan(List *qptlist,
54575477
node->scan.scanrelid = scanrelid;
54585478
node->indexid = indexid;
54595479
node->indexqual = indexqual;
5480+
node->recheckqual = recheckqual;
54605481
node->indexorderby = indexorderby;
54615482
node->indextlist = indextlist;
54625483
node->indexorderdir = indexscandir;
54635484

54645485
return node;
54655486
}
54665487

5467-
/*
5468-
* make_indexonly_tlist
5469-
*
5470-
* Construct the indextlist for an IndexOnlyScan plan node.
5471-
* We must replace any column that can't be returned by the index AM
5472-
* with a null Const of the appropriate datatype. This is necessary
5473-
* to prevent setrefs.c from trying to use the value of such a column,
5474-
* and anyway it makes the indextlist a better representative of what
5475-
* the indexscan will really return. (We do this here, not where the
5476-
* IndexOptInfo is originally constructed, because earlier planner
5477-
* steps need to know what is in such columns.)
5478-
*/
5479-
static List *
5480-
make_indexonly_tlist(IndexOptInfo *indexinfo)
5481-
{
5482-
List *result;
5483-
int i;
5484-
ListCell *lc;
5485-
5486-
/* We needn't work hard for the common case of btrees. */
5487-
if (indexinfo->relam == BTREE_AM_OID)
5488-
return indexinfo->indextlist;
5489-
5490-
result = NIL;
5491-
i = 0;
5492-
foreach(lc, indexinfo->indextlist)
5493-
{
5494-
TargetEntry *indextle = (TargetEntry *) lfirst(lc);
5495-
5496-
if (indexinfo->canreturn[i])
5497-
result = lappend(result, indextle);
5498-
else
5499-
{
5500-
TargetEntry *newtle = makeNode(TargetEntry);
5501-
Node *texpr = (Node *) indextle->expr;
5502-
5503-
memcpy(newtle, indextle, sizeof(TargetEntry));
5504-
newtle->expr = (Expr *) makeNullConst(exprType(texpr),
5505-
exprTypmod(texpr),
5506-
exprCollation(texpr));
5507-
result = lappend(result, newtle);
5508-
}
5509-
i++;
5510-
}
5511-
return result;
5512-
}
5513-
55145488
static BitmapIndexScan *
55155489
make_bitmap_indexscan(Index scanrelid,
55165490
Oid indexid,

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

Lines changed: 26 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1152,8 +1152,26 @@ set_indexonlyscan_references(PlannerInfo *root,
11521152
int rtoffset)
11531153
{
11541154
indexed_tlist *index_itlist;
1155+
List *stripped_indextlist;
1156+
ListCell *lc;
1157+
1158+
/*
1159+
* Vars in the plan node's targetlist, qual, and recheckqual must only
1160+
* reference columns that the index AM can actually return. To ensure
1161+
* this, remove non-returnable columns (which are marked as resjunk) from
1162+
* the indexed tlist. We can just drop them because the indexed_tlist
1163+
* machinery pays attention to TLE resnos, not physical list position.
1164+
*/
1165+
stripped_indextlist = NIL;
1166+
foreach(lc, plan->indextlist)
1167+
{
1168+
TargetEntry *indextle = (TargetEntry *) lfirst(lc);
1169+
1170+
if (!indextle->resjunk)
1171+
stripped_indextlist = lappend(stripped_indextlist, indextle);
1172+
}
11551173

1156-
index_itlist = build_tlist_index(plan->indextlist);
1174+
index_itlist = build_tlist_index(stripped_indextlist);
11571175

11581176
plan->scan.scanrelid += rtoffset;
11591177
plan->scan.plan.targetlist = (List *)
@@ -1170,6 +1188,13 @@ set_indexonlyscan_references(PlannerInfo *root,
11701188
INDEX_VAR,
11711189
rtoffset,
11721190
NUM_EXEC_QUAL((Plan *) plan));
1191+
plan->recheckqual = (List *)
1192+
fix_upper_expr(root,
1193+
(Node *) plan->recheckqual,
1194+
index_itlist,
1195+
INDEX_VAR,
1196+
rtoffset,
1197+
NUM_EXEC_QUAL((Plan *) plan));
11731198
/* indexqual is already transformed to reference index columns */
11741199
plan->indexqual = fix_scan_list(root, plan->indexqual,
11751200
rtoffset, 1);

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

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2336,6 +2336,8 @@ finalize_plan(PlannerInfo *root, Plan *plan,
23362336
case T_IndexOnlyScan:
23372337
finalize_primnode((Node *) ((IndexOnlyScan *) plan)->indexqual,
23382338
&context);
2339+
finalize_primnode((Node *) ((IndexOnlyScan *) plan)->recheckqual,
2340+
&context);
23392341
finalize_primnode((Node *) ((IndexOnlyScan *) plan)->indexorderby,
23402342
&context);
23412343

‎src/include/nodes/execnodes.h

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1489,7 +1489,7 @@ typedef struct IndexScanState
14891489
/* ----------------
14901490
* IndexOnlyScanState information
14911491
*
1492-
* indexqual execution state for indexqual expressions
1492+
* recheckqual execution state for recheckqual expressions
14931493
* ScanKeys Skey structures for index quals
14941494
* NumScanKeys number of ScanKeys
14951495
* OrderByKeys Skey structures for index ordering operators
@@ -1508,7 +1508,7 @@ typedef struct IndexScanState
15081508
typedef struct IndexOnlyScanState
15091509
{
15101510
ScanState ss; /* its first field is NodeTag */
1511-
ExprState *indexqual;
1511+
ExprState *recheckqual;
15121512
struct ScanKeyData *ioss_ScanKeys;
15131513
int ioss_NumScanKeys;
15141514
struct ScanKeyData *ioss_OrderByKeys;

‎src/include/nodes/plannodes.h

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

441455
/* ----------------

‎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)