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

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.

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?

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.

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

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

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

[6] As I mentioned upthread, isn't it better to have
"IsRecoveryPaused(void)" than "RecoveryIsPaused(void)"?

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

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

Instead of "while (RecoveryIsPaused())", can't we change it to "while
(GetRecoveryPauseState() != RECOVERY_NOT_PAUSED)" and remove the
RecoveryIsPaused()?

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

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