Re: Race condition in InvalidateObsoleteReplicationSlots()

On Thu, Apr 8, 2021 at 7:39 AM Andres Freund <andres@anarazel.de> wrote:
>
> On 2021-04-07 17:10:37 -0700, Andres Freund wrote:
> > I think this can be solved in two different ways:
> >
> > 1) Hold ReplicationSlotAllocationLock with LW_SHARED across most of
> >    InvalidateObsoleteReplicationSlots(). That way nobody could re-create a new
> >    slot in the to-be-obsoleted-slot's place.
> >
> > 2) Atomically check whether the slot needs to be invalidated and try to
> >    acquire if needed. Don't release ReplicationSlotControlLock between those
> >    two steps. Signal the owner to release the slot iff we couldn't acquire the
> >    slot. In the latter case wait and then recheck if the slot still needs to
> >    be dropped.
> >
> > To me 2) seems better, because we then can also be sure that the slot still
> > needs to be obsoleted, rather than potentially doing so unnecessarily.
> >

+1.

> >
> > It looks to me like several of the problems here stem from trying to reuse
> > code from ReplicationSlotAcquireInternal() (which before this was just named
> > ReplicationSlotAcquire()).  I don't think that makes sense, because cases like
> > this want to check if a condition is true, and acquire it only if so.
> >
> > IOW, I think this basically needs to look like ReplicationSlotsDropDBSlots(),
> > except that a different condition is checked, and the if (active_pid) case
> > needs to prepare a condition variable, signal the owner and then wait on the
> > condition variable, to restart after.
>
> I'm also confused by the use of ConditionVariableTimedSleep(timeout =
> 10). Why do we need a timed sleep here in the first place? And why with
> such a short sleep?
>
> I also noticed that the code is careful to use CHECK_FOR_INTERRUPTS(); -
> but is aware it's running in checkpointer. I don't think CFI does much
> there? If we are worried about needing to check for interrupts, more
> work is needed.
>
>
> Sketch for a fix attached. I did leave the odd
> ConditionVariableTimedSleep(10ms) in, because I wasn't sure why it's
> there...
>

I haven't tested the patch but I couldn't spot any problems while
reading it. A minor point, don't we need to use
ConditionVariableCancelSleep() at some point after doing
ConditionVariableTimedSleep?

-- 
With Regards,
Amit Kapila.