Location via proxy:   [ UP ]  
[Report a bug]   [Manage cookies]                
Skip to content

Commit fd91fed

Browse files
committed
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 2a63683, 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 ac9099f. 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 ac9099f. Discussion: https://postgr.es/m/3728741.1617381471@sss.pgh.pa.us
1 parent 45aea47 commit fd91fed

File tree

2 files changed

+23
-6
lines changed

2 files changed

+23
-6
lines changed

src/backend/access/spgist/spgscan.c

Lines changed: 16 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -446,10 +446,22 @@ spgNewHeapItem(SpGistScanOpaque so, int level, ItemPointer heapPtr,
446446

447447
item->level = level;
448448
item->heapPtr = *heapPtr;
449-
/* copy value to queue cxt out of tmp cxt */
450-
item->value = isnull ? (Datum) 0 :
451-
datumCopy(leafValue, so->state.attLeafType.attbyval,
452-
so->state.attLeafType.attlen);
449+
450+
/*
451+
* If we need the reconstructed value, copy it to queue cxt out of tmp
452+
* cxt. Caution: the leaf_consistent method may not have supplied a value
453+
* if we didn't ask it to, and mildly-broken methods might supply one of
454+
* the wrong type. Also, while the correct leafValue type is attType not
455+
* leafType, pre-v14 Postgres versions have historically used attLeafType
456+
* here; let's not confuse matters even more by changing that in a minor
457+
* release.
458+
*/
459+
if (so->want_itup)
460+
item->value = isnull ? (Datum) 0 :
461+
datumCopy(leafValue, so->state.attLeafType.attbyval,
462+
so->state.attLeafType.attlen);
463+
else
464+
item->value = (Datum) 0;
453465
item->traversalValue = NULL;
454466
item->isLeaf = true;
455467
item->recheck = recheck;

src/backend/utils/adt/geo_spgist.c

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -749,8 +749,13 @@ spg_box_quad_leaf_consistent(PG_FUNCTION_ARGS)
749749
/* All tests are exact. */
750750
out->recheck = false;
751751

752-
/* leafDatum is what it is... */
753-
out->leafValue = in->leafDatum;
752+
/*
753+
* Don't return leafValue unless told to; this is used for both box and
754+
* polygon opclasses, and in the latter case the leaf datum is not even of
755+
* the right type to return.
756+
*/
757+
if (in->returnData)
758+
out->leafValue = leaf;
754759

755760
/* Perform the required comparison(s) */
756761
for (i = 0; i < in->nkeys; i++)

0 commit comments

Comments
 (0)