Horton

Thread

Commits

  1. Fix more confusion in SP-GiST.

  2. Fix confusion in SP-GiST between attribute type and leaf storage type.

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

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-04-02T16:37:51Z

    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

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

    Peter Geoghegan <pg@bowt.ie> — 2021-04-02T16:46:28Z

    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

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-04-02T23:24:23Z

    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

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-04-03T02:05:02Z

    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

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-04-03T17:16:27Z

    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

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-04-03T20:06:38Z

    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

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-04-04T18:40:01Z

    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