Re: Is Recovery actually paused?

On Wed, Feb 24, 2021 at 12:39 PM Kyotaro Horiguchi
<horikyota(dot)ntt(at)gmail(dot)com> wrote:
>
> At Tue, 23 Feb 2021 12:03:32 +0530, Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote in
> > On Fri, Feb 12, 2021 at 3:26 AM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> > > There might be some more to say here, but those are things I notice on
> > > a first read-through.
> >
> > Okay.
>
> It seems to me all the suggestions are addressed in this version.
>
> + Request to pause recovery. A request doesn't mean that recovery stops
> + right away. If you want a guarantee that recovery is actually paused,
> + you need to check for the recovery pause state returned by
> + <function>pg_get_wal_replay_pause_state()</function>. Note that
> + <function>pg_is_wal_replay_paused()</function> returns whether a request
> + is made. While recovery is paused, no further database changes are applied.
>
> This looks like explainig the same thing twice. ("A request doesn't
> mean.." and "While recovery is paused, ...")
>
> How about something like this?
>
> Request to pause recovery. Server actually stops recovery at a
> convenient time. This can take a few seconds after the request. If you
> need to strictly guarantee that no further database change will occur,
> you can check using pg_get_wal_replay_ause_state(). Note that
> pg_is_wal_replay_paused() may return true before recovery actually
> stops.

I still think that for the user-facing documentation purpose the
current paragraph looks better.

> The patch adds two loops whth the following logic:
>
> while (GetRecoveryPauseState() != RECOVERY_NOT_PAUSED)
> {
> ...
> ConfirmRecoveryPaused();
> <wait>
> }
>
> After the renaming of the function, the following structure looks
> simpler and more natural.
>
> while (ConfirmRecoveryPaused())
> {
> ...
> <wait>
> }

So do you mean that if the pause is requested ConfirmRecoveryPaused
will set it to paused and if it is not paused then it will return
false? With the current function name, I don't think that will look
clean maybe we should change the name to something like
CheckAndConfirmRecoveryPaused? Or I am fine with the way it is now.
Any other thoughts?

>
> + /* test for recovery pause, if user has requested the pause */
> + if (((volatile XLogCtlData *) XLogCtl)->recoveryPauseState !=
>
> The reason for the checkpoint is to move to "paused" state in a
> reasonable time. I think we need to mention that reason rather than
> what is done here.

I will do that.

>
> + /* get the recovery pause state */
> + switch(GetRecoveryPauseState())
> + {
> + case RECOVERY_NOT_PAUSED:
> + state = "not paused";
> + break;
> ...
> + default:
> + elog(ERROR, "invalid recovery pause state");
>
> This disables the static enum coverage check and it is not likely to
> have a wrong value here, other than the case of shared memory
> corruption. So we can remove the default case
> here. pg_get_replication_slots() is going that direction and
> apply_dispatch() is taking a slightly different way. Anyway I think
> that we can take away the default case.

So do you think we should put an assert(0) in the default case?

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com