Re: heap vacuum & cleanup locks

On Thu, Nov 3, 2011 at 12:57 PM, Simon Riggs <simon@2ndquadrant.com> wrote:
> On Thu, Nov 3, 2011 at 2:22 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>> On Thu, Nov 3, 2011 at 9:52 AM, Simon Riggs <simon@2ndquadrant.com> wrote:
>>> On Thu, Nov 3, 2011 at 1:26 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>>>
>>>> I think that should be OK, but:
>>>>
>>>> - It looks to me like you haven't done anything about the second heap
>>>> pass.  That should probably get a similar fix.
>>>
>>> I was assuming this worked with Pavan's patch to remove second pass.
>>
>> It's not entirely certain that will make it into 9.2, so I would
>> rather get this done first.  If you want I can pick up what you've
>> done and send you back a version that addresses this.
>
> OK, that seems efficient. Thanks.

Here's a new version.  I fixed the second pass as discussed (which
turned out to be trivial).  To address the concern about relpages, I
moved this pre-existing line to after we get the buffer lock:

+               vacrelstats->scanned_pages++;

That appears to do the right thing.

I found it kind of confusing that lazy_scan_page_for_wraparound()
returns with the pin either held or not held depending on the return
value, so I rearranged things very slightly so that it doesn't need to
do that.  I'm wondering whether we should rename that function to
something like lazy_check_needs_freeze().

I tested this out and discovered that "VACUUM FREEZE" doesn't set the
for_wraparound flag.  On further review, I think that we should
probably conditionalize the behavior on the scan_all flag that
lazy_vacuum_rel sets, rather than for_wraparound.  Otherwise, there's
no way for the user to manually force relfrozenxid to advance, which
doesn't seem good.  I haven't made that change in this version,
though.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company