Skip to content

Commit ac9099f

Browse files
tglsfdctglsfdc
committed
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 parent d9c5b9a commit ac9099f

File tree

17 files changed

+937
-44
lines changed

17 files changed

+937
-44
lines changed

‎doc/src/sgml/ref/create_opclass.sgml

+1-1
Original file line numberDiff line numberDiff line change
@@ -234,7 +234,7 @@ CREATE OPERATOR CLASS <replaceable class="parameter">name</replaceable> [ DEFAUL
234234
<para>
235235
The data type actually stored in the index. Normally this is
236236
the same as the column data type, but some index methods
237-
(currently GiST, GIN and BRIN) allow it to be different. The
237+
(currently GiST, GIN, SP-GiST and BRIN) allow it to be different. The
238238
<literal>STORAGE</literal> clause must be omitted unless the index
239239
method allows a different type to be used.
240240
If the column <replaceable class="parameter">data_type</replaceable> is specified

‎doc/src/sgml/spgist.sgml

+48-26
Original file line numberDiff line numberDiff line change
@@ -205,10 +205,12 @@
205205
</para>
206206

207207
<para>
208-
Leaf tuples of an <acronym>SP-GiST</acronym> tree contain values of the
209-
same data type as the indexed column. Leaf tuples at the root level will
210-
always contain the original indexed data value, but leaf tuples at lower
211-
levels might contain only a compressed representation, such as a suffix.
208+
Leaf tuples of an <acronym>SP-GiST</acronym> tree usually contain values
209+
of the same data type as the indexed column, although it is also possible
210+
for them to contain lossy representations of the indexed column.
211+
Leaf tuples stored at the root level will directly represent
212+
the original indexed data value, but leaf tuples at lower
213+
levels might contain only a partial value, such as a suffix.
212214
In that case the operator class support functions must be able to
213215
reconstruct the original value using information accumulated from the
214216
inner tuples that are passed through to reach the leaf level.
@@ -330,19 +332,29 @@ typedef struct spgConfigOut
330332
</para>
331333

332334
<para>
333-
<structfield>leafType</structfield> is typically the same as
334-
<structfield>attType</structfield>. For the reasons of backward
335-
compatibility, method <function>config</function> can
336-
leave <structfield>leafType</structfield> uninitialized; that would
337-
give the same effect as setting <structfield>leafType</structfield> equal
338-
to <structfield>attType</structfield>. When <structfield>attType</structfield>
339-
and <structfield>leafType</structfield> are different, then optional
335+
<structfield>leafType</structfield> should match the index storage type
336+
defined by the operator class's <structfield>opckeytype</structfield>
337+
catalog entry.
338+
(Note that <structfield>opckeytype</structfield> can be zero,
339+
implying the storage type is the same as the operator class's input
340+
type, which is the most common situation.)
341+
For reasons of backward compatibility, the <function>config</function>
342+
method can set <structfield>leafType</structfield> to some other value,
343+
and that value will be used; but this is deprecated since the index
344+
contents are then incorrectly identified in the catalogs.
345+
Also, it's permissible to
346+
leave <structfield>leafType</structfield> uninitialized (zero);
347+
that is interpreted as meaning the index storage type derived from
348+
<structfield>opckeytype</structfield>.
349+
</para>
350+
351+
<para>
352+
When <structfield>attType</structfield>
353+
and <structfield>leafType</structfield> are different, the optional
340354
method <function>compress</function> must be provided.
341355
Method <function>compress</function> is responsible
342356
for transformation of datums to be indexed from <structfield>attType</structfield>
343357
to <structfield>leafType</structfield>.
344-
Note: both consistent functions will get <structfield>scankeys</structfield>
345-
unchanged, without transformation using <function>compress</function>.
346358
</para>
347359
</listitem>
348360
</varlistentry>
@@ -677,8 +689,7 @@ typedef struct spgInnerConsistentOut
677689
<structfield>reconstructedValue</structfield> is the value reconstructed for the
678690
parent tuple; it is <literal>(Datum) 0</literal> at the root level or if the
679691
<function>inner_consistent</function> function did not provide a value at the
680-
parent level. <structfield>reconstructedValue</structfield> is always of
681-
<structname>spgConfigOut</structname>.<structfield>leafType</structfield> type.
692+
parent level.
682693
<structfield>traversalValue</structfield> is a pointer to any traverse data
683694
passed down from the previous call of <function>inner_consistent</function>
684695
on the parent index tuple, or NULL at the root level.
@@ -713,9 +724,14 @@ typedef struct spgInnerConsistentOut
713724
necessarily so, so an array is used.)
714725
If value reconstruction is needed, set
715726
<structfield>reconstructedValues</structfield> to an array of the values
716-
of <structname>spgConfigOut</structname>.<structfield>leafType</structfield> type
717727
reconstructed for each child node to be visited; otherwise, leave
718728
<structfield>reconstructedValues</structfield> as NULL.
729+
The reconstructed values are assumed to be of type
730+
<structname>spgConfigOut</structname>.<structfield>leafType</structfield>.
731+
(However, since the core system will do nothing with them except
732+
possibly copy them, it is sufficient for them to have the
733+
same <literal>typlen</literal> and <literal>typbyval</literal>
734+
properties as <structfield>leafType</structfield>.)
719735
If ordered search is performed, set <structfield>distances</structfield>
720736
to an array of distance values according to <structfield>orderbys</structfield>
721737
array (nodes with lowest distances will be processed first). Leave it
@@ -797,8 +813,7 @@ typedef struct spgLeafConsistentOut
797813
<structfield>reconstructedValue</structfield> is the value reconstructed for the
798814
parent tuple; it is <literal>(Datum) 0</literal> at the root level or if the
799815
<function>inner_consistent</function> function did not provide a value at the
800-
parent level. <structfield>reconstructedValue</structfield> is always of
801-
<structname>spgConfigOut</structname>.<structfield>leafType</structfield> type.
816+
parent level.
802817
<structfield>traversalValue</structfield> is a pointer to any traverse data
803818
passed down from the previous call of <function>inner_consistent</function>
804819
on the parent index tuple, or NULL at the root level.
@@ -816,8 +831,8 @@ typedef struct spgLeafConsistentOut
816831
The function must return <literal>true</literal> if the leaf tuple matches the
817832
query, or <literal>false</literal> if not. In the <literal>true</literal> case,
818833
if <structfield>returnData</structfield> is <literal>true</literal> then
819-
<structfield>leafValue</structfield> must be set to the value of
820-
<structname>spgConfigIn</structname>.<structfield>attType</structfield> type
834+
<structfield>leafValue</structfield> must be set to the value (of type
835+
<structname>spgConfigIn</structname>.<structfield>attType</structfield>)
821836
originally supplied to be indexed for this leaf tuple. Also,
822837
<structfield>recheck</structfield> may be set to <literal>true</literal> if the match
823838
is uncertain and so the operator(s) must be re-applied to the actual
@@ -834,23 +849,30 @@ typedef struct spgLeafConsistentOut
834849
</variablelist>
835850

