Rethink handling of pass-by-value leaf datums in SP-GiST.

The existing convention in SP-GiST is that any pass-by-value datatype
is stored in Datum representation, i.e. it's of width sizeof(Datum)
even when typlen is less than that.  This is okay, or at least it's
too late to change it, for prefix datums and node-label datums in inner
(upper) tuples.  But it's problematic for leaf datums, because we'd
prefer those to be stored in Postgres' standard on-disk representation
so that we can easily extend leaf tuples to carry additional "included"
columns.

I believe, however, that we can get away with just up and changing that.
This would be an unacceptable on-disk-format break, but there are two
big mitigating factors:

1. It seems quite unlikely that there are any SP-GiST opclasses out
there that use pass-by-value leaf datatypes.  Certainly none of the
ones in core do, nor has codesearch.debian.net heard of any.  Given
what SP-GiST is good for, it's hard to conceive of a use-case where
the leaf-level values would be both small and fixed-width.  (As an
example, if you wanted to index text values with the leaf level being
just a byte, then every text string would have to be represented
with one level of inner tuple per preceding byte, which would be
horrendously space-inefficient and slow to access.  You always want
to use as few inner-tuple levels as possible, leaving as much as
possible in the leaf values.)

2. Even granting that you have such an index, this change only
breaks things on big-endian machines.  On little-endian, the high
order bytes of the Datum format will now just appear to be alignment
padding space.

So, change the code to store pass-by-value leaf datums in their
usual on-disk form.  Inner-tuple datums are not touched.

This is extracted from a larger patch that intends to add support for
"included" columns.  I'm committing it separately for visibility in
our commit logs.

Pavel Borisov and Tom Lane, reviewed by Andrey Borodin

Discussion: https://postgr.es/m/CALT9ZEFi-vMp4faht9f9Junb1nO3NOSjhpxTmbm1UGLMsLqiEQ@mail.gmail.com

1 files changed, 50 insertions, 12 deletions

diff --git a/src/backend/access/spgist/spgutils.c b/src/backend/access/spgist/spgutils.c
index d8b18150612..949352ada76 100644
--- a/src/backend/access/spgist/spgutils.c
+++ b/src/backend/access/spgist/spgutils.c
@@ -603,13 +603,14 @@ spgoptions(Datum reloptions, bool validate)
 }
 
 /*
- * Get the space needed to store a non-null datum of the indicated type.
+ * Get the space needed to store a non-null datum of the indicated type
+ * in an inner tuple (that is, as a prefix or node label).
  * Note the result is already rounded up to a MAXALIGN boundary.
- * Also, we follow the SPGiST convention that pass-by-val types are
- * just stored in their Datum representation (compare memcpyDatum).
+ * Here we follow the convention that pass-by-val types are just stored
+ * in their Datum representation (compare memcpyInnerDatum).
  */
 unsigned int
-SpGistGetTypeSize(SpGistTypeDesc *att, Datum datum)
+SpGistGetInnerTypeSize(SpGistTypeDesc *att, Datum datum)
 {
 	unsigned int size;
 
@@ -624,10 +625,28 @@ SpGistGetTypeSize(SpGistTypeDesc *att, Datum datum)
 }
 
 /*
- * Copy the given non-null datum to *target
+ * Get the space needed to store a non-null datum of the indicated type
+ * in a leaf tuple.  This is just the usual storage space for the type,
+ * but rounded up to a MAXALIGN boundary.
+ */
+unsigned int
+SpGistGetLeafTypeSize(SpGistTypeDesc *att, Datum datum)
+{
+	unsigned int size;
+
+	if (att->attlen > 0)
+		size = att->attlen;
+	else
+		size = VARSIZE_ANY(datum);
+
+	return MAXALIGN(size);
+}
+
+/*
+ * Copy the given non-null datum to *target, in the inner-tuple case
  */
 static void
-memcpyDatum(void *target, SpGistTypeDesc *att, Datum datum)
+memcpyInnerDatum(void *target, SpGistTypeDesc *att, Datum datum)
 {
 	unsigned int size;
 
@@ -643,6 +662,25 @@ memcpyDatum(void *target, SpGistTypeDesc *att, Datum datum)
 }
 
 /*
+ * Copy the given non-null datum to *target, in the leaf-tuple case
+ */
+static void
+memcpyLeafDatum(void *target, SpGistTypeDesc *att, Datum datum)
+{
+	unsigned int size;
+
+	if (att->attbyval)
+	{
+		store_att_byval(target, datum, att->attlen);
+	}
+	else
+	{
+		size = (att->attlen > 0) ? att->attlen : VARSIZE_ANY(datum);
+		memcpy(target, DatumGetPointer(datum), size);
+	}
+}
+
+/*
  * Construct a leaf tuple containing the given heap TID and datum value
  */
 SpGistLeafTuple
@@ -655,7 +693,7 @@ spgFormLeafTuple(SpGistState *state, ItemPointer heapPtr,
 	/* compute space needed (note result is already maxaligned) */
 	size = SGLTHDRSZ;
 	if (!isnull)
-		size += SpGistGetTypeSize(&state->attLeafType, datum);
+		size += SpGistGetLeafTypeSize(&state->attLeafType, datum);
 
 	/*
 	 * Ensure that we can replace the tuple with a dead tuple later.  This
@@ -671,7 +709,7 @@ spgFormLeafTuple(SpGistState *state, ItemPointer heapPtr,
 	tup->nextOffset = InvalidOffsetNumber;
 	tup->heapPtr = *heapPtr;
 	if (!isnull)
-		memcpyDatum(SGLTDATAPTR(tup), &state->attLeafType, datum);
+		memcpyLeafDatum(SGLTDATAPTR(tup), &state->attLeafType, datum);
 
 	return tup;
 }
@@ -692,7 +730,7 @@ spgFormNodeTuple(SpGistState *state, Datum label, bool isnull)
 	/* compute space needed (note result is already maxaligned) */
 	size = SGNTHDRSZ;
 	if (!isnull)
-		size += SpGistGetTypeSize(&state->attLabelType, label);
+		size += SpGistGetInnerTypeSize(&state->attLabelType, label);
 
 	/*
 	 * Here we make sure that the size will fit in the field reserved for it
@@ -716,7 +754,7 @@ spgFormNodeTuple(SpGistState *state, Datum label, bool isnull)
 	ItemPointerSetInvalid(&tup->t_tid);
 
 	if (!isnull)
-		memcpyDatum(SGNTDATAPTR(tup), &state->attLabelType, label);
+		memcpyInnerDatum(SGNTDATAPTR(tup), &state->attLabelType, label);
 
 	return tup;
 }
@@ -736,7 +774,7 @@ spgFormInnerTuple(SpGistState *state, bool hasPrefix, Datum prefix,
 
 	/* Compute size needed */
 	if (hasPrefix)
-		prefixSize = SpGistGetTypeSize(&state->attPrefixType, prefix);
+		prefixSize = SpGistGetInnerTypeSize(&state->attPrefixType, prefix);
 	else
 		prefixSize = 0;
 
@@ -781,7 +819,7 @@ spgFormInnerTuple(SpGistState *state, bool hasPrefix, Datum prefix,
 	tup->size = size;
 
 	if (hasPrefix)
-		memcpyDatum(SGITDATAPTR(tup), &state->attPrefixType, prefix);
+		memcpyInnerDatum(SGITDATAPTR(tup), &state->attPrefixType, prefix);
 
 	ptr = (char *) SGITNODEPTR(tup);