Re: Missing LWLock protection in pgstat_reset_replslot()

On Wed, Mar 6, 2024 at 2:36 PM Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:
>
> Hi,
>
> On Wed, Mar 06, 2024 at 10:24:46AM +0530, shveta malik wrote:
> > On Tue, Mar 5, 2024 at 6:52 PM Bertrand Drouvot
> > <bertranddrouvot.pg@gmail.com> wrote:
> > Thanks.  Can we try to get rid of multiple LwLockRelease in
> > pgstat_reset_replslot(). Is this any better?
> >
> >         /*
> > -        * Nothing to do for physical slots as we collect stats only for logical
> > -        * slots.
> > +        * Reset stats if it is a logical slot. Nothing to do for physical slots
> > +        * as we collect stats only for logical slots.
> >          */
> > -       if (SlotIsPhysical(slot))
> > -       {
> > -               LWLockRelease(ReplicationSlotControlLock);
> > -               return;
> > -       }
> > -
> > -       /* reset this one entry */
> > -       pgstat_reset(PGSTAT_KIND_REPLSLOT, InvalidOid,
> > -                                ReplicationSlotIndex(slot));
> > +       if (SlotIsLogical(slot))
> > +               pgstat_reset(PGSTAT_KIND_REPLSLOT, InvalidOid,
> > +                                        ReplicationSlotIndex(slot));
> >
> >         LWLockRelease(ReplicationSlotControlLock);
> >
>
> Yeah, it's easier to read and probably reduce the pgstat_replslot.o object file
> size a bit for non optimized build.
>
> > Something similar in pgstat_fetch_replslot() perhaps?
>
> Yeah, all of the above done in v3 attached.
>

Thanks for the patch.

For the fix in pgstat_fetch_replslot(), even with the lock in fetch
call, there are chances that the concerned slot can be dropped and
recreated.

--It can happen in a small window in pg_stat_get_replication_slot()
when we are consuming the return values of pgstat_fetch_replslot
(using slotent).

--Also it can happen at a later stage when we have switched to
fetching the next slot (obtained from 'pg_replication_slots' through
view' pg_stat_replication_slots'), the previous one can be dropped.

Ultimately the results of system view 'pg_replication_slots' can still
give us non-existing or re-created slots. But yes, I do not deny that
it gives us better consistency protection.

Do you feel that the lock in pgstat_fetch_replslot() should be moved
to its caller when we are done copying the results of that slot? This
will improve the protection slightly.

thanks
Shveta