From: Peter Geoghegan Date: Fri, 17 Jul 2026 19:53:05 +0000 (-0400) Subject: Fix GiST index-only scan column alignment issue. X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=refs%2Fheads%2FREL_18_STABLE;p=thirdparty%2Fpostgresql.git Fix GiST index-only scan column alignment issue. An index-only scan filled its result slot from the HeapTuple an index AM returns in scan->xs_hitup by deforming it with the virtual slot's own tuple descriptor (during GiST and SP-GiST index-only scans). But index AMs form that heap tuple using their own descriptor, scan->xs_hitupdesc. The AM's descriptor may disagree with the IoS virtual slot's descriptor about each column's precise alignment, leading to "can't happen" errors in certain rare edge cases. Hard crashes were possible but much less likely. To fix, deform the tuple with the descriptor it was formed with. This is simpler, and makes xs_hitup handling (used by GiST and SP-GiST) uniform with the nearby existing xs_itup handling (used by nbtree). In practice this issue was very unlikely to be hit (it was found during testing of a patch that will change the table AM API used during index scans). The only currently affected core opclass is GiST's range_ops. It was only possible for the datum to be accessed at an incorrectly aligned offset when reading the second or subsequent column from a multicolumn GiST index. This couldn't happen in the common case where the datum used an unaligned short varlena header. Moreover, an earlier column had to leave the range datum at an offset where the two alignments actually disagree (e.g., an odd-length varlena datum). Author: Peter Geoghegan Reviewed-by: Tomas Vondra Discussion: https://postgr.es/m/CAH2-WzkGXa2SKnebdW29RT1hCcQBo_p03v3iqif2u9bjzLB-aQ@mail.gmail.com Backpatch-through: 14 --- diff --git a/src/backend/executor/nodeIndexonlyscan.c b/src/backend/executor/nodeIndexonlyscan.c index a6a00d594c0..867a8e40e78 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" @@ -48,7 +49,7 @@ static TupleTableSlot *IndexOnlyNext(IndexOnlyScanState *node); static void StoreIndexTuple(IndexOnlyScanState *node, TupleTableSlot *slot, - IndexTuple itup, TupleDesc itupdesc); + IndexScanDesc scandesc); /* ---------------------------------------------------------------- @@ -190,27 +191,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. @@ -260,56 +242,79 @@ 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. * * 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. + * We must deform the tuple using the tupdesc the index AM formed it with + * (xs_hitupdesc or xs_itupdesc), not the slot's tupdesc. The datums + * returned by the index AM must be binary compatible, but the descriptors + * may align each column differently in certain rare cases. (Actually, + * btree's "name" opclass stores cstring tuples that _aren't_ even binary + * compatible, in the strictest sense. We directly handle that here.) */ - 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++) - { - int attnum = node->ioss_NameCStringAttNums[idx]; - Name name; + 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); - /* skip null Datums */ - if (slot->tts_isnull[attnum]) - continue; + index_deform_tuple(scandesc->xs_itup, scandesc->xs_itupdesc, + slot->tts_values, slot->tts_isnull); - /* allocate the NAMEDATALEN and copy the datum into that memory */ - name = (Name) MemoryContextAlloc(node->ss.ps.ps_ExprContext->ecxt_per_tuple_memory, - NAMEDATALEN); + /* + * 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 attcount = node->ioss_NameCStringCount; - /* use namestrcpy to zero-pad all trailing bytes */ - namestrcpy(name, DatumGetCString(slot->tts_values[attnum])); - slot->tts_values[attnum] = NameGetDatum(name); + for (int idx = 0; idx < attcount; idx++) + { + int attnum = node->ioss_NameCStringAttNums[idx]; + Name name; + + /* skip null Datums */ + if (slot->tts_isnull[attnum]) + continue; + + /* + * 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 c75bbb23b6e..ae5b522b3c6 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 inet_ops, 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 6f1fc65f128..1ebb1d9ee43 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 inet_ops, 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);