Hi,
On 3/1/23 1:48 AM, Jeff Davis wrote:
On Mon, 2023-02-27 at 09:40 +0100, Drouvot, Bertrand wrote:
Please find attached V51 tiny rebase due to a6cd1fc692 (for 0001) and
8a8661828a (for 0005).
[ Jumping into this thread late, so I apologize if these comments have
already been covered. ]
Thanks for looking at it!
Regarding v51-0004:
* Why is the CV sleep not being canceled?
I think that's an oversight, I'll look at it.
* Comments on WalSndWaitForWal need to be updated to explain the
difference between the flush (primary) and the replay (standby) cases.
Yeah, will do.
Overall, it seems like what you really want for the sleep/wakeup logic
in WalSndWaitForLSN
Typo for WalSndWaitForWal()?
is something like this:
condVar = RecoveryInProgress() ? replayCV : flushCV;
waitEvent = RecoveryInProgress() ?
WAIT_EVENT_WAL_SENDER_WAIT_REPLAY :
WAIT_EVENT_WAL_SENDER_WAIT_FLUSH;
ConditionVariablePrepareToSleep(condVar);
for(;;)
{
...
sleeptime = WalSndComputeSleepTime(GetCurrentTimestamp());
socketEvents = WL_SOCKET_READABLE;
if (pq_is_send_pending())
socketEvents = WL_SOCKET_WRITABLE;
ConditionVariableTimedSleepOrEvents(
condVar, sleeptime, socketEvents, waitEvent);
}
ConditionVariableCancelSleep();
But the problem is that ConditionVariableTimedSleepOrEvents() doesn't
exist, and I think that's what Andres was suggesting here[1].
WalSndWait() only waits for a timeout or a socket event, but not a CV;
ConditionVariableTimedSleep() only waits for a timeout or a CV, but not
a socket event.
I'm also missing how WalSndWait() works currently. It calls
ModifyWaitEvent() with NULL for the latch, so how does WalSndWakeup()
wake it up?
I think it works because the latch is already assigned to the FeBeWaitSet
in pq_init()->AddWaitEventToSet() (for latch_pos).
Assuming I'm wrong, and WalSndWait() does use the latch, then I guess
it could be extended by having two different latches in the WalSnd
structure, and waking them up separately and waiting on the right one.
I'm not sure this is needed in this particular case, because:
Why not "simply" call ConditionVariablePrepareToSleep() without any call to
ConditionVariableTimedSleep() later?
In that case the walsender will be put in the wait queue (thanks to
ConditionVariablePrepareToSleep())
and will be waked up by the event on the socket, the timeout or the CV
broadcast (since IIUC they all rely on the same latch).
So, something like:
condVar = RecoveryInProgress() ? replayCV : flushCV;
ConditionVariablePrepareToSleep(condVar);
for(;;)
{
...
sleeptime = WalSndComputeSleepTime(GetCurrentTimestamp());
socketEvents = WL_SOCKET_READABLE;
if (pq_is_send_pending())
socketEvents = WL_SOCKET_WRITABLE;
WalSndWait(wakeEvents, sleeptime, WAIT_EVENT_WAL_SENDER_WAIT_WAL);
<-- Note: the code within the loop does not change at all
}
ConditionVariableCancelSleep();
If the walsender is waked up by the CV broadcast, then it means the
flush/replay occurred and then we should exit the loop
right after due to:
"
/* check whether we're done */
if (loc <= RecentFlushPtr)
break;
"
meaning that in this particular case there is only one wake up due to the CV
broadcast before exiting the loop.
That looks weird to use ConditionVariablePrepareToSleep() without actually
using ConditionVariableTimedSleep()
but it looks to me that it would achieve the same goal: having the walsender
being waked up
by the event on the socket, the timeout or the CV broadcast.
In that case we would be missing the WAIT_EVENT_WAL_SENDER_WAIT_REPLAY and/or
the WAIT_EVENT_WAL_SENDER_WAIT_FLUSH
wait events thought (and we'd just provide the WAIT_EVENT_WAL_SENDER_WAIT_WAL
one) but I'm not sure that's a big issue.
What do you think?
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
