Re: compress method for spgist - 2
| От | Alexander Korotkov |
|---|---|
| Тема | Re: compress method for spgist - 2 |
| Дата | |
| Msg-id | CAPpHfds3Wuco3iyNmQaP724iqiaRNSEW4qR17KOFqr6mqzLGYQ@mail.gmail.com обсуждение исходный текст |
| Ответ на | Re: compress method for spgist - 2 (Nikita Glukhov <n.gluhov@postgrespro.ru>) |
| Ответы |
Re: compress method for spgist - 2
(Teodor Sigaev <teodor@sigaev.ru>)
|
| Список | pgsql-hackers |
On Thu, Dec 7, 2017 at 3:17 AM, Nikita Glukhov <n.gluhov@postgrespro.ru> wrote:
------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
On 06.12.2017 21:49, Alexander Korotkov wrote:
But if we have compress method and do not have decompress method then index-only scan seems to be impossible.On Wed, Dec 6, 2017 at 6:08 PM, Nikita Glukhov <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> 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 CommitterI 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.
Sorry, I didn't realize that we don't return reconstructed value immediately, but return only leafValue provided by leafConsistent. In this case, leafConsistent is responsible to convert value from spgConfigOut.leafType to spgConfigIn.attType.
TBH, practical example of SP-GiST opclass with both compress method and index-only scan support doesn't come to my mind, because compress method is typically needed when we have lossy representation of index keys. For example, in GiST all the opclasses where compress method do useful work use lossy representation of keys. Nevertheless, it's good to not cut possibility of index-only scans when spgConfigOut.leafType != spgConfigIn.attType. And to lay responsibility for conversion on leafConsistent seems like elegant way to do this. So, that's OK for me.
I've added compress method existence check to spgvalidate().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 decided to simply set compress method's bit in functionset when this method it is not required.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.
Looks good for me.
Now, this patch is ready for committer from my point of view.
------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
В списке pgsql-hackers по дате отправления: