diff --git a/src/backend/access/spgist/spgdoinsert.c b/src/backend/access/spgist/spgdoinsert.c index 7bd269fd2a..e14c71abd0 100644 --- a/src/backend/access/spgist/spgdoinsert.c +++ b/src/backend/access/spgist/spgdoinsert.c @@ -731,13 +731,6 @@ doPickSplit(Relation index, SpGistState *state, * also, count up the amount of space that will be freed from current. * (Note that in the non-root case, we won't actually delete the old * tuples, only replace them with redirects or placeholders.) - * - * Note: the SGLTDATUM calls here are safe even when dealing with a nulls - * page. For a pass-by-value data type we will fetch a word that must - * exist even though it may contain garbage (because of the fact that leaf - * tuples must have size at least SGDTSIZE). For a pass-by-reference type - * we are just computing a pointer that isn't going to get dereferenced. - * So it's not worth guarding the calls with isNulls checks. */ nToInsert = 0; nToDelete = 0; @@ -757,7 +750,8 @@ doPickSplit(Relation index, SpGistState *state, PageGetItemId(current->page, i)); if (it->tupstate == SPGIST_LIVE) { - in.datums[nToInsert] = SGLTDATUM(it, state); + in.datums[nToInsert] = + isNulls ? (Datum) 0 : SGLTDATUM(it, state); heapPtrs[nToInsert] = it->heapPtr; nToInsert++; toDelete[nToDelete] = i; @@ -782,7 +776,8 @@ doPickSplit(Relation index, SpGistState *state, PageGetItemId(current->page, i)); if (it->tupstate == SPGIST_LIVE) { - in.datums[nToInsert] = SGLTDATUM(it, state); + in.datums[nToInsert] = + isNulls ? (Datum) 0 : SGLTDATUM(it, state); heapPtrs[nToInsert] = it->heapPtr; nToInsert++; toDelete[nToDelete] = i; @@ -814,7 +809,8 @@ doPickSplit(Relation index, SpGistState *state, * space to include it; and in any case it has to be included in the input * for the picksplit function. So don't increment nToInsert yet. */ - in.datums[in.nTuples] = SGLTDATUM(newLeafTuple, state); + in.datums[in.nTuples] = + isNulls ? (Datum) 0 : SGLTDATUM(newLeafTuple, state); heapPtrs[in.nTuples] = newLeafTuple->heapPtr; in.nTuples++; @@ -1940,17 +1936,21 @@ spgdoinsert(Relation index, SpGistState *state, leafDatum = (Datum) 0; /* - * Compute space needed for a leaf tuple containing the given datum. - * + * Compute space needed for a leaf tuple containing the given datum. This + * must match spgFormLeafTuple. + */ + leafSize = SGLTHDRSZ; + if (!isnull) + leafSize += SpGistGetLeafTypeSize(&state->attLeafType, leafDatum); + if (leafSize < SGDTSIZE) + leafSize = SGDTSIZE; + /* Account for an item pointer, too */ + leafSize += sizeof(ItemIdData); + + /* * If it isn't gonna fit, and the opclass can't reduce the datum size by * suffixing, bail out now rather than getting into an endless loop. */ - if (!isnull) - leafSize = SGLTHDRSZ + sizeof(ItemIdData) + - SpGistGetTypeSize(&state->attLeafType, leafDatum); - else - leafSize = SGDTSIZE + sizeof(ItemIdData); - if (leafSize > SPGIST_PAGE_CAPACITY && !state->config.longValuesOK) ereport(ERROR, (errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED), @@ -2161,8 +2161,9 @@ spgdoinsert(Relation index, SpGistState *state, if (!isnull) { leafDatum = out.result.matchNode.restDatum; - leafSize = SGLTHDRSZ + sizeof(ItemIdData) + - SpGistGetTypeSize(&state->attLeafType, leafDatum); + leafSize = SGLTHDRSZ + + SpGistGetLeafTypeSize(&state->attLeafType, leafDatum); + leafSize += sizeof(ItemIdData); } /* diff --git a/src/backend/access/spgist/spgutils.c b/src/backend/access/spgist/spgutils.c index d8b1815061..949352ada7 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; @@ -642,6 +661,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 */ @@ -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); diff --git a/src/include/access/spgist_private.h b/src/include/access/spgist_private.h index a81bab24ea..b6f95421d8 100644 --- a/src/include/access/spgist_private.h +++ b/src/include/access/spgist_private.h @@ -266,6 +266,14 @@ typedef struct SpGistCache * header/optional prefix/array of nodes, which are SpGistNodeTuples * * size and prefixSize must be multiples of MAXALIGN + * + * If the prefix datum is of a pass-by-value type, it is stored in its + * Datum representation, that is its on-disk representation is of length + * sizeof(Datum). This is a fairly unfortunate choice, because in no other + * place does Postgres use Datum as an on-disk representation; it creates + * an unnecessary incompatibility between 32-bit and 64-bit builds. But the + * compatibility loss is mostly theoretical since MAXIMUM_ALIGNOF typically + * differs between such builds, too. Anyway we're stuck with it now. */ typedef struct SpGistInnerTupleData { @@ -307,8 +315,7 @@ typedef SpGistInnerTupleData *SpGistInnerTuple; * Node tuples use the same header as ordinary Postgres IndexTuples, but * we do not use a null bitmap, because we know there is only one column * so the INDEX_NULL_MASK bit suffices. Also, pass-by-value datums are - * stored as a full Datum, the same convention as for inner tuple prefixes - * and leaf tuple datums. + * stored in Datum form, the same convention as for inner tuple prefixes. */ typedef IndexTupleData SpGistNodeTupleData; @@ -322,12 +329,15 @@ typedef SpGistNodeTupleData *SpGistNodeTuple; PointerGetDatum(SGNTDATAPTR(x))) /* - * SPGiST leaf tuple: carries a datum and a heap tuple TID + * SPGiST leaf tuple: carries a leaf datum and a heap tuple TID * - * In the simplest case, the datum is the same as the indexed value; but - * it could also be a suffix or some other sort of delta that permits + * In the simplest case, the leaf datum is the same as the indexed value; + * but it could also be a suffix or some other sort of delta that permits * reconstruction given knowledge of the prefix path traversed to get here. * + * If the leaf datum is NULL, it's not stored. This is not represented + * explicitly; we infer it from the tuple being stored on a nulls page. + * * The size field is wider than could possibly be needed for an on-disk leaf * tuple, but this allows us to form leaf tuples even when the datum is too * wide to be stored immediately, and it costs nothing because of alignment @@ -339,13 +349,8 @@ typedef SpGistNodeTupleData *SpGistNodeTuple; * * size must be a multiple of MAXALIGN; also, it must be at least SGDTSIZE * so that the tuple can be converted to REDIRECT status later. (This - * restriction only adds bytes for the null-datum case, otherwise alignment - * restrictions force it anyway.) - * - * In a leaf tuple for a NULL indexed value, there's no useful datum value; - * however, the SGDTSIZE limit ensures that's there's a Datum word there - * anyway, so SGLTDATUM can be applied safely as long as you don't do - * anything with the result. + * restriction only adds bytes for a NULL leaf datum stored on a 32-bit + * machine; otherwise alignment restrictions force it anyway.) */ typedef struct SpGistLeafTupleData { @@ -353,16 +358,16 @@ typedef struct SpGistLeafTupleData size:30; /* large enough for any palloc'able value */ OffsetNumber nextOffset; /* next tuple in chain, or InvalidOffsetNumber */ ItemPointerData heapPtr; /* TID of represented heap tuple */ - /* leaf datum follows */ + /* leaf datum follows on a MAXALIGN boundary */ } SpGistLeafTupleData; typedef SpGistLeafTupleData *SpGistLeafTuple; #define SGLTHDRSZ MAXALIGN(sizeof(SpGistLeafTupleData)) #define SGLTDATAPTR(x) (((char *) (x)) + SGLTHDRSZ) -#define SGLTDATUM(x, s) ((s)->attLeafType.attbyval ? \ - *(Datum *) SGLTDATAPTR(x) : \ - PointerGetDatum(SGLTDATAPTR(x))) +#define SGLTDATUM(x, s) fetch_att(SGLTDATAPTR(x), \ + (s)->attLeafType.attbyval, \ + (s)->attLeafType.attlen) /* * SPGiST dead tuple: declaration for examining non-live tuples @@ -455,7 +460,8 @@ extern void SpGistSetLastUsedPage(Relation index, Buffer buffer); extern void SpGistInitPage(Page page, uint16 f); extern void SpGistInitBuffer(Buffer b, uint16 f); extern void SpGistInitMetapage(Page page); -extern unsigned int SpGistGetTypeSize(SpGistTypeDesc *att, Datum datum); +extern unsigned int SpGistGetInnerTypeSize(SpGistTypeDesc *att, Datum datum); +extern unsigned int SpGistGetLeafTypeSize(SpGistTypeDesc *att, Datum datum); extern SpGistLeafTuple spgFormLeafTuple(SpGistState *state, ItemPointer heapPtr, Datum datum, bool isnull);