On 10/06/2016 06:44 PM, Peter Geoghegan wrote:
> While the fix you pushed was probably a good idea anyway, I still
> think you should not use swhichtate->maxTapes to exhaustively call
> LogicalTapeAssignReadBufferSize() on every tape, even non-active
> tapes. That's the confusing part.
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 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.
- Heikki