On Mon, Oct 3, 2016 at 3:38 AM, Heikki Linnakangas
<heikki.linnakangas@iki.fi> wrote:
> Change the way pre-reading in external sort's merge phase works.
I noticed a simple oversight in this patch. It looks like you missed
one place where state->maxTapes ought to be replaced with
numInputTapes -- the loop that calls LogicalTapeAssignReadBufferSize()
needs that changed too, in order to continue to respect workMem as a
budget. There is a lesson for me, here: Start running tests of sorting
patches only after setting "sysctl -w vm.overcommit_memory=2", because
over commit can obscure things. Doing that as standard procedure for
testing would have allowed me to catch this immediately.
Maybe you should do the same with the other loop that iterates based
on a state->maxTapes invariant that was added to the end of
mergeruns() by this commit (use numInputTapes there in place of
state->maxTapes, too). And, I wonder if it's worth it to not actually
rewind in that loop at all -- perhaps you could instead call a new
logtape.c function that just frees preload buffer memory. You'd also
call this new simple "free preload buffer" function in place of the
LogicalTapeRewind() call within tuplesort_gettuple_common(), of
course.
I've found that I have to do this within the rebased parallel CREATE
INDEX patch anyway, since LogicalTapeRewind() has various irrelevant
pre and post conditions that don't interact well with tape unification
(so you get assertion failures that are probably harmless, but not
really fixable within LogicalTapeRewind() itself). Might be best to
get ahead of that now; my problem with using LogicalTapeRewind()
suggests to me that not using LogicalTapeRewind() to simply free
preload memory could be good *general* future-proofing.
Thanks
--
Peter Geoghegan
