Re: [HACKERS] Fix for seg picksplit function

Yeb Havinga writes: > I think it is time to mark this patch ready for committer: > The unintuitive result thus far is that sorting outperforms the R-tree > bounding boxes style index, as Alexander has demonstrated with several > different distributions on 20-11 (uniform, natural (is that a bell

Re: [HACKERS] Fix for seg picksplit function

On 2010-11-16 09:57, Alexander Korotkov wrote: On Tue, Nov 16, 2010 at 3:07 AM, Robert Haas > wrote: The loop that begins here: for (i = 0; i < maxoff; i++) { /* First half of segs goes to the left datum. */ if (i < seed_2)

Re: [HACKERS] Fix for seg picksplit function

On 2010-11-20 21:57, Yeb Havinga wrote:8K blocksize: postgres=# create index seg_test_idx on seg_test using gist (a); CREATE INDEX Time: 99613.308 ms SELECT Total runtime: 81.482 ms 1K blocksize: CREATE INDEX Time: 40113.252 ms SELECT Total runtime: 3.363 ms Details of explain analyze are below

Re: [HACKERS] Fix for seg picksplit function

On 2010-11-20 13:36, Yeb Havinga wrote: On 2010-11-20 04:46, Robert Haas wrote: Well, the problem with just comparing on< is that it takes very little account of the upper bounds. I think the cases where a simple split would hurt you the most are those where examining the upper bound is necess

Re: [HACKERS] Fix for seg picksplit function

On Sat, Nov 20, 2010 at 6:46 AM, Robert Haas wrote: > Well, the problem with just comparing on < is that it takes very > little account of the upper bounds. I think the cases where a simple > split would hurt you the most are those where examining the upper > bound is necessary to to get a good

Re: [HACKERS] Fix for seg picksplit function

On 2010-11-20 04:46, Robert Haas wrote: On Tue, Nov 16, 2010 at 6:07 AM, Alexander Korotkov wrote: On Tue, Nov 16, 2010 at 3:07 AM, Robert Haas wrote: But on a broader note, I'm not very certain the sorting algorithm is sensible. For example, suppose you have 10 segments that are exactly '0

Re: [HACKERS] Fix for seg picksplit function

On Tue, Nov 16, 2010 at 6:07 AM, Alexander Korotkov wrote: > On Tue, Nov 16, 2010 at 3:07 AM, Robert Haas wrote: >> But on a broader note, I'm not very certain the sorting algorithm is >> sensible.  For example, suppose you have 10 segments that are exactly >> '0' and 20 segments that are exactly

Re: [HACKERS] Fix for seg picksplit function

On Tue, Nov 16, 2010 at 3:07 AM, Robert Haas wrote: > But on a broader note, I'm not very certain the sorting algorithm is > sensible. For example, suppose you have 10 segments that are exactly > '0' and 20 segments that are exactly '1'. Maybe I'm misunderstanding, > but it seems like this will

Re: [HACKERS] Fix for seg picksplit function

On Tue, Nov 16, 2010 at 3:07 AM, Robert Haas wrote: > The loop that begins here: > >for (i = 0; i < maxoff; i++) >{ >/* First half of segs goes to the left datum. */ >if (i < seed_2) > > ...looks like it should perhaps be broken into two separate loops. > That might also h

Re: [HACKERS] Fix for seg picksplit function

On Mon, Nov 15, 2010 at 4:06 AM, Alexander Korotkov wrote: > With help of Oleg I found, that line "*left = *right = FirstOffsetNumber;" > was needed only for 7.X compatibility, and it isn't needed any more. > Also, I've replaced "i - 1" by "i - FirstOffsetNumber" in array filling. > I believe it's

Re: [HACKERS] Fix for seg picksplit function

With help of Oleg I found, that line "*left = *right = FirstOffsetNumber;" was needed only for 7.X compatibility, and it isn't needed any more. Also, I've replaced "i - 1" by "i - FirstOffsetNumber" in array filling. I believe it's more correct way, because it'll work correctly in the case when Fir

Re: [HACKERS] Fix for seg picksplit function

On 2010-11-10 14:53, Alexander Korotkov wrote: Actually I can't understand the purpose of FirstOffsetNumber and OffsetNumberNext macros. When I wrote the patch I though about sortItems as about "clean from all these strange things" array, that's why I didn't use OffsetNumberNext there. :) I

Re: [HACKERS] Fix for seg picksplit function

Alexander Korotkov writes: > On Wed, Nov 10, 2010 at 4:53 PM, Alexander Korotkov > wrote: >> Actually I can't understand the purpose of FirstOffsetNumber >> and OffsetNumberNext macros. > For example, if we assume, that OffsetNumberNext can do something other that > just increment, that we shoul

Re: [HACKERS] Fix for seg picksplit function

On Wed, Nov 10, 2010 at 6:05 PM, Yeb Havinga wrote: > On 2010-11-10 15:46, Alexander Korotkov wrote: > > On Wed, Nov 10, 2010 at 5:37 PM, Yeb Havinga wrote: > >> They are necessary and it is code untouched by this patch, and the same >> line occurs in other picksplit functions as well. The gbt_

Re: [HACKERS] Fix for seg picksplit function

On 2010-11-10 15:46, Alexander Korotkov wrote: On Wed, Nov 10, 2010 at 5:37 PM, Yeb Havinga > wrote: They are necessary and it is code untouched by this patch, and the same line occurs in other picksplit functions as well. The gbt_num_picksplit function s

Re: [HACKERS] Fix for seg picksplit function

On Wed, Nov 10, 2010 at 5:37 PM, Yeb Havinga wrote: > They are necessary and it is code untouched by this patch, and the same > line occurs in other picksplit functions as well. The gbt_num_picksplit > function shows that it can be avoided, by rewriting in the second loop > > *left++ = sortItems[

Re: [HACKERS] Fix for seg picksplit function

On 2010-11-10 14:27, Alvaro Herrera wrote: Hmm, the second for loop in gseg_picksplit uses "i< maxoff" whereas the other one uses<=. The first is probably correct; if the second is also correct it merits a comment on the discrepancy (To be honest, I'd get rid of the "-1" in computing maxoff and

Re: [HACKERS] Fix for seg picksplit function

On Wed, Nov 10, 2010 at 4:53 PM, Alexander Korotkov wrote: > Hmm, the second for loop in gseg_picksplit uses "i < maxoff" whereas the >> other one uses <=. The first is probably correct; if the second is also >> correct it merits a comment on the discrepancy (To be honest, I'd get >> rid of the "

Re: [HACKERS] Fix for seg picksplit function

> > Hmm, the second for loop in gseg_picksplit uses "i < maxoff" whereas the > other one uses <=. The first is probably correct; if the second is also > correct it merits a comment on the discrepancy (To be honest, I'd get > rid of the "-1" in computing maxoff and use < in both places, given that

Re: [HACKERS] Fix for seg picksplit function

Hmm, the second for loop in gseg_picksplit uses "i < maxoff" whereas the other one uses <=. The first is probably correct; if the second is also correct it merits a comment on the discrepancy (To be honest, I'd get rid of the "-1" in computing maxoff and use < in both places, given that offsets a

Re: [HACKERS] Fix for seg picksplit function

Alexander Korotkov wrote: Do you think now patch is ready for committer or it require further review by you or somebody else? It's probably ready for committer, however the code now doesn't mention any reference or bit of information that it is faster than the original one. I was wondering how

Re: [HACKERS] Fix for seg picksplit function

Do you think now patch is ready for committer or it require further review by you or somebody else? With best regards, Alexander Korotkov.

Re: [HACKERS] Fix for seg picksplit function

Hello Yeb, Thank you for review and code refactoring. PS: when comparing with gbt_num_picksplit, I noticed that that one does not > update v->spl_ldatum and spl_rdatum to the union datums, but initializes > these to 0 at the beginning and never seems to update them. Not sure if this > is a proble

Re: [HACKERS] Fix for seg picksplit function

Hello Alexander, Here follows a review of your patch. Hackers, Seg contrib module contains the same bug in picksplit function as cube contrib module. Good catch! :-) Also, Guttman's split algorithm is not needed in unidimensional case, because sorting based algorithm is good in this case.

[HACKERS] Fix for seg picksplit function

Hackers, Seg contrib module contains the same bug in picksplit function as cube contrib module. Also, Guttman's split algorithm is not needed in unidimensional case, because sorting based algorithm is good in this case. I propose the patch which replace current picksplit implementation with sortin