On Mon, Oct 3, 2016 at 3:38 AM, Heikki Linnakangas <heikki.linnakan...@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 -- Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-committers