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