Обсуждение: LWLockAcquire and LockBuffer mode argument

Hi,

Currently lwlock acquisition, whether direct, or via the LockBuffer()
wrapper, have a mode argument. I don't like that much, for three
reasons:

1) I've tried to add annotations for static analyzers to help with
   locking correctness. The ones I looked at don't support annotating
   shared/exclusive locks where the mode is specified as a variable.
2) When doing performance analysis it's quite useful to be able to see
   the difference between exlusive and shared acquisition. Typically all
   one has access to is the symbol name though.
3) I don't like having the unnecessary branches for the lock mode, after
   all a lot of the lock protected code is fairly hot. It's pretty
   unnecessary because the caller almost (?) always uses a static lock
   mode.

Therefore I'd like to replace the current lock functions with ones where
the lock mode is specified as part of the function name rather than an
argument.

To avoid unnecessary backward compat pains it seems best to first
introduce compat wrappers using the current signature, and then
subsequently replace in-core callers with the direct calls.


There's several harder calls though:
1) All of the above would benefit from lock release also being annotated
   with the lock mode. That'd be a lot more invasive however. I think
   it'd be best to add explicit functions (which would just assert
   held_lwlocks[] being correct), but keep a wrapper that determines the
   current lock level using held_lwlocks.

2) For performance it'd be nice if we could move the BufferIsLocal()
   checks for LockBuffer* into the caller. Even better would be if we
   made them inline wrappers around
   LWLockAcquire(Shared|Exclusive). However, as the latter would require
   making BufferDescriptorGetContentLock() available in bufmgr.h I think
   that's not worth it. So I think we'd be best off having
   LockBufferExclusive() be a static inline wrapper doing the
   BufferIsLocal() check and then calling LockBufferExclusiveImpl
   which'd do the LWLockAcquireExclusive().

Thoughts?

Greetings,

Andres Freund

On Mon, Aug 24, 2020 at 6:35 PM Andres Freund <andres@anarazel.de> wrote:
> Thoughts?

This is likely to cause a certain amount of annoyance to many
PostgreSQL developers, but if you have evidence that it will improve
performance significantly, I think it's very reasonable to do it
anyway. However, if we do it all in a backward-compatible way as you
propose, then we're likely to keep reintroducing code that does it the
old way for a really long time. I'm not sure that actually makes a lot
of sense. It might be better to just bite the bullet and make a hard
break.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Hi,

On 2020-08-25 13:59:35 -0400, Robert Haas wrote:
> On Mon, Aug 24, 2020 at 6:35 PM Andres Freund <andres@anarazel.de> wrote:
> > Thoughts?
> 
> This is likely to cause a certain amount of annoyance to many
> PostgreSQL developers, but if you have evidence that it will improve
> performance significantly, I think it's very reasonable to do it
> anyway.

I don't think it'll be a "significant" performance benefit directly. It
appears to be measurable, but I think to reach significant performance
improvements it'll take a while and it'll come from profilers and other
tools working better.

> However, if we do it all in a backward-compatible way as you propose,
> then we're likely to keep reintroducing code that does it the old way
> for a really long time. I'm not sure that actually makes a lot of
> sense. It might be better to just bite the bullet and make a hard
> break.

It seems easy enough to slap a compiler "enforced" deprecation warning
on the new compat version, in master only. Seems unnecessary to make
life immediately harder for extensions authors desiring cross-version
compatibility.

Greetings,

Andres Freund

On Tue, Aug 25, 2020 at 2:17 PM Andres Freund <andres@anarazel.de> wrote:
> It seems easy enough to slap a compiler "enforced" deprecation warning
> on the new compat version, in master only. Seems unnecessary to make
> life immediately harder for extensions authors desiring cross-version
> compatibility.

I don't know exactly how you'd go about implementing that, but I am
not against compatibility. I *am* against coding rules that require a
lot of manual enforcement.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Hi,

