Re: About "Our CLUSTER implementation is pessimal" patch

Leonardo F wrote:
>> Yeah, I think you could do that, I agree it feels better that way.
>> You'll still need new copytup and comparetup functions, though, to deal
>> with HeapTupleHeaders instead of MinimalTuples, or modify the existing
>> ones to handle both.
>
> You meant HeapTuple, not HeapTupleHeaders, right?

No, I did mean HeapTupleHeader. MinimalTuple struct is cut-down version
HeapTupleHeader, while HeapTuple is structure that holds a pointer to
HeapTupleHeader + some extra information. SortTuple takes the role of
HeapTUple in tuplesort.c. A bit confusing, yes.

That said, I didn't really look closely, maybe I'm missing something and
HeapTuple is in fact the right struct to pass around.

>> And some way to indicate that you want to preserve
>> the visibility information when you create the tuplesort, maybe a new
>> parameter to tuplesort_begin_heap().
>
> I guess that using Gregory Stark's patch there's no need for it, since it uses
> HeapTuples, right?

Hmm, you still need to set different comparison function in
Tuplesortstate->comparetup, so you'll still need a separate begin()
function too, or a flag to the existing one.

> A patch that:
>
> 1) uses always the old CLUSTER method for non-btree indexes and for
> expression indexes
> 2) add a whole set of new functions to tuplesort (as in Gregory Stark's patch)
>
> would be rejected "for sure"? Or can be thought as a "better than nothing,
> works in 90% cases" patch?

I'm fine with 1), though I wish we didn't have to add all that
boilerplate code 2). I guess it's not a show-stopper.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com