On Mon, Mar 21, 2016 at 2:01 AM, Peter Geoghegan <p...@heroku.com> wrote: > On Thu, Mar 17, 2016 at 1:13 PM, Robert Haas <robertmh...@gmail.com> wrote: >> OK, I have now committed 0001 > > I attach a revision of the external quicksort patch and supplementary > small patches, rebased on top of the master branch.
I spent some time today reading through the new 0001 and in general I think it looks pretty good. But I think that there is some stuff in there that logically seems to me to deserve to be separate patches. In particular: 1. Changing cost_sort to consider disk access as 90% sequential, 10% random rather than 75% sequential, 25% random. As far as I can recall from the thread, zero test results have been posted to demonstrate that this is a good idea. It also seems completely unprincipled. If the cost of sorts decreases as a result of this patch, it is because we've reduced the CPU cost, not the I/O cost. The changes we're talking about here make I/O more random, not less random, because we will now have more tapes, not fewer; which means merges will have to seek the disk head more frequently, not less frequently. Now, it's tempting to say that this patch should result in some change to the cost model: if the patch doesn't make sorting faster, we shouldn't commit it at all, and if it does, then surely the cost model should change accordingly. But the question for the cost model isn't whether the change to the model somehow reflects the increase in execution speed. It's whether we get better query plans with the change than without. I don't think there's been a degree of review of that aspect of this patch on list that would give me confidence to commit a change like this. 2. Restricting the maximum number of tapes to 500. This seems like a sound change and I don't object to it in theory. But I've seen no benchmark results which demonstrate that this is a good idea, and it is quite separate from the core purpose of the patch. Since time is short, I recommend we remove both of these things from the patch and you can resubmit them as separate patches later. As far as I can see, neither of them is so tied into the rest of the patch that the main part of the patch can't be committed without those changes. -- 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