Hi MichaelOn Mon, Oct 2, 2017 at 6:48 PM, Michael Paquier <michael.paquier@gmail.com> wrote:
>
> Jacob, here are some ideas to make this thread move on. I would
> suggest to produce a set of patches that do things incrementally:
> 1) One patch that changes the calls of PageGetLSN to
> BufferGetLSNAtomic which are now not appropriate. You have spotted
> some of them in the btree and gist code, but not all based on my first
> lookup. There is still one in gistFindCorrectParent and one in btree
> _bt_split. The monitoring of the other calls (sequence.c and
> vacuumlazy.c) looked safe. There is another one in XLogRecordAssemble
> that should be fixed I think.
Thank you for your suggestions. Please find the first patch attached as "0001-...". We verified both, gistFindCorrectParent and _bt_split and all calls to PageGetLSN are made with exclusive lock on the buffer contents held.
> 2) A second patch that strengthens a bit checks around
> BufferGetLSNAtomic. One idea would be to use LWLockHeldByMe, as you
> are originally suggesting.
> A comment could be as well added in bufpage.h for PageGetLSN to let
> users know that it should be used carefully, in the vein of what is
> mentioned in src/backend/access/transam/README.
The second patch "0002-..." does the above. We have a comment added to AssertPageIsLockedForLSN as suggested.
The assertion added caught at least one code path where TestForOldSnapshot calls PageGetLSN without holding any lock. The snapshot_too_old test in "check-world" failed due to the assertion failure. This needs to be fixed, see the open question in the opening mail on this thread.
Asim and Jacob
