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.gluhov@postgrespro.ru <mailto:n.gluhov@postgrespro.ru>> wrote:
>
>     On 05.12.2017 23:59, Alexander Korotkov wrote:
>
>>     On Tue, Dec 5, 2017 at 1:14 PM, Darafei Praliaskouski
>>     <me@komzpa.net <mailto:me@komzpa.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