On Wed, Nov 22, 2023 at 3:06 PM Alvaro Herrera <alvhe...@alvh.no-ip.org> wrote: > > Hello, > > On 2023-Nov-22, Bharath Rupireddy wrote: > > > While looking at the use of session_replication_state, I noticed that > > ReplicationOriginLock is acquired in ReplicationOriginExitCleanup() > > even if session_replication_state is reset to NULL by > > replorigin_session_reset(). Why can't there be a lockless exit path > > something like [1] similar to > > replorigin_session_reset() which checks session_replication_state == > > NULL without a lock? > > I suppose we can do this on consistency grounds -- I'm pretty sure you'd > have a really hard time proving that this makes a performance difference --
Yes, can't measure the perf gain, however, as said upthread https://www.postgresql.org/message-id/CALj2ACVVhPn7BVQZLGPVvBrLoDZNRaV0LS9rBpt5y%2Bj%3DxRebWw%40mail.gmail.com, it avoids unnecessary lock acquisition and release. > but this patch is incomplete: just two lines below, we're still testing > session_replication_state for nullness, which would now be dead code. > Please repair. Done. > The comment on session_replication_state is confusing also: > > /* > * Backend-local, cached element from ReplicationState for use in a backend > * replaying remote commits, so we don't have to search ReplicationState for > * the backends current RepOriginId. > */ > > My problem with it is that this is not a "cached element", but instead a > "cached pointer to [shared memory]". This is what makes testing that > pointer for null-ness doable, but access to each member therein > requiring lwlock acquisition. Right. I've reworded the comment a bit. PSA v1 patch. -- Bharath Rupireddy PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
v1-0001-Introduce-lockless-exit-path-for-ReplicationOrigi.patch
Description: Binary data
