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)