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 <pg@bowt.ie>
Reviewed-by: Tomas Vondra <tomas@vondra.me>
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 d52012e8a6987ab34dcef47afbdfe9b211ff093e..8afcbde6b97dc3051e24b7c78fcdc485ae6c8e9b 100644 (file)
--- 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++)
-               {
-                       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 c75bbb23b6e387e5c29cf248a617c938e16aff0c..ae5b522b3c698f775402178b1faa97212890cc40 100644 (file)
--- 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 6f1fc65f128a2f86d02e0a2f12ff95f06f6b57d1..1ebb1d9ee43970a7b9a5976d6c5156465e7e86af 100644 (file)
--- 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);