Re: Is Recovery actually paused?

On Sun, Jan 24, 2021 at 12:17 PM Bharath Rupireddy
<bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
>
> 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.

Ok

> > > [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.

I have removed this function

> > > [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.

Changed to state

> > > [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.
>

I have changed as per other functions for consistency.

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