Re: heap vacuum & cleanup locks

On Fri, Nov 4, 2011 at 3:12 PM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> On Fri, Nov 4, 2011 at 6:18 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
>> 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 think we need to count skipped pages also. I don't like the idea
> that vacuum would just report less pages than there are in the table.
> We'll just get requests to explain that.

It's been skipping pages due to the visibility map since 8.4. This
seems like small potatoes by comparison, but we could add some
counters if you like.

>> 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().
>
> OK
>
>> 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.
>
> Agreed, separate patch.

Doing that actually makes the patch simpler, so I went ahead and did
that in the attached version, along with the renaming mentioned above.

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