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>
Cc: Darafei Praliaskouski <me(at)komzpa(dot)net>, 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-07 00:17:58
Message-ID: ede7be56-793c-6ab4-bb42-67e8e591c96d@postgrespro.ru
Views: Raw Message | Whole Thread | Download mbox
Thread:
Lists: pgsql-hackers

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

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

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Masahiko Sawada 2017-12-07 02:07:55 Re: User defined data types in Logical Replication
Previous Message Vitaliy Garnashevich 2017-12-07 00:17:13 Re: Bitmap scan is undercosted?