Re: Is Recovery actually paused?
От | Dilip Kumar |
---|---|
Тема | Re: Is Recovery actually paused? |
Дата | |
Msg-id | CAFiTN-t2C6v=U5jNGEPYzmB6OS=wK2ELoxuFUC-J50tEPKsYjA@mail.gmail.com обсуждение исходный текст |
Ответ на | Re: Is Recovery actually paused? (Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>) |
Ответы |
Re: Is Recovery actually paused?
(Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>)
|
Список | 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
В списке pgsql-hackers по дате отправления:
Следующее
От: Julien RouhaudДата:
Сообщение: Faulty HEAP_XMAX_LOCK_ONLY & HEAP_KEYS_UPDATED hintbit combination