Re: Missing LWLock protection in pgstat_reset_replslot()

Hi,

On Tue, Mar 05, 2024 at 09:55:32AM +0200, Heikki Linnakangas wrote:
> 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.

Thanks for looking at it!

> We have a similar issue in
> pgstat_fetch_replslot(); it might return stats for wrong slot, if the slot
> is dropped/recreated concurrently.

Good catch!

> Do we care?

Yeah, I think we should: done in v2 attached.

> > --- 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.
>

Right, done in v2. Also had to add an extra "need_lock" argument to
get_replslot_index() for the same reason while taking care of pgstat_fetch_replslot().

Regards,

--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com