From 7b208cba2c9f458e822546e2c3215fd3b035be52 Mon Sep 17 00:00:00 2001 From: Peter Geoghegan Date: Mon, 13 Jul 2026 16:28:39 -0400 Subject: [PATCH v1] 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 (XXX hard crash might also be possible), at least during certain kinds of GiST multicolumn index scans. To fix, deform the tuple with the descriptor it was formed with. This happens to be simpler. It also makes xs_hitup handling uniform with the existing xs_itup handling. Author: Peter Geoghegan --- src/backend/executor/nodeIndexonlyscan.c | 110 +++++++++++------------ src/test/regress/expected/rangetypes.out | 29 ++++++ src/test/regress/sql/rangetypes.sql | 19 ++++ 3 files changed, 103 insertions(+), 55 deletions(-) diff --git a/src/backend/executor/nodeIndexonlyscan.c b/src/backend/executor/nodeIndexonlyscan.c index d52012e8a..8bb8bb639 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); /* ---------------------------------------------------------------- @@ -194,26 +195,9 @@ IndexOnlyNext(IndexOnlyScanState *node) } /* - * 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. + * Fill the scan tuple slot with data from the index. */ - 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"); + StoreIndexTuple(node, slot, scandesc); /* * If the index was lossy, we have to recheck the index quals. @@ -263,56 +247,72 @@ IndexOnlyNext(IndexOnlyScanState *node) /* * StoreIndexTuple - * Fill the slot with data from the index tuple. + * Fill the slot with the data the index AM returned. * - * At some point this might be generally-useful functionality, but - * right now we don't need it elsewhere. + * 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 with the tupdesc the AM supplied + * for it, not the slot's tupdesc. The two will always agree about columns + * being binary compatible, but they might not agree about tuple alignment. + * (Actually, opclasses like btree/name_ops store cstrings instead of "name" + * datums, and so aren't even binary compatible. We handle such indexes here + * directly, as a special case.) */ 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/rangetypes.out b/src/test/regress/expected/rangetypes.out index 083e948bc..16f5e3c73 100644 --- a/src/test/regress/expected/rangetypes.out +++ b/src/test/regress/expected/rangetypes.out @@ -1283,6 +1283,35 @@ select count(*) from test_range_gist where ir -|- int4multirange(int4range(100,2 5 (1 row) +-- test an index-only scan on a multicolumn GiST range index: the tuple +-- GiST fetches is formed with anyrange columns, whose alignment ('d') +-- differs from the concrete range type's ('i'), so it must also be +-- deformed that way +create temp table test_range_gist_ios (ir1 int4range, ir2 numrange); +insert into test_range_gist_ios + values (int4range(1, null), + numrange(repeat('7', 200)::numeric, repeat('8', 200)::numeric)); +create index test_range_gist_ios_idx on test_range_gist_ios + using gist (ir1, ir2); +vacuum test_range_gist_ios; +explain (costs off) +select ir1, lower(ir2) = repeat('7', 200)::numeric as lower_ok, + upper(ir2) = repeat('8', 200)::numeric as upper_ok + from test_range_gist_ios where ir1 && int4range(0, 5); + QUERY PLAN +---------------------------------------------------------------------- + Index Only Scan using test_range_gist_ios_idx on test_range_gist_ios + Index Cond: (ir1 && '[0,5)'::int4range) +(2 rows) + +select ir1, lower(ir2) = repeat('7', 200)::numeric as lower_ok, + upper(ir2) = repeat('8', 200)::numeric as upper_ok + from test_range_gist_ios where ir1 && int4range(0, 5); + ir1 | lower_ok | upper_ok +------+----------+---------- + [1,) | t | t +(1 row) + -- test SP-GiST index that's been built incrementally create table test_range_spgist(ir int4range); create index test_range_spgist_idx on test_range_spgist using spgist (ir); diff --git a/src/test/regress/sql/rangetypes.sql b/src/test/regress/sql/rangetypes.sql index dbfe0d049..fa05f52e9 100644 --- a/src/test/regress/sql/rangetypes.sql +++ b/src/test/regress/sql/rangetypes.sql @@ -320,6 +320,25 @@ select count(*) from test_range_gist where ir &< int4multirange(int4range(100,20 select count(*) from test_range_gist where ir &> int4multirange(int4range(100,200), int4range(400,500)); select count(*) from test_range_gist where ir -|- int4multirange(int4range(100,200), int4range(400,500)); +-- test an index-only scan on a multicolumn GiST range index: the tuple +-- GiST fetches is formed with anyrange columns, whose alignment ('d') +-- differs from the concrete range type's ('i'), so it must also be +-- deformed that way +create temp table test_range_gist_ios (ir1 int4range, ir2 numrange); +insert into test_range_gist_ios + values (int4range(1, null), + numrange(repeat('7', 200)::numeric, repeat('8', 200)::numeric)); +create index test_range_gist_ios_idx on test_range_gist_ios + using gist (ir1, ir2); +vacuum test_range_gist_ios; +explain (costs off) +select ir1, lower(ir2) = repeat('7', 200)::numeric as lower_ok, + upper(ir2) = repeat('8', 200)::numeric as upper_ok + from test_range_gist_ios where ir1 && int4range(0, 5); +select ir1, lower(ir2) = repeat('7', 200)::numeric as lower_ok, + upper(ir2) = repeat('8', 200)::numeric as upper_ok + from test_range_gist_ios where ir1 && int4range(0, 5); + -- test SP-GiST index that's been built incrementally create table test_range_spgist(ir int4range); create index test_range_spgist_idx on test_range_spgist using spgist (ir); -- 2.53.0