Re: compress method for spgist - 2

From: Nikita Glukhov <n(dot)gluhov(at)postgrespro(dot)ru>
To: Alexander Korotkov <a(dot)korotkov(at)postgrespro(dot)ru>, Darafei Praliaskouski <me(at)komzpa(dot)net>
Cc: pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>, Heikki Linnakangas <hlinnaka(at)iki(dot)fi>, Fedor Sigaev <teodor(at)sigaev(dot)ru>
Subject: Re: compress method for spgist - 2
Date: 2017-12-06 15:08:33
Message-ID: afbf0789-50c8-82ac-55b8-898724068622@postgrespro.ru
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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.

> 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

Attachment Content-Type Size
0001-spgist-compress-method-10.patch text/x-patch 20.5 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Tom Lane 2017-12-06 15:16:34 Re: ALTER TABLE ADD COLUMN fast default
Previous Message Ildus Kurbangaliev 2017-12-06 15:07:16 Re: [HACKERS] Custom compression methods