Re: compress method for spgist - 2

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

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

--
Nikita Glukhov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company