On 2020-08-25 14:22:28 -0400, Robert Haas wrote:
> On Tue, Aug 25, 2020 at 2:17 PM Andres Freund <andres@anarazel.de> wrote:
> > It seems easy enough to slap a compiler "enforced" deprecation warning
> > on the new compat version, in master only. Seems unnecessary to make
> > life immediately harder for extensions authors desiring cross-version
> > compatibility.
> 
> I don't know exactly how you'd go about implementing that, but I am
> not against compatibility. I *am* against coding rules that require a
> lot of manual enforcement.

#if I_AM_GCC_OR_CLANG
#define pg_attribute_deprecated __attribute__((deprecated))
#elif I_AM_MSVC
#define pg_attribute_deprecated __declspec(deprecated)
#else
#define pg_attribute_deprecated
#endif

Greetings,

Andres Freund

On Mon, Aug 24, 2020 at 3:35 PM Andres Freund <andres@anarazel.de> wrote:
> To avoid unnecessary backward compat pains it seems best to first
> introduce compat wrappers using the current signature, and then
> subsequently replace in-core callers with the direct calls.

I like the idea of doing this, purely to make profiler output easier
to interpret.

Passing a shared-or-exclusive flag is kind of a natural thing to do
within code like _bt_search(), where we sometimes want to
exclusive-lock the leaf level page but not the internal pages that we
descend through first. Fortunately we can handle the flag inside the
existing nbtree wrapper functions quite easily -- the recently added
_bt_lockbuf() can test the flag directly. We already have
nbtree-private flags (BT_READ and BT_WRITE) that we can continue to
use after the old interface is fully deprecated.

More generally, it probably is kind of natural to have a flag like
BUFFER_LOCK_SHARE/BUFFER_LOCK_EXCLUSIVE (though not like
BUFFER_LOCK_UNLOCK) within index access methods. But I think that
there are several good reasons to add something equivalent to
_bt_lockbuf() to all index access methods.

-- 
Peter Geoghegan

Andres Freund <andres@anarazel.de> writes:

> Hi,
>
> On 2020-08-25 13:59:35 -0400, Robert Haas wrote:
>
>> However, if we do it all in a backward-compatible way as you propose,
>> then we're likely to keep reintroducing code that does it the old way
>> for a really long time. I'm not sure that actually makes a lot of
>> sense. It might be better to just bite the bullet and make a hard
>> break.
>
> It seems easy enough to slap a compiler "enforced" deprecation warning
> on the new compat version, in master only. Seems unnecessary to make
> life immediately harder for extensions authors desiring cross-version
> compatibility.

Would it be possible to make the compat versions only available when
building extensions, but not to core code?

In Perl we do that a lot, using #ifndef PERL_CORE.

- ilmari
-- 
- Twitter seems more influential [than blogs] in the 'gets reported in
  the mainstream press' sense at least.               - Matt McLeod
- That'd be because the content of a tweet is easier to condense down
  to a mainstream media article.                      - Calle Dybedahl

On Wed, Aug 26, 2020 at 7:47 AM Dagfinn Ilmari Mannsåker
<ilmari@ilmari.org> wrote:
> Would it be possible to make the compat versions only available when
> building extensions, but not to core code?

I think that would be good if we can do it. We could even have it
inside #ifdef LWLOCK_API_COMPAT, and extension authors who want the
compatibility interface can define that.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2020-Aug-26, Robert Haas wrote:

> On Wed, Aug 26, 2020 at 7:47 AM Dagfinn Ilmari Mannsåker
> <ilmari@ilmari.org> wrote:
> > Would it be possible to make the compat versions only available when
> > building extensions, but not to core code?
> 
> I think that would be good if we can do it. We could even have it
> inside #ifdef LWLOCK_API_COMPAT, and extension authors who want the
> compatibility interface can define that.

We had ENABLE_LIST_COMPAT available for 16 years; see commits
d0b4399d81f3, 72b6ad631338, 1cff1b95ab6d.  We could do the same here.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services