Re: compress method for spgist - 2

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.

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

> 0002-spgist-polygon-8.patch is OK for me so soon.

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