On 12/21/2016 03:22 PM, Robert Haas wrote:
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?

Ok.

     /*
      * 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.  :-)

Yes, quite right. That was a mishap in rebasing, that change to remove the comment really belongs to the pause/resume patch rather than the 1st one.

Thanks for the review!

- Heikki



--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Reply via email to