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 -- 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. 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. -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/ "Once again, thank you and all of the developers for your hard work on PostgreSQL. This is by far the most pleasant management experience of any database I've worked on." (Dan Harris) http://archives.postgresql.org/pgsql-performance/2006-04/msg00247.php
