On Wed, Feb 24, 2021 at 2:26 PM Kyotaro Horiguchi <horikyota....@gmail.com> wrote: > > At Wed, 24 Feb 2021 13:15:27 +0530, Dilip Kumar <dilipbal...@gmail.com> wrote > in > > On Wed, Feb 24, 2021 at 12:39 PM Kyotaro Horiguchi > > <horikyota....@gmail.com> wrote: > > > > > > At Tue, 23 Feb 2021 12:03:32 +0530, Dilip Kumar <dilipbal...@gmail.com> > > > wrote in > > > > On Fri, Feb 12, 2021 at 3:26 AM Robert Haas <robertmh...@gmail.com> > > > > wrote: > > > How about something like this? > > > > > > Request to pause recovery. Server actually stops recovery at a > > > convenient time. This can take a few seconds after the request. If you > > > need to strictly guarantee that no further database change will occur, > > > you can check using pg_get_wal_replay_ause_state(). Note that > > > pg_is_wal_replay_paused() may return true before recovery actually > > > stops. > > > > I still think that for the user-facing documentation purpose the > > current paragraph looks better. > > Ok. > > > > The patch adds two loops whth the following logic: > > > > > > while (GetRecoveryPauseState() != RECOVERY_NOT_PAUSED) > > > { > > > ... > > > ConfirmRecoveryPaused(); > > > <wait> > > > } > > > > > > After the renaming of the function, the following structure looks > > > simpler and more natural. > > > > > > while (ConfirmRecoveryPaused()) > > > { > > > ... > > > <wait> > > > } > > > > So do you mean that if the pause is requested ConfirmRecoveryPaused > > will set it to paused and if it is not paused then it will return > > false? With the current function name, I don't think that will look > > clean maybe we should change the name to something like > > CheckAndConfirmRecoveryPaused? Or I am fine with the way it is now. > > Any other thoughts? > > I should have took the meaning of "confirm" wrongly. I took that as > "somehow determine if the recovery is to be paused". If that reading > is completely wrong, I don't mind either re-chaging the function name > or leaving all it alone.
I am fine with leaving it the way it is unless someone feels that we should change it. > > > > > > + /* test for recovery pause, if user has requested the > > > pause */ > > > + if (((volatile XLogCtlData *) > > > XLogCtl)->recoveryPauseState != > > > > > > The reason for the checkpoint is to move to "paused" state in a > > > reasonable time. I think we need to mention that reason rather than > > > what is done here. > > > > I will do that. > > Thanks. > > > > > > > + /* get the recovery pause state */ > > > + switch(GetRecoveryPauseState()) > > > + { > > > + case RECOVERY_NOT_PAUSED: > > > + state = "not paused"; > > > + break; > > > ... > > > + default: > > > + elog(ERROR, "invalid recovery pause state"); > > > > > > This disables the static enum coverage check and it is not likely to > > > have a wrong value here, other than the case of shared memory > > > corruption. So we can remove the default case > > > here. pg_get_replication_slots() is going that direction and > > > apply_dispatch() is taking a slightly different way. Anyway I think > > > that we can take away the default case. > > > > So do you think we should put an assert(0) in the default case? > > No. Just removing the default in the switch. If the value comes from > some other source typically from disk or user-interraction, the > default is necessary, but, in the first place if we have other than > the defined value there, it is a sign of something worse than > ERROR. If we care about that case, we *could* do the same thing with > apply_dispatch(). > > switch (GetRecoveryPauseState()) > { > case RECOVERY_NOT_PAUSED: > return cstring_to_text("not paused"); > .. > } > > /* we shouldn't reach here */ > Assert (0); I think for such cases IMHO the preferred style for PostgreSQL is that we add Assert(0) in the default case, at least it appeared to me that way. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com