From f9a041ff2a2a5b57b7160afe88658a282e6523bc Mon Sep 17 00:00:00 2001 From: Peter Geoghegan Date: Mon, 13 Jul 2026 16:28:39 -0400 Subject: [PATCH v2] Fix index-only scan misreading GiST fetch tuples. An index-only scan fills its result slot from the HeapTuple an index AM returns in scan->xs_hitup by deforming it with the slot's tuple descriptor. But the index AM formed that tuple with its own descriptor, scan->xs_hitupdesc, and the two may disagree about a column's physical layout/alignment. This could cause spurious errors (and even hard crashes), at least during certain kinds of GiST multicolumn index scans. This only affects GiST's range_ops in core. GiST builds the descriptor for the tuples it reconstructs during an index-only scan (xs_hitupdesc) from each key column's opclass input type, and range_ops declares the polymorphic anyrange, which has alignment 'd'; a concrete range type such as int4range or numrange instead has alignment 'i'. range_ops is the only in-core opclass that is both polymorphic in this way and returnable: having no compress function, it stores the indexed value verbatim and so can hand it back. The other polymorphic GiST opclass, multirange_ops, is lossy and cannot return (every other returnable opclass has a non-polymorphic input type). In practice this issue was hard to hit, even given a multicolumn GiST index with these concrete types. The differing alignment only changes where a column is read when that column is a varlena stored with a four-byte header. Short varlenas (those that fit in a one-byte header, up to 127 bytes) are stored without alignment padding, and deforming locates them by content rather than by the descriptor's alignment, so they are read correctly either way. To fix, deform the tuple with the descriptor it was formed with. This is simpler, and makes xs_hitup handling uniform with the nearby existing xs_itup handling. Author: Peter Geoghegan Discussion: https://postgr.es/m/CAH2-WzkGXa2SKnebdW29RT1hCcQBo_p03v3iqif2u9bjzLB-aQ@mail.gmail.com --- src/backend/executor/nodeIndexonlyscan.c | 121 ++++++++++++----------- src/test/regress/expected/gist.out | 36 +++++++ src/test/regress/sql/gist.sql | 29 ++++++ 3 files changed, 131 insertions(+), 55 deletions(-) diff --git a/src/backend/executor/nodeIndexonlyscan.c b/src/backend/executor/nodeIndexonlyscan.c index d52012e8a..2668f9c18 100644 --- a/src/backend/executor/nodeIndexonlyscan.c +++ b/src/backend/executor/nodeIndexonlyscan.c @@ -31,6 +31,7 @@ #include "postgres.h" #include "access/genam.h" +#include "access/htup_details.h" #include "access/relscan.h" #include "access/tableam.h" #include "access/tupdesc.h" @@ -49,7 +50,7 @@ static TupleTableSlot *IndexOnlyNext(IndexOnlyScanState *node); static void StoreIndexTuple(IndexOnlyScanState *node, TupleTableSlot *slot, - IndexTuple itup, TupleDesc itupdesc); + IndexScanDesc scandesc); /* ---------------------------------------------------------------- @@ -193,27 +194,8 @@ IndexOnlyNext(IndexOnlyScanState *node) tuple_from_heap = true; } - /* - * Fill the scan tuple slot with data from the index. This might be - * provided in either HeapTuple or IndexTuple format. Conceivably an - * index AM might fill both fields, in which case we prefer the heap - * format, since it's probably a bit cheaper to fill a slot from. - */ - if (scandesc->xs_hitup) - { - /* - * We don't take the trouble to verify that the provided tuple has - * exactly the slot's format, but it seems worth doing a quick - * check on the number of fields. - */ - Assert(slot->tts_tupleDescriptor->natts == - scandesc->xs_hitupdesc->natts); - ExecForceStoreHeapTuple(scandesc->xs_hitup, slot, false); - } - else if (scandesc->xs_itup) - StoreIndexTuple(node, slot, scandesc->xs_itup, scandesc->xs_itupdesc); - else - elog(ERROR, "no data returned for index-only scan"); + /* Fill the scan tuple slot with data from the index */ + StoreIndexTuple(node, slot, scandesc); /* * If the index was lossy, we have to recheck the index quals. @@ -263,56 +245,85 @@ IndexOnlyNext(IndexOnlyScanState *node) /* * StoreIndexTuple - * Fill the slot with data from the index tuple. + * Fill the slot with the data the index AM returned. + * + * The data might be provided in either HeapTuple (xs_hitup) or IndexTuple + * (xs_itup) format. Conceivably an index AM might fill both fields, in + * which case we prefer the heap format, since it's probably a bit cheaper + * to fill a slot from. + * + * In either case we must deform the tuple using the tupdesc the AM formed it + * with (xs_hitupdesc or xs_itupdesc), not the slot's tupdesc. An AM builds + * that descriptor from its opclass, so a column's type there can differ from + * the actual indexed column's type that the slot's tupdesc uses. The datums + * are binary compatible either way, but the two descriptors can still lay the + * tuple out differently: an opclass with a polymorphic input type such as + * anyrange forms its tuples with that type's alignment, which need not match + * the alignment of the actual type. + * + * The one core opclass that goes further, storing a datum that isn't even + * binary compatible with the indexed column, is btree's name_ops, which + * stores "name" columns as cstrings. Those are fixed up after deforming, as + * an xs_itup-only special case. * * At some point this might be generally-useful functionality, but * right now we don't need it elsewhere. */ static void StoreIndexTuple(IndexOnlyScanState *node, TupleTableSlot *slot, - IndexTuple itup, TupleDesc itupdesc) + IndexScanDesc scandesc) { - /* - * Note: we must use the tupdesc supplied by the AM in index_deform_tuple, - * not the slot's tupdesc, in case the latter has different datatypes - * (this happens for btree name_ops in particular). They'd better have - * the same number of columns though, as well as being datatype-compatible - * which is something we can't so easily check. - */ - Assert(slot->tts_tupleDescriptor->natts == itupdesc->natts); - ExecClearTuple(slot); - index_deform_tuple(itup, itupdesc, slot->tts_values, slot->tts_isnull); - /* - * Copy all name columns stored as cstrings back into a NAMEDATALEN byte - * sized allocation. We mark this branch as unlikely as generally "name" - * is used only for the system catalogs and this would have to be a user - * query running on those or some other user table with an index on a name - * column. - */ - if (unlikely(node->ioss_NameCStringAttNums != NULL)) + if (scandesc->xs_hitup) { - int attcount = node->ioss_NameCStringCount; + Assert(slot->tts_tupleDescriptor->natts == scandesc->xs_hitupdesc->natts); - for (int idx = 0; idx < attcount; idx++) + heap_deform_tuple(scandesc->xs_hitup, scandesc->xs_hitupdesc, + slot->tts_values, slot->tts_isnull); + } + else if (scandesc->xs_itup) + { + Assert(slot->tts_tupleDescriptor->natts == scandesc->xs_itupdesc->natts); + + index_deform_tuple(scandesc->xs_itup, scandesc->xs_itupdesc, + slot->tts_values, slot->tts_isnull); + + /* + * Copy all name columns stored as cstrings back into a NAMEDATALEN + * byte sized allocation. We mark this branch as unlikely as + * generally "name" is used only for the system catalogs and this + * would have to be a user query running on those or some other user + * table with an index on a name column. + */ + if (unlikely(node->ioss_NameCStringAttNums != NULL)) { - int attnum = node->ioss_NameCStringAttNums[idx]; - Name name; + int attcount = node->ioss_NameCStringCount; - /* skip null Datums */ - if (slot->tts_isnull[attnum]) - continue; + for (int idx = 0; idx < attcount; idx++) + { + int attnum = node->ioss_NameCStringAttNums[idx]; + Name name; - /* allocate the NAMEDATALEN and copy the datum into that memory */ - name = (Name) MemoryContextAlloc(node->ss.ps.ps_ExprContext->ecxt_per_tuple_memory, - NAMEDATALEN); + /* skip null Datums */ + if (slot->tts_isnull[attnum]) + continue; - /* use namestrcpy to zero-pad all trailing bytes */ - namestrcpy(name, DatumGetCString(slot->tts_values[attnum])); - slot->tts_values[attnum] = NameGetDatum(name); + /* + * allocate the NAMEDATALEN and copy the datum into that + * memory + */ + name = (Name) MemoryContextAlloc(node->ss.ps.ps_ExprContext->ecxt_per_tuple_memory, + NAMEDATALEN); + + /* use namestrcpy to zero-pad all trailing bytes */ + namestrcpy(name, DatumGetCString(slot->tts_values[attnum])); + slot->tts_values[attnum] = NameGetDatum(name); + } } } + else + elog(ERROR, "no data returned for index-only scan"); ExecStoreVirtualTuple(slot); } diff --git a/src/test/regress/expected/gist.out b/src/test/regress/expected/gist.out index c75bbb23b..be470293c 100644 --- a/src/test/regress/expected/gist.out +++ b/src/test/regress/expected/gist.out @@ -387,6 +387,42 @@ 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; ERROR: lossy distance functions are not supported in index-only scans +-- Test that an index-only scan deforms the tuple it reconstructs with the +-- descriptor the AM formed it with, not the scan slot's descriptor. +create temp table gist_ios_tupdesc (a inet, r numrange); +-- range_ops forms its tuples using the opclass input type, the polymorphic +-- anyrange (alignment 'd'), while the scan slot uses the actual range type +-- numrange (alignment 'i'). A buggy implementation will incorrectly access +-- the r/numrange column at the wrong offset. +-- +-- The range bounds are made long so the value needs a four-byte varlena +-- header; shorter values get a one-byte header and are stored without +-- alignment padding, which would mask the problem. +insert into gist_ios_tupdesc +values ( + '::1', -- shifts "r" datum value to differing offset + numrange(repeat('7', 200)::numeric, repeat('8', 200)::numeric)); +create index on gist_ios_tupdesc using gist (a, r); +vacuum analyze gist_ios_tupdesc; +explain (costs off) +select lower(r) = repeat('7', 200)::numeric as lower_ok, + upper(r) = repeat('8', 200)::numeric as upper_ok + from gist_ios_tupdesc where r && numrange(null, null); + QUERY PLAN +-------------------------------------------------------------------- + Index Only Scan using gist_ios_tupdesc_a_r_idx on gist_ios_tupdesc + Index Cond: (r && '(,)'::numrange) +(2 rows) + +select lower(r) = repeat('7', 200)::numeric as lower_ok, + upper(r) = repeat('8', 200)::numeric as upper_ok + from gist_ios_tupdesc where r && numrange(null, null); + lower_ok | upper_ok +----------+---------- + t | t +(1 row) + +drop table gist_ios_tupdesc; -- Force an index build using buffering. create index gist_tbl_box_index_forcing_buffering on gist_tbl using gist (p) with (buffering=on, fillfactor=50); diff --git a/src/test/regress/sql/gist.sql b/src/test/regress/sql/gist.sql index 6f1fc65f1..8aac210cf 100644 --- a/src/test/regress/sql/gist.sql +++ b/src/test/regress/sql/gist.sql @@ -169,6 +169,35 @@ 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; +-- Test that an index-only scan deforms the tuple it reconstructs with the +-- descriptor the AM formed it with, not the scan slot's descriptor. +create temp table gist_ios_tupdesc (a inet, r numrange); + +-- range_ops forms its tuples using the opclass input type, the polymorphic +-- anyrange (alignment 'd'), while the scan slot uses the actual range type +-- numrange (alignment 'i'). A buggy implementation will incorrectly access +-- the r/numrange column at the wrong offset. +-- +-- The range bounds are made long so the value needs a four-byte varlena +-- header; shorter values get a one-byte header and are stored without +-- alignment padding, which would mask the problem. +insert into gist_ios_tupdesc +values ( + '::1', -- shifts "r" datum value to differing offset + numrange(repeat('7', 200)::numeric, repeat('8', 200)::numeric)); +create index on gist_ios_tupdesc using gist (a, r); +vacuum analyze gist_ios_tupdesc; + +explain (costs off) +select lower(r) = repeat('7', 200)::numeric as lower_ok, + upper(r) = repeat('8', 200)::numeric as upper_ok + from gist_ios_tupdesc where r && numrange(null, null); +select lower(r) = repeat('7', 200)::numeric as lower_ok, + upper(r) = repeat('8', 200)::numeric as upper_ok + from gist_ios_tupdesc where r && numrange(null, null); + +drop table gist_ios_tupdesc; + -- Force an index build using buffering. create index gist_tbl_box_index_forcing_buffering on gist_tbl using gist (p) with (buffering=on, fillfactor=50); -- 2.53.0