Re: Missing LWLock protection in pgstat_reset_replslot()

On 01/03/2024 12:15, Bertrand Drouvot wrote:
> Hi hackers,
> 
> I think that pgstat_reset_replslot() is missing LWLock protection. Indeed, we
> don't have any guarantee that the slot is active (then preventing it to be
> dropped/recreated) when this function is executed.

Yes, so it seems at quick glance. We have a similar issue in 
pgstat_fetch_replslot(); it might return stats for wrong slot, if the 
slot is dropped/recreated concurrently. Do we care?

> --- a/src/backend/utils/activity/pgstat_replslot.c
> +++ b/src/backend/utils/activity/pgstat_replslot.c
> @@ -46,6 +46,8 @@ pgstat_reset_replslot(const char *name)
>  
>      Assert(name != NULL);
>  
> +    LWLockAcquire(ReplicationSlotControlLock, LW_SHARED);
> +
>      /* Check if the slot exits with the given name. */
>      slot = SearchNamedReplicationSlot(name, true);

SearchNamedReplicationSlot() will also acquire the lock in LW_SHARED 
mode, when you pass need_lock=true. So that at least should be changed 
to false.

-- 
Heikki Linnakangas
Neon (https://neon.tech)