Re: 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 use<  in both places, given that
> offsets are 1-indexed).
Good point. The second loop walks over the sorted array, which is 
0-indexed. Do you favor making the sort array 1-indexed, like done in 
e.g. gbt_num_picksplit? The downside is that the sort array is 
initialized with length maxoff + 1: puzzling on its own, even more since 
maxoff itself is initialized as entryvec->n -1. Note also that the qsort 
call for the 1-indexed variant is more complex since it must skip the 
first element.
> probably should be OffsetNumberNext just to stay consistent with the
> rest of the code.
Yes.
> The assignment to *left and *right at the end of the routine seem pretty
> useless (not to mention the comment talking about a routine that doesn't
> exist anywhere).
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[i].index;
into
v->spl_left[v->spl_nleft] = sortItems[i].index

Even though this is longer code, I prefer this variant over the shorter one.

regards,
Yeb Havinga