Re: memory leak in e94568ecc10 (pre-reading in external sort)

On Mon, Oct 10, 2016 at 5:21 AM, Heikki Linnakangas <hlinnaka(at)iki(dot)fi> wrote:
> Admittedly that's confusing. Thinking about this some more, I came up with
> the attached. I removed the separate LogicalTapeAssignReadBufferSize() call
> altogether - the read buffer size is now passed as argument to the
> LogicalTapeRewindForRead() call.
>
> I split LogicalTapeRewind() into two: LogicalTapeRewindForRead() and
> LogicalTapeRewindForWrite(). A boolean argument like 'forWrite' is mentally
> challenging, because when reading a call site, you have to remember which
> way round the boolean is. And now you also pass the extra buffer_size
> argument when rewinding for reading, but not when writing.

I like the general idea here.

> I gave up on the logic to calculate the quotient and reminder of the
> available memory, and assigning one extra buffer to some of the tapes. I'm
> now just rounding it down to the nearest BLCKSZ boundary. That simplifies
> the code further, although we're now not using all the memory we could. I'm
> pretty sure that's OK, although I haven't done any performance testing of
> this.

The only thing I notice is that this new struct field could use a comment:

> --- a/src/backend/utils/sort/tuplesort.c
> +++ b/src/backend/utils/sort/tuplesort.c
> @@ -366,6 +366,8 @@ struct Tuplesortstate
> char *slabMemoryEnd; /* end of slab memory arena */
> SlabSlot *slabFreeHead; /* head of free list */
>
> + size_t read_buffer_size;
> +

Also, something about trace_sort here:

> +#ifdef TRACE_SORT
> + if (trace_sort)
> + elog(LOG, "using " INT64_FORMAT " KB of memory for read buffers among %d input tapes",
> + (state->availMem) / 1024, numInputTapes);
> +#endif
> +
> + state->read_buffer_size = state->availMem / numInputTapes;
> + USEMEM(state, state->availMem);

Maybe you should just make the trace_sort output talk about blocks at
this point?

--
Peter Geoghegan