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.
This patch moves the block of actions back after the WAL insert, but
also puts them outside of the critical section.
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?
> 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.
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?
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.
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. 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.
Regards,
Jeff Davis
[1] Not that we didn't make mistakes; at least one of which you
heroically diagnosed just in time.
