SP-GiST confusion: indexed column's type vs. index column type

On Fri, Apr 2, 2021 at 9:37 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

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.

+1 to fixing it on HEAD only.

--
Peter Geoghegan

Peter Geoghegan <pg@bowt.ie> writes:

On Fri, Apr 2, 2021 at 9:37 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

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.

+1 to fixing it on HEAD only.

Here's a draft patch for that, in case anyone wants to look it
over.

The confusion went even deeper than I thought, as some of the code
mistakenly thought that reconstructed "leafValue" values were of the
leaf data type rather than the input attribute type. (Which is not
too surprising, given that that's such a misleading name, but the
docs are clear and correct on the point.)

Also, both the code and docs thought that the "reconstructedValue"
datums that are passed down the tree during a search should be of
the leaf data type. This seems to me to be arrant nonsense.
As an example, if you made an opclass that indexes 1-D arrays
by labeling each inner node with successive array elements,
right down to the leaf which is the last array element, it will
absolutely not work for the reconstructedValues to be of the
leaf type --- they have to be of the array type. (As I said
in commit 1ebdec8c0, this'd be a fairly poorly-chosen opclass
design, but it seems like it ought to physically work.)

Given the amount of confusion here, I don't have a lot of confidence
that an opclass that wants to reconstruct values while having
leafType different from input type will work even with this patch.
I'm strongly tempted to make a src/test/modules module that
implements exactly the silly design given above, just so we have
some coverage for this scenario.

regards, tom lane

I wrote:

Also, both the code and docs thought that the "reconstructedValue"
datums that are passed down the tree during a search should be of
the leaf data type. This seems to me to be arrant nonsense.
As an example, if you made an opclass that indexes 1-D arrays
by labeling each inner node with successive array elements,
right down to the leaf which is the last array element, it will
absolutely not work for the reconstructedValues to be of the
leaf type --- they have to be of the array type. (As I said
in commit 1ebdec8c0, this'd be a fairly poorly-chosen opclass
design, but it seems like it ought to physically work.)

So after trying to build an opclass that does that, I have a clearer
understanding of why opclasses that'd break the existing code are
so thin on the ground. You can't do the above, because the opclass
cannot force the AM to add inner nodes that it doesn't want to.
For example, the first few index entries will simply be dumped into
the root page as undifferentiated leaf tuples. This means that,
if you'd like to be able to return reconstructed index entries, the
leaf data type *must* be able to hold all the data that is in an
input value. In principle you could represent it in some other
format, but the path of least resistance is definitely to make the
leaf type the same as the input.

I still want to make an opclass in which those types are different,
if only for testing purposes, but I'm having a hard time coming up
with a plan that's not totally lame. Best idea I can think of is
to wrap the input in a bytea, which just begs the question "why
would you do that?". Anybody have a less lame thought?

regards, tom lane

I wrote:

I still want to make an opclass in which those types are different,
if only for testing purposes, but I'm having a hard time coming up
with a plan that's not totally lame. Best idea I can think of is
to wrap the input in a bytea, which just begs the question "why
would you do that?". Anybody have a less lame thought?

I thought of a plan that's at least simple to code: make an opclass
that takes "name" but does all the internal storage as "text". Then
all the code can be stolen from spgtextproc.c with very minor changes.
I'd been too fixated on finding an example in which attType and
leafType differ as to pass-by-ref vs pass-by-value, but actually a
test case with positive typlen vs. varlena typlen will do just as well
for finding wrong-type references.

And, having coded that up, my first test result is

regression=# create extension spgist_name_ops ;
ERROR: storage type cannot be different from data type for access method "spgist"

evidently because SPGiST doesn't set amroutine->amstorage.

That's silly on its face because we have built-in opclasses in which
those types are different, but it probably helps explain why there are
no field reports of trouble with these bugs ...

regards, tom lane

Here's a patch that, in addition to what I mentioned upthread,
rescinds the limitation that user-defined SPGIST opclasses can't
set the STORAGE parameter, and cleans up some residual confusion
about whether values are of the indexed type (attType) or the
storage type (leafType). Once I'd wrapped my head around the idea
that indeed intermediate-level "reconstructed" values ought to be
of the leafType, there were fewer bugs of that sort than I thought
yesterday ... but still a nonzero number.

I've also attached a test module that exercises reconstruction
during index-only scan with leafType being meaningfully different
from attType. I'm not quite sure whether this is worth
committing, but I'm leaning towards doing so.

regards, tom lane

I wrote:

I propose changing things so that
(B) We enforce that leafType agrees with the opclass opckeytype,
ensuring the index tupdesc can be used for leaf tuples.

After looking at PostGIS I realized that a hard restriction of this
sort won't fly, because it'd make upgrades impossible for them.
They have some lossy SPGiST opclasses, in which leafType is returned
as different from the original input datatype. Since up to now
we've disallowed the STORAGE clause for user-defined SPGiST
opclasses, they are unable to declare these opclasses honestly in
existing releases, but it didn't matter. If we enforce that STORAGE
matches leafType then their existing opclass definitions won't work
in v14, but they can't fix them before upgrading either.

So I backed off the complaint about that to be just an amvalidate
warning, and pushed it.

This means the INCLUDE patch will still have to account for the
possibility that the index tupdesc is an inaccurate representation
of the actual leaf tuple contents, but we can treat that case less
efficiently without feeling bad about it. So we should be able to
do something similar for the leaf tupdesc as for the index-only-scan
output tupdesc, that is use the relcache's tupdesc if it's got the
right first column type, otherwise copy-and-modify that tupdesc.

regards, tom lane