Re: Race condition in InvalidateObsoleteReplicationSlots()

Hi,

On 2021-04-29 13:28:20 -0400, Álvaro Herrera wrote:
> On 2021-Apr-07, Andres Freund wrote:
>
> > 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 was scared of the possibility that a process would not set the CV for
> whatever reason, causing checkpointing to become stuck. Maybe that's
> misguided thinking if CVs are reliable enough.

They better be, or we have bigger problems. And if it's an escape hatch
we surely ought not to use 10ms as the timeout. That's an appropriate
time for something *not* using condition variables...

> I attach a couple of changes to your 0001. It's all cosmetic; what
> looks not so cosmetic is the change of "continue" to "break" in helper
> routine; if !s->in_use, we'd loop infinitely. The other routine
> already checks that before calling the helper; since you hold
> ReplicationSlotControlLock at that point, it should not be possible to
> drop it in between. Anyway, it's a trivial change to make, so it should
> be correct.

> I also added a "continue" at the bottom of one block; currently that
> doesn't change any behavior, but if we add code at the other block, it
> might not be what's intended.

Seems sane.

> Are you getting this set pushed, or would you like me to handle it?
> (There seems to be some minor conflict in 13)

I'd be quite happy for you to handle it...

Greetings,

Andres Freund