While reviewing Pavel Borisov's patch to enable INCLUDE columns in
SP-GiST, I found some things that seem like pre-existing bugs.
These only accidentally fail to cause any problems in the existing
SP-GiST opclasses:
1. The attType passed to an opclass's config method is documented as
Oid attType; /* Data type to be indexed */
Now, I would read that as meaning the type of the underlying heap
column; the documentation and code about when a "compress" method
is required certainly seem to think so too. What is actually being
passed, though, is the data type of the index column, that is the
"opckeytype" of the index opclass. We've failed to notice this because
(1) for most of the core SP-GiST opclasses, the two types are the same,
and (2) none of the core opclasses bother to examine attType anyway.
2. When performing an index-only scan on an SP-GiST index, what we
pass back as the tuple descriptor of the output tuples is just the
index relation's own tupdesc, cf spgbeginscan:
/* Set up indexTupDesc and xs_hitupdesc in case it's an index-only scan */
so->indexTupDesc = scan->xs_hitupdesc = RelationGetDescr(rel);
Again, what this is going to report is the opckeytype, not the
reconstructed heap column datatype. That's just flat out wrong.
We've failed to notice because the only core opclass for which
those types are different is poly_ops, which does not support
reconstructing the polygons for index-only scan.
We need to do something about this because the INCLUDE patch needs
the relation descriptor of an SP-GiST index to match the reality
of what is stored in the leaf tuples. Right now, as far as I can tell,
there isn't really any necessary connection between the atttype
claimed by the relation descriptor and the leaf type that's physically
stored. They're accidentally the same in all existing opclasses,
but only accidentally.
I propose changing things so that
(A) attType really is the input (heap) data type.
(B) We enforce that leafType agrees with the opclass opckeytype,
ensuring the index tupdesc can be used for leaf tuples.
(C) The tupdesc passed back for an index-only scan reports the
input (heap) data type.
This might be too much change for the back branches. Given the
lack of complaints to date, I think we can just fix it in HEAD.
regards, tom lane
