Thread: Adding an LWLockHeldByMe()-like function that reports if any buffercontent lock is held

During recent review of the INCLUDE covering index patch, I pushed to
formalize the slightly delicate assumptions that we make around how
index_truncate_tuple() is called. It's natural to call
index_truncate_tuple() during a page split, when a buffer lock is
held. This is what we actually do in most cases.

It occurred to me that it would be nice to be able to
Assert(!AnyBufferLockHeldByMe()) at a certain point within
index_form_tuple(), to make sure that our assumptions hold. If
index_truncate_tuple() (or any other function) ever called
index_form_tuple(), and ended up actually performing table access with
an exclusive buffer lock held, we'd at least be able to catch the bug
when assertions are enabled. A function that lets code assert that no
buffer locks are held, for the rare cases where external table access
is required seems like good general infrastructure.

Does this seem like a good idea? This could get pretty expensive if it
was overused, even by the standards of what we expect from
assertion-enabled builds, but we could make it optional if the
overhead got out of hand.

-- 
Peter Geoghegan

On Wed, Apr 18, 2018 at 04:53:29PM -0700, Peter Geoghegan wrote:
> It occurred to me that it would be nice to be able to
> Assert(!AnyBufferLockHeldByMe()) at a certain point within
> index_form_tuple(), to make sure that our assumptions hold. If
> index_truncate_tuple() (or any other function) ever called
> index_form_tuple(), and ended up actually performing table access with
> an exclusive buffer lock held, we'd at least be able to catch the bug
> when assertions are enabled. A function that lets code assert that no
> buffer locks are held, for the rare cases where external table access
> is required seems like good general infrastructure.

Personally, I favor approaches like that, because it allows to catch up
problems in using some APIs when people working on a patch miss any kind
of warning comments at the top of the function or within it which
summarize the conditions under which something needs to be used.

> Does this seem like a good idea? This could get pretty expensive if it
> was overused, even by the standards of what we expect from
> assertion-enabled builds, but we could make it optional if the
> overhead got out of hand.

There are other places which could be improved, mentioned one here:
https://www.postgresql.org/message-id/CAB7nPqRJtKO5NLZZis3xxzH05sdAodG39qFdLQRg371Pi94PzQ%40mail.gmail.com
And if you see downthread this is a topic where it seems hard to reach a
consensus.
--
Michael

On Wed, Apr 18, 2018 at 5:46 PM, Michael Paquier <michael@paquier.xyz> wrote:
> Personally, I favor approaches like that, because it allows to catch up
> problems in using some APIs when people working on a patch miss any kind
> of warning comments at the top of the function or within it which
> summarize the conditions under which something needs to be used.

Right. Imagine how long it would take to figure out when there is a
bug without something like this assertion. It's fairly difficult to
debug LWLock deadlocks in production, even for experts.

What I have in mind here is something that's a bit like
AssertNotInCriticalSection(). We don't need to pepper
AssertNotInCriticalSection() everywhere in practice, because calling
palloc() is a pretty good proxy for "function should not be called in
a critical section" -- palloc() calls AssertNotInCriticalSection(),
which probably catches most unsafe code in critical sections
immediately. We could probably also get decent
Assert(!AnyBufferLockHeldByMe()) coverage without adding many new
asserts.

I'm curious about what we'll find by just by adding
Assert(!AnyBufferLockHeldByMe()) to the top of
heap_tuple_fetch_attr(). AssertNotInCriticalSection() certainly found
several bugs when it was first added.

-- 
Peter Geoghegan

On Wed, Apr 18, 2018 at 06:44:00PM -0700, Peter Geoghegan wrote:
> What I have in mind here is something that's a bit like
> AssertNotInCriticalSection(). We don't need to pepper
> AssertNotInCriticalSection() everywhere in practice, because calling
> palloc() is a pretty good proxy for "function should not be called in
> a critical section" -- palloc() calls AssertNotInCriticalSection(),
> which probably catches most unsafe code in critical sections
> immediately.

In this case, the prospect of limiting unnecessary PANIC exists on OOM
was the deal breaker.

> We could probably also get decent
> Assert(!AnyBufferLockHeldByMe()) coverage without adding many new
> asserts.
>
> I'm curious about what we'll find by just by adding
> Assert(!AnyBufferLockHeldByMe()) to the top of
> heap_tuple_fetch_attr(). AssertNotInCriticalSection() certainly found
> several bugs when it was first added.

Yep.
--
Michael

On Wed, Apr 18, 2018 at 6:53 PM, Michael Paquier <michael@paquier.xyz> wrote:
>> I'm curious about what we'll find by just by adding
>> Assert(!AnyBufferLockHeldByMe()) to the top of
>> heap_tuple_fetch_attr(). AssertNotInCriticalSection() certainly found
>> several bugs when it was first added.
>
> Yep.

I wrote a simple prototype of this. No assertion failed during a "make
installcheck". Assertions were added to heap_tuple_fetch_attr(), and a
couple of other places.

-- 
Peter Geoghegan