On Thu, Jan 24, 2013 at 9:31 PM, Jeff Janes <jeff.ja...@gmail.com> wrote: > On Thu, Jan 24, 2013 at 1:28 AM, Pavan Deolasee > <pavan.deola...@gmail.com> wrote:
>> >> Good idea. Even though the cost of pinning/unpinning may not be high >> with respect to the vacuum cost itself, but it seems to be a good idea >> because we already do that at other places. Do you have any other >> review comments on the patch or I'll fix this and send an updated >> patch soon. > > That was the only thing that stood out to me. The attached patch gets that improvement. Also rebased on the latest head. > You had some doubts > about visibility_cutoff_xid, but I don't know enough about that to > address them. I agree that it would be nice to avoid the code > duplication, but I don't see a reasonable way to do that. > I have left a comment there to ensure that someone changing the code also takes pain to look at the other part. > It applies cleanly (some offsets), builds without warnings, passes > make check under cassert. No documentation or regression tests are > needed. We want this, and it does it. > > I have verified the primary objective (setting vm sooner) but haven't > experimentally verified that this actually increases throughput for > some workload. For pgbench when all data fits in shared_buffers, at > least it doesn't cause a readily noticeable slow down. > > I haven't devised any patch-specific test cases, either for > correctness or performance. I just used make check, pgbench, and the > "vacuum verbose" verification you told us about in the original > submission. Thanks for the tests and all the review. > > If we are going to scan a block twice, I wonder if it should be done > the other way around. If there are any dead tuples that need to be > removed from indexes, there is no point in dirtying the page with a > HOT prune on the first pass when it will just be dirtied again after > the indexes are vacuumed. I don't see this idea holding up your patch > though, as surely it would be more invasive than what you are doing. > I think there is a merit in this idea. But as you rightly said, we should tackle that as a separate patch. Thanks, Pavan -- Pavan Deolasee http://www.linkedin.com/in/pavandeolasee
vacuum-secondphase-setvm-v3.patch
Description: Binary data
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
