Re: pgsql: Do all-visible handling in lazy_vacuum_page() outside its critic

On 06/24/2014 01:27 AM, Andres Freund wrote:
> On 2014-06-23 13:00:11 -0700, Jeff Davis wrote:
>> On Fri, 2014-06-20 at 09:10 +0000, Andres Freund wrote:
>>> To fix, simply move all the all-visible handling outside the critical
>>> section. Doing so means that the PD_ALL_VISIBLE on the page won't be
>>> included in the full page image of the HEAP2_CLEAN record anymore. But
>>> that's fine, the flag will be set by the HEAP2_VISIBLE logged later.
>>
>> Trying to follow along. I read the discussion about bug #10533.
>>
>> Before 2a8e1ac5, a block of actions -- visibility test, VM test, set VM
>> bit, set all-visible -- were inside the critical section and after the
>> WAL insert.
>>
>> 2a8e1ac5 left the block of actions in the critical section but moved
>> them before the WAL insert. The commit message seems to indicate that
>> there's a real problem setting the all-visible flag after the WAL
>> insert, but I couldn't identify a serious problem there.
>
> Yes, that was confusing. Imo it made the state worse, not better. I'd
> asked for clarification somewhere... Heikki?

Hmm. I don't see any live bug caused before 2a8e1ac5 anymore either. I
think I missed the fact that replaying the XLOG_HEAP2_VISIBLE record
always sets the bit in the heap page.

> Does your change still make
> sense to you and do you see problem with the current state (as of ecac0e2b)?

Hmm, in the current state, it's again possible that the full-page image
doesn't contain the all-visible flag, even though the page in the buffer
does. I guess that's OK, because replaying XLOG_HEAP2_VISIBLE always
sets the flag. My page-comparison tool will complain, so it would be
nice to not do that, but it's a false alarm.

Or we could call heap_page_is_all_visible() before entering the critical
section, if we passed heap_page_is_all_visible() the list of dead tuples
we're removing so that it could ignore them.

>> I'm a little concerned that we're approaching the WAL rules in an ad-hoc
>> manner. I don't see an actual problem right now, but we've been fixing
>> problems with PD_ALL_VISIBLE since the VM was made crash safe almost
>> exactly three years ago (503c7305). Do we finally feel confident in the
>> design and its implications?
>
> I'm not particularly happy about all this either, but this section of
> code is actually much newer. It was added in fdf9e21196a6/9.3.

I'm not happy with the way we deviate from the normal WAL rules with in
the visibility map either. In the long run, we may be better off to just
take the hit and WAL-log the VM bits like any other update. Basically,
always behave as if checksums were enabled. But for now we just have to
fix bugs as they're found.

- Heikki