Re: [PATCH] Assert that the correct locks are held when calling PageGetLSN()

On Tue, Oct 3, 2017 at 12:54 AM, Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:
> Michael Paquier wrote:
>> On Mon, Oct 2, 2017 at 11:44 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> > On Mon, Oct 2, 2017 at 10:19 AM, Michael Paquier
>> > <michael(dot)paquier(at)gmail(dot)com> wrote:
>> >> So those bits could be considered for integration.
>> >
>> > Yes, and they also tend to suggest that the rest of the patch has value.
>>
>> Well, there are cases where you don't need any locking checks, and the
>> proposed patch ignores that. Take for example pageinspect which works
>> on a copy of a page, or just WAL replay which serializes everything,
>> and in both cases PageGetLSN can be used safely. So for compatibility
>> reasons I think that PageGetLSN should be kept alone to not surprise
>> anybody using it, or at least an equivalent should be introduced. It
>> would be interesting to make BufferGetLSNAtomic hold tighter checks,
>> like something that makes use of LWLockHeldByMe for example.

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

> I'd argue about this in the same direction I argued about
> BufferGetPage() needing an LSN check that's applied separately: if it's
> too easy for a developer to do the wrong thing (i.e. fail to apply the
> separate LSN check after reading the page) then the routine should be
> patched to do the check itself, and another routine should be offered
> for the cases that explicitly can do without the check.
>
> I was eventually outvoted in the BufferGetPage() saga, though, so I'm
> not sure that that discussion sets precedent.

Oh... I don't recall this discussion. A quick lookup at the archives
does not show me a specific thread either.
--
Michael