Re: compress method for spgist - 2

On Thu, Dec 7, 2017 at 3:17 AM, Nikita Glukhov <n(dot)gluhov(at)postgrespro(dot)ru>
wrote:

> On 06.12.2017 21:49, Alexander Korotkov wrote:
>
> On Wed, Dec 6, 2017 at 6:08 PM, Nikita Glukhov <n(dot)gluhov(at)postgrespro(dot)ru>
> wrote:
>
>> On 05.12.2017 23:59, Alexander Korotkov wrote:
>>
>> On Tue, Dec 5, 2017 at 1:14 PM, Darafei Praliaskouski <me(at)komzpa(dot)net>
>> wrote:
>>
>>> The following review has been posted through the commitfest application:
>>> make installcheck-world: not tested
>>> Implements feature: not tested
>>> Spec compliant: not tested
>>> Documentation: tested, passed
>>>
>>> I've read the updated patch and see my concerns addressed.
>>>
>>> I'm looking forward to SP-GiST compress method support, as it will allow
>>> usage of SP-GiST index infrastructure for PostGIS.
>>>
>>> The new status of this patch is: Ready for Committer
>>>
>>
>> I went trough this patch. I found documentation changes to be not
>> sufficient. And I've made some improvements.
>>
>> In particular, I didn't understand why is reconstructedValue claimed to
>> be of spgConfigOut.leafType while it should be of spgConfigIn.attType both
>> from general logic and code. I've fixed that. Nikita, correct me if I'm
>> wrong.
>>
>>
>> I think we are reconstructing a leaf datum, so documentation was correct
>> but the code in spgWalk() and freeScanStackEntry() wrongly used attType
>> instead of attLeafType. Fixed patch is attached.
>>
>
> Reconstructed datum is used for index-only scan. Thus, it should be
> original indexed datum of attType, unless we have decompress method and
> pass reconstructed datum through it.
>
> But if we have compress method and do not have decompress method then
> index-only scan seems to be impossible.
>

Sorry, I didn't realize that we don't return reconstructed value
immediately, but return only leafValue provided by leafConsistent. In this
case, leafConsistent is responsible to convert value from
spgConfigOut.leafType to spgConfigIn.attType.

TBH, practical example of SP-GiST opclass with both compress method and
index-only scan support doesn't come to my mind, because compress method is
typically needed when we have lossy representation of index keys. For
example, in GiST all the opclasses where compress method do useful work use
lossy representation of keys. Nevertheless, it's good to not cut
possibility of index-only scans when spgConfigOut.leafType !=
spgConfigIn.attType. And to lay responsibility for conversion on
leafConsistent seems like elegant way to do this. So, that's OK for me.

Also, I wonder should we check for existence of compress method when
>> attType and leafType are not the same in spgvalidate() function? We don't
>> do this for now.
>>
>> I've added compress method existence check to spgvalidate().
>>
> It would be nice to evade double calling of config method. Possible
> option could be to memorize difference between attribute type and leaf type
> in high bit of functionset, which is guaranteed to be free.
>
> I decided to simply set compress method's bit in functionset when this
> method it is not required.
>

Looks good for me.

Now, this patch is ready for committer from my point of view.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company