Fix more confusion in SP-GiST.

spg_box_quad_leaf_consistent unconditionally returned the leaf
datum as leafValue, even though in its usage for poly_ops that
value is of completely the wrong type.

In versions before 12, that was harmless because the core code did
nothing with leafValue in non-index-only scans ... but since commit
2a6368343, if we were doing a KNN-style scan, spgNewHeapItem would
unconditionally try to copy the value using the wrong datatype
parameters.  Said copying is a waste of time and space if we're not
going to return the data, but it accidentally failed to fail until
I fixed the datatype confusion in ac9099fc1.

Hence, change spgNewHeapItem to not copy the datum unless we're
actually going to return it later.  This saves cycles and dodges
the question of whether lossy opclasses are returning the right
type.  Also change spg_box_quad_leaf_consistent to not return
data that might be of the wrong type, as insurance against
somebody introducing a similar bug into the core code in future.

It seems like a good idea to back-patch these two changes into
v12 and v13, although I'm afraid to change spgNewHeapItem's
mistaken idea of which datatype to use in those branches.

Per buildfarm results from ac9099fc1.

Discussion: https://postgr.es/m/3728741.1617381471@sss.pgh.pa.us

diff --git a/src/backend/access/spgist/spgscan.c b/src/backend/access/spgist/spgscan.c
index 4d506bfb9a9b8d3e7f9bf0acce623721c062dfb8..93e3ec9e7eb1d8a139008f75846cda470cc636fc 100644 (file)
--- a/src/backend/access/spgist/spgscan.c
+++ b/src/backend/access/spgist/spgscan.c
@@ -446,10 +446,22 @@ spgNewHeapItem(SpGistScanOpaque so, int level, ItemPointer heapPtr,
 
    item->level = level;
    item->heapPtr = *heapPtr;
-   /* copy value to queue cxt out of tmp cxt */
-   item->value = isnull ? (Datum) 0 :
-       datumCopy(leafValue, so->state.attLeafType.attbyval,
-                 so->state.attLeafType.attlen);
+
+   /*
+    * If we need the reconstructed value, copy it to queue cxt out of tmp
+    * cxt.  Caution: the leaf_consistent method may not have supplied a value
+    * if we didn't ask it to, and mildly-broken methods might supply one of
+    * the wrong type.  Also, while the correct leafValue type is attType not
+    * leafType, pre-v14 Postgres versions have historically used attLeafType
+    * here; let's not confuse matters even more by changing that in a minor
+    * release.
+    */
+   if (so->want_itup)
+       item->value = isnull ? (Datum) 0 :
+           datumCopy(leafValue, so->state.attLeafType.attbyval,
+                     so->state.attLeafType.attlen);
+   else
+       item->value = (Datum) 0;
    item->traversalValue = NULL;
    item->isLeaf = true;
    item->recheck = recheck;

diff --git a/src/backend/utils/adt/geo_spgist.c b/src/backend/utils/adt/geo_spgist.c
index de7e6fa4042541ee223865ff14a0e46b7588c5b2..846381abebcbedc89a48a9186186ca482d137b58 100644 (file)
--- a/src/backend/utils/adt/geo_spgist.c
+++ b/src/backend/utils/adt/geo_spgist.c
@@ -749,8 +749,13 @@ spg_box_quad_leaf_consistent(PG_FUNCTION_ARGS)
    /* All tests are exact. */
    out->recheck = false;
 
-   /* leafDatum is what it is... */
-   out->leafValue = in->leafDatum;
+   /*
+    * Don't return leafValue unless told to; this is used for both box and
+    * polygon opclasses, and in the latter case the leaf datum is not even of
+    * the right type to return.
+    */
+   if (in->returnData)
+       out->leafValue = leaf;
 
    /* Perform the required comparison(s) */
    for (i = 0; i < in->nkeys; i++)