On Thu, Jan 24, 2013 at 9:31 PM, Jeff Janes <jeff.janes@gmail.com> wrote:
> On Thu, Jan 24, 2013 at 1:28 AM, Pavan Deolasee
> <pavan.deolasee@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