Thread: 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
Re: LWLockAcquire and LockBuffer mode argument
From
ilmari@ilmari.org (Dagfinn Ilmari Mannsåker)
Date:
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