Hi,
On 1/31/23 12:50 PM, Drouvot, Bertrand wrote:
> Hi,
>
> On 1/6/23 4:40 AM, Andres Freund wrote:
>> Hi,
>> On 2023-01-05 16:15:39 -0500, Robert Haas wrote:
>>> On Tue, Jan 3, 2023 at 2:42 AM Drouvot, Bertrand
>>> <bertranddrouvot.pg@gmail.com> wrote:
>> 0006:
>>
>>> diff --git a/src/backend/access/transam/xlogrecovery.c b/src/backend/access/transam/xlogrecovery.c
>>> index bc3c3eb3e7..98c96eb864 100644
>>> --- a/src/backend/access/transam/xlogrecovery.c
>>> +++ b/src/backend/access/transam/xlogrecovery.c
>>> @@ -358,6 +358,9 @@ typedef struct XLogRecoveryCtlData
>>> RecoveryPauseState recoveryPauseState;
>>> ConditionVariable recoveryNotPausedCV;
>>>
>>> + /* Replay state (see getReplayedCV() for more explanation) */
>>> + ConditionVariable replayedCV;
>>> +
>>> slock_t info_lck; /* locks shared variables shown above */
>>> } XLogRecoveryCtlData;
>>>
>>
>> getReplayedCV() doesn't seem to fit into any of the naming scheems in use for
>> xlogrecovery.h.
>
> Changed to check_for_replay() in V46 attached.
>
>>> - * Sleep until something happens or we time out. Also wait for the
>>> - * socket becoming writable, if there's still pending output.
>>> + * When not in recovery, sleep until something happens or we time out.
>>> + * Also wait for the socket becoming writable, if there's still pending output.
>>
>> Hm. Is there a problem with not handling the becoming-writable case in the
>> in-recovery case?
>>
>>
>
> Yes, when not in recovery we'd wait for the timeout to occur in ConditionVariableTimedSleep()
> (as the CV is broadcasted only in ApplyWalRecord()).
>
>>> + else
>>> + /*
>>> + * We are in the logical decoding on standby case.
>>> + * We are waiting for the startup process to replay wal record(s) using
>>> + * a timeout in case we are requested to stop.
>>> + */
>>> + {
>>
>> I don't think pgindent will like that formatting....
>
> Oops, fixed.
>
>>
>>
>>> + ConditionVariablePrepareToSleep(replayedCV);
>>> + ConditionVariableTimedSleep(replayedCV, 1000,
>>> + WAIT_EVENT_WAL_SENDER_WAIT_REPLAY);
>>> + }
>>
>> I think this is racy, see ConditionVariablePrepareToSleep()'s comment:
>>
>> * Caution: "before entering the loop" means you *must* test the exit
>> * condition between calling ConditionVariablePrepareToSleep and calling
>> * ConditionVariableSleep. If that is inconvenient, omit calling
>> * ConditionVariablePrepareToSleep.
>>
>> Basically, the ConditionVariablePrepareToSleep() should be before the loop
>> body.
>>
>
> I missed it, thanks! Moved it before the loop body.
>
>>
>> I don't think the fixed timeout here makes sense. For one, we need to wake up
>> based on WalSndComputeSleeptime(), otherwise we're ignoring wal_sender_timeout
>> (which can be quite small).
>
> Good point. Making use of WalSndComputeSleeptime() instead in V46.
>
>> It's also just way too frequent - we're trying to
>> avoid constantly waking up unnecessarily.
>>
>>
>> Perhaps we could deal with the pq_is_send_pending() issue by having a version
>> of ConditionVariableTimedSleep() that accepts a WaitEventSet?
>>
>
> What issue do you see?
> The one that I see with V46 (keeping the in/not recovery branches) is that one may need to wait
> for wal_sender_timeout to see changes that occurred right after the promotion.
>
> Regards,
>
Attaching a tiny rebase (V47) due to f9bc34fcb6.
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com