Re: Add missing ConditionVariableCancelSleep() in slot.c

On Sat, Apr 13, 2024 at 4:37 PM Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:
>
> Hmm, but shouldn't we cancel the sleep after we have completed sleeping
> altogether, that is, until we've determined that we're no longer to
> sleep waiting for this slot? That would suggest to put the call to
> cancel sleep after the for(;;) loop is complete, rather than immediately
> after sleeping. No?
>
> diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c
> index cebf44bb0f..59e9bef642 100644
> --- a/src/backend/replication/slot.c
> +++ b/src/backend/replication/slot.c
> @@ -1756,6 +1756,8 @@ InvalidatePossiblyObsoleteSlot(ReplicationSlotInvalidationCause cause,
> }
> }
>
> + ConditionVariableCancelSleep();
> +
> Assert(released_lock == !LWLockHeldByMe(ReplicationSlotControlLock));
>
> return released_lock;

We can do that and +1 for it since the prepare to sleep cancel the
previous one anyway. I mentioned that approach in the original email:

>> Alternatively, we can
>> just add ConditionVariableCancelSleep() outside of the for loop to
>> cancel the sleep of the last cycle if any. This also looks correct
>> because every prepare to sleep does ensure the previous sleep is
>> canceled, and if no sleep at all, the cacnel sleep call exits.

> However, I noticed that ConditionVariablePrepareToSleep() does a
> CancelSleep upon being called ... so what I suggest would not have any
> effect whatsoever, because the sleep would be cancelled next time
> through the loop anyway.

But what if we break from the loop and never come back? We have to
wait until the sigsetjmp/exit path of the backend to hit and cancel
the sleep.

> But shouldn't we also modify PrepareToSleep to
> exit without doing anything if our current sleep CV is the same one
> being newly installed?
>
> diff --git a/src/backend/storage/lmgr/condition_variable.c b/src/backend/storage/lmgr/condition_variable.c
> index 112a518bae..65811ff989 100644
> --- a/src/backend/storage/lmgr/condition_variable.c
> +++ b/src/backend/storage/lmgr/condition_variable.c
> @@ -57,6 +57,14 @@ ConditionVariablePrepareToSleep(ConditionVariable *cv)
> {
> int pgprocno = MyProcNumber;
>
> + /*
> + * If we're preparing to sleep on the same CV we were already going to
> + * sleep on, we don't actually need to do anything. This may seem like
> + * supporting sloppy coding, which is what it actually does, so ¯\_(ツ)_/¯
> + */
> + if (cv_sleep_target == cv)
> + return;
> +
> /*
> * If some other sleep is already prepared, cancel it; this is necessary
> * because we have just one static variable tracking the prepared sleep,

That seems to work as a quick exit path avoiding spin lock acquisition
and release if the CV is already prepared to sleep. Specifically in
the InvalidatePossiblyObsoleteSlot for loop, it can avoid a bunch of
spin lock acquisitions and releases if we ever sleep on the same
slot's CV. However, I'm not sure if it will have any other issues.

BTW, I like the emoji "¯\_(ツ)_/¯" in the code comments :).

> Alternatively, maybe we need to not prepare-to-sleep in
> InvalidatePossiblyObsoleteSlot() if we have already prepared to sleep in
> a previous iteration through the loop (and of course, don't cancel the
> sleep until we're out of the loop).

I think this looks complicated. To keep things simple, I prefer to add
the ConditionVariableCancelSleep() out of the for loop in
InvalidatePossiblyObsoleteSlot().

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