Re: [PATCH] we have added support for box type in SP-GiST index

From: Teodor Sigaev <teodor(at)sigaev(dot)ru>
To: emre(at)hasegeli(dot)com, Oleg Bartunov <obartunov(at)gmail(dot)com>
Cc: David Steele <david(at)pgmasters(dot)net>, Pgsql Hackers <pgsql-hackers(at)postgresql(dot)org>, Alexander Lebedev <a(dot)lebedev(at)postgrespro(dot)ru>
Subject: Re: [PATCH] we have added support for box type in SP-GiST index
Date: 2016-03-18 17:24:35
Message-ID: 56EC39D3.30708@sigaev.ru
Views: Raw Message | Whole Thread | Download mbox
Thread:
Lists: pgsql-hackers

> Do reconstructedValues is required now? Wouldn't it be cleaner to use
> the new field on the prefix tree implementation, too?
reconstructedValues is needed to reconctruct full indexed value and should match
to type info indexed column

>
> We haven't had specific memory context for reconstructedValues. Why
> is it required for the new field?
because pg knows type of reconstructedValues (see above why) but traversalValue
just a void*, SP-GiST core doesn't knnow how to free memory of allocated for it.

>
>> + Memory for <structfield>traversalValues</> should be allocated in
>> + the default context, but make sure to switch to
>> + <structfield>traversalMemoryContext</> before allocating memory
>> + for pointers themselves.
>
> This sentence is not understandable. Shouldn't it say "the memory
> should *not* be allocated in the default context"? What are the
> "pointers themselves"?
Clarified, I hope

>
> The box opclass is broken because of the floating point arithmetics of
> the box type. You can reproduce it with the attached script. We
hope, fixed. At least, seems, syncronized with seqscan

> really need to fix the geometric types, before building more on them.
> They fail to serve the only purpose they are useful for, demonstrating
> features.
agree, but it's not a deal for this patch

>
> It think the opclass should support the cross data type operators like
> box @> point. Ideally we should do it by using multiple opclasses in
> an opfamily. The floating problem will bite us again in here, because
> some of the operators are not using FP macros.
Again, agree. But I suggest to do it by separate patch.

>
> The tests check not supported operator "@". It should be "<@". It is
> nice that the opclass doesn't support long deprecated operators.
fixed

>> + #define LT -1
>> + #define GT 1
>> + #define EQ 0
>
> Using these numbers is a very well established pattern. I don't think
> macros make the code any more readable.
fixed

>
>> + /* SP-GiST API functions */
>> + Datum spg_box_quad_config(PG_FUNCTION_ARGS);
>> + Datum spg_box_quad_choose(PG_FUNCTION_ARGS);
>> + Datum spg_box_quad_picksplit(PG_FUNCTION_ARGS);
>> + Datum spg_box_quad_inner_consistent(PG_FUNCTION_ARGS);
>> + Datum spg_box_quad_leaf_consistent(PG_FUNCTION_ARGS);
>
> I guess they should go to the header file.
Remove them because they are presented in auto-generated file
./src/include/utils/builtins.h

range patch is unchanged, but still attached to keep them altogether.

--
Teodor Sigaev E-mail: teodor(at)sigaev(dot)ru
WWW: http://www.sigaev.ru/

Attachment Content-Type Size
q4d-2.patch.gz application/x-gzip 7.1 KB
traversalValue-2.patch.gz application/x-gzip 1.9 KB
range-1.patch.gz application/x-gzip 1.1 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Dmitry Ivanov 2016-03-18 17:40:16 Re: [WIP] speeding up GIN build with parallel workers
Previous Message Anastasia Lubennikova 2016-03-18 17:19:37 Re: [WIP] Effective storage of duplicates in B-tree index.