On Wed, Dec 21, 2016 at 7:25 AM, Heikki Linnakangas <hlinn...@iki.fi> wrote: > For now, barring objections, I'm going to commit the first patch. It seems > like a worthwhile simplification in any case, especially for Peter's > Parallel tuplesort patch set.
Well, it removes more code than it adds. That's definitely something. And saving memory per-empty-tape is good, too. A few random comments: "seeked" is kind of a lame variable name. How about "seekpos" or "newpos" or something like that? /* * Even in minimum memory, use at least a MINORDER merge. On the other * hand, even when we have lots of memory, do not use more than a MAXORDER - * merge. Tapes are pretty cheap, but they're not entirely free. Each - * additional tape reduces the amount of memory available to build runs, - * which in turn can cause the same sort to need more runs, which makes - * merging slower even if it can still be done in a single pass. Also, - * high order merges are quite slow due to CPU cache effects; it can be - * faster to pay the I/O cost of a polyphase merge than to perform a single - * merge pass across many hundreds of tapes. + * merge. Tapes are pretty cheap, but they're not entirely free. High order + * merges are quite slow due to CPU cache effects; it can be faster to pay + * the I/O cost of a polyphase merge than to perform a single merge pass + * across many hundreds of tapes. */ I think you could leave this comment as-is. You haven't zeroed out the overhead of a tape, and I like those additional bits I crammed in there. :-) -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers