Re: compress method for spgist - 2

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.

> 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.

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