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