Re: Is Recovery actually paused? - Mailing list pgsql-hackers

On Sat, Jan 23, 2021 at 4:40 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> On Sat, Jan 23, 2021 at 1:36 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> > Please find the patch for the same.  I haven't added a test case for
> > this yet.  I mean we can write a test case to pause the recovery and
> > get the status.  But I am not sure that we can really write a reliable
> > test case for 'pause requested' and 'paused'.
>
> +1 to just show the recovery pause state in the output of
> pg_is_wal_replay_paused. But, should the function name
> "pg_is_wal_replay_paused" be something like
> "pg_get_wal_replay_pause_state" or some other? To me, when "is" exists
> in a function, I expect a boolean output. Others may have better
> thoughts.

I am fine with the name change, but don't feel that it will be
completely wrong if pg_is_wal_replay_paused returns a different state
of the recovery pause.
So I would like to see what others thinks and based on that we can decide.

> IIUC the above change, ensuring the recovery is paused after it's
> requested lies with the user. IMHO, the main problem we are trying to
> solve is not met. Isn't it better if we have a new function(wait
> version) along with the above change to pg_is_wal_replay_paused,
> something like "pg_wal_replay_pause_and_wait" returning true or false?
> The functionality is pg_wal_replay_pause + wait until it's actually
> paused.
>
> Thoughts?

Already replied in the last mail.

> Some comments on the v6 patch:
>
> [1] How about
> + * This function returns the current state of the recovery pause.
> instead of
> + * This api will return the current state of the recovery pause.

Okay

> [2] Typo - it's "requested" + * 'paused requested' - if pause is
> reqested but recovery is not yet paused
>
> [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.

> [4] Isn't it good to have an example usage and output of the function
> in the documentaion?
> +        Returns recovery pause status, which is <literal>not
> paused</literal> if
> +        pause is not requested, <literal>pause requested</literal> if pause is
> +        requested but recovery is not yet paused and,
> <literal>paused</literal> if
> +        the recovery is actually paused.
>         </para></entry>

I will add.

> [5] Is it
> + * Wait until shared recoveryPause state is set to RECOVERY_NOT_PAUSED.
> instead of
> + * Wait until shared recoveryPause is set to RECOVERY_NOT_PAUSED.

Ok

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

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

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

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

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