|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|
|Views:||Raw Message | Whole Thread | Download mbox | Resend email|
> 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
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.
>> + #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.
>> + /* 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
range patch is unchanged, but still attached to keep them altogether.
Teodor Sigaev E-mail: teodor(at)sigaev(dot)ru
|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.|