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

Hi,

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? Does your change still make
sense to you and do you see problem with the current state (as of ecac0e2b)?

> If I understand correctly, you essentially reverted 2a8e1ac5 (which
> implies that you think it didn't fix a real problem) and then fixed a
> real problem that was missed by 2a8e1ac5. Is that correct?

Well, I wouldn't say 2a8e1ac5 'missed' that problem. It tried to solve
something else (which it imo didn't). But yes, it didn't really improve
the state with regard to doing dangerous things in a critical section.

> > Backpatch to 9.3 where the problem was introduced. The bug only came
> > to light due to the assertion added in 4a170ee9 and isn't likely to
> > cause problems in production scenarios. The worst outcome is a
> > avoidable PANIC restart.
> >
> > This also gets rid of the difference in the order of operations
> > between master and standby mentioned in 2a8e1ac5.
>
> Can you explain? It looks like the master will still see the bit right
> away after the HEAP2_CLEAN record, but there is nothing in the
> HEAP2_CLEAN record to tell the standby to set all-visible. The standby
> has to wait until the HEAP2_VISIBLE record comes along.

Nobody can see the page in that state on the master - it'll be locked
exclusively. But that's not really the point.

What could happen with 2a8e1ac5's coding is that log_heap_clean()'s
XLogInsert() took a full page image which then contained the all visible
bit because the PageSetAllVisible() was done *before* it. The
standby would have replayed the HEAP2_CLEAN including the FPI and then
unlocked the page. At that point the heap page's all visible will be set,
but the vm bit wouldn't.
With the new coding (ecac0e2b9) the HEAP2_CLEAN cannot log the all
visible bit because it's not set yet. Only the HEAP2_VISIBLE will set
it on both the heap and the vm. Makes sense?

> 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.

> With checksums, Simon and I tried to create some new abstractions, like
> MarkBufferDirtyHint, and document them in transam/README. I hope that
> gives us some greater confidence in the checksums code[1], because we
> can see a lot of the tricky aspects in one place, along with the rules
> that callers should follow.

Hm. I don't think the current callsites dealing with this are easily
amenable to something like that.

> A quick look at the comment above the function tells us that
> MarkBufferDirtyHint won't work for the all-visible bit, but perhaps an
> adapted version could work. If so, we could separate the all-visible bit
> from the HEAP2_CLEAN action, and it would look a lot more like setting a
> tuple hint.

Huh? HEAP2_CLEAN doesn't set all visible? The only reason we're now
doing a HEAP2_VISIBLE there is that removing the tuples in the _CLEAN
step increases the chance of it the page now being all visible? It's
separate steps.

> Also, we might be able to simplify visibilitymap_set if the
> logging of the heap page for checksums could be done beforehand with
> this variant of MarkBufferDirtyHint.

Hm. I have a hard time seing how that'd simplify things.

Greetings,

Andres Freund

--
 Andres Freund                       http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services