Re: Missing LWLock protection in pgstat_reset_replslot()

On Wed, Mar 06, 2024 at 09:05:59AM +0000, Bertrand Drouvot wrote:
> Yeah, all of the above done in v3 attached.

Interesting, so this relies on the slot index to ensure the unicity of
the stat entries.  And if the entry pointing to this ID is updated
we may refer to just incorrect data.

The inconsistency you could get for the fetch and reset cases are
narrow, but at least what you are proposing here would protect the
index lookup until the entry is copied from shmem, so this offers a
better consistency protection when querying
pg_stat_get_replication_slot() on a fetch, while avoiding a reset of
incorrect data under concurrent activity.

In passing..  pgstat_create_replslot() and pgstat_drop_replslot() rely
on the assumption that the LWLock ReplicationSlotAllocationLock is
taken while calling these routines.  Perhaps that would be worth some
extra Assert(LWLockHeldByMeInMode()) in pgstat_replslot.c for these
two?  Not directly related to this problem.
--
Michael