Re: Minimal logical decoding on standbys

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(dot)pg(at)gmail(dot)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,

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