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

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Peter Geoghegan <pg(at)bowt(dot)ie>
Cc: PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>, Pavel Borisov <pashkin(dot)elfe(at)gmail(dot)com>
Subject: Re: SP-GiST confusion: indexed column's type vs. index column type
Date: 2021-04-02 23:24:23
Message-ID: 3928777.1617405863@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Peter Geoghegan <pg(at)bowt(dot)ie> writes:
> On Fri, Apr 2, 2021 at 9:37 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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

Attachment Content-Type Size
fix-spgist-type-confusion-1.patch text/x-diff 17.6 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Bruce Momjian 2021-04-02 23:47:32 Re: Have I found an interval arithmetic bug?
Previous Message Tom Lane 2021-04-02 23:07:24 Re: libpq debug log