836851
<para>
837-
The optional user-defined method are:
852+
The optional user-defined methods are:
838853
</para>
839854

840855
<variablelist>
841856
<varlistentry>
842857
<term><function>Datum compress(Datum in)</function></term>
843858
<listitem>
844859
<para>
845-
Converts the data item into a format suitable for physical storage in
846-
a leaf tuple of index page. It accepts
860+
Converts a data item into a format suitable for physical storage in
861+
a leaf tuple of the index. It accepts a value of type
847862
<structname>spgConfigIn</structname>.<structfield>attType</structfield>
848-
value and returns
849-
<structname>spgConfigOut</structname>.<structfield>leafType</structfield>
850-
value. Output value should not be toasted.
863+
and returns a value of type
864+
<structname>spgConfigOut</structname>.<structfield>leafType</structfield>.
865+
The output value must not contain an out-of-line TOAST pointer.
866+
</para>
867+
868+
<para>
869+
Note: the <function>compress</function> method is only applied to
870+
values to be stored. The consistent methods receive query scankeys
871+
unchanged, without transformation using <function>compress</function>.
851872
</para>
852873
</listitem>
853874
</varlistentry>
875+
854876
<varlistentry>
855877
<term><function>options</function></term>
856878
<listitem>

‎src/backend/access/spgist/spgscan.c

+26-5
Original file line numberDiff line numberDiff line change
@@ -81,7 +81,10 @@ pairingheap_SpGistSearchItem_cmp(const pairingheap_node *a,
8181
static void
8282
spgFreeSearchItem(SpGistScanOpaque so, SpGistSearchItem *item)
8383
{
84-
if (!so->state.attLeafType.attbyval &&
84+
/* value is of type attType if isLeaf, else of type attLeafType */
85+
/* (no, that is not backwards; yes, it's confusing) */
86+
if (!(item->isLeaf ? so->state.attType.attbyval :
87+
so->state.attLeafType.attbyval) &&
8588
DatumGetPointer(item->value) != NULL)
8689
pfree(DatumGetPointer(item->value));
8790

@@ -296,6 +299,7 @@ spgbeginscan(Relation rel, int keysz, int orderbysz)
296299
{
297300
IndexScanDesc scan;
298301
SpGistScanOpaque so;
302+
TupleDesc outTupDesc;
299303
int i;
300304

301305
scan = RelationGetIndexScan(rel, keysz, orderbysz);
@@ -314,8 +318,21 @@ spgbeginscan(Relation rel, int keysz, int orderbysz)
314318
"SP-GiST traversal-value context",
315319
ALLOCSET_DEFAULT_SIZES);
316320

317-
/* Set up indexTupDesc and xs_hitupdesc in case it's an index-only scan */
318-
so->indexTupDesc = scan->xs_hitupdesc = RelationGetDescr(rel);
321+
/*
322+
* Set up indexTupDesc and xs_hitupdesc in case it's an index-only scan.
323+
* (It's rather annoying to do this work when it might be wasted, but for
324+
* most opclasses we can re-use the index reldesc instead of making one.)
325+
*/
326+
if (so->state.attType.type ==
327+
TupleDescAttr(RelationGetDescr(rel), 0)->atttypid)
328+
outTupDesc = RelationGetDescr(rel);
329+
else
330+
{
331+
outTupDesc = CreateTemplateTupleDesc(1);
332+
TupleDescInitEntry(outTupDesc, 1, NULL,
333+
so->state.attType.type, -1, 0);
334+
}
335+
so->indexTupDesc = scan->xs_hitupdesc = outTupDesc;
319336

320337
/* Allocate various arrays needed for order-by scans */
321338
if (scan->numberOfOrderBys > 0)
@@ -447,9 +464,10 @@ spgNewHeapItem(SpGistScanOpaque so, int level, ItemPointer heapPtr,
447464
item->level = level;
448465
item->heapPtr = *heapPtr;
449466
/* copy value to queue cxt out of tmp cxt */
467+
/* caution: "leafValue" is of type attType not leafType */
450468
item->value = isnull ? (Datum) 0 :
451-
datumCopy(leafValue, so->state.attLeafType.attbyval,
452-
so->state.attLeafType.attlen);
469+
datumCopy(leafValue, so->state.attType.attbyval,
470+
so->state.attType.attlen);
453471
item->traversalValue = NULL;
454472
item->isLeaf = true;
455473
item->recheck = recheck;
@@ -497,6 +515,7 @@ spgLeafTest(SpGistScanOpaque so, SpGistSearchItem *item,
497515
in.nkeys = so->numberOfKeys;
498516
in.orderbys = so->orderByData;
499517
in.norderbys = so->numberOfNonNullOrderBys;
518+
Assert(!item->isLeaf); /* else reconstructedValue would be wrong type */
500519
in.reconstructedValue = item->value;
501520
in.traversalValue = item->traversalValue;
502521
in.level = item->level;
@@ -563,6 +582,7 @@ spgInitInnerConsistentIn(spgInnerConsistentIn *in,
563582
in->orderbys = so->orderByData;
564583
in->nkeys = so->numberOfKeys;
565584
in->norderbys = so->numberOfNonNullOrderBys;
585+
Assert(!item->isLeaf); /* else reconstructedValue would be wrong type */
566586
in->reconstructedValue = item->value;
567587
in->traversalMemoryContext = so->traversalCxt;
568588
in->traversalValue = item->traversalValue;
@@ -589,6 +609,7 @@ spgMakeInnerItem(SpGistScanOpaque so,
589609
: parentItem->level;
590610

591611
/* Must copy value out of temp context */
612+
/* (recall that reconstructed values are of type leafType) */
592613
item->value = out->reconstructedValues
593614
? datumCopy(out->reconstructedValues[i],
594615
so->state.attLeafType.attbyval,

‎src/backend/access/spgist/spgutils.c

+78-6
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323
#include "access/xact.h"
2424
#include "catalog/pg_amop.h"
2525
#include "commands/vacuum.h"
26+
#include "nodes/nodeFuncs.h"
2627
#include "storage/bufmgr.h"
2728
#include "storage/indexfsm.h"
2829
#include "storage/lmgr.h"
@@ -53,7 +54,7 @@ spghandler(PG_FUNCTION_ARGS)
5354
amroutine->amoptionalkey = true;
5455
amroutine->amsearcharray = false;
5556
amroutine->amsearchnulls = true;
56-
amroutine->amstorage = false;
57+
amroutine->amstorage = true;
5758
amroutine->amclusterable = false;
5859
amroutine->ampredlocks = false;
5960
amroutine->amcanparallel = false;
@@ -89,6 +90,66 @@ spghandler(PG_FUNCTION_ARGS)
8990
PG_RETURN_POINTER(amroutine);
9091
}
9192

93+
/*
94+
* GetIndexInputType
95+
* Determine the nominal input data type for an index column
96+
*
97+
* We define the "nominal" input type as the associated opclass's opcintype,
98+
* or if that is a polymorphic type, the base type of the heap column or
99+
* expression that is the index's input. The reason for preferring the
100+
* opcintype is that non-polymorphic opclasses probably don't want to hear
101+
* about binary-compatible input types. For instance, if a text opclass
102+
* is being used with a varchar heap column, we want to report "text" not
103+
* "varchar". Likewise, opclasses don't want to hear about domain types,
104+
* so if we do consult the actual input type, we make sure to flatten domains.
105+
*
106+
* At some point maybe this should go somewhere else, but it's not clear
107+
* if any other index AMs have a use for it.
108+
*/
109+
static Oid
110+
GetIndexInputType(Relation index, AttrNumber indexcol)
111+
{
112+
Oid opcintype;
113+
AttrNumber heapcol;
114+
List *indexprs;
115+
ListCell *indexpr_item;
116+
117+
Assert(index->rd_index != NULL);
118+
Assert(indexcol > 0 && indexcol <= index->rd_index->indnkeyatts);
119+
opcintype = index->rd_opcintype[indexcol - 1];
120+
if (!IsPolymorphicType(opcintype))
121+
return opcintype;
122+
heapcol = index->rd_index->indkey.values[indexcol - 1];
123+
if (heapcol != 0) /* Simple index column? */
124+
return getBaseType(get_atttype(index->rd_index->indrelid, heapcol));
125+
126+
/*
127+
* If the index expressions are already cached, skip calling
128+
* RelationGetIndexExpressions, as it will make a copy which is overkill.
129+
* We're not going to modify the trees, and we're not going to do anything
130+
* that would invalidate the relcache entry before we're done.
131+
*/
132+
if (index->rd_indexprs)
133+
indexprs = index->rd_indexprs;
134+
else
135+
indexprs = RelationGetIndexExpressions(index);
136+
indexpr_item = list_head(indexprs);
137+
for (int i = 1; i <= index->rd_index->indnkeyatts; i++)
138+
{
139+
if (index->rd_index->indkey.values[i - 1] == 0)
140+
{
141+
/* expression column */
142+
if (indexpr_item == NULL)
143+
elog(ERROR, "wrong number of index expressions");
144+
if (i == indexcol)
145+
return getBaseType(exprType((Node *) lfirst(indexpr_item)));
146+
indexpr_item = lnext(indexprs, indexpr_item);
147+
}
148+
}
149+
elog(ERROR, "wrong number of index expressions");
150+
return InvalidOid; /* keep compiler quiet */
151+
}
152+
92153
/* Fill in a SpGistTypeDesc struct with info about the specified data type */
93154
static void
94155
fillTypeDesc(SpGistTypeDesc *desc, Oid type)
@@ -121,11 +182,11 @@ spgGetCache(Relation index)
121182
Assert(index->rd_att->natts == 1);
122183

123184
/*
124-
* Get the actual data type of the indexed column from the index
125-
* tupdesc. We pass this to the opclass config function so that
185+
* Get the actual (well, nominal) data type of the column being
186+
* indexed. We pass this to the opclass config function so that
126187
* polymorphic opclasses are possible.
127188
*/
128-
atttype = TupleDescAttr(index->rd_att, 0)->atttypid;
189+
atttype = GetIndexInputType(index, 1);
129190

130191
/* Call the config function to get config info for the opclass */
131192
in.attType = atttype;
@@ -136,11 +197,21 @@ spgGetCache(Relation index)
136197
PointerGetDatum(&in),
137198
PointerGetDatum(&cache->config));
138199

200+
/*
201+
* If leafType isn't specified, use the declared index column type,
202+
* which index.c will have derived from the opclass's opcintype.
203+
* (Although we now make spgvalidate.c warn if these aren't the same,
204+
* old user-defined opclasses may not set the STORAGE parameter
205+
* correctly, so believe leafType if it's given.)
206+
*/
207+
if (!OidIsValid(cache->config.leafType))
208+
cache->config.leafType =
209+
TupleDescAttr(RelationGetDescr(index), 0)->atttypid;
210+
139211
/* Get the information we need about each relevant datatype */
140212
fillTypeDesc(&cache->attType, atttype);
141213

142-
if (OidIsValid(cache->config.leafType) &&
143-
cache->config.leafType != atttype)
214+
if (cache->config.leafType != atttype)
144215
{
145216
if (!OidIsValid(index_getprocid(index, 1, SPGIST_COMPRESS_PROC)))
146217
ereport(ERROR,
@@ -151,6 +222,7 @@ spgGetCache(Relation index)
151222
}
152223
else
153224
{
225+
/* Save lookups in this common case */
154226
cache->attLeafType = cache->attType;
155227
}
156228

0 commit comments

Comments
 (0)