On Mon, Oct 10, 2016 at 5:21 AM, Heikki Linnakangas <hlinn...@iki.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
