Fix confusion in SP-GiST between attribute type and leaf storage type.

According to the documentation, the attType passed to the opclass
config function (and also relied on by the core code) is the type
of the heap column or expression being indexed.  But what was
actually being passed was the type stored for the index column.
This made no difference for user-defined SP-GiST opclasses,
because we weren't allowing the STORAGE clause of CREATE OPCLASS
to be used, so the two types would be the same.  But it's silly
not to allow that, seeing that the built-in poly_ops opclass
has a different value for opckeytype than opcintype, and that if you
want to do lossy storage then the types must really be different.
(Thus, user-defined opclasses doing lossy storage had to lie about
what type is in the index.)  Hence, remove the restriction, and make
sure that we use the input column type not opckeytype where relevant.

For reasons of backwards compatibility with existing user-defined
opclasses, we can't quite insist that the specified leafType match
the STORAGE clause; instead just add an amvalidate() warning if
they don't match.

Also fix some bugs that would only manifest when trying to return
index entries when attType is different from attLeafType.  It's not
too surprising that these have not been reported, because the only
usual reason for such a difference is to store the leaf value
lossily, rendering index-only scans impossible.

Add a src/test/modules module to exercise cases where attType is
different from attLeafType and yet index-only scan is supported.

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

1 files changed, 26 insertions, 5 deletions

diff --git a/src/backend/access/spgist/spgscan.c b/src/backend/access/spgist/spgscan.c
index 20e67c3f7d1..06011d2c024 100644
--- a/src/backend/access/spgist/spgscan.c
+++ b/src/backend/access/spgist/spgscan.c
@@ -81,7 +81,10 @@ pairingheap_SpGistSearchItem_cmp(const pairingheap_node *a,
 static void
 spgFreeSearchItem(SpGistScanOpaque so, SpGistSearchItem *item)
 {
-	if (!so->state.attLeafType.attbyval &&
+	/* value is of type attType if isLeaf, else of type attLeafType */
+	/* (no, that is not backwards; yes, it's confusing) */
+	if (!(item->isLeaf ? so->state.attType.attbyval :
+		  so->state.attLeafType.attbyval) &&
 		DatumGetPointer(item->value) != NULL)
 		pfree(DatumGetPointer(item->value));
 
@@ -296,6 +299,7 @@ spgbeginscan(Relation rel, int keysz, int orderbysz)
 {
 	IndexScanDesc scan;
 	SpGistScanOpaque so;
+	TupleDesc	outTupDesc;
 	int			i;
 
 	scan = RelationGetIndexScan(rel, keysz, orderbysz);
@@ -314,8 +318,21 @@ spgbeginscan(Relation rel, int keysz, int orderbysz)
 											 "SP-GiST traversal-value context",
 											 ALLOCSET_DEFAULT_SIZES);
 
-	/* Set up indexTupDesc and xs_hitupdesc in case it's an index-only scan */
-	so->indexTupDesc = scan->xs_hitupdesc = RelationGetDescr(rel);
+	/*
+	 * Set up indexTupDesc and xs_hitupdesc in case it's an index-only scan.
+	 * (It's rather annoying to do this work when it might be wasted, but for
+	 * most opclasses we can re-use the index reldesc instead of making one.)
+	 */
+	if (so->state.attType.type ==
+		TupleDescAttr(RelationGetDescr(rel), 0)->atttypid)
+		outTupDesc = RelationGetDescr(rel);
+	else
+	{
+		outTupDesc = CreateTemplateTupleDesc(1);
+		TupleDescInitEntry(outTupDesc, 1, NULL,
+						   so->state.attType.type, -1, 0);
+	}
+	so->indexTupDesc = scan->xs_hitupdesc = outTupDesc;
 
 	/* Allocate various arrays needed for order-by scans */
 	if (scan->numberOfOrderBys > 0)
@@ -447,9 +464,10 @@ spgNewHeapItem(SpGistScanOpaque so, int level, ItemPointer heapPtr,
 	item->level = level;
 	item->heapPtr = *heapPtr;
 	/* copy value to queue cxt out of tmp cxt */
+	/* caution: "leafValue" is of type attType not leafType */
 	item->value = isnull ? (Datum) 0 :
-		datumCopy(leafValue, so->state.attLeafType.attbyval,
-				  so->state.attLeafType.attlen);
+		datumCopy(leafValue, so->state.attType.attbyval,
+				  so->state.attType.attlen);
 	item->traversalValue = NULL;
 	item->isLeaf = true;
 	item->recheck = recheck;
@@ -497,6 +515,7 @@ spgLeafTest(SpGistScanOpaque so, SpGistSearchItem *item,
 		in.nkeys = so->numberOfKeys;
 		in.orderbys = so->orderByData;
 		in.norderbys = so->numberOfNonNullOrderBys;
+		Assert(!item->isLeaf);	/* else reconstructedValue would be wrong type */
 		in.reconstructedValue = item->value;
 		in.traversalValue = item->traversalValue;
 		in.level = item->level;
@@ -563,6 +582,7 @@ spgInitInnerConsistentIn(spgInnerConsistentIn *in,
 	in->orderbys = so->orderByData;
 	in->nkeys = so->numberOfKeys;
 	in->norderbys = so->numberOfNonNullOrderBys;
+	Assert(!item->isLeaf);		/* else reconstructedValue would be wrong type */
 	in->reconstructedValue = item->value;
 	in->traversalMemoryContext = so->traversalCxt;
 	in->traversalValue = item->traversalValue;
@@ -589,6 +609,7 @@ spgMakeInnerItem(SpGistScanOpaque so,
 		: parentItem->level;
 
 	/* Must copy value out of temp context */
+	/* (recall that reconstructed values are of type leafType) */
 	item->value = out->reconstructedValues
 		? datumCopy(out->reconstructedValues[i],
 					so->state.attLeafType.attbyval,