Re: Tuplesort merge pre-reading

On Mon, Oct 3, 2016 at 3:39 AM, Heikki Linnakangas <hlinnaka(at)iki(dot)fi> wrote:
>> Can't you just use state->tapeRange, and remove the "continue"? I
>> recommend referring to "now-exhausted input tapes" here, too.
>
>
> Don't think so. result_tape == tapeRange only when the merge was done in a
> single pass (or you're otherwise lucky).

Ah, yes. Logical tape assignment/physical tape number confusion on my part here.

>> * I'm not completely prepared to give up on using
>> MemoryContextAllocHuge() within logtape.c just yet. Maybe you're right
>> that it couldn't possibly matter that we impose a MaxAllocSize cap
>> within logtape.c (per tape), but I have slight reservations that I
>> need to address. Maybe a better way of putting it would be that I have
>> some reservations about possible regressions at the very high end,
>> with very large workMem. Any thoughts on that?
>
>
> Meh, I can't imagine that using more than 1 GB for a read-ahead buffer could
> make any difference in practice. If you have a very large work_mem, you'll
> surely get away with a single merge pass, and fragmentation won't become an
> issue. And 1GB should be more than enough to trigger OS read-ahead.

I had a non-specific concern, not an intuition of suspicion about
this. I think that I'll figure it out when I rebase the parallel
CREATE INDEX patch on top of this and test that.

> Committed with some final kibitzing. Thanks for the review!

Thanks for working on this!

> PS. This patch didn't fix bug #14344, the premature reuse of memory with
> tuplesort_gettupleslot. We'll still need to come up with 1. a backportable
> fix for that, and 2. perhaps a different fix for master.

Agreed. It seemed like you favor not changing memory ownership
semantics for 9.6. I'm not sure that that's the easiest approach for
9.6, but let's discuss that over on the dedicated thread soon.

--
Peter Geoghegan