On Tue, 19 Jan 2021 21:32:31 +0530 Dilip Kumar <dilipbal...@gmail.com> wrote:
> On Tue, Jan 19, 2021 at 8:34 AM Dilip Kumar <dilipbal...@gmail.com> wrote: > > > > On Tue, 19 Jan 2021 at 8:12 AM, Yugo NAGATA <nag...@sraoss.co.jp> wrote: > >> > >> On Sun, 17 Jan 2021 11:33:52 +0530 > >> Dilip Kumar <dilipbal...@gmail.com> wrote: > >> > >> > On Thu, Jan 14, 2021 at 6:49 PM Yugo NAGATA <nag...@sraoss.co.jp> wrote: > >> > > > >> > > On Wed, 13 Jan 2021 17:49:43 +0530 > >> > > Dilip Kumar <dilipbal...@gmail.com> wrote: > >> > > > >> > > > On Wed, Jan 13, 2021 at 3:35 PM Dilip Kumar <dilipbal...@gmail.com> > >> > > > wrote: > >> > > > > > >> > > > > On Wed, Jan 13, 2021 at 3:27 PM Yugo NAGATA <nag...@sraoss.co.jp> > >> > > > > wrote: > >> > > > > > > >> > > > > > On Thu, 10 Dec 2020 11:25:23 +0530 > >> > > > > > Dilip Kumar <dilipbal...@gmail.com> wrote: > >> > > > > > > >> > > > > > > > > However, I wonder users don't expect > >> > > > > > > > > pg_is_wal_replay_paused to wait. > >> > > > > > > > > Especially, if max_standby_streaming_delay is -1, this > >> > > > > > > > > will be blocked forever, > >> > > > > > > > > although this setting may not be usual. In addition, some > >> > > > > > > > > users may set > >> > > > > > > > > recovery_min_apply_delay for a large. If such users call > >> > > > > > > > > pg_is_wal_replay_paused, > >> > > > > > > > > it could wait for a long time. > >> > > > > > > > > > >> > > > > > > > > At least, I think we need some descriptions on document to > >> > > > > > > > > explain > >> > > > > > > > > pg_is_wal_replay_paused could wait while a time. > >> > > > > > > > > >> > > > > > > > Ok > >> > > > > > > > >> > > > > > > Fixed this, added some comments in .sgml as well as in > >> > > > > > > function header > >> > > > > > > >> > > > > > Thank you for fixing this. > >> > > > > > > >> > > > > > Also, is it better to fix the description of pg_wal_replay_pause > >> > > > > > from > >> > > > > > "Pauses recovery." to "Request to pause recovery." in according > >> > > > > > with > >> > > > > > pg_is_wal_replay_paused? > >> > > > > > >> > > > > Okay > >> > > > > > >> > > > > > > >> > > > > > > > > Also, how about adding a new boolean argument to > >> > > > > > > > > pg_is_wal_replay_paused to > >> > > > > > > > > control whether this waits for recovery to get paused or > >> > > > > > > > > not? By setting its > >> > > > > > > > > default value to true or false, users can use the old > >> > > > > > > > > format for calling this > >> > > > > > > > > and the backward compatibility can be maintained. > >> > > > > > > > > >> > > > > > > > So basically, if the wait_recovery_pause flag is false then > >> > > > > > > > we will > >> > > > > > > > immediately return true if the pause is requested? I agree > >> > > > > > > > that it is > >> > > > > > > > good to have an API to know whether the recovery pause is > >> > > > > > > > requested or > >> > > > > > > > not but I am not sure is it good idea to make this API serve > >> > > > > > > > both the > >> > > > > > > > purpose? Anyone else have any thoughts on this? > >> > > > > > > > > >> > > > > > > >> > > > > > I think the current pg_is_wal_replay_paused() already has > >> > > > > > another purpose; > >> > > > > > this waits recovery to actually get paused. If we want to limit > >> > > > > > this API's > >> > > > > > purpose only to return the pause state, it seems better to fix > >> > > > > > this to return > >> > > > > > the actual state at the cost of lacking the backward > >> > > > > > compatibility. If we want > >> > > > > > to know whether pause is requested, we may add a new API like > >> > > > > > pg_is_wal_replay_paluse_requeseted(). Also, if we want to wait > >> > > > > > recovery to actually > >> > > > > > get paused, we may add an option to pg_wal_replay_pause() for > >> > > > > > this purpose. > >> > > > > > > >> > > > > > However, this might be a bikeshedding. If anyone don't care that > >> > > > > > pg_is_wal_replay_paused() can make user wait for a long time, I > >> > > > > > don't care either. > >> > > > > > >> > > > > I don't think that it will be blocked ever, because > >> > > > > pg_wal_replay_pause is sending the WakeupRecovery() which means the > >> > > > > recovery process will not be stuck on waiting for the WAL. > >> > > > >> > > Yes, there is no stuck on waiting for the WAL. However, it can be > >> > > stuck during resolving > >> > > a recovery conflict. The process could wait for > >> > > max_standby_streaming_delay or > >> > > max_standby_archive_delay at most before recovery get completely > >> > > paused. > >> > > >> > Okay, I agree that it is possible so for handling this we have a > >> > couple of options > >> > 1. pg_is_wal_replay_paused(), interface will wait for recovery to > >> > actually get paused, but user have an option to cancel that. So I > >> > agree that there is currently no option to just know that recovery > >> > pause is requested without waiting for its actually get paused if it > >> > is requested. So one option is we can provide an another interface as > >> > you mentioned pg_is_wal_replay_paluse_requeseted(), which can just > >> > return the request status. I am not sure how useful it is. > >> > >> If it is acceptable that pg_is_wal_replay_paused() makes users wait, > >> I'm ok for the current interface. I don't feel the need of > >> pg_is_wal_replay_paluse_requeseted(). > >> > >> > > >> > 2. Pass an option to pg_is_wal_replay_paused whether to wait for > >> > recovery to actually get paused or not. > >> > > >> > 3. Pass an option to pg_wal_replay_pause(), whether to wait for > >> > recovery pause or just request and return. > >> > > >> > I like the option 1, any other opinion on this? > >> > > >> > > Also, it could wait for recovery_min_apply_delay if it has a valid > >> > > value. It is possible > >> > > that a user set this parameter to a large value, so it could wait for > >> > > a long time. However, > >> > > this will be avoided by calling recoveryPausesHere() or > >> > > CheckAndSetRecoveryPause() in > >> > > recoveryApplyDelay(). > >> > > >> > Right > >> > >> Is there any reason not to do it? > > > > > > > > I think I missed that.. I will do in the next version > > > > In the last patch there were some local changes which I did not add to > the patch and it was giving compilation warning so fixed that along > with that I have addressed your this comment as well. Thank you fixing this! I noticed that, after this fix, the following recoveryPausesHere() might be unnecessary because this test and pause is already done in recoveryApplyDelay What do you think about it? if (recoveryApplyDelay(xlogreader)) { /* * We test for paused recovery again here. If user sets * delayed apply, it may be because they expect to pause * recovery in case of problems, so we must test again * here otherwise pausing during the delay-wait wouldn't * work. */ if (((volatile XLogCtlData *) XLogCtl)->recoveryPauseState) recoveryPausesHere(false); } Regards, Yugo Nagata -- Yugo NAGATA <nag...@sraoss.co.jp>