Re: [HACKERS] [PATCH] Assert that the correct locks are held whencalling PageGetLSN()

On Tue, Sep 5, 2017 at 10:49 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Wed, Sep 6, 2017 at 1:15 AM, Jacob Champion <pchampion@pivotal.io> wrote:
>> On Sun, Sep 3, 2017 at 11:14 PM, Michael Paquier
>> <michael.paquier@gmail.com> wrote:
>>> +#if defined(USE_ASSERT_CHECKING) && !defined(FRONTEND)
>>> +void
>>> +AssertPageIsLockedForLSN(Page page)
>>> +{
>>> From a design point of view, bufmgr.c is an API interface for buffer
>>> management on the backend-size, so just using FRONTEND there looks
>>> wrong to me (bufmgr.h is already bad enough IMO now).
>>
>> The comments suggested that bufmgr.h could be incidentally pulled into
>> frontend code through other headers. Or do you mean that I don't need
>> to check for FRONTEND in the C source file (since, I assume, it's only
>> ever being compiled for the backend)? I'm not sure what you mean by
>> "bufmgr.h is already bad enough".
>
> I mean that this should not become worse without a better reason. This
> patch makes it so.
>
>>> This bit in src/backend/access/transam/README may interest you:
>>> Note that we must only use PageSetLSN/PageGetLSN() when we know the action
>>> is serialised. Only Startup process may modify data blocks during recovery,
>>> so Startup process may execute PageGetLSN() without fear of serialisation
>>> problems. All other processes must only call PageSet/GetLSN when holding
>>> either an exclusive buffer lock or a shared lock plus buffer header lock,
>>> or be writing the data block directly rather than through shared buffers
>>> while holding AccessExclusiveLock on the relation.
>>>
>>> So I see no problem with the exiting caller.
>>
>> Is heap_xlog_visible only called during startup?
>
> Recovery is more exact. When replaying a XLOG_HEAP2_VISIBLE record.
>
>> If so, is there a
>> general rule for which locks are required during startup and which
>> aren't?
>
> You can surely say that a process is fine to read a variable without a
> lock if it is the only one updating it, other processes would need a
> lock to read a variable. In this case the startup process is the only
> one updating blocks, so that's at least one code path where the is no
> need to take a lock when looking at a page LSN with PageGetLSN.

Is there a way to programmatically determine whether or not we're in
such a situation? That could be added to the assertion.

The code here is pretty inconsistent on whether or not PageGetLSN is
called inside or outside a lock -- see XLogReadBufferForRedoExtended
for instance.

> Another example is pageinspect which works on a copy of a page and
> uses PageGetLSN, so no direct locks on the buffer page is needed.

And again -- this case should be correctly handled by the new
assertion. Copies of pages are not checked for locks; we can't recover
a buffer header from a page that isn't part of the BufferBlocks array.

> In short, it seems to me that this patch should be rejected in its
> current shape.

Is the half of the patch that switches PageGetLSN to
BufferGetLSNAtomic correct, at least?

> There is no need to put more constraints on a API which
> does not need more constraints and which is used widely by extensions.

The goal here is not to add more constraints, but to verify that the
existing constraints are followed. Perhaps this patch doesn't do that
well/correctly, but I want to make sure we're on the same page
regarding the end goal.

--Jacob