Re: Is Recovery actually paused?

On Sun, Jan 24, 2021 at 11:29 AM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> > Some comments on the v6 patch:

> > [2] Typo - it's "requested" + * 'paused requested' - if pause is
> > reqested but recovery is not yet paused

Here I meant the typo "reqested" in "if pause is reqested but recovery
is not yet paused" statement from v6 patch.

> > [3] I think it's "+ * 'pause requested'" instead of "+ * 'paused requested'"
>
> Which code does it refer, can give put the snippet from the patch.
> However, I have found there were 'paused requested' in two places so I
> have fixed.

Thanks.

> > [6] As I mentioned upthread, isn't it better to have
> > "IsRecoveryPaused(void)" than "RecoveryIsPaused(void)"?
>
> That is an existing function so I think it's fine to keep the same name.

Personally, I think the function RecoveryIsPaused itself is
unnecessary with the new function GetRecoveryPauseState introduced in
your patch. IMHO, we can remove it. If not okay, then we are at it,
can we at least change the function name to be meaningful
"IsRecoveryPaused"? Others may have better thoughts than me.

> > [7] Can we have the function variable name "recoveryPause" as "state"
> > or "pauseState? Because that variable context is set by the enum name
> > RecoveryPauseState and the function name.
> >
> > +SetRecoveryPause(RecoveryPauseState recoveryPause)
> >
> > Here as well, "recoveryPauseState" to "state"?
> > +GetRecoveryPauseState(void)
> > {
> > - bool recoveryPause;
> > + RecoveryPauseState recoveryPauseState;
>
> I don't think it is required but while changing the patch I will see
> whether to change or not.

It will be good to change that. I personally don't like structure
names and variable names to be the same.

> > [6] Function name RecoveryIsPaused and it's comment "Check whether the
> > recovery pause is requested." doesn't seem to be matching. Seems like
> > it returns true even when RECOVERY_PAUSE_REQUESTED or RECOVERY_PAUSED.
> > Should it return true only when the state is RECOVERY_PAUSE_REQUESTED?
>
> Code is doing right, I will change the comments.
>
> > Instead of "while (RecoveryIsPaused())", can't we change it to "while
> > (GetRecoveryPauseState() != RECOVERY_NOT_PAUSED)" and remove the
> > RecoveryIsPaused()?
>
> I think it looks clean with the function

As I said earlier, I see no use of RecoveryIsPaused() with the
introduction of the new function GetRecoveryPauseState(). Others may
have better thoughts than me.

> > [7] Can we change the switch-case in pg_is_wal_replay_paused to
> > something like below?
> >
> > Datum
> > pg_is_wal_replay_paused(PG_FUNCTION_ARGS)
> > {
> > + char *state;
> > + /* get the recovery pause state */
> > + switch(GetRecoveryPauseState())
> > + {
> > + case RECOVERY_NOT_PAUSED:
> > + state = "not paused";
> > + case RECOVERY_PAUSE_REQUESTED:
> > + state = "paused requested";
> > + case RECOVERY_PAUSED:
> > + state = "paused";
> > + default:
> > + elog(ERROR, "invalid recovery pause state");
> > + }
> > +
> > + PG_RETURN_TEXT_P(cstring_to_text(type));
>
> Why do you think it is better to use an extra variable?

I see no wrong in having PG_RETURN_TEXT_P and cstring_to_text 's in
every case statement. But, just to make sure the code looks cleaner, I
said that we can have a local state variable and just one
PG_RETURN_TEXT_P(cstring_to_text(state));. See some existing functions
brin_page_type, hash_page_type, json_typeof,
pg_stat_get_backend_activity, pg_stat_get_backend_wait_event_type,
pg_stat_get_backend_wait_event, get_command_type.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com