diff --git a/src/backend/executor/nodeIndexonlyscan.c b/src/backend/executor/nodeIndexonlyscan.c index d52012e8a69..8afcbde6b97 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,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++) + 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 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);