Thread: Is Recovery actually paused?
Hello, We have an interface to pause the WAL replay (pg_wal_replay_pause) and to know whether the WAL replay pause is requested (pg_is_wal_replay_paused). But there is no way to know whether the recovery is actually paused or not. Actually, the recovery process might process an extra WAL before pausing the recovery. So does it make sense to provide a new interface to tell whether the recovery is actually paused or not? One solution could be that we convert the XLogCtlData->recoveryPause from bool to tri-state variable (0-> recovery not paused 1-> pause requested 2-> actually paused). Any opinion on this? -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
On Mon, 19 Oct 2020 at 15:11, Dilip Kumar <dilipbalaut@gmail.com> wrote: > We have an interface to pause the WAL replay (pg_wal_replay_pause) and > to know whether the WAL replay pause is requested > (pg_is_wal_replay_paused). But there is no way to know whether the > recovery is actually paused or not. Actually, the recovery process > might process an extra WAL before pausing the recovery. So does it > make sense to provide a new interface to tell whether the recovery is > actually paused or not? > > One solution could be that we convert the XLogCtlData->recoveryPause > from bool to tri-state variable (0-> recovery not paused 1-> pause > requested 2-> actually paused). > > Any opinion on this? Why would we want this? What problem are you trying to solve? If we do care, why not fix pg_is_wal_replay_paused() so it responds as you wish? -- Simon Riggs http://www.EnterpriseDB.com/
On Tue, Oct 20, 2020 at 1:11 PM Simon Riggs <simon@2ndquadrant.com> wrote: > > On Mon, 19 Oct 2020 at 15:11, Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > We have an interface to pause the WAL replay (pg_wal_replay_pause) and > > to know whether the WAL replay pause is requested > > (pg_is_wal_replay_paused). But there is no way to know whether the > > recovery is actually paused or not. Actually, the recovery process > > might process an extra WAL before pausing the recovery. So does it > > make sense to provide a new interface to tell whether the recovery is > > actually paused or not? > > > > One solution could be that we convert the XLogCtlData->recoveryPause > > from bool to tri-state variable (0-> recovery not paused 1-> pause > > requested 2-> actually paused). > > > > Any opinion on this? > > Why would we want this? What problem are you trying to solve? The requirement is to know the last replayed WAL on the standby so unless we can guarantee that the recovery is actually paused we can never get the safe last_replay_lsn value. > If we do care, why not fix pg_is_wal_replay_paused() so it responds as you wish? Maybe we can also do that but pg_is_wal_replay_paused is an existing API and the behavior is to know whether the recovery paused is requested or not, So I am not sure is it a good idea to change the behavior of the existing API? -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
On Tue, Oct 20, 2020 at 1:22 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > On Tue, Oct 20, 2020 at 1:11 PM Simon Riggs <simon@2ndquadrant.com> wrote: > > > > On Mon, 19 Oct 2020 at 15:11, Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > > > We have an interface to pause the WAL replay (pg_wal_replay_pause) and > > > to know whether the WAL replay pause is requested > > > (pg_is_wal_replay_paused). But there is no way to know whether the > > > recovery is actually paused or not. Actually, the recovery process > > > might process an extra WAL before pausing the recovery. So does it > > > make sense to provide a new interface to tell whether the recovery is > > > actually paused or not? > > > > > > One solution could be that we convert the XLogCtlData->recoveryPause > > > from bool to tri-state variable (0-> recovery not paused 1-> pause > > > requested 2-> actually paused). > > > > > > Any opinion on this? > > > > Why would we want this? What problem are you trying to solve? > > The requirement is to know the last replayed WAL on the standby so > unless we can guarantee that the recovery is actually paused we can > never get the safe last_replay_lsn value. > > > If we do care, why not fix pg_is_wal_replay_paused() so it responds as you wish? > > Maybe we can also do that but pg_is_wal_replay_paused is an existing > API and the behavior is to know whether the recovery paused is > requested or not, So I am not sure is it a good idea to change the > behavior of the existing API? > Attached is the POC patch to show what I have in mind. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
Attachment
On Tue, 20 Oct 2020 at 09:50, Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > Why would we want this? What problem are you trying to solve? > > > > The requirement is to know the last replayed WAL on the standby so > > unless we can guarantee that the recovery is actually paused we can > > never get the safe last_replay_lsn value. > > > > > If we do care, why not fix pg_is_wal_replay_paused() so it responds as you wish? > > > > Maybe we can also do that but pg_is_wal_replay_paused is an existing > > API and the behavior is to know whether the recovery paused is > > requested or not, So I am not sure is it a good idea to change the > > behavior of the existing API? > > > > Attached is the POC patch to show what I have in mind. If you don't like it, I doubt anyone else cares for the exact current behavior either. Thanks for pointing those issues out. It would make sense to alter pg_wal_replay_pause() so that it blocks until paused. I suggest you add the 3-value state as you suggest, but make pg_is_wal_replay_paused() respond: if paused, true if requested, wait until paused, then return true else false That then solves your issues with a smoother interface. -- Simon Riggs http://www.EnterpriseDB.com/
On Tue, Oct 20, 2020 at 3:00 PM Simon Riggs <simon@2ndquadrant.com> wrote: > > On Tue, 20 Oct 2020 at 09:50, Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > > > Why would we want this? What problem are you trying to solve? > > > > > > The requirement is to know the last replayed WAL on the standby so > > > unless we can guarantee that the recovery is actually paused we can > > > never get the safe last_replay_lsn value. > > > > > > > If we do care, why not fix pg_is_wal_replay_paused() so it responds as you wish? > > > > > > Maybe we can also do that but pg_is_wal_replay_paused is an existing > > > API and the behavior is to know whether the recovery paused is > > > requested or not, So I am not sure is it a good idea to change the > > > behavior of the existing API? > > > > > > > Attached is the POC patch to show what I have in mind. > > If you don't like it, I doubt anyone else cares for the exact current > behavior either. Thanks for pointing those issues out. > > It would make sense to alter pg_wal_replay_pause() so that it blocks > until paused. > > I suggest you add the 3-value state as you suggest, but make > pg_is_wal_replay_paused() respond: > if paused, true > if requested, wait until paused, then return true > else false > > That then solves your issues with a smoother interface. > Make sense to me, I will change as per the suggestion. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
On Tue, Oct 20, 2020 at 5:59 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > On Tue, Oct 20, 2020 at 3:00 PM Simon Riggs <simon@2ndquadrant.com> wrote: > > > > On Tue, 20 Oct 2020 at 09:50, Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > > > > > Why would we want this? What problem are you trying to solve? > > > > > > > > The requirement is to know the last replayed WAL on the standby so > > > > unless we can guarantee that the recovery is actually paused we can > > > > never get the safe last_replay_lsn value. > > > > > > > > > If we do care, why not fix pg_is_wal_replay_paused() so it responds as you wish? > > > > > > > > Maybe we can also do that but pg_is_wal_replay_paused is an existing > > > > API and the behavior is to know whether the recovery paused is > > > > requested or not, So I am not sure is it a good idea to change the > > > > behavior of the existing API? > > > > > > > > > > Attached is the POC patch to show what I have in mind. > > > > If you don't like it, I doubt anyone else cares for the exact current > > behavior either. Thanks for pointing those issues out. > > > > It would make sense to alter pg_wal_replay_pause() so that it blocks > > until paused. > > > > I suggest you add the 3-value state as you suggest, but make > > pg_is_wal_replay_paused() respond: > > if paused, true > > if requested, wait until paused, then return true > > else false > > > > That then solves your issues with a smoother interface. > > > > Make sense to me, I will change as per the suggestion. I have noticed one more issue, the problem is that if the recovery process is currently not processing any WAL and just waiting for the WAL to become available then the pg_is_wal_replay_paused will be stuck forever. Having said that there is the same problem even if we design the new interface which checks whether the recovery is actually paused or not because until the recovery process gets the next wal it will not check whether the recovery pause is requested or not so the actual recovery paused flag will never be set. One idea could be, if the recovery process is waiting for WAL and a recovery pause is requested then we can assume that the recovery is paused because before processing the next wal it will always check whether the recovery pause is requested or not. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
On Wed, Oct 21, 2020 at 7:16 AM Dilip Kumar <dilipbalaut@gmail.com> wrote: > I have noticed one more issue, the problem is that if the recovery > process is currently not processing any WAL and just waiting for the > WAL to become available then the pg_is_wal_replay_paused will be stuck > forever. Having said that there is the same problem even if we design > the new interface which checks whether the recovery is actually paused > or not because until the recovery process gets the next wal it will > not check whether the recovery pause is requested or not so the actual > recovery paused flag will never be set. > > One idea could be, if the recovery process is waiting for WAL and a > recovery pause is requested then we can assume that the recovery is > paused because before processing the next wal it will always check > whether the recovery pause is requested or not. That seems fine, because the user's question is presumably whether the pause has taken effect so that no more records will be replayed barring an un-paused. However, it might be better to implement this by having the system absorb the pause immediately when it's in this state, rather than trying to detect this state and treat it specially. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Wed, 21 Oct 2020 at 12:16, Dilip Kumar <dilipbalaut@gmail.com> wrote: > > On Tue, Oct 20, 2020 at 5:59 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > > On Tue, Oct 20, 2020 at 3:00 PM Simon Riggs <simon@2ndquadrant.com> wrote: > > > > > > On Tue, 20 Oct 2020 at 09:50, Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > > > > > > > Why would we want this? What problem are you trying to solve? > > > > > > > > > > The requirement is to know the last replayed WAL on the standby so > > > > > unless we can guarantee that the recovery is actually paused we can > > > > > never get the safe last_replay_lsn value. > > > > > > > > > > > If we do care, why not fix pg_is_wal_replay_paused() so it responds as you wish? > > > > > > > > > > Maybe we can also do that but pg_is_wal_replay_paused is an existing > > > > > API and the behavior is to know whether the recovery paused is > > > > > requested or not, So I am not sure is it a good idea to change the > > > > > behavior of the existing API? > > > > > > > > > > > > > Attached is the POC patch to show what I have in mind. > > > > > > If you don't like it, I doubt anyone else cares for the exact current > > > behavior either. Thanks for pointing those issues out. > > > > > > It would make sense to alter pg_wal_replay_pause() so that it blocks > > > until paused. > > > > > > I suggest you add the 3-value state as you suggest, but make > > > pg_is_wal_replay_paused() respond: > > > if paused, true > > > if requested, wait until paused, then return true > > > else false > > > > > > That then solves your issues with a smoother interface. > > > > > > > Make sense to me, I will change as per the suggestion. > > I have noticed one more issue, the problem is that if the recovery > process is currently not processing any WAL and just waiting for the > WAL to become available then the pg_is_wal_replay_paused will be stuck > forever. Having said that there is the same problem even if we design > the new interface which checks whether the recovery is actually paused > or not because until the recovery process gets the next wal it will > not check whether the recovery pause is requested or not so the actual > recovery paused flag will never be set. > > One idea could be, if the recovery process is waiting for WAL and a > recovery pause is requested then we can assume that the recovery is > paused because before processing the next wal it will always check > whether the recovery pause is requested or not. If ReadRecord() is waiting for WAL (at bottom of recovery loop), then when it does return it will immediately move to pause (at top of next loop). Which makes it easy to cover these cases. It would be easy enough to create another variable that shows "waiting for WAL", since that is in itself a useful and interesting thing to be able to report. pg_is_wal_replay_paused() and pg_wal_replay_pause() would then return whenever it is either (fully paused || waiting for WAL && pause_requested)) We can then create a new function called pg_wal_replay_status() that returns multiple values: RECOVERING | WAITING_FOR_WAL | PAUSED -- Simon Riggs http://www.EnterpriseDB.com/
At Wed, 21 Oct 2020 11:14:24 -0400, Robert Haas <robertmhaas@gmail.com> wrote in > On Wed, Oct 21, 2020 at 7:16 AM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > One idea could be, if the recovery process is waiting for WAL and a > > recovery pause is requested then we can assume that the recovery is > > paused because before processing the next wal it will always check > > whether the recovery pause is requested or not. .. > However, it might be better to implement this by having the system > absorb the pause immediately when it's in this state, rather than > trying to detect this state and treat it specially. The paused state is shown in pg_stat_activity.wait_event and it is strange that pg_is_wal_replay_paused() is inconsistent with the column. To make them consistent, we need to call recoveryPausesHere() at the end of WaitForWALToBecomeAvailable() and let pg_wal_replay_pause() call WakeupRecovery(). I think we don't need a separate function to find the state. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
On Thu, Oct 22, 2020 at 6:59 AM Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote: > > At Wed, 21 Oct 2020 11:14:24 -0400, Robert Haas <robertmhaas@gmail.com> wrote in > > On Wed, Oct 21, 2020 at 7:16 AM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > One idea could be, if the recovery process is waiting for WAL and a > > > recovery pause is requested then we can assume that the recovery is > > > paused because before processing the next wal it will always check > > > whether the recovery pause is requested or not. > .. > > However, it might be better to implement this by having the system > > absorb the pause immediately when it's in this state, rather than > > trying to detect this state and treat it specially. > > The paused state is shown in pg_stat_activity.wait_event and it is > strange that pg_is_wal_replay_paused() is inconsistent with the > column. Right To make them consistent, we need to call recoveryPausesHere() > at the end of WaitForWALToBecomeAvailable() and let > pg_wal_replay_pause() call WakeupRecovery(). > > I think we don't need a separate function to find the state. The idea makes sense to me. I will try to change the patch as per the suggestion. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
On Thu, Oct 22, 2020 at 7:50 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > On Thu, Oct 22, 2020 at 6:59 AM Kyotaro Horiguchi > <horikyota.ntt@gmail.com> wrote: > > > > At Wed, 21 Oct 2020 11:14:24 -0400, Robert Haas <robertmhaas@gmail.com> wrote in > > > On Wed, Oct 21, 2020 at 7:16 AM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > > One idea could be, if the recovery process is waiting for WAL and a > > > > recovery pause is requested then we can assume that the recovery is > > > > paused because before processing the next wal it will always check > > > > whether the recovery pause is requested or not. > > .. > > > However, it might be better to implement this by having the system > > > absorb the pause immediately when it's in this state, rather than > > > trying to detect this state and treat it specially. > > > > The paused state is shown in pg_stat_activity.wait_event and it is > > strange that pg_is_wal_replay_paused() is inconsistent with the > > column. > > Right > > To make them consistent, we need to call recoveryPausesHere() > > at the end of WaitForWALToBecomeAvailable() and let > > pg_wal_replay_pause() call WakeupRecovery(). > > > > I think we don't need a separate function to find the state. > > The idea makes sense to me. I will try to change the patch as per the > suggestion. Here is the patch based on this idea. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
Attachment
On Thu, 22 Oct 2020 20:36:48 +0530 Dilip Kumar <dilipbalaut@gmail.com> wrote: > On Thu, Oct 22, 2020 at 7:50 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > > On Thu, Oct 22, 2020 at 6:59 AM Kyotaro Horiguchi > > <horikyota.ntt@gmail.com> wrote: > > > > > > At Wed, 21 Oct 2020 11:14:24 -0400, Robert Haas <robertmhaas@gmail.com> wrote in > > > > On Wed, Oct 21, 2020 at 7:16 AM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > > > One idea could be, if the recovery process is waiting for WAL and a > > > > > recovery pause is requested then we can assume that the recovery is > > > > > paused because before processing the next wal it will always check > > > > > whether the recovery pause is requested or not. > > > .. > > > > However, it might be better to implement this by having the system > > > > absorb the pause immediately when it's in this state, rather than > > > > trying to detect this state and treat it specially. > > > > > > The paused state is shown in pg_stat_activity.wait_event and it is > > > strange that pg_is_wal_replay_paused() is inconsistent with the > > > column. > > > > Right > > > > To make them consistent, we need to call recoveryPausesHere() > > > at the end of WaitForWALToBecomeAvailable() and let > > > pg_wal_replay_pause() call WakeupRecovery(). > > > > > > I think we don't need a separate function to find the state. > > > > The idea makes sense to me. I will try to change the patch as per the > > suggestion. > > Here is the patch based on this idea. I reviewd this patch. First, I made a recovery conflict situation using a table lock. Standby: #= begin; #= select * from t; Primary: #= begin; #= lock t in ; After this, WAL of the table lock cannot be replayed due to a lock acquired in the standby. Second, during the delay, I executed pg_wal_replay_pause() and pg_is_wal_replay_paused(). Then, pg_is_wal_replay_paused was blocked until max_standby_streaming_delay was expired, and eventually returned true. I can also see the same behaviour by setting recovery_min_apply_delay. So, pg_is_wal_replay_paused waits for recovery to get paused and this works successfully as expected. 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. 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. As another comment, while pg_is_wal_replay_paused is blocking, I can not cancel the query. I think CHECK_FOR_INTERRUPTS() is necessary in the waiting loop. + errhint("Recovery control functions can only be executed during recovery."))); There are a few tabs at the end of this line. Regards, Yugo Nagata -- Yugo NAGATA <nagata@sraoss.co.jp>
On Mon, Nov 30, 2020 at 12:17 PM Yugo NAGATA <nagata@sraoss.co.jp> wrote: Thanks for looking into this. > On Thu, 22 Oct 2020 20:36:48 +0530 > Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > On Thu, Oct 22, 2020 at 7:50 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > > > > On Thu, Oct 22, 2020 at 6:59 AM Kyotaro Horiguchi > > > <horikyota.ntt@gmail.com> wrote: > > > > > > > > At Wed, 21 Oct 2020 11:14:24 -0400, Robert Haas <robertmhaas@gmail.com> wrote in > > > > > On Wed, Oct 21, 2020 at 7:16 AM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > > > > One idea could be, if the recovery process is waiting for WAL and a > > > > > > recovery pause is requested then we can assume that the recovery is > > > > > > paused because before processing the next wal it will always check > > > > > > whether the recovery pause is requested or not. > > > > .. > > > > > However, it might be better to implement this by having the system > > > > > absorb the pause immediately when it's in this state, rather than > > > > > trying to detect this state and treat it specially. > > > > > > > > The paused state is shown in pg_stat_activity.wait_event and it is > > > > strange that pg_is_wal_replay_paused() is inconsistent with the > > > > column. > > > > > > Right > > > > > > To make them consistent, we need to call recoveryPausesHere() > > > > at the end of WaitForWALToBecomeAvailable() and let > > > > pg_wal_replay_pause() call WakeupRecovery(). > > > > > > > > I think we don't need a separate function to find the state. > > > > > > The idea makes sense to me. I will try to change the patch as per the > > > suggestion. > > > > Here is the patch based on this idea. > > I reviewd this patch. > > First, I made a recovery conflict situation using a table lock. > > Standby: > #= begin; > #= select * from t; > > Primary: > #= begin; > #= lock t in ; > > After this, WAL of the table lock cannot be replayed due to a lock acquired > in the standby. > > Second, during the delay, I executed pg_wal_replay_pause() and > pg_is_wal_replay_paused(). Then, pg_is_wal_replay_paused was blocked until > max_standby_streaming_delay was expired, and eventually returned true. > > I can also see the same behaviour by setting recovery_min_apply_delay. > > So, pg_is_wal_replay_paused waits for recovery to get paused and this works > successfully as expected. > > 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 > 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? > > As another comment, while pg_is_wal_replay_paused is blocking, I can not cancel > the query. I think CHECK_FOR_INTERRUPTS() is necessary in the waiting loop. > > > + errhint("Recovery control functions can only be executed during recovery."))); > > There are a few tabs at the end of this line. I will fix. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
On Mon, Nov 30, 2020 at 2:40 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > On Mon, Nov 30, 2020 at 12:17 PM Yugo NAGATA <nagata@sraoss.co.jp> wrote: > > Thanks for looking into this. > > > On Thu, 22 Oct 2020 20:36:48 +0530 > > Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > > > On Thu, Oct 22, 2020 at 7:50 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > > > > > > On Thu, Oct 22, 2020 at 6:59 AM Kyotaro Horiguchi > > > > <horikyota.ntt@gmail.com> wrote: > > > > > > > > > > At Wed, 21 Oct 2020 11:14:24 -0400, Robert Haas <robertmhaas@gmail.com> wrote in > > > > > > On Wed, Oct 21, 2020 at 7:16 AM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > > > > > One idea could be, if the recovery process is waiting for WAL and a > > > > > > > recovery pause is requested then we can assume that the recovery is > > > > > > > paused because before processing the next wal it will always check > > > > > > > whether the recovery pause is requested or not. > > > > > .. > > > > > > However, it might be better to implement this by having the system > > > > > > absorb the pause immediately when it's in this state, rather than > > > > > > trying to detect this state and treat it specially. > > > > > > > > > > The paused state is shown in pg_stat_activity.wait_event and it is > > > > > strange that pg_is_wal_replay_paused() is inconsistent with the > > > > > column. > > > > > > > > Right > > > > > > > > To make them consistent, we need to call recoveryPausesHere() > > > > > at the end of WaitForWALToBecomeAvailable() and let > > > > > pg_wal_replay_pause() call WakeupRecovery(). > > > > > > > > > > I think we don't need a separate function to find the state. > > > > > > > > The idea makes sense to me. I will try to change the patch as per the > > > > suggestion. > > > > > > Here is the patch based on this idea. > > > > I reviewd this patch. > > > > First, I made a recovery conflict situation using a table lock. > > > > Standby: > > #= begin; > > #= select * from t; > > > > Primary: > > #= begin; > > #= lock t in ; > > > > After this, WAL of the table lock cannot be replayed due to a lock acquired > > in the standby. > > > > Second, during the delay, I executed pg_wal_replay_pause() and > > pg_is_wal_replay_paused(). Then, pg_is_wal_replay_paused was blocked until > > max_standby_streaming_delay was expired, and eventually returned true. > > > > I can also see the same behaviour by setting recovery_min_apply_delay. > > > > So, pg_is_wal_replay_paused waits for recovery to get paused and this works > > successfully as expected. > > > > 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 > > 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? > > > > > As another comment, while pg_is_wal_replay_paused is blocking, I can not cancel > > the query. I think CHECK_FOR_INTERRUPTS() is necessary in the waiting loop. > > > > > > + errhint("Recovery control functions can only be executed during recovery."))); > > > > There are a few tabs at the end of this line. > > I will fix. Fixed this as well. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
Attachment
On Thu, 10 Dec 2020 11:25:23 +0530 Dilip Kumar <dilipbalaut@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? > > > 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. > > > As another comment, while pg_is_wal_replay_paused is blocking, I can not cancel > > > the query. I think CHECK_FOR_INTERRUPTS() is necessary in the waiting loop. How about this fix? I think users may want to cancel pg_is_wal_replay_paused() during this is blocking. Regards, Yugo Nagata -- Yugo NAGATA <nagata@sraoss.co.jp>
On Wed, Jan 13, 2021 at 3:27 PM Yugo NAGATA <nagata@sraoss.co.jp> wrote: > > On Thu, 10 Dec 2020 11:25:23 +0530 > Dilip Kumar <dilipbalaut@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. > > > > As another comment, while pg_is_wal_replay_paused is blocking, I can not cancel > > > > the query. I think CHECK_FOR_INTERRUPTS() is necessary in the waiting loop. > > How about this fix? I think users may want to cancel pg_is_wal_replay_paused() during > this is blocking. Yeah, we can do this. I will send the updated patch after putting some more thought into these comments. Thanks again for the feedback. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
On Wed, Jan 13, 2021 at 3:35 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > On Wed, Jan 13, 2021 at 3:27 PM Yugo NAGATA <nagata@sraoss.co.jp> wrote: > > > > On Thu, 10 Dec 2020 11:25:23 +0530 > > Dilip Kumar <dilipbalaut@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. > > > > > > As another comment, while pg_is_wal_replay_paused is blocking, I can not cancel > > > > > the query. I think CHECK_FOR_INTERRUPTS() is necessary in the waiting loop. > > > > How about this fix? I think users may want to cancel pg_is_wal_replay_paused() during > > this is blocking. > > Yeah, we can do this. I will send the updated patch after putting > some more thought into these comments. Thanks again for the feedback. > Please find the updated patch. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
Attachment
On Wed, 13 Jan 2021 17:49:43 +0530 Dilip Kumar <dilipbalaut@gmail.com> wrote: > On Wed, Jan 13, 2021 at 3:35 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > > On Wed, Jan 13, 2021 at 3:27 PM Yugo NAGATA <nagata@sraoss.co.jp> wrote: > > > > > > On Thu, 10 Dec 2020 11:25:23 +0530 > > > Dilip Kumar <dilipbalaut@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. 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(). > > > > > > As another comment, while pg_is_wal_replay_paused is blocking, I can not cancel > > > > > > the query. I think CHECK_FOR_INTERRUPTS() is necessary in the waiting loop. > > > > > > How about this fix? I think users may want to cancel pg_is_wal_replay_paused() during > > > this is blocking. > > > > Yeah, we can do this. I will send the updated patch after putting > > some more thought into these comments. Thanks again for the feedback. > > > > Please find the updated patch. Thanks. I confirmed that I can cancel pg_is_wal_repaly_paused() during stuck. Although it is a very trivial comment, I think that the new line before HandleStartupProcInterrupts() is unnecessary. @@ -6052,12 +6062,20 @@ recoveryPausesHere(bool endOfRecovery) (errmsg("recovery has paused"), errhint("Execute pg_wal_replay_resume() to continue."))); - while (RecoveryIsPaused()) + while (RecoveryPauseRequested()) { + HandleStartupProcInterrupts(); Regards, Yugo Nagata -- Yugo NAGATA <nagata@sraoss.co.jp>
On Wed, Jan 13, 2021 at 9:20 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > On Wed, Jan 13, 2021 at 3:35 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > > On Wed, Jan 13, 2021 at 3:27 PM Yugo NAGATA <nagata@sraoss.co.jp> wrote: > > > > > > On Thu, 10 Dec 2020 11:25:23 +0530 > > > Dilip Kumar <dilipbalaut@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. > > > > > > > > As another comment, while pg_is_wal_replay_paused is blocking, I can not cancel > > > > > > the query. I think CHECK_FOR_INTERRUPTS() is necessary in the waiting loop. > > > > > > How about this fix? I think users may want to cancel pg_is_wal_replay_paused() during > > > this is blocking. > > > > Yeah, we can do this. I will send the updated patch after putting > > some more thought into these comments. Thanks again for the feedback. > > > > Please find the updated patch. I've looked at the patch. Here are review comments: + /* Recovery pause state */ + RecoveryPauseState recoveryPause; Now that the value can have tri-state, how about renaming it to recoveryPauseState? --- bool RecoveryIsPaused(void) +{ + bool recoveryPause; + + SpinLockAcquire(&XLogCtl->info_lck); + recoveryPause = (XLogCtl->recoveryPause == RECOVERY_PAUSED) ? true : false; + SpinLockRelease(&XLogCtl->info_lck); + + return recoveryPause; +} + +bool +RecoveryPauseRequested(void) { bool recoveryPause; SpinLockAcquire(&XLogCtl->info_lck); - recoveryPause = XLogCtl->recoveryPause; + recoveryPause = (XLogCtl->recoveryPause != RECOVERY_IN_PROGRESS) ? true : false; SpinLockRelease(&XLogCtl->info_lck); return recoveryPause; } We can write like recoveryPause = (XLogCtl->recoveryPause == RECOVERY_PAUSED); Also, since these functions do the almost same thing, I think we can have a common function to get XLogCtl->recoveryPause, say GetRecoveryPauseState() or GetRecoveryPause(), and both RecoveryIsPaused() and RecoveryPauseRequested() use the returned value. What do you think? --- +static void +CheckAndSetRecoveryPause(void) Maybe we need to declare the prototype of this function like other functions in xlog.c. --- + /* + * If recovery is not in progress anymore then report an error this + * could happen if the standby is promoted while we were waiting for + * recovery to get paused. + */ + if (!RecoveryInProgress()) + ereport(ERROR, + (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), + errmsg("recovery is not in progress"), + errhint("Recovery control functions can only be executed during recovery."))); I think we can improve the error message so that we can tell users the standby has been promoted during the wait. For example, errmsg("the standby was promoted during waiting for recovery to be paused"))); --- + /* test for recovery pause if user has requested the pause */ + if (((volatile XLogCtlData *) XLogCtl)->recoveryPause) + recoveryPausesHere(false); + + now = GetCurrentTimestamp(); + Hmm, if the recovery pauses here, the wal receiver isn't launched even when wal_retrieve_retry_interval has passed, which seems not good. I think we want the recovery to be paused but want the wal receiver to continue receiving WAL. And why do we need to set 'now' here? --- /* * Wait until shared recoveryPause flag is cleared. * * endOfRecovery is true if the recovery target is reached and * the paused state starts at the end of recovery because of * recovery_target_action=pause, and false otherwise. * * XXX Could also be done with shared latch, avoiding the pg_usleep loop. * Probably not worth the trouble though. This state shouldn't be one that * anyone cares about server power consumption in. */ static void recoveryPausesHere(bool endOfRecovery) We can improve the first sentence in the above function comment to "Wait until shared recoveryPause is set to RECOVERY_IN_PROGRESS" or something. --- - PG_RETURN_BOOL(RecoveryIsPaused()); + if (!RecoveryPauseRequested()) + PG_RETURN_BOOL(false); + + /* loop until the recovery is actually paused */ + while(!RecoveryIsPaused()) + { + pg_usleep(10000L); /* wait for 10 msec */ + + /* meanwhile if recovery is resume requested then return false */ + if (!RecoveryPauseRequested()) + PG_RETURN_BOOL(false); + + CHECK_FOR_INTERRUPTS(); + + /* + * If recovery is not in progress anymore then report an error this + * could happen if the standby is promoted while we were waiting for + * recovery to get paused. + */ + if (!RecoveryInProgress()) + ereport(ERROR, + (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), + errmsg("recovery is not in progress"), + errhint("Recovery control functions can only be executed during recovery."))); + } + + PG_RETURN_BOOL(true); We have the same !RecoveryPauseRequested() check twice, how about the following arrangement? for (;;) { if (!RecoveryPauseRequested()) PG_RETURN_BOOL(false); if (RecoveryIsPaused()) break; pg_usleep(10000L); CHECK_FOR_INTERRUPTS(); if (!RecoveryInProgress()) ereport(...); } PG_RETURN_BOOL(true); Regards, -- Masahiko Sawada EnterpriseDB: https://www.enterprisedb.com/
On Sat, Jan 16, 2021 at 12:28 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote: > > --- > + /* test for recovery pause if user has requested the pause */ > + if (((volatile XLogCtlData *) XLogCtl)->recoveryPause) > + recoveryPausesHere(false); > + > + now = GetCurrentTimestamp(); > + > > Hmm, if the recovery pauses here, the wal receiver isn't launched even > when wal_retrieve_retry_interval has passed, which seems not good. I > think we want the recovery to be paused but want the wal receiver to > continue receiving WAL. I had misunderstood the code and the patch, please ignore this comment. Pausing the recovery here is fine with me. Regards, -- Masahiko Sawada EnterpriseDB: https://www.enterprisedb.com/
On Sun, Jan 17, 2021 at 3:52 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote: > > On Sat, Jan 16, 2021 at 12:28 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote: > > > > --- > > + /* test for recovery pause if user has requested the pause */ > > + if (((volatile XLogCtlData *) XLogCtl)->recoveryPause) > > + recoveryPausesHere(false); > > + > > + now = GetCurrentTimestamp(); > > + > > > > Hmm, if the recovery pauses here, the wal receiver isn't launched even > > when wal_retrieve_retry_interval has passed, which seems not good. I > > think we want the recovery to be paused but want the wal receiver to > > continue receiving WAL. > > I had misunderstood the code and the patch, please ignore this > comment. Pausing the recovery here is fine with me. Thanks for the review Sawada-San, I will work on your other comments and post the patch. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
On Thu, Jan 14, 2021 at 6:49 PM Yugo NAGATA <nagata@sraoss.co.jp> wrote: > > On Wed, 13 Jan 2021 17:49:43 +0530 > Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > On Wed, Jan 13, 2021 at 3:35 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > > > > On Wed, Jan 13, 2021 at 3:27 PM Yugo NAGATA <nagata@sraoss.co.jp> wrote: > > > > > > > > On Thu, 10 Dec 2020 11:25:23 +0530 > > > > Dilip Kumar <dilipbalaut@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. 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 > > > > > > > As another comment, while pg_is_wal_replay_paused is blocking, I can not cancel > > > > > > > the query. I think CHECK_FOR_INTERRUPTS() is necessary in the waiting loop. > > > > > > > > How about this fix? I think users may want to cancel pg_is_wal_replay_paused() during > > > > this is blocking. > > > > > > Yeah, we can do this. I will send the updated patch after putting > > > some more thought into these comments. Thanks again for the feedback. > > > > > > > Please find the updated patch. > > Thanks. I confirmed that I can cancel pg_is_wal_repaly_paused() during stuck. Thanks > Although it is a very trivial comment, I think that the new line before > HandleStartupProcInterrupts() is unnecessary. > > @@ -6052,12 +6062,20 @@ recoveryPausesHere(bool endOfRecovery) > (errmsg("recovery has paused"), > errhint("Execute pg_wal_replay_resume() to continue."))); > > - while (RecoveryIsPaused()) > + while (RecoveryPauseRequested()) > { > + > HandleStartupProcInterrupts(); > > I will fix in the next version. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
On Sat, Jan 16, 2021 at 8:59 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote: > > On Wed, Jan 13, 2021 at 9:20 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > > On Wed, Jan 13, 2021 at 3:35 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > > > > On Wed, Jan 13, 2021 at 3:27 PM Yugo NAGATA <nagata@sraoss.co.jp> wrote: > > > > > > > > On Thu, 10 Dec 2020 11:25:23 +0530 > > > > Dilip Kumar <dilipbalaut@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. > > > > > > > > > > As another comment, while pg_is_wal_replay_paused is blocking, I can not cancel > > > > > > > the query. I think CHECK_FOR_INTERRUPTS() is necessary in the waiting loop. > > > > > > > > How about this fix? I think users may want to cancel pg_is_wal_replay_paused() during > > > > this is blocking. > > > > > > Yeah, we can do this. I will send the updated patch after putting > > > some more thought into these comments. Thanks again for the feedback. > > > > > > > Please find the updated patch. > > I've looked at the patch. Here are review comments: > > + /* Recovery pause state */ > + RecoveryPauseState recoveryPause; > > Now that the value can have tri-state, how about renaming it to > recoveryPauseState? This makes sense to me. > --- > bool > RecoveryIsPaused(void) > +{ > + bool recoveryPause; > + > + SpinLockAcquire(&XLogCtl->info_lck); > + recoveryPause = (XLogCtl->recoveryPause == RECOVERY_PAUSED) ? > true : false; > + SpinLockRelease(&XLogCtl->info_lck); > + > + return recoveryPause; > +} > + > +bool > +RecoveryPauseRequested(void) > { > bool recoveryPause; > > SpinLockAcquire(&XLogCtl->info_lck); > - recoveryPause = XLogCtl->recoveryPause; > + recoveryPause = (XLogCtl->recoveryPause != > RECOVERY_IN_PROGRESS) ? true : false; > SpinLockRelease(&XLogCtl->info_lck); > > return recoveryPause; > } > > We can write like recoveryPause = (XLogCtl->recoveryPause == RECOVERY_PAUSED); In RecoveryPauseRequested, we just want to know whether the pause is requested or not, even if the pause requested and not yet pause then also we want to return true. So how recoveryPause = (XLogCtl->recoveryPause == RECOVERY_PAUSED) will work? > Also, since these functions do the almost same thing, I think we can > have a common function to get XLogCtl->recoveryPause, say > GetRecoveryPauseState() or GetRecoveryPause(), and both > RecoveryIsPaused() and RecoveryPauseRequested() use the returned > value. What do you think? Yeah we can do that. > --- > +static void > +CheckAndSetRecoveryPause(void) > > Maybe we need to declare the prototype of this function like other > functions in xlog.c. Okay > --- > + /* > + * If recovery is not in progress anymore then report an error this > + * could happen if the standby is promoted while we were waiting for > + * recovery to get paused. > + */ > + if (!RecoveryInProgress()) > + ereport(ERROR, > + (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), > + errmsg("recovery is not in progress"), > + errhint("Recovery control functions can only be > executed during recovery."))); > > I think we can improve the error message so that we can tell users the > standby has been promoted during the wait. For example, > > errmsg("the standby was promoted during waiting for > recovery to be paused"))); > > --- > + /* test for recovery pause if user has requested the pause */ > + if (((volatile XLogCtlData *) XLogCtl)->recoveryPause) > + recoveryPausesHere(false); > + > + now = GetCurrentTimestamp(); > + > > Hmm, if the recovery pauses here, the wal receiver isn't launched even > when wal_retrieve_retry_interval has passed, which seems not good. I > think we want the recovery to be paused but want the wal receiver to > continue receiving WAL. > > And why do we need to set 'now' here? > > --- > /* > * Wait until shared recoveryPause flag is cleared. > * > * endOfRecovery is true if the recovery target is reached and > * the paused state starts at the end of recovery because of > * recovery_target_action=pause, and false otherwise. > * > * XXX Could also be done with shared latch, avoiding the pg_usleep loop. > * Probably not worth the trouble though. This state shouldn't be one that > * anyone cares about server power consumption in. > */ > static void > recoveryPausesHere(bool endOfRecovery) > > We can improve the first sentence in the above function comment to > "Wait until shared recoveryPause is set to RECOVERY_IN_PROGRESS" or > something. > > --- > - PG_RETURN_BOOL(RecoveryIsPaused()); > + if (!RecoveryPauseRequested()) > + PG_RETURN_BOOL(false); > + > + /* loop until the recovery is actually paused */ > + while(!RecoveryIsPaused()) > + { > + pg_usleep(10000L); /* wait for 10 msec */ > + > + /* meanwhile if recovery is resume requested then return false */ > + if (!RecoveryPauseRequested()) > + PG_RETURN_BOOL(false); > + > + CHECK_FOR_INTERRUPTS(); > + > + /* > + * If recovery is not in progress anymore then report an error this > + * could happen if the standby is promoted while we were waiting for > + * recovery to get paused. > + */ > + if (!RecoveryInProgress()) > + ereport(ERROR, > + (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), > + errmsg("recovery is not in progress"), > + errhint("Recovery control functions can only be > executed during recovery."))); > + } > + > + PG_RETURN_BOOL(true); > > We have the same !RecoveryPauseRequested() check twice, how about the > following arrangement? > > for (;;) > { > if (!RecoveryPauseRequested()) > PG_RETURN_BOOL(false); > > if (RecoveryIsPaused()) > break; > > pg_usleep(10000L); > > CHECK_FOR_INTERRUPTS(); > > if (!RecoveryInProgress()) > ereport(...); > } > > PG_RETURN_BOOL(true); > > Regards, > Okay, we can do that. I will make these changes in the next patch. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
On Sun, Jan 17, 2021 at 1:48 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > On Sat, Jan 16, 2021 at 8:59 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote: > > > > On Wed, Jan 13, 2021 at 9:20 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > > > > On Wed, Jan 13, 2021 at 3:35 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > > > > > > On Wed, Jan 13, 2021 at 3:27 PM Yugo NAGATA <nagata@sraoss.co.jp> wrote: > > > > > > > > > > On Thu, 10 Dec 2020 11:25:23 +0530 > > > > > Dilip Kumar <dilipbalaut@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. > > > > > > > > > > > > As another comment, while pg_is_wal_replay_paused is blocking, I can not cancel > > > > > > > > the query. I think CHECK_FOR_INTERRUPTS() is necessary in the waiting loop. > > > > > > > > > > How about this fix? I think users may want to cancel pg_is_wal_replay_paused() during > > > > > this is blocking. > > > > > > > > Yeah, we can do this. I will send the updated patch after putting > > > > some more thought into these comments. Thanks again for the feedback. > > > > > > > > > > Please find the updated patch. > > > > I've looked at the patch. Here are review comments: > > > > + /* Recovery pause state */ > > + RecoveryPauseState recoveryPause; > > > > Now that the value can have tri-state, how about renaming it to > > recoveryPauseState? > > This makes sense to me. > > > --- > > bool > > RecoveryIsPaused(void) > > +{ > > + bool recoveryPause; > > + > > + SpinLockAcquire(&XLogCtl->info_lck); > > + recoveryPause = (XLogCtl->recoveryPause == RECOVERY_PAUSED) ? > > true : false; > > + SpinLockRelease(&XLogCtl->info_lck); > > + > > + return recoveryPause; > > +} > > + > > +bool > > +RecoveryPauseRequested(void) > > { > > bool recoveryPause; > > > > SpinLockAcquire(&XLogCtl->info_lck); > > - recoveryPause = XLogCtl->recoveryPause; > > + recoveryPause = (XLogCtl->recoveryPause != > > RECOVERY_IN_PROGRESS) ? true : false; > > SpinLockRelease(&XLogCtl->info_lck); > > > > return recoveryPause; > > } > > > > We can write like recoveryPause = (XLogCtl->recoveryPause == RECOVERY_PAUSED); > > In RecoveryPauseRequested, we just want to know whether the pause is > requested or not, even if the pause requested and not yet pause then > also we want to return true. So how > recoveryPause = (XLogCtl->recoveryPause == RECOVERY_PAUSED) will work? > > > Also, since these functions do the almost same thing, I think we can > > have a common function to get XLogCtl->recoveryPause, say > > GetRecoveryPauseState() or GetRecoveryPause(), and both > > RecoveryIsPaused() and RecoveryPauseRequested() use the returned > > value. What do you think? > > Yeah we can do that. > > > --- > > +static void > > +CheckAndSetRecoveryPause(void) > > > > Maybe we need to declare the prototype of this function like other > > functions in xlog.c. > > Okay > > > --- > > + /* > > + * If recovery is not in progress anymore then report an error this > > + * could happen if the standby is promoted while we were waiting for > > + * recovery to get paused. > > + */ > > + if (!RecoveryInProgress()) > > + ereport(ERROR, > > + (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), > > + errmsg("recovery is not in progress"), > > + errhint("Recovery control functions can only be > > executed during recovery."))); > > > > I think we can improve the error message so that we can tell users the > > standby has been promoted during the wait. For example, > > > > errmsg("the standby was promoted during waiting for > > recovery to be paused"))); > > > > --- > > + /* test for recovery pause if user has requested the pause */ > > + if (((volatile XLogCtlData *) XLogCtl)->recoveryPause) > > + recoveryPausesHere(false); > > + > > + now = GetCurrentTimestamp(); > > + > > > > Hmm, if the recovery pauses here, the wal receiver isn't launched even > > when wal_retrieve_retry_interval has passed, which seems not good. I > > think we want the recovery to be paused but want the wal receiver to > > continue receiving WAL. > > > > And why do we need to set 'now' here? > > > > --- > > /* > > * Wait until shared recoveryPause flag is cleared. > > * > > * endOfRecovery is true if the recovery target is reached and > > * the paused state starts at the end of recovery because of > > * recovery_target_action=pause, and false otherwise. > > * > > * XXX Could also be done with shared latch, avoiding the pg_usleep loop. > > * Probably not worth the trouble though. This state shouldn't be one that > > * anyone cares about server power consumption in. > > */ > > static void > > recoveryPausesHere(bool endOfRecovery) > > > > We can improve the first sentence in the above function comment to > > "Wait until shared recoveryPause is set to RECOVERY_IN_PROGRESS" or > > something. > > > > --- > > - PG_RETURN_BOOL(RecoveryIsPaused()); > > + if (!RecoveryPauseRequested()) > > + PG_RETURN_BOOL(false); > > + > > + /* loop until the recovery is actually paused */ > > + while(!RecoveryIsPaused()) > > + { > > + pg_usleep(10000L); /* wait for 10 msec */ > > + > > + /* meanwhile if recovery is resume requested then return false */ > > + if (!RecoveryPauseRequested()) > > + PG_RETURN_BOOL(false); > > + > > + CHECK_FOR_INTERRUPTS(); > > + > > + /* > > + * If recovery is not in progress anymore then report an error this > > + * could happen if the standby is promoted while we were waiting for > > + * recovery to get paused. > > + */ > > + if (!RecoveryInProgress()) > > + ereport(ERROR, > > + (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), > > + errmsg("recovery is not in progress"), > > + errhint("Recovery control functions can only be > > executed during recovery."))); > > + } > > + > > + PG_RETURN_BOOL(true); > > > > We have the same !RecoveryPauseRequested() check twice, how about the > > following arrangement? > > > > for (;;) > > { > > if (!RecoveryPauseRequested()) > > PG_RETURN_BOOL(false); > > > > if (RecoveryIsPaused()) > > break; > > > > pg_usleep(10000L); > > > > CHECK_FOR_INTERRUPTS(); > > > > if (!RecoveryInProgress()) > > ereport(...); > > } > > > > PG_RETURN_BOOL(true); > > > > Regards, > > > > Okay, we can do that. I will make these changes in the next patch. > I have fixed the above agreed comments. Please have a look. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
Attachment
On Sun, 17 Jan 2021 11:33:52 +0530 Dilip Kumar <dilipbalaut@gmail.com> wrote: > On Thu, Jan 14, 2021 at 6:49 PM Yugo NAGATA <nagata@sraoss.co.jp> wrote: > > > > On Wed, 13 Jan 2021 17:49:43 +0530 > > Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > > > On Wed, Jan 13, 2021 at 3:35 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > > > > > > On Wed, Jan 13, 2021 at 3:27 PM Yugo NAGATA <nagata@sraoss.co.jp> wrote: > > > > > > > > > > On Thu, 10 Dec 2020 11:25:23 +0530 > > > > > Dilip Kumar <dilipbalaut@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? > > > > > > > > > As another comment, while pg_is_wal_replay_paused is blocking, I can not cancel > > > > > > > > the query. I think CHECK_FOR_INTERRUPTS() is necessary in the waiting loop. > > > > > > > > > > How about this fix? I think users may want to cancel pg_is_wal_replay_paused() during > > > > > this is blocking. > > > > > > > > Yeah, we can do this. I will send the updated patch after putting > > > > some more thought into these comments. Thanks again for the feedback. > > > > > > > > > > Please find the updated patch. > > > > Thanks. I confirmed that I can cancel pg_is_wal_repaly_paused() during stuck. > > Thanks > > > Although it is a very trivial comment, I think that the new line before > > HandleStartupProcInterrupts() is unnecessary. > > > > @@ -6052,12 +6062,20 @@ recoveryPausesHere(bool endOfRecovery) > > (errmsg("recovery has paused"), > > errhint("Execute pg_wal_replay_resume() to continue."))); > > > > - while (RecoveryIsPaused()) > > + while (RecoveryPauseRequested()) > > { > > + > > HandleStartupProcInterrupts(); > > > > > > I will fix in the next version. > > -- > Regards, > Dilip Kumar > EnterpriseDB: http://www.enterprisedb.com -- Yugo NAGATA <nagata@sraoss.co.jp>
On Tue, 19 Jan 2021 at 8:12 AM, Yugo NAGATA <nagata@sraoss.co.jp> wrote:
On Sun, 17 Jan 2021 11:33:52 +0530
Dilip Kumar <dilipbalaut@gmail.com> wrote:
> On Thu, Jan 14, 2021 at 6:49 PM Yugo NAGATA <nagata@sraoss.co.jp> wrote:
> >
> > On Wed, 13 Jan 2021 17:49:43 +0530
> > Dilip Kumar <dilipbalaut@gmail.com> wrote:
> >
> > > On Wed, Jan 13, 2021 at 3:35 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> > > >
> > > > On Wed, Jan 13, 2021 at 3:27 PM Yugo NAGATA <nagata@sraoss.co.jp> wrote:
> > > > >
> > > > > On Thu, 10 Dec 2020 11:25:23 +0530
> > > > > Dilip Kumar <dilipbalaut@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
At Tue, 19 Jan 2021 11:41:18 +0900, Yugo NAGATA <nagata@sraoss.co.jp> wrote in > On Sun, 17 Jan 2021 11:33:52 +0530 > Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > > > > > > > > > > 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(). FWIW, the name "pg_is_wal_replay_paused" is suggesting "to know whether recovery is paused or not at present" and it would be surprising to see it to wait for the recovery actually paused by default. I think there's no functions to wait for some situation at least for now. If we wanted to wait for some condition to make, we would loop over check-and-wait using plpgsql. If you desire to wait to replication to pause by a function, I would do that by adding a parameter to the function. pg_is_wal_replay_paused(OPTIONAL bool wait_for_pause) regards. -- Kyotaro Horiguchi NTT Open Source Software Center
On Tue, Jan 19, 2021 at 10:30 AM Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote: > > At Tue, 19 Jan 2021 11:41:18 +0900, Yugo NAGATA <nagata@sraoss.co.jp> wrote in > > On Sun, 17 Jan 2021 11:33:52 +0530 > > Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > > > > > > > > > > > > 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(). > > FWIW, the name "pg_is_wal_replay_paused" is suggesting "to know > whether recovery is paused or not at present" and it would be > surprising to see it to wait for the recovery actually paused by > default. > > I think there's no functions to wait for some situation at least for > now. If we wanted to wait for some condition to make, we would loop > over check-and-wait using plpgsql. > > If you desire to wait to replication to pause by a function, I would > do that by adding a parameter to the function. > > pg_is_wal_replay_paused(OPTIONAL bool wait_for_pause) This seems to be a fair point to me. So I will add an option to the API, and if that is passed true then we will wait for recovery to get paused. otherwise, this will just return true if the pause is requested same as the current behavior. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
On Tue, Jan 19, 2021 at 8:34 AM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > On Tue, 19 Jan 2021 at 8:12 AM, Yugo NAGATA <nagata@sraoss.co.jp> wrote: >> >> On Sun, 17 Jan 2021 11:33:52 +0530 >> Dilip Kumar <dilipbalaut@gmail.com> wrote: >> >> > On Thu, Jan 14, 2021 at 6:49 PM Yugo NAGATA <nagata@sraoss.co.jp> wrote: >> > > >> > > On Wed, 13 Jan 2021 17:49:43 +0530 >> > > Dilip Kumar <dilipbalaut@gmail.com> wrote: >> > > >> > > > On Wed, Jan 13, 2021 at 3:35 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: >> > > > > >> > > > > On Wed, Jan 13, 2021 at 3:27 PM Yugo NAGATA <nagata@sraoss.co.jp> wrote: >> > > > > > >> > > > > > On Thu, 10 Dec 2020 11:25:23 +0530 >> > > > > > Dilip Kumar <dilipbalaut@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. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
Attachment
On Tue, Jan 19, 2021 at 9:32 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: > 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. Thanks for the patch. I took a look at the v5 patch, below are some comments. Please ignore if I'm repeating any of the comments discussed upthread. [1] Can we also have a wait version for pg_wal_replay_pause that waits until recovery is actually paused right after setting the recovery state to RECOVERY_PAUSE_REQUESTED? Something like this - pg_wal_replay_pause_and_wait(wait boolean, wait_seconds integer DEFAULT 60) returns boolean. It waits until either default or provided wait_seconds and returns true if the recovery is paused within that wait_seconds otherwise false. If wait_seconds is 0 or -1, then it waits until recovery is paused and returns true. One advantage of this function is that users don't need to call pg_is_wal_replay_paused(). IMHO, the job of ensuring whether or not the recovery is actually paused, is better done by the one who requests it(pg_wal_replay_pause/pg_wal_replay_pause_and_wait). [2] Is it intentional that RecoveryPauseRequested() returns true even if XLogCtl->recoveryPauseState is either RECOVERY_PAUSE_REQUESTED or RECOVERY_PAUSED? [3] Can we change IsRecoveryPaused() instead of RecoveryIsPaused() and IsRecoveryPauseRequested() instead of RecoveryPauseRequested()? How about having inline(because they have one line of code) functions like IsRecoveryPauseRequested(), IsRecoveryPaused() and IsRecoveryInProgress(), returning true when RECOVERY_PAUSE_REQUESTED, RECOVERY_PAUSED and RECOVERY_IN_PROGRESS respectively? [4] Can we have at least one line of comments for each of the new functions, I know the function names mean everything? And also for existing SetRecoveryPause() and GetRecoveryPauseState()? [5] Typo, it's "every time" ---> + * this everytime. [6] Do we reach PG_RETURN_BOOL(true); at the end of pg_is_wal_replay_paused()? If not, is it there for satisfying the compiler? [7] In pg_is_wal_replay_paused(), do we need if (!RecoveryPauseRequested()) inside the for (;;)? If yes, can we add comments about why we need it there? [8] Isn't it good to have pgstat_report_wait_start(WAIT_EVENT_RECOVERY_PAUSE) and pgstat_report_wait_end(), just before for (;;) and at the end respectively? This will help users in knowing what they are waiting for? Alternatively we can issue notice/warnings/write to server log while we are waiting in for (;;) for the recovery to get paused? [9] In pg_is_wal_replay_paused(), is 10 msec sleep time chosen randomly or based on some analysis that the time it takes to get to recovery paused state from request state or some other? [10] errhint("The standby was promoted while waiting for recovery to be paused."))); Can we know whether standby is actually promoted and throw this error? Because the error "recovery is not in progress" here is being thrown by just relying on if (!RecoveryInProgress()). IIUC, using pg_promote [11] Can we try to add tests for these functions in TAP? Currently, we don't have any tests for pg_is_wal_replay_paused, pg_wal_replay_resume or pg_wal_replay_pause, but we have tests for pg_promote in timeline switch. [12] Isn't it better to change RecoveryPauseState enum RECOVERY_IN_PROGRESS value to start from 1 instead of 0? Because when XLogCtl shared memory is initialized, I think recoveryPauseState can be 0, so should it mean recovery in progress? With Regards, Bharath Rupireddy. EnterpriseDB: http://www.enterprisedb.com
On Thu, Jan 21, 2021 at 3:29 PM Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote: Thanks for reviewing Bharat. > On Tue, Jan 19, 2021 at 9:32 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > 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. > > Thanks for the patch. I took a look at the v5 patch, below are some > comments. Please ignore if I'm repeating any of the comments discussed > upthread. > > [1] Can we also have a wait version for pg_wal_replay_pause that waits > until recovery is actually paused right after setting the recovery > state to RECOVERY_PAUSE_REQUESTED? Something like this - > pg_wal_replay_pause_and_wait(wait boolean, wait_seconds integer > DEFAULT 60) returns boolean. It waits until either default or provided > wait_seconds and returns true if the recovery is paused within that > wait_seconds otherwise false. If wait_seconds is 0 or -1, then it > waits until recovery is paused and returns true. One advantage of this > function is that users don't need to call pg_is_wal_replay_paused(). > IMHO, the job of ensuring whether or not the recovery is actually > paused, is better done by the one who requests > it(pg_wal_replay_pause/pg_wal_replay_pause_and_wait). I don't think we need wait/onwait version for all the APIs, IMHO it would be enough for the user to know whether the recovery is actually paused or not and for that, we are changing pg_is_wal_replay_paused to wait for the pause. However, in the next version in pg_is_wal_replay_paused I will provide a flag so that the user can decide whether to wait for the pause or just get the request status. > [2] Is it intentional that RecoveryPauseRequested() returns true even > if XLogCtl->recoveryPauseState is either RECOVERY_PAUSE_REQUESTED or > RECOVERY_PAUSED? Yes this is intended > [3] Can we change IsRecoveryPaused() instead of RecoveryIsPaused() and > IsRecoveryPauseRequested() instead of RecoveryPauseRequested()? How > about having inline(because they have one line of code) functions like > IsRecoveryPauseRequested(), IsRecoveryPaused() and > IsRecoveryInProgress(), returning true when RECOVERY_PAUSE_REQUESTED, > RECOVERY_PAUSED and RECOVERY_IN_PROGRESS respectively? Yeah, we can do that, I am not sure whether we need IsRecoveryInProgress function though. > [4] Can we have at least one line of comments for each of the new > functions, I know the function names mean everything? And also for > existing SetRecoveryPause() and GetRecoveryPauseState()? Will do that > [5] Typo, it's "every time" ---> + * this everytime. Ok > [6] Do we reach PG_RETURN_BOOL(true); at the end of > pg_is_wal_replay_paused()? If not, is it there for satisfying the > compiler? Yes > [7] In pg_is_wal_replay_paused(), do we need if > (!RecoveryPauseRequested()) inside the for (;;)? If yes, can we add > comments about why we need it there? Yes, we need it if replay resumed during the loop, I will add the comments. > [8] Isn't it good to have > pgstat_report_wait_start(WAIT_EVENT_RECOVERY_PAUSE) and > pgstat_report_wait_end(), just before for (;;) and at the end > respectively? This will help users in knowing what they are waiting > for? Alternatively we can issue notice/warnings/write to server log > while we are waiting in for (;;) for the recovery to get paused? I think we can do that, let me think about this and get back to you. > [9] In pg_is_wal_replay_paused(), is 10 msec sleep time chosen > randomly or based on some analysis that the time it takes to get to > recovery paused state from request state or some other? I don't think we can identify that when actually recovery can get paused. Though in pg_wal_replay_pause() we are sending a signal to all the places we are waiting for WAL and right after we are checking. But it depends upon other configuration parameters like max_standby_streaming_delay. > [10] errhint("The standby was promoted while waiting for recovery to > be paused."))); Can we know whether standby is actually promoted and > throw this error? Because the error "recovery is not in progress" here > is being thrown by just relying on if (!RecoveryInProgress()). IIUC, > using pg_promote Because before checking this it has already checked RecoveryPauseRequested() and if that is true then the RecoveryInProgress was in progress at some time and now it is not anymore and that can happen due to promotion. But I am fine with reverting to the old error that can not execute if recovery is not in progress. > [11] Can we try to add tests for these functions in TAP? Currently, we > don't have any tests for pg_is_wal_replay_paused, pg_wal_replay_resume > or pg_wal_replay_pause, but we have tests for pg_promote in timeline > switch. I will work on this. > [12] Isn't it better to change RecoveryPauseState enum > RECOVERY_IN_PROGRESS value to start from 1 instead of 0? Because when > XLogCtl shared memory is initialized, I think recoveryPauseState can > be 0, so should it mean recovery in progress? I think this state is to track the pause status, either pause requested or actually pause, we don't really want to trace the RECOVERY_IN_PROGRESS. Maybe we can change the name of that status to RECOVERY_PAUSE_NOT_REQUESTED or RECOVERY_PAUSE_NONE? -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
On Tue, 19 Jan 2021 21:32:31 +0530 Dilip Kumar <dilipbalaut@gmail.com> wrote: > On Tue, Jan 19, 2021 at 8:34 AM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > > On Tue, 19 Jan 2021 at 8:12 AM, Yugo NAGATA <nagata@sraoss.co.jp> wrote: > >> > >> On Sun, 17 Jan 2021 11:33:52 +0530 > >> Dilip Kumar <dilipbalaut@gmail.com> wrote: > >> > >> > On Thu, Jan 14, 2021 at 6:49 PM Yugo NAGATA <nagata@sraoss.co.jp> wrote: > >> > > > >> > > On Wed, 13 Jan 2021 17:49:43 +0530 > >> > > Dilip Kumar <dilipbalaut@gmail.com> wrote: > >> > > > >> > > > On Wed, Jan 13, 2021 at 3:35 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: > >> > > > > > >> > > > > On Wed, Jan 13, 2021 at 3:27 PM Yugo NAGATA <nagata@sraoss.co.jp> wrote: > >> > > > > > > >> > > > > > On Thu, 10 Dec 2020 11:25:23 +0530 > >> > > > > > Dilip Kumar <dilipbalaut@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 <nagata@sraoss.co.jp>
On Mon, Jan 18, 2021 at 9:42 PM Yugo NAGATA <nagata@sraoss.co.jp> wrote: > 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(). Another idea could be that pg_is_wal_replay_paused() could be changed to text, and the string could be either 'paused' or 'pause requested' or 'not paused'. That way we'd be returning a direct representation of the state we're keeping in memory. Some of the complexity in this discussion seems to come from trying to squeeze 3 possibilities into a Boolean. Let's also consider that we don't really know whether the client wants us to wait or not, and different clients may want different things, or maybe not, but we don't really know at this point. If we provide an interface that waits, and the client doesn't want to wait but just know the current state, they don't necessarily have any great options. If we provide an interface that doesn't wait, and the client wants to wait, it can poll until it gets the answer it wants. Polling can be inefficient, but anybody who is writing a tool that uses this should be able to manage an algorithm with some reasonable back-off behavior (e.g. try after 10ms, 20ms, keep doubling, max of 5s, or something of that sort), so I'm not sure there's actually any real problem in practice. So to me it seems more likely that an interface that is based on waiting will cause difficulty for tool-writers than one that does not. Other people may feel differently, of course... -- Robert Haas EDB: http://www.enterprisedb.com
On Fri, Jan 22, 2021 at 2:18 AM Robert Haas <robertmhaas@gmail.com> wrote: > > On Mon, Jan 18, 2021 at 9:42 PM Yugo NAGATA <nagata@sraoss.co.jp> wrote: > > 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(). > > Another idea could be that pg_is_wal_replay_paused() could be changed > to text, and the string could be either 'paused' or 'pause requested' > or 'not paused'. That way we'd be returning a direct representation of > the state we're keeping in memory. Some of the complexity in this > discussion seems to come from trying to squeeze 3 possibilities into a > Boolean. > > Let's also consider that we don't really know whether the client wants > us to wait or not, and different clients may want different things, or > maybe not, but we don't really know at this point. If we provide an > interface that waits, and the client doesn't want to wait but just > know the current state, they don't necessarily have any great options. > If we provide an interface that doesn't wait, and the client wants to > wait, it can poll until it gets the answer it wants. Polling can be > inefficient, but anybody who is writing a tool that uses this should > be able to manage an algorithm with some reasonable back-off behavior > (e.g. try after 10ms, 20ms, keep doubling, max of 5s, or something of > that sort), so I'm not sure there's actually any real problem in > practice. So to me it seems more likely that an interface that is > based on waiting will cause difficulty for tool-writers than one that > does not. > > Other people may feel differently, of course... I think this is the better way of handling this. So +1 from my side, I will send an updated patch. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
On Thu, Jan 21, 2021 at 6:20 PM Yugo NAGATA <nagata@sraoss.co.jp> wrote: > > On Tue, 19 Jan 2021 21:32:31 +0530 > Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > On Tue, Jan 19, 2021 at 8:34 AM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > > > > On Tue, 19 Jan 2021 at 8:12 AM, Yugo NAGATA <nagata@sraoss.co.jp> wrote: > > >> > > >> On Sun, 17 Jan 2021 11:33:52 +0530 > > >> Dilip Kumar <dilipbalaut@gmail.com> wrote: > > >> > > >> > On Thu, Jan 14, 2021 at 6:49 PM Yugo NAGATA <nagata@sraoss.co.jp> wrote: > > >> > > > > >> > > On Wed, 13 Jan 2021 17:49:43 +0530 > > >> > > Dilip Kumar <dilipbalaut@gmail.com> wrote: > > >> > > > > >> > > > On Wed, Jan 13, 2021 at 3:35 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > >> > > > > > > >> > > > > On Wed, Jan 13, 2021 at 3:27 PM Yugo NAGATA <nagata@sraoss.co.jp> wrote: > > >> > > > > > > > >> > > > > > On Thu, 10 Dec 2020 11:25:23 +0530 > > >> > > > > > Dilip Kumar <dilipbalaut@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); > } Yeah, a valid point. Thanks! -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
On Sat, Jan 23, 2021 at 9:56 AM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > On Fri, Jan 22, 2021 at 2:18 AM Robert Haas <robertmhaas@gmail.com> wrote: > > > > On Mon, Jan 18, 2021 at 9:42 PM Yugo NAGATA <nagata@sraoss.co.jp> wrote: > > > 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(). > > > > Another idea could be that pg_is_wal_replay_paused() could be changed > > to text, and the string could be either 'paused' or 'pause requested' > > or 'not paused'. That way we'd be returning a direct representation of > > the state we're keeping in memory. Some of the complexity in this > > discussion seems to come from trying to squeeze 3 possibilities into a > > Boolean. > > > > Let's also consider that we don't really know whether the client wants > > us to wait or not, and different clients may want different things, or > > maybe not, but we don't really know at this point. If we provide an > > interface that waits, and the client doesn't want to wait but just > > know the current state, they don't necessarily have any great options. > > If we provide an interface that doesn't wait, and the client wants to > > wait, it can poll until it gets the answer it wants. Polling can be > > inefficient, but anybody who is writing a tool that uses this should > > be able to manage an algorithm with some reasonable back-off behavior > > (e.g. try after 10ms, 20ms, keep doubling, max of 5s, or something of > > that sort), so I'm not sure there's actually any real problem in > > practice. So to me it seems more likely that an interface that is > > based on waiting will cause difficulty for tool-writers than one that > > does not. > > > > Other people may feel differently, of course... > > I think this is the better way of handling this. So +1 from my side, > I will send an updated patch. 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'. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
Attachment
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
On Sat, 23 Jan 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.
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
Basically earlier their was no way for the user yo know whether the recovery is actually paused or not because it was always returning true after pause requested. Now, we will return whether pause requested or actually paused. So for tool designer who want to wait for recovery to get paused can have a loop and wait until the recovery state reaches to paused. That will give a better control.
I will check other comments and respond along with the patch.
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
On Sun, Jan 24, 2021 at 7:17 AM Dilip Kumar <dilipbalaut@gmail.com> wrote: > On Sat, 23 Jan 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. >> >> 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 > > > Basically earlier their was no way for the user yo know whether the recovery is actually paused or not because it was alwaysreturning true after pause requested. Now, we will return whether pause requested or actually paused. So > for tooldesigner who want to wait for recovery to get paused can have a loop and wait until the recovery state reaches to paused. That will give a better control. I get it and I agree to have that change. My point was whether we can have a new function pg_wal_replay_pause_and_wait that waits until recovery is actually paused ((along with pg_is_wal_replay_paused returning the actual state than a true/false) so that tool developers don't need to have the waiting code outside, if at all they care about it? Others may have better thoughts than me. With Regards, Bharath Rupireddy. EnterpriseDB: http://www.enterprisedb.com
On Sun, Jan 24, 2021 at 11:29 AM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > Some comments on the v6 patch: > > [2] Typo - it's "requested" + * 'paused requested' - if pause is > > reqested but recovery is not yet paused Here I meant the typo "reqested" in "if pause is reqested but recovery is not yet paused" statement from v6 patch. > > [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. Thanks. > > [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. Personally, I think the function RecoveryIsPaused itself is unnecessary with the new function GetRecoveryPauseState introduced in your patch. IMHO, we can remove it. If not okay, then we are at it, can we at least change the function name to be meaningful "IsRecoveryPaused"? Others may have better thoughts than me. > > [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. It will be good to change that. I personally don't like structure names and variable names to be the same. > > [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 As I said earlier, I see no use of RecoveryIsPaused() with the introduction of the new function GetRecoveryPauseState(). Others may have better thoughts than me. > > [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? I see no wrong in having PG_RETURN_TEXT_P and cstring_to_text 's in every case statement. But, just to make sure the code looks cleaner, I said that we can have a local state variable and just one PG_RETURN_TEXT_P(cstring_to_text(state));. See some existing functions brin_page_type, hash_page_type, json_typeof, pg_stat_get_backend_activity, pg_stat_get_backend_wait_event_type, pg_stat_get_backend_wait_event, get_command_type. With Regards, Bharath Rupireddy. EnterpriseDB: http://www.enterprisedb.com
On Sun, Jan 24, 2021 at 12:16 PM Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote: > > On Sun, Jan 24, 2021 at 7:17 AM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > On Sat, 23 Jan 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. > >> > >> 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 > > > > > > Basically earlier their was no way for the user yo know whether the recovery is actually paused or not because it wasalways returning true after pause requested. Now, we will return whether pause requested or actually paused. So > fortool designer who want to wait for recovery to get paused can have a loop and wait until the recovery state reaches topaused. That will give a better control. > > I get it and I agree to have that change. My point was whether we can > have a new function pg_wal_replay_pause_and_wait that waits until > recovery is actually paused ((along with pg_is_wal_replay_paused > returning the actual state than a true/false) so that tool developers > don't need to have the waiting code outside, if at all they care about > it? Others may have better thoughts than me. I think the previous patch was based on that idea where we thought that we can pass an argument to pg_is_wal_replay_paused which can decide whether to wait or return without the wait. I think this version looks better to me where we give the status instead of waiting. I am not sure whether we want another version of pg_wal_replay_pause which will wait for actually it to get paused. I mean there is always a scope to include the functionality in the database which can be achieved by the tool, but this patch was trying to solve the problem that there was no way to know the status so I think returning the correct status should be the scope of this. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
On Sun, Jan 17, 2021 at 5:19 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > On Sat, Jan 16, 2021 at 8:59 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote: > > > > On Wed, Jan 13, 2021 at 9:20 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > > > > On Wed, Jan 13, 2021 at 3:35 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > > > > > > On Wed, Jan 13, 2021 at 3:27 PM Yugo NAGATA <nagata@sraoss.co.jp> wrote: > > > > > > > > > > On Thu, 10 Dec 2020 11:25:23 +0530 > > > > > Dilip Kumar <dilipbalaut@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. > > > > > > > > > > > > As another comment, while pg_is_wal_replay_paused is blocking, I can not cancel > > > > > > > > the query. I think CHECK_FOR_INTERRUPTS() is necessary in the waiting loop. > > > > > > > > > > How about this fix? I think users may want to cancel pg_is_wal_replay_paused() during > > > > > this is blocking. > > > > > > > > Yeah, we can do this. I will send the updated patch after putting > > > > some more thought into these comments. Thanks again for the feedback. > > > > > > > > > > Please find the updated patch. > > > > I've looked at the patch. Here are review comments: > > > > + /* Recovery pause state */ > > + RecoveryPauseState recoveryPause; > > > > Now that the value can have tri-state, how about renaming it to > > recoveryPauseState? > > This makes sense to me. > > > --- > > bool > > RecoveryIsPaused(void) > > +{ > > + bool recoveryPause; > > + > > + SpinLockAcquire(&XLogCtl->info_lck); > > + recoveryPause = (XLogCtl->recoveryPause == RECOVERY_PAUSED) ? > > true : false; > > + SpinLockRelease(&XLogCtl->info_lck); > > + > > + return recoveryPause; > > +} > > + > > +bool > > +RecoveryPauseRequested(void) > > { > > bool recoveryPause; > > > > SpinLockAcquire(&XLogCtl->info_lck); > > - recoveryPause = XLogCtl->recoveryPause; > > + recoveryPause = (XLogCtl->recoveryPause != > > RECOVERY_IN_PROGRESS) ? true : false; > > SpinLockRelease(&XLogCtl->info_lck); > > > > return recoveryPause; > > } > > > > We can write like recoveryPause = (XLogCtl->recoveryPause == RECOVERY_PAUSED); > > In RecoveryPauseRequested, we just want to know whether the pause is > requested or not, even if the pause requested and not yet pause then > also we want to return true. So how > recoveryPause = (XLogCtl->recoveryPause == RECOVERY_PAUSED) will work? Sorry for the late response. What I wanted to say is that the ternary operator is not necessary in those cases. The codes, recoveryPause = (XLogCtl->recoveryPause == RECOVERY_PAUSED) ? true : false; recoveryPause = (XLogCtl->recoveryPause != RECOVERY_IN_PROGRESS) ? true : false; are equivalent with, recoveryPause = (XLogCtl->recoveryPause == RECOVERY_PAUSED); recoveryPause = (XLogCtl->recoveryPause != RECOVERY_IN_PROGRESS); respectively. Regards, -- Masahiko Sawada EDB: https://www.enterprisedb.com/
At Sun, 24 Jan 2021 14:26:08 +0530, Dilip Kumar <dilipbalaut@gmail.com> wrote in > On Sun, Jan 24, 2021 at 12:16 PM Bharath Rupireddy > <bharath.rupireddyforpostgres@gmail.com> wrote: > > > > On Sun, Jan 24, 2021 at 7:17 AM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > On Sat, 23 Jan 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. > > >> > > >> 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 > > > > > > > > > Basically earlier their was no way for the user yo know whether the recovery is actually paused or not because it wasalways returning true after pause requested. Now, we will return whether pause requested or actually paused. So > fortool designer who want to wait for recovery to get paused can have a loop and wait until the recovery state reaches topaused. That will give a better control. > > > > I get it and I agree to have that change. My point was whether we can > > have a new function pg_wal_replay_pause_and_wait that waits until > > recovery is actually paused ((along with pg_is_wal_replay_paused > > returning the actual state than a true/false) so that tool developers > > don't need to have the waiting code outside, if at all they care about > > it? Others may have better thoughts than me. > > I think the previous patch was based on that idea where we thought > that we can pass an argument to pg_is_wal_replay_paused which can > decide whether to wait or return without the wait. I think this > version looks better to me where we give the status instead of > waiting. I am not sure whether we want another version of > pg_wal_replay_pause which will wait for actually it to get paused. I > mean there is always a scope to include the functionality in the > database which can be achieved by the tool, but this patch was trying > to solve the problem that there was no way to know the status so I > think returning the correct status should be the scope of this. I understand that the requirement here is that no record is replayed after pg_wal_replay_pause() is returned, or pg_is_wal_replay_paused() returns true, and delays taken while recovery don't delay the state change. That requirements are really synchronous. On the other hand the machinery is designed to be asynchronous. > * Note that we intentionally don't take the info_lck spinlock > * here. We might therefore read a slightly stale value of > * the recoveryPause flag, but it can't be very stale (no > * worse than the last spinlock we did acquire). Since a > * pause request is a pretty asynchronous thing anyway, > * possibly responding to it one WAL record later than we > * otherwise would is a minor issue, so it doesn't seem worth > * adding another spinlock cycle to prevent that. As the result, this patch tries to introduce several new checkpoints to some delaying point so that that waits can find pause request in a timely manner. I think we had better use locking (or atomics) for the information instead of such scattered checkpoints if we expect that machinery to work in a such syhcnronous manner. That would make the tri-state state variable and many checkpoints unnecessary. Maybe. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
On Mon, Jan 25, 2021 at 6:12 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote: > > On Sun, Jan 17, 2021 at 5:19 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > > On Sat, Jan 16, 2021 at 8:59 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote: > > > > > > On Wed, Jan 13, 2021 at 9:20 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > > > > > > On Wed, Jan 13, 2021 at 3:35 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > > > > > > > > On Wed, Jan 13, 2021 at 3:27 PM Yugo NAGATA <nagata@sraoss.co.jp> wrote: > > > > > > > > > > > > On Thu, 10 Dec 2020 11:25:23 +0530 > > > > > > Dilip Kumar <dilipbalaut@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. > > > > > > > > > > > > > > As another comment, while pg_is_wal_replay_paused is blocking, I can not cancel > > > > > > > > > the query. I think CHECK_FOR_INTERRUPTS() is necessary in the waiting loop. > > > > > > > > > > > > How about this fix? I think users may want to cancel pg_is_wal_replay_paused() during > > > > > > this is blocking. > > > > > > > > > > Yeah, we can do this. I will send the updated patch after putting > > > > > some more thought into these comments. Thanks again for the feedback. > > > > > > > > > > > > > Please find the updated patch. > > > > > > I've looked at the patch. Here are review comments: > > > > > > + /* Recovery pause state */ > > > + RecoveryPauseState recoveryPause; > > > > > > Now that the value can have tri-state, how about renaming it to > > > recoveryPauseState? > > > > This makes sense to me. > > > > > --- > > > bool > > > RecoveryIsPaused(void) > > > +{ > > > + bool recoveryPause; > > > + > > > + SpinLockAcquire(&XLogCtl->info_lck); > > > + recoveryPause = (XLogCtl->recoveryPause == RECOVERY_PAUSED) ? > > > true : false; > > > + SpinLockRelease(&XLogCtl->info_lck); > > > + > > > + return recoveryPause; > > > +} > > > + > > > +bool > > > +RecoveryPauseRequested(void) > > > { > > > bool recoveryPause; > > > > > > SpinLockAcquire(&XLogCtl->info_lck); > > > - recoveryPause = XLogCtl->recoveryPause; > > > + recoveryPause = (XLogCtl->recoveryPause != > > > RECOVERY_IN_PROGRESS) ? true : false; > > > SpinLockRelease(&XLogCtl->info_lck); > > > > > > return recoveryPause; > > > } > > > > > > We can write like recoveryPause = (XLogCtl->recoveryPause == RECOVERY_PAUSED); > > > > In RecoveryPauseRequested, we just want to know whether the pause is > > requested or not, even if the pause requested and not yet pause then > > also we want to return true. So how > > recoveryPause = (XLogCtl->recoveryPause == RECOVERY_PAUSED) will work? > > Sorry for the late response. > > What I wanted to say is that the ternary operator is not necessary in > those cases. > > The codes, > > recoveryPause = (XLogCtl->recoveryPause == RECOVERY_PAUSED) ? true : false; > recoveryPause = (XLogCtl->recoveryPause != RECOVERY_IN_PROGRESS) ? true : false; > > are equivalent with, > > recoveryPause = (XLogCtl->recoveryPause == RECOVERY_PAUSED); > recoveryPause = (XLogCtl->recoveryPause != RECOVERY_IN_PROGRESS); > > respectively. > Yeah absolutely correct. Will changes. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
On Mon, Jan 25, 2021 at 8:42 AM Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote: > > At Sun, 24 Jan 2021 14:26:08 +0530, Dilip Kumar <dilipbalaut@gmail.com> wrote in > > On Sun, Jan 24, 2021 at 12:16 PM Bharath Rupireddy > > <bharath.rupireddyforpostgres@gmail.com> wrote: > > > > > > On Sun, Jan 24, 2021 at 7:17 AM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > > On Sat, 23 Jan 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. > > > >> > > > >> 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 > > > > > > > > > > > > Basically earlier their was no way for the user yo know whether the recovery is actually paused or not because itwas always returning true after pause requested. Now, we will return whether pause requested or actually paused. So >for tool designer who want to wait for recovery to get paused can have a loop and wait until the recovery state reachesto paused. That will give a better control. > > > > > > I get it and I agree to have that change. My point was whether we can > > > have a new function pg_wal_replay_pause_and_wait that waits until > > > recovery is actually paused ((along with pg_is_wal_replay_paused > > > returning the actual state than a true/false) so that tool developers > > > don't need to have the waiting code outside, if at all they care about > > > it? Others may have better thoughts than me. > > > > I think the previous patch was based on that idea where we thought > > that we can pass an argument to pg_is_wal_replay_paused which can > > decide whether to wait or return without the wait. I think this > > version looks better to me where we give the status instead of > > waiting. I am not sure whether we want another version of > > pg_wal_replay_pause which will wait for actually it to get paused. I > > mean there is always a scope to include the functionality in the > > database which can be achieved by the tool, but this patch was trying > > to solve the problem that there was no way to know the status so I > > think returning the correct status should be the scope of this. > > I understand that the requirement here is that no record is replayed > after pg_wal_replay_pause() is returned, or pg_is_wal_replay_paused() > returns true, and delays taken while recovery don't delay the state > change. That requirements are really synchronous. > > On the other hand the machinery is designed to be asynchronous. > > > * Note that we intentionally don't take the info_lck spinlock > > * here. We might therefore read a slightly stale value of > > * the recoveryPause flag, but it can't be very stale (no > > * worse than the last spinlock we did acquire). Since a > > * pause request is a pretty asynchronous thing anyway, > > * possibly responding to it one WAL record later than we > > * otherwise would is a minor issue, so it doesn't seem worth > > * adding another spinlock cycle to prevent that. > > As the result, this patch tries to introduce several new checkpoints > to some delaying point so that that waits can find pause request in a > timely manner. I think we had better use locking (or atomics) for the > information instead of such scattered checkpoints if we expect that > machinery to work in a such syhcnronous manner. > > That would make the tri-state state variable and many checkpoints > unnecessary. Maybe. I don't think the intention was so to make it synchronous, I think the main intention was that pg_is_wal_replay_paused can return us the correct state, in short user can know that whether any more wal will be replayed after pg_is_wal_replay_paused returns true or some other state. I agree that along with that we have also introduced some extra checkpoint where the recovery process is waiting for WAL and apply delay and from the pg_wal_replay_pause we had sent a signal to wakeup the recovery process. So I am not sure is it worth adding the lock/atomic variable to make this synchronous. Any other thoughts on this? -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
At Mon, 25 Jan 2021 10:05:19 +0530, Dilip Kumar <dilipbalaut@gmail.com> wrote in > On Mon, Jan 25, 2021 at 8:42 AM Kyotaro Horiguchi > <horikyota.ntt@gmail.com> wrote: > > > > At Sun, 24 Jan 2021 14:26:08 +0530, Dilip Kumar <dilipbalaut@gmail.com> wrote in > > > On Sun, Jan 24, 2021 at 12:16 PM Bharath Rupireddy > > > <bharath.rupireddyforpostgres@gmail.com> wrote: > > > > > > > > On Sun, Jan 24, 2021 at 7:17 AM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > > > On Sat, 23 Jan 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. > > > > >> > > > > >> 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 > > > > > > > > > > > > > > > Basically earlier their was no way for the user yo know whether the recovery is actually paused or not becauseit was always returning true after pause requested. Now, we will return whether pause requested or actually paused. So > for tool designer who want to wait for recovery to get paused can have a loop and wait until the recovery statereaches to paused. That will give a better control. > > > > > > > > I get it and I agree to have that change. My point was whether we can > > > > have a new function pg_wal_replay_pause_and_wait that waits until > > > > recovery is actually paused ((along with pg_is_wal_replay_paused > > > > returning the actual state than a true/false) so that tool developers > > > > don't need to have the waiting code outside, if at all they care about > > > > it? Others may have better thoughts than me. > > > > > > I think the previous patch was based on that idea where we thought > > > that we can pass an argument to pg_is_wal_replay_paused which can > > > decide whether to wait or return without the wait. I think this > > > version looks better to me where we give the status instead of > > > waiting. I am not sure whether we want another version of > > > pg_wal_replay_pause which will wait for actually it to get paused. I > > > mean there is always a scope to include the functionality in the > > > database which can be achieved by the tool, but this patch was trying > > > to solve the problem that there was no way to know the status so I > > > think returning the correct status should be the scope of this. > > > > I understand that the requirement here is that no record is replayed > > after pg_wal_replay_pause() is returned, or pg_is_wal_replay_paused() > > returns true, and delays taken while recovery don't delay the state > > change. That requirements are really synchronous. > > > > On the other hand the machinery is designed to be asynchronous. > > > > > * Note that we intentionally don't take the info_lck spinlock > > > * here. We might therefore read a slightly stale value of > > > * the recoveryPause flag, but it can't be very stale (no > > > * worse than the last spinlock we did acquire). Since a > > > * pause request is a pretty asynchronous thing anyway, > > > * possibly responding to it one WAL record later than we > > > * otherwise would is a minor issue, so it doesn't seem worth > > > * adding another spinlock cycle to prevent that. > > > > As the result, this patch tries to introduce several new checkpoints > > to some delaying point so that that waits can find pause request in a > > timely manner. I think we had better use locking (or atomics) for the > > information instead of such scattered checkpoints if we expect that > > machinery to work in a such syhcnronous manner. > > > > That would make the tri-state state variable and many checkpoints > > unnecessary. Maybe. > > I don't think the intention was so to make it synchronous, I think > the main intention was that pg_is_wal_replay_paused can return us the > correct state, in short user can know that whether any more wal will > be replayed after pg_is_wal_replay_paused returns true or some other > state. I agree that along with that we have also introduced some I meant that kind of correctness in a time-series by using the word "synchronous". So it can be implemented both by adopting many checkpoints and by just makeing the state-change synchronous. > extra checkpoint where the recovery process is waiting for WAL and > apply delay and from the pg_wal_replay_pause we had sent a signal to > wakeup the recovery process. So I am not sure is it worth adding the > lock/atomic variable to make this synchronous. Any other thoughts on > this? +1 There're only one reader process (startup) and at-most (in the sane usage) one writer process (the caller to pg_wal_replay_pause) so the chance of conflicting is negligibely low. However, I'm not sure how much penalty non-conflicted atomic updates/read imposes on performance.. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
On Sun, Jan 24, 2021 at 12:17 PM Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote: > > On Sun, Jan 24, 2021 at 11:29 AM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > Some comments on the v6 patch: > > > > [2] Typo - it's "requested" + * 'paused requested' - if pause is > > > reqested but recovery is not yet paused > > Here I meant the typo "reqested" in "if pause is reqested but recovery > is not yet paused" statement from v6 patch. Ok > > > [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. > > Thanks. > > > > [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. > > Personally, I think the function RecoveryIsPaused itself is > unnecessary with the new function GetRecoveryPauseState introduced in > your patch. IMHO, we can remove it. If not okay, then we are at it, > can we at least change the function name to be meaningful > "IsRecoveryPaused"? Others may have better thoughts than me. I have removed this function > > > [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. > > It will be good to change that. I personally don't like structure > names and variable names to be the same. Changed to state > > > [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 > > As I said earlier, I see no use of RecoveryIsPaused() with the > introduction of the new function GetRecoveryPauseState(). Others may > have better thoughts than me. > > > > [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? > > I see no wrong in having PG_RETURN_TEXT_P and cstring_to_text 's in > every case statement. But, just to make sure the code looks cleaner, I > said that we can have a local state variable and just one > PG_RETURN_TEXT_P(cstring_to_text(state));. See some existing functions > brin_page_type, hash_page_type, json_typeof, > pg_stat_get_backend_activity, pg_stat_get_backend_wait_event_type, > pg_stat_get_backend_wait_event, get_command_type. > I have changed as per other functions for consistency. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
Attachment
On Mon, Jan 25, 2021 at 2:53 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: > I have changed as per other functions for consistency. Thanks for the v7 patch. Here are some quick comments on it: [1] I think we need to change return value from boolean to text in documentation: <primary>pg_is_wal_replay_paused</primary> </indexterm> <function>pg_is_wal_replay_paused</function> () <returnvalue>boolean</returnvalue> </para> [2] Do we intentionally ignore the return type of below function? If yes, can we change the return type to void and change the function comment? If we do care about the return value, shouldn't we use it? static bool recoveryApplyDelay(XLogReaderState *record); + recoveryApplyDelay(xlogreader); [3] Although it's not necessary, I just thought, it will be good to have an example for the new output of pg_is_wal_replay_paused in the documentation, something like below for brin_page_type: <screen> test=# SELECT brin_page_type(get_raw_page('brinidx', 0)); brin_page_type ---------------- meta </screen> With Regards, Bharath Rupireddy. EnterpriseDB: http://www.enterprisedb.com
On Mon, 25 Jan 2021 14:53:18 +0530 Dilip Kumar <dilipbalaut@gmail.com> wrote: > I have changed as per other functions for consistency. Thank you for updating the patch. Here are a few comments: (1) - SetRecoveryPause(true); + SetRecoveryPause(RECOVERY_PAUSE_REQUESTED); ereport(LOG (errmsg("recovery has paused"), errdetail("If recovery is unpaused, the server will shut down."), errhint("You can then restart the server after making the necessary configuration changes."))); - while (RecoveryIsPaused()) + while (GetRecoveryPauseState() != RECOVERY_NOT_PAUSED) { HandleStartupProcInterrupts(); This fix would be required for code added by the following commit. https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=15251c0a60be76eedee74ac0e94b433f9acca5af Due to this, the recovery could get paused after the configuration change in the primary. However, after applying this patch, pg_is_wal_replay_paused returns "pause requested" although it should return "paused". To fix this, we must pass RECOVERY_PAUSED to SetRecoveryPause() instead of RECOVERY_PAUSE_REQUESTED. Or, we can call CheckAndSetRecoveryPause() in the loop like recoveryPausesHere(), but this seems redundant. (2) - while (RecoveryIsPaused()) + while (GetRecoveryPauseState() != RECOVERY_NOT_PAUSED) { + HandleStartupProcInterrupts(); Though it is trivial, I think the new line after the brace is unnecessary. Regards, Yugo Nagata -- Yugo NAGATA <nagata@sraoss.co.jp>
On Sat, Jan 23, 2021 at 6:10 AM Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote: > +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. Maybe we should leave the existing function pg_is_wal_replay_paused() alone and add a new one with the name you suggest that returns text. That would create less burden for tool authors. -- Robert Haas EDB: http://www.enterprisedb.com
On Tue, Jan 26, 2021 at 2:00 AM Robert Haas <robertmhaas@gmail.com> wrote: > > On Sat, Jan 23, 2021 at 6:10 AM Bharath Rupireddy > <bharath.rupireddyforpostgres@gmail.com> wrote: > > +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. > > Maybe we should leave the existing function pg_is_wal_replay_paused() > alone and add a new one with the name you suggest that returns text. > That would create less burden for tool authors. +1 Regards, -- Masahiko Sawada EDB: https://www.enterprisedb.com/
On Wed, Jan 27, 2021 at 12:50 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote: > > On Tue, Jan 26, 2021 at 2:00 AM Robert Haas <robertmhaas@gmail.com> wrote: > > > > On Sat, Jan 23, 2021 at 6:10 AM Bharath Rupireddy > > <bharath.rupireddyforpostgres@gmail.com> wrote: > > > +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. > > > > Maybe we should leave the existing function pg_is_wal_replay_paused() > > alone and add a new one with the name you suggest that returns text. > > That would create less burden for tool authors. > > +1 > Yeah, we can do that, I will send an updated patch soon. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
On Wed, 27 Jan 2021 13:29:23 +0530 Dilip Kumar <dilipbalaut@gmail.com> wrote: > On Wed, Jan 27, 2021 at 12:50 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote: > > > > On Tue, Jan 26, 2021 at 2:00 AM Robert Haas <robertmhaas@gmail.com> wrote: > > > > > > On Sat, Jan 23, 2021 at 6:10 AM Bharath Rupireddy > > > <bharath.rupireddyforpostgres@gmail.com> wrote: > > > > +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. > > > > > > Maybe we should leave the existing function pg_is_wal_replay_paused() > > > alone and add a new one with the name you suggest that returns text. > > > That would create less burden for tool authors. > > > > +1 > > > > Yeah, we can do that, I will send an updated patch soon. This means pg_is_wal_replay_paused is left without any change and this returns whether pause is requested or not? If so, it seems good to modify the documentation of this function in order to note that this could not return the actual pause state. Regards, Yugo Nagata -- Yugo NAGATA <nagata@sraoss.co.jp>
On Wed, Jan 27, 2021 at 2:06 PM Yugo NAGATA <nagata@sraoss.co.jp> wrote: > > On Wed, 27 Jan 2021 13:29:23 +0530 > Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > On Wed, Jan 27, 2021 at 12:50 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote: > > > > > > On Tue, Jan 26, 2021 at 2:00 AM Robert Haas <robertmhaas@gmail.com> wrote: > > > > > > > > On Sat, Jan 23, 2021 at 6:10 AM Bharath Rupireddy > > > > <bharath.rupireddyforpostgres@gmail.com> wrote: > > > > > +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. > > > > > > > > Maybe we should leave the existing function pg_is_wal_replay_paused() > > > > alone and add a new one with the name you suggest that returns text. > > > > That would create less burden for tool authors. > > > > > > +1 > > > > > > > Yeah, we can do that, I will send an updated patch soon. > > This means pg_is_wal_replay_paused is left without any change and this > returns whether pause is requested or not? If so, it seems good to modify > the documentation of this function in order to note that this could not > return the actual pause state. Yes, we can say that it will return true if the replay pause is requested. I am changing that in my new patch. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
On Wed, Jan 27, 2021 at 2:28 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > On Wed, Jan 27, 2021 at 2:06 PM Yugo NAGATA <nagata@sraoss.co.jp> wrote: > > > > On Wed, 27 Jan 2021 13:29:23 +0530 > > Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > > > On Wed, Jan 27, 2021 at 12:50 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote: > > > > > > > > On Tue, Jan 26, 2021 at 2:00 AM Robert Haas <robertmhaas@gmail.com> wrote: > > > > > > > > > > On Sat, Jan 23, 2021 at 6:10 AM Bharath Rupireddy > > > > > <bharath.rupireddyforpostgres@gmail.com> wrote: > > > > > > +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. > > > > > > > > > > Maybe we should leave the existing function pg_is_wal_replay_paused() > > > > > alone and add a new one with the name you suggest that returns text. > > > > > That would create less burden for tool authors. > > > > > > > > +1 > > > > > > > > > > Yeah, we can do that, I will send an updated patch soon. > > > > This means pg_is_wal_replay_paused is left without any change and this > > returns whether pause is requested or not? If so, it seems good to modify > > the documentation of this function in order to note that this could not > > return the actual pause state. > > Yes, we can say that it will return true if the replay pause is > requested. I am changing that in my new patch. I have modified the patch, changes - I have added a new interface pg_get_wal_replay_pause_state to get the pause request state - Now, we are not waiting for the recovery to actually get paused so I think it doesn't make sense to put a lot of checkpoints to check the pause requested so I have removed that check from the recoveryApplyDelay but I think it better we still keep that check in the WaitForWalToBecomeAvailable because it can wait forever before the next wal get available. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
Attachment
On Thu, 28 Jan 2021 09:55:42 +0530 Dilip Kumar <dilipbalaut@gmail.com> wrote: > On Wed, Jan 27, 2021 at 2:28 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > > On Wed, Jan 27, 2021 at 2:06 PM Yugo NAGATA <nagata@sraoss.co.jp> wrote: > > > > > > On Wed, 27 Jan 2021 13:29:23 +0530 > > > Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > > > > > On Wed, Jan 27, 2021 at 12:50 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote: > > > > > > > > > > On Tue, Jan 26, 2021 at 2:00 AM Robert Haas <robertmhaas@gmail.com> wrote: > > > > > > > > > > > > On Sat, Jan 23, 2021 at 6:10 AM Bharath Rupireddy > > > > > > <bharath.rupireddyforpostgres@gmail.com> wrote: > > > > > > > +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. > > > > > > > > > > > > Maybe we should leave the existing function pg_is_wal_replay_paused() > > > > > > alone and add a new one with the name you suggest that returns text. > > > > > > That would create less burden for tool authors. > > > > > > > > > > +1 > > > > > > > > > > > > > Yeah, we can do that, I will send an updated patch soon. > > > > > > This means pg_is_wal_replay_paused is left without any change and this > > > returns whether pause is requested or not? If so, it seems good to modify > > > the documentation of this function in order to note that this could not > > > return the actual pause state. > > > > Yes, we can say that it will return true if the replay pause is > > requested. I am changing that in my new patch. > > I have modified the patch, changes > > - I have added a new interface pg_get_wal_replay_pause_state to get > the pause request state > - Now, we are not waiting for the recovery to actually get paused so I > think it doesn't make sense to put a lot of checkpoints to check the > pause requested so I have removed that check from the > recoveryApplyDelay but I think it better we still keep that check in > the WaitForWalToBecomeAvailable because it can wait forever before the > next wal get available. I think basically the check in WaitForWalToBecomeAvailable is independent of the feature of pg_get_wal_replay_pause_state, that is, reporting the actual pause state. This function could just return 'pause requested' if a pause is requested during waiting for WAL. However, I agree the change to allow recovery to transit the state to 'paused' during WAL waiting because 'paused' has more useful information for users than 'pause requested'. Returning 'paused' lets users know clearly that no more WAL are applied until recovery is resumed. On the other hand, when 'pause requested' is returned, user can't say whether the next WAL wiill be applied or not from this information. For the same reason, I think it is also useful to call recoveryPausesHere in recoveryApplyDelay. In addition, in RecoveryRequiresIntParameter, recovery should get paused if a parameter value has a problem. However, pg_get_wal_replay_pause_state will return 'pause requested' in this case. So, I think, we should pass RECOVERY_PAUSED to SetRecoveryPause() instead of RECOVERY_PAUSE_REQUESTED, or call CheckAndSetRecoveryPause() in the loop like recoveryPausesHere(). Regrads, Yugo Nagata -- Yugo NAGATA <nagata@sraoss.co.jp>
On Fri, Jan 29, 2021 at 3:25 PM Yugo NAGATA <nagata@sraoss.co.jp> wrote: > > On Thu, 28 Jan 2021 09:55:42 +0530 > Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > On Wed, Jan 27, 2021 at 2:28 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > > > > On Wed, Jan 27, 2021 at 2:06 PM Yugo NAGATA <nagata@sraoss.co.jp> wrote: > > > > > > > > On Wed, 27 Jan 2021 13:29:23 +0530 > > > > Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > > > > > > > On Wed, Jan 27, 2021 at 12:50 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote: > > > > > > > > > > > > On Tue, Jan 26, 2021 at 2:00 AM Robert Haas <robertmhaas@gmail.com> wrote: > > > > > > > > > > > > > > On Sat, Jan 23, 2021 at 6:10 AM Bharath Rupireddy > > > > > > > <bharath.rupireddyforpostgres@gmail.com> wrote: > > > > > > > > +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. > > > > > > > > > > > > > > Maybe we should leave the existing function pg_is_wal_replay_paused() > > > > > > > alone and add a new one with the name you suggest that returns text. > > > > > > > That would create less burden for tool authors. > > > > > > > > > > > > +1 > > > > > > > > > > > > > > > > Yeah, we can do that, I will send an updated patch soon. > > > > > > > > This means pg_is_wal_replay_paused is left without any change and this > > > > returns whether pause is requested or not? If so, it seems good to modify > > > > the documentation of this function in order to note that this could not > > > > return the actual pause state. > > > > > > Yes, we can say that it will return true if the replay pause is > > > requested. I am changing that in my new patch. > > > > I have modified the patch, changes > > > > - I have added a new interface pg_get_wal_replay_pause_state to get > > the pause request state > > - Now, we are not waiting for the recovery to actually get paused so I > > think it doesn't make sense to put a lot of checkpoints to check the > > pause requested so I have removed that check from the > > recoveryApplyDelay but I think it better we still keep that check in > > the WaitForWalToBecomeAvailable because it can wait forever before the > > next wal get available. > > I think basically the check in WaitForWalToBecomeAvailable is independent > of the feature of pg_get_wal_replay_pause_state, that is, reporting the > actual pause state. This function could just return 'pause requested' > if a pause is requested during waiting for WAL. > > However, I agree the change to allow recovery to transit the state to > 'paused' during WAL waiting because 'paused' has more useful information > for users than 'pause requested'. Returning 'paused' lets users know > clearly that no more WAL are applied until recovery is resumed. On the > other hand, when 'pause requested' is returned, user can't say whether > the next WAL wiill be applied or not from this information. > > For the same reason, I think it is also useful to call recoveryPausesHere > in recoveryApplyDelay. IMHO the WaitForWalToBecomeAvailable can wait until the next wal get available so it can not be controlled by user so it is good to put a check for the recovery pause, however recoveryApplyDelay wait for the apply delay which is configured by user and it is predictable value by the user. I don't have much objection to putting that check in the recoveryApplyDelay as well but I feel it is not necessary. Any other thoughts on this? > In addition, in RecoveryRequiresIntParameter, recovery should get paused > if a parameter value has a problem. However, pg_get_wal_replay_pause_state > will return 'pause requested' in this case. So, I think, we should pass > RECOVERY_PAUSED to SetRecoveryPause() instead of RECOVERY_PAUSE_REQUESTED, > or call CheckAndSetRecoveryPause() in the loop like recoveryPausesHere(). Yeah, absolutely right, it must pass RECOVERY_PAUSED. I will change this, thanks for noticing this. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
On Fri, 29 Jan 2021 16:33:32 +0530 Dilip Kumar <dilipbalaut@gmail.com> wrote: > On Fri, Jan 29, 2021 at 3:25 PM Yugo NAGATA <nagata@sraoss.co.jp> wrote: > > > > On Thu, 28 Jan 2021 09:55:42 +0530 > > Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > > > On Wed, Jan 27, 2021 at 2:28 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > > > > > > On Wed, Jan 27, 2021 at 2:06 PM Yugo NAGATA <nagata@sraoss.co.jp> wrote: > > > > > > > > > > On Wed, 27 Jan 2021 13:29:23 +0530 > > > > > Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > > > > > > > > > On Wed, Jan 27, 2021 at 12:50 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote: > > > > > > > > > > > > > > On Tue, Jan 26, 2021 at 2:00 AM Robert Haas <robertmhaas@gmail.com> wrote: > > > > > > > > > > > > > > > > On Sat, Jan 23, 2021 at 6:10 AM Bharath Rupireddy > > > > > > > > <bharath.rupireddyforpostgres@gmail.com> wrote: > > > > > > > > > +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. > > > > > > > > > > > > > > > > Maybe we should leave the existing function pg_is_wal_replay_paused() > > > > > > > > alone and add a new one with the name you suggest that returns text. > > > > > > > > That would create less burden for tool authors. > > > > > > > > > > > > > > +1 > > > > > > > > > > > > > > > > > > > Yeah, we can do that, I will send an updated patch soon. > > > > > > > > > > This means pg_is_wal_replay_paused is left without any change and this > > > > > returns whether pause is requested or not? If so, it seems good to modify > > > > > the documentation of this function in order to note that this could not > > > > > return the actual pause state. > > > > > > > > Yes, we can say that it will return true if the replay pause is > > > > requested. I am changing that in my new patch. > > > > > > I have modified the patch, changes > > > > > > - I have added a new interface pg_get_wal_replay_pause_state to get > > > the pause request state > > > - Now, we are not waiting for the recovery to actually get paused so I > > > think it doesn't make sense to put a lot of checkpoints to check the > > > pause requested so I have removed that check from the > > > recoveryApplyDelay but I think it better we still keep that check in > > > the WaitForWalToBecomeAvailable because it can wait forever before the > > > next wal get available. > > > > I think basically the check in WaitForWalToBecomeAvailable is independent > > of the feature of pg_get_wal_replay_pause_state, that is, reporting the > > actual pause state. This function could just return 'pause requested' > > if a pause is requested during waiting for WAL. > > > > However, I agree the change to allow recovery to transit the state to > > 'paused' during WAL waiting because 'paused' has more useful information > > for users than 'pause requested'. Returning 'paused' lets users know > > clearly that no more WAL are applied until recovery is resumed. On the > > other hand, when 'pause requested' is returned, user can't say whether > > the next WAL wiill be applied or not from this information. > > > > For the same reason, I think it is also useful to call recoveryPausesHere > > in recoveryApplyDelay. > > IMHO the WaitForWalToBecomeAvailable can wait until the next wal get > available so it can not be controlled by user so it is good to put a > check for the recovery pause, however recoveryApplyDelay wait for the > apply delay which is configured by user and it is predictable value by > the user. I don't have much objection to putting that check in the > recoveryApplyDelay as well but I feel it is not necessary. Any other > thoughts on this? I'm not sure if the user can figure out easily that the reason why pg_get_wal_replay_pause_state returns 'pause requested' is due to recovery_min_apply_delay because it would needs knowledge of the internal mechanism of recovery. However, if there are not any other opinions of it, I don't care that recoveryApplyDelay is left as is because such check and state transition is independent of the goal of pg_get_wal_replay_pause_state itself as I mentioned above. Regards, Yugo Nagata -- Yugo NAGATA <nagata@sraoss.co.jp>
On Fri, Jan 29, 2021 at 4:33 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > On Fri, Jan 29, 2021 at 3:25 PM Yugo NAGATA <nagata@sraoss.co.jp> wrote: > > > > On Thu, 28 Jan 2021 09:55:42 +0530 > > Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > > > On Wed, Jan 27, 2021 at 2:28 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > > > > > > On Wed, Jan 27, 2021 at 2:06 PM Yugo NAGATA <nagata@sraoss.co.jp> wrote: > > > > > > > > > > On Wed, 27 Jan 2021 13:29:23 +0530 > > > > > Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > > > > > > > > > On Wed, Jan 27, 2021 at 12:50 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote: > > > > > > > > > > > > > > On Tue, Jan 26, 2021 at 2:00 AM Robert Haas <robertmhaas@gmail.com> wrote: > > > > > > > > > > > > > > > > On Sat, Jan 23, 2021 at 6:10 AM Bharath Rupireddy > > > > > > > > <bharath.rupireddyforpostgres@gmail.com> wrote: > > > > > > > > > +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. > > > > > > > > > > > > > > > > Maybe we should leave the existing function pg_is_wal_replay_paused() > > > > > > > > alone and add a new one with the name you suggest that returns text. > > > > > > > > That would create less burden for tool authors. > > > > > > > > > > > > > > +1 > > > > > > > > > > > > > > > > > > > Yeah, we can do that, I will send an updated patch soon. > > > > > > > > > > This means pg_is_wal_replay_paused is left without any change and this > > > > > returns whether pause is requested or not? If so, it seems good to modify > > > > > the documentation of this function in order to note that this could not > > > > > return the actual pause state. > > > > > > > > Yes, we can say that it will return true if the replay pause is > > > > requested. I am changing that in my new patch. > > > > > > I have modified the patch, changes > > > > > > - I have added a new interface pg_get_wal_replay_pause_state to get > > > the pause request state > > > - Now, we are not waiting for the recovery to actually get paused so I > > > think it doesn't make sense to put a lot of checkpoints to check the > > > pause requested so I have removed that check from the > > > recoveryApplyDelay but I think it better we still keep that check in > > > the WaitForWalToBecomeAvailable because it can wait forever before the > > > next wal get available. > > > > I think basically the check in WaitForWalToBecomeAvailable is independent > > of the feature of pg_get_wal_replay_pause_state, that is, reporting the > > actual pause state. This function could just return 'pause requested' > > if a pause is requested during waiting for WAL. > > > > However, I agree the change to allow recovery to transit the state to > > 'paused' during WAL waiting because 'paused' has more useful information > > for users than 'pause requested'. Returning 'paused' lets users know > > clearly that no more WAL are applied until recovery is resumed. On the > > other hand, when 'pause requested' is returned, user can't say whether > > the next WAL wiill be applied or not from this information. > > > > For the same reason, I think it is also useful to call recoveryPausesHere > > in recoveryApplyDelay. > > IMHO the WaitForWalToBecomeAvailable can wait until the next wal get > available so it can not be controlled by user so it is good to put a > check for the recovery pause, however recoveryApplyDelay wait for the > apply delay which is configured by user and it is predictable value by > the user. I don't have much objection to putting that check in the > recoveryApplyDelay as well but I feel it is not necessary. Any other > thoughts on this? > > > In addition, in RecoveryRequiresIntParameter, recovery should get paused > > if a parameter value has a problem. However, pg_get_wal_replay_pause_state > > will return 'pause requested' in this case. So, I think, we should pass > > RECOVERY_PAUSED to SetRecoveryPause() instead of RECOVERY_PAUSE_REQUESTED, > > or call CheckAndSetRecoveryPause() in the loop like recoveryPausesHere(). > > Yeah, absolutely right, it must pass RECOVERY_PAUSED. I will change > this, thanks for noticing this. I have changed this in the new patch. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
Attachment
At Sun, 31 Jan 2021 11:24:30 +0530, Dilip Kumar <dilipbalaut@gmail.com> wrote in > On Fri, Jan 29, 2021 at 4:33 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > > On Fri, Jan 29, 2021 at 3:25 PM Yugo NAGATA <nagata@sraoss.co.jp> wrote: > > > > > > On Thu, 28 Jan 2021 09:55:42 +0530 > > > Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > > > > > On Wed, Jan 27, 2021 at 2:28 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > > > > > > > > On Wed, Jan 27, 2021 at 2:06 PM Yugo NAGATA <nagata@sraoss.co.jp> wrote: > > > > > > > > > > > > On Wed, 27 Jan 2021 13:29:23 +0530 > > > > > > Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > > > > > > > > > > > On Wed, Jan 27, 2021 at 12:50 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote: > > > > > > > > > > > > > > > > On Tue, Jan 26, 2021 at 2:00 AM Robert Haas <robertmhaas@gmail.com> wrote: > > > > > > > > > > > > > > > > > > On Sat, Jan 23, 2021 at 6:10 AM Bharath Rupireddy > > > > > > > > > <bharath.rupireddyforpostgres@gmail.com> wrote: > > > > > > > > > > +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. > > > > > > > > > > > > > > > > > > Maybe we should leave the existing function pg_is_wal_replay_paused() > > > > > > > > > alone and add a new one with the name you suggest that returns text. > > > > > > > > > That would create less burden for tool authors. > > > > > > > > > > > > > > > > +1 > > > > > > > > > > > > > > > > > > > > > > Yeah, we can do that, I will send an updated patch soon. > > > > > > > > > > > > This means pg_is_wal_replay_paused is left without any change and this > > > > > > returns whether pause is requested or not? If so, it seems good to modify > > > > > > the documentation of this function in order to note that this could not > > > > > > return the actual pause state. > > > > > > > > > > Yes, we can say that it will return true if the replay pause is > > > > > requested. I am changing that in my new patch. > > > > > > > > I have modified the patch, changes > > > > > > > > - I have added a new interface pg_get_wal_replay_pause_state to get > > > > the pause request state > > > > - Now, we are not waiting for the recovery to actually get paused so I > > > > think it doesn't make sense to put a lot of checkpoints to check the > > > > pause requested so I have removed that check from the > > > > recoveryApplyDelay but I think it better we still keep that check in > > > > the WaitForWalToBecomeAvailable because it can wait forever before the > > > > next wal get available. > > > > > > I think basically the check in WaitForWalToBecomeAvailable is independent > > > of the feature of pg_get_wal_replay_pause_state, that is, reporting the > > > actual pause state. This function could just return 'pause requested' > > > if a pause is requested during waiting for WAL. > > > > > > However, I agree the change to allow recovery to transit the state to > > > 'paused' during WAL waiting because 'paused' has more useful information > > > for users than 'pause requested'. Returning 'paused' lets users know > > > clearly that no more WAL are applied until recovery is resumed. On the > > > other hand, when 'pause requested' is returned, user can't say whether > > > the next WAL wiill be applied or not from this information. > > > > > > For the same reason, I think it is also useful to call recoveryPausesHere > > > in recoveryApplyDelay. > > > > IMHO the WaitForWalToBecomeAvailable can wait until the next wal get > > available so it can not be controlled by user so it is good to put a > > check for the recovery pause, however recoveryApplyDelay wait for the > > apply delay which is configured by user and it is predictable value by > > the user. I don't have much objection to putting that check in the > > recoveryApplyDelay as well but I feel it is not necessary. Any other > > thoughts on this? > > > > > In addition, in RecoveryRequiresIntParameter, recovery should get paused > > > if a parameter value has a problem. However, pg_get_wal_replay_pause_state > > > will return 'pause requested' in this case. So, I think, we should pass > > > RECOVERY_PAUSED to SetRecoveryPause() instead of RECOVERY_PAUSE_REQUESTED, > > > or call CheckAndSetRecoveryPause() in the loop like recoveryPausesHere(). > > > > Yeah, absolutely right, it must pass RECOVERY_PAUSED. I will change > > this, thanks for noticing this. > > I have changed this in the new patch. It seems to work well. The checkpoints seems to be placed properly. +SetRecoveryPause(RecoveryPauseState state) { + Assert(state >= RECOVERY_NOT_PAUSED && state <= RECOVERY_PAUSED); I'm not sure that state worth FATAL. Isn't it enough to just ERROR out like XLogFileRead? CheckAndSetRecovery() has only one caller. I think it's better to write the code directly. I think the documentation of pg_wal_replay_pause needs to be a bit more detailed about the difference between the two states "pause requested" and "paused". Something like "A request doesn't mean that recovery stops right away. If you want a guarantee that recovery is actually paused, you need to check for the recovery pause state returned by pg_wal_replay_pause_state(). Note that pg_is_wal_repay_paused() returns whether a request is made." regards. -- Kyotaro Horiguchi NTT Open Source Software Center
On Mon, Feb 1, 2021 at 11:59 AM Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote: > > At Sun, 31 Jan 2021 11:24:30 +0530, Dilip Kumar <dilipbalaut@gmail.com> wrote in > > On Fri, Jan 29, 2021 at 4:33 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > > > > On Fri, Jan 29, 2021 at 3:25 PM Yugo NAGATA <nagata@sraoss.co.jp> wrote: > > > > > > > > On Thu, 28 Jan 2021 09:55:42 +0530 > > > > Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > > > > > > > On Wed, Jan 27, 2021 at 2:28 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > > > > > > > > > > On Wed, Jan 27, 2021 at 2:06 PM Yugo NAGATA <nagata@sraoss.co.jp> wrote: > > > > > > > > > > > > > > On Wed, 27 Jan 2021 13:29:23 +0530 > > > > > > > Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > > > > > > > > > > > > > On Wed, Jan 27, 2021 at 12:50 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote: > > > > > > > > > > > > > > > > > > On Tue, Jan 26, 2021 at 2:00 AM Robert Haas <robertmhaas@gmail.com> wrote: > > > > > > > > > > > > > > > > > > > > On Sat, Jan 23, 2021 at 6:10 AM Bharath Rupireddy > > > > > > > > > > <bharath.rupireddyforpostgres@gmail.com> wrote: > > > > > > > > > > > +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. > > > > > > > > > > > > > > > > > > > > Maybe we should leave the existing function pg_is_wal_replay_paused() > > > > > > > > > > alone and add a new one with the name you suggest that returns text. > > > > > > > > > > That would create less burden for tool authors. > > > > > > > > > > > > > > > > > > +1 > > > > > > > > > > > > > > > > > > > > > > > > > Yeah, we can do that, I will send an updated patch soon. > > > > > > > > > > > > > > This means pg_is_wal_replay_paused is left without any change and this > > > > > > > returns whether pause is requested or not? If so, it seems good to modify > > > > > > > the documentation of this function in order to note that this could not > > > > > > > return the actual pause state. > > > > > > > > > > > > Yes, we can say that it will return true if the replay pause is > > > > > > requested. I am changing that in my new patch. > > > > > > > > > > I have modified the patch, changes > > > > > > > > > > - I have added a new interface pg_get_wal_replay_pause_state to get > > > > > the pause request state > > > > > - Now, we are not waiting for the recovery to actually get paused so I > > > > > think it doesn't make sense to put a lot of checkpoints to check the > > > > > pause requested so I have removed that check from the > > > > > recoveryApplyDelay but I think it better we still keep that check in > > > > > the WaitForWalToBecomeAvailable because it can wait forever before the > > > > > next wal get available. > > > > > > > > I think basically the check in WaitForWalToBecomeAvailable is independent > > > > of the feature of pg_get_wal_replay_pause_state, that is, reporting the > > > > actual pause state. This function could just return 'pause requested' > > > > if a pause is requested during waiting for WAL. > > > > > > > > However, I agree the change to allow recovery to transit the state to > > > > 'paused' during WAL waiting because 'paused' has more useful information > > > > for users than 'pause requested'. Returning 'paused' lets users know > > > > clearly that no more WAL are applied until recovery is resumed. On the > > > > other hand, when 'pause requested' is returned, user can't say whether > > > > the next WAL wiill be applied or not from this information. > > > > > > > > For the same reason, I think it is also useful to call recoveryPausesHere > > > > in recoveryApplyDelay. > > > > > > IMHO the WaitForWalToBecomeAvailable can wait until the next wal get > > > available so it can not be controlled by user so it is good to put a > > > check for the recovery pause, however recoveryApplyDelay wait for the > > > apply delay which is configured by user and it is predictable value by > > > the user. I don't have much objection to putting that check in the > > > recoveryApplyDelay as well but I feel it is not necessary. Any other > > > thoughts on this? > > > > > > > In addition, in RecoveryRequiresIntParameter, recovery should get paused > > > > if a parameter value has a problem. However, pg_get_wal_replay_pause_state > > > > will return 'pause requested' in this case. So, I think, we should pass > > > > RECOVERY_PAUSED to SetRecoveryPause() instead of RECOVERY_PAUSE_REQUESTED, > > > > or call CheckAndSetRecoveryPause() in the loop like recoveryPausesHere(). > > > > > > Yeah, absolutely right, it must pass RECOVERY_PAUSED. I will change > > > this, thanks for noticing this. > > > > I have changed this in the new patch. > > It seems to work well. The checkpoints seems to be placed properly. Okay > +SetRecoveryPause(RecoveryPauseState state) > { > + Assert(state >= RECOVERY_NOT_PAUSED && state <= RECOVERY_PAUSED); > > I'm not sure that state worth FATAL. Isn't it enough to just ERROR > out like XLogFileRead? Yeah, that makes sense to me. > CheckAndSetRecovery() has only one caller. I think it's better to > write the code directly. Okay, I will change. > I think the documentation of pg_wal_replay_pause needs to be a bit > more detailed about the difference between the two states "pause > requested" and "paused". Something like "A request doesn't mean that > recovery stops right away. If you want a guarantee that recovery is > actually paused, you need to check for the recovery pause state > returned by pg_wal_replay_pause_state(). Note that > pg_is_wal_repay_paused() returns whether a request is made." That seems like better idea, I will change. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
On Mon, Feb 1, 2021 at 1:41 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > On Mon, Feb 1, 2021 at 11:59 AM Kyotaro Horiguchi > <horikyota.ntt@gmail.com> wrote: > > > > At Sun, 31 Jan 2021 11:24:30 +0530, Dilip Kumar <dilipbalaut@gmail.com> wrote in > > > On Fri, Jan 29, 2021 at 4:33 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > > > > > > On Fri, Jan 29, 2021 at 3:25 PM Yugo NAGATA <nagata@sraoss.co.jp> wrote: > > > > > > > > > > On Thu, 28 Jan 2021 09:55:42 +0530 > > > > > Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > > > > > > > > > On Wed, Jan 27, 2021 at 2:28 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > > > > > > > > > > > > On Wed, Jan 27, 2021 at 2:06 PM Yugo NAGATA <nagata@sraoss.co.jp> wrote: > > > > > > > > > > > > > > > > On Wed, 27 Jan 2021 13:29:23 +0530 > > > > > > > > Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > > > > > > > > > > > > > > > On Wed, Jan 27, 2021 at 12:50 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote: > > > > > > > > > > > > > > > > > > > > On Tue, Jan 26, 2021 at 2:00 AM Robert Haas <robertmhaas@gmail.com> wrote: > > > > > > > > > > > > > > > > > > > > > > On Sat, Jan 23, 2021 at 6:10 AM Bharath Rupireddy > > > > > > > > > > > <bharath.rupireddyforpostgres@gmail.com> wrote: > > > > > > > > > > > > +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. > > > > > > > > > > > > > > > > > > > > > > Maybe we should leave the existing function pg_is_wal_replay_paused() > > > > > > > > > > > alone and add a new one with the name you suggest that returns text. > > > > > > > > > > > That would create less burden for tool authors. > > > > > > > > > > > > > > > > > > > > +1 > > > > > > > > > > > > > > > > > > > > > > > > > > > > Yeah, we can do that, I will send an updated patch soon. > > > > > > > > > > > > > > > > This means pg_is_wal_replay_paused is left without any change and this > > > > > > > > returns whether pause is requested or not? If so, it seems good to modify > > > > > > > > the documentation of this function in order to note that this could not > > > > > > > > return the actual pause state. > > > > > > > > > > > > > > Yes, we can say that it will return true if the replay pause is > > > > > > > requested. I am changing that in my new patch. > > > > > > > > > > > > I have modified the patch, changes > > > > > > > > > > > > - I have added a new interface pg_get_wal_replay_pause_state to get > > > > > > the pause request state > > > > > > - Now, we are not waiting for the recovery to actually get paused so I > > > > > > think it doesn't make sense to put a lot of checkpoints to check the > > > > > > pause requested so I have removed that check from the > > > > > > recoveryApplyDelay but I think it better we still keep that check in > > > > > > the WaitForWalToBecomeAvailable because it can wait forever before the > > > > > > next wal get available. > > > > > > > > > > I think basically the check in WaitForWalToBecomeAvailable is independent > > > > > of the feature of pg_get_wal_replay_pause_state, that is, reporting the > > > > > actual pause state. This function could just return 'pause requested' > > > > > if a pause is requested during waiting for WAL. > > > > > > > > > > However, I agree the change to allow recovery to transit the state to > > > > > 'paused' during WAL waiting because 'paused' has more useful information > > > > > for users than 'pause requested'. Returning 'paused' lets users know > > > > > clearly that no more WAL are applied until recovery is resumed. On the > > > > > other hand, when 'pause requested' is returned, user can't say whether > > > > > the next WAL wiill be applied or not from this information. > > > > > > > > > > For the same reason, I think it is also useful to call recoveryPausesHere > > > > > in recoveryApplyDelay. > > > > > > > > IMHO the WaitForWalToBecomeAvailable can wait until the next wal get > > > > available so it can not be controlled by user so it is good to put a > > > > check for the recovery pause, however recoveryApplyDelay wait for the > > > > apply delay which is configured by user and it is predictable value by > > > > the user. I don't have much objection to putting that check in the > > > > recoveryApplyDelay as well but I feel it is not necessary. Any other > > > > thoughts on this? > > > > > > > > > In addition, in RecoveryRequiresIntParameter, recovery should get paused > > > > > if a parameter value has a problem. However, pg_get_wal_replay_pause_state > > > > > will return 'pause requested' in this case. So, I think, we should pass > > > > > RECOVERY_PAUSED to SetRecoveryPause() instead of RECOVERY_PAUSE_REQUESTED, > > > > > or call CheckAndSetRecoveryPause() in the loop like recoveryPausesHere(). > > > > > > > > Yeah, absolutely right, it must pass RECOVERY_PAUSED. I will change > > > > this, thanks for noticing this. > > > > > > I have changed this in the new patch. > > > > It seems to work well. The checkpoints seems to be placed properly. > > Okay > > > +SetRecoveryPause(RecoveryPauseState state) > > { > > + Assert(state >= RECOVERY_NOT_PAUSED && state <= RECOVERY_PAUSED); > > > > I'm not sure that state worth FATAL. Isn't it enough to just ERROR > > out like XLogFileRead? > > Yeah, that makes sense to me. > > > CheckAndSetRecovery() has only one caller. I think it's better to > > write the code directly. > > Okay, I will change. > > > I think the documentation of pg_wal_replay_pause needs to be a bit > > more detailed about the difference between the two states "pause > > requested" and "paused". Something like "A request doesn't mean that > > recovery stops right away. If you want a guarantee that recovery is > > actually paused, you need to check for the recovery pause state > > returned by pg_wal_replay_pause_state(). Note that > > pg_is_wal_repay_paused() returns whether a request is made." > > That seems like better idea, I will change. > Please find an updated patch which addresses these comments. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
Attachment
On Thu, Feb 4, 2021 at 10:28 AM Dilip Kumar <dilipbalaut@gmail.com> wrote: > Please find an updated patch which addresses these comments. Thanks for the patch. I tested the new function pg_get_wal_replay_pause_state: postgres=# select pg_get_wal_replay_pause_state(); pg_get_wal_replay_pause_state ------------------------------- not paused postgres=# select pg_wal_replay_pause(); pg_wal_replay_pause --------------------- (1 row) I can also see the "pause requested" state after I put a gdb breakpoint in WaitForWALToBecomeAvailable in the standby startup process . postgres=# select pg_get_wal_replay_pause_state(); pg_get_wal_replay_pause_state ------------------------------- pause requested (1 row) postgres=# select pg_get_wal_replay_pause_state(); pg_get_wal_replay_pause_state ------------------------------- paused (1 row) Mostly, the v10 patch looks good to me, except below minor comments: 1) A typo in commit message - "just check" --> "just checks" 2) How about + Returns recovery pause state. The return values are <literal>not paused instead of + Returns recovery pause state, the return values are <literal>not paused 3) I think it is 'get wal replay pause state', instead of { oid => '1137', descr => 'get wal replay is pause state', 4) can we just do this /* * If recovery pause is requested then set it paused. While we are in * the loop, user might resume and pause again so set this every time. */ if (((volatile XLogCtlData *) XLogCtl)->recoveryPauseState == RECOVERY_PAUSE_REQUESTED) SetRecoveryPause(RECOVERY_PAUSED); instead of /* * If recovery pause is requested then set it paused. While we are in * the loop, user might resume and pause again so set this every time. */ SpinLockAcquire(&XLogCtl->info_lck); if (XLogCtl->recoveryPauseState == RECOVERY_PAUSE_REQUESTED) XLogCtl->recoveryPauseState = RECOVERY_PAUSED; SpinLockRelease(&XLogCtl->info_lck); I think it's okay, since we take a spinlock anyways in GetRecoveryPauseState(). See the below comment and also a relevant commit 6ba4ecbf477e0b25dd7bde1b0c4e07fc2da19348 on why it's not necessary taking spinlock always: /* * Pause WAL replay, if requested by a hot-standby session via * SetRecoveryPause(). * * Note that we intentionally don't take the info_lck spinlock * here. We might therefore read a slightly stale value of * the recoveryPause flag, but it can't be very stale (no * worse than the last spinlock we did acquire). Since a * pause request is a pretty asynchronous thing anyway, * possibly responding to it one WAL record later than we * otherwise would is a minor issue, so it doesn't seem worth * adding another spinlock cycle to prevent that. */ With Regards, Bharath Rupireddy. EnterpriseDB: http://www.enterprisedb.com
On Thu, Feb 4, 2021 at 4:58 PM Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote: > > On Thu, Feb 4, 2021 at 10:28 AM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > Please find an updated patch which addresses these comments. > > Thanks for the patch. I tested the new function pg_get_wal_replay_pause_state: > > postgres=# select pg_get_wal_replay_pause_state(); > pg_get_wal_replay_pause_state > ------------------------------- > not paused > postgres=# select pg_wal_replay_pause(); > pg_wal_replay_pause > --------------------- > > (1 row) > > I can also see the "pause requested" state after I put a gdb > breakpoint in WaitForWALToBecomeAvailable in the standby startup > process . > > postgres=# select pg_get_wal_replay_pause_state(); > pg_get_wal_replay_pause_state > ------------------------------- > pause requested > (1 row) > > postgres=# select pg_get_wal_replay_pause_state(); > pg_get_wal_replay_pause_state > ------------------------------- > paused > (1 row) > > Mostly, the v10 patch looks good to me, except below minor comments: Thanks for the testing. > 1) A typo in commit message - "just check" --> "just checks" > > 2) How about > + Returns recovery pause state. The return values are <literal>not paused > instead of > + Returns recovery pause state, the return values are <literal>not paused > > 3) I think it is 'get wal replay pause state', instead of { oid => > '1137', descr => 'get wal replay is pause state', > > 4) can we just do this > /* > * If recovery pause is requested then set it paused. While we are in > * the loop, user might resume and pause again so set this every time. > */ > if (((volatile XLogCtlData *) XLogCtl)->recoveryPauseState == > RECOVERY_PAUSE_REQUESTED) > SetRecoveryPause(RECOVERY_PAUSED); > instead of > /* > * If recovery pause is requested then set it paused. While we are in > * the loop, user might resume and pause again so set this every time. > */ > SpinLockAcquire(&XLogCtl->info_lck); > if (XLogCtl->recoveryPauseState == RECOVERY_PAUSE_REQUESTED) > XLogCtl->recoveryPauseState = RECOVERY_PAUSED; > SpinLockRelease(&XLogCtl->info_lck); > > I think it's okay, since we take a spinlock anyways in > GetRecoveryPauseState(). See the below comment and also a relevant > commit 6ba4ecbf477e0b25dd7bde1b0c4e07fc2da19348 on why it's not > necessary taking spinlock always: > /* > * Pause WAL replay, if requested by a hot-standby session via > * SetRecoveryPause(). > * > * Note that we intentionally don't take the info_lck spinlock > * here. We might therefore read a slightly stale value of > * the recoveryPause flag, but it can't be very stale (no > * worse than the last spinlock we did acquire). Since a > * pause request is a pretty asynchronous thing anyway, > * possibly responding to it one WAL record later than we > * otherwise would is a minor issue, so it doesn't seem worth > * adding another spinlock cycle to prevent that. > */ > I will work on these comments and send the updated patch soon. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
On Thu, Feb 4, 2021 at 4:58 PM Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote: > > On Thu, Feb 4, 2021 at 10:28 AM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > Please find an updated patch which addresses these comments. > > Thanks for the patch. I tested the new function pg_get_wal_replay_pause_state: > > postgres=# select pg_get_wal_replay_pause_state(); > pg_get_wal_replay_pause_state > ------------------------------- > not paused > postgres=# select pg_wal_replay_pause(); > pg_wal_replay_pause > --------------------- > > (1 row) > > I can also see the "pause requested" state after I put a gdb > breakpoint in WaitForWALToBecomeAvailable in the standby startup > process . > > postgres=# select pg_get_wal_replay_pause_state(); > pg_get_wal_replay_pause_state > ------------------------------- > pause requested > (1 row) > > postgres=# select pg_get_wal_replay_pause_state(); > pg_get_wal_replay_pause_state > ------------------------------- > paused > (1 row) > > Mostly, the v10 patch looks good to me, except below minor comments: > > 1) A typo in commit message - "just check" --> "just checks" > > 2) How about > + Returns recovery pause state. The return values are <literal>not paused > instead of > + Returns recovery pause state, the return values are <literal>not paused > > 3) I think it is 'get wal replay pause state', instead of { oid => > '1137', descr => 'get wal replay is pause state', > > 4) can we just do this > /* > * If recovery pause is requested then set it paused. While we are in > * the loop, user might resume and pause again so set this every time. > */ > if (((volatile XLogCtlData *) XLogCtl)->recoveryPauseState == > RECOVERY_PAUSE_REQUESTED) > SetRecoveryPause(RECOVERY_PAUSED); > instead of > /* > * If recovery pause is requested then set it paused. While we are in > * the loop, user might resume and pause again so set this every time. > */ > SpinLockAcquire(&XLogCtl->info_lck); > if (XLogCtl->recoveryPauseState == RECOVERY_PAUSE_REQUESTED) > XLogCtl->recoveryPauseState = RECOVERY_PAUSED; > SpinLockRelease(&XLogCtl->info_lck); > > I think it's okay, since we take a spinlock anyways in > GetRecoveryPauseState(). See the below comment and also a relevant > commit 6ba4ecbf477e0b25dd7bde1b0c4e07fc2da19348 on why it's not > necessary taking spinlock always: > /* > * Pause WAL replay, if requested by a hot-standby session via > * SetRecoveryPause(). > * > * Note that we intentionally don't take the info_lck spinlock > * here. We might therefore read a slightly stale value of > * the recoveryPause flag, but it can't be very stale (no > * worse than the last spinlock we did acquire). Since a > * pause request is a pretty asynchronous thing anyway, > * possibly responding to it one WAL record later than we > * otherwise would is a minor issue, so it doesn't seem worth > * adding another spinlock cycle to prevent that. > */ How can we do that this is not a 1 byte flag this is enum so I don't think we can read any atomic state without a spin lock here. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
On Thu, Feb 4, 2021 at 6:16 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > On Thu, Feb 4, 2021 at 4:58 PM Bharath Rupireddy > <bharath.rupireddyforpostgres@gmail.com> wrote: > > > > On Thu, Feb 4, 2021 at 10:28 AM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > Please find an updated patch which addresses these comments. > > > > Thanks for the patch. I tested the new function pg_get_wal_replay_pause_state: > > > > postgres=# select pg_get_wal_replay_pause_state(); > > pg_get_wal_replay_pause_state > > ------------------------------- > > not paused > > postgres=# select pg_wal_replay_pause(); > > pg_wal_replay_pause > > --------------------- > > > > (1 row) > > > > I can also see the "pause requested" state after I put a gdb > > breakpoint in WaitForWALToBecomeAvailable in the standby startup > > process . > > > > postgres=# select pg_get_wal_replay_pause_state(); > > pg_get_wal_replay_pause_state > > ------------------------------- > > pause requested > > (1 row) > > > > postgres=# select pg_get_wal_replay_pause_state(); > > pg_get_wal_replay_pause_state > > ------------------------------- > > paused > > (1 row) > > > > Mostly, the v10 patch looks good to me, except below minor comments: > > > > 1) A typo in commit message - "just check" --> "just checks" > > > > 2) How about > > + Returns recovery pause state. The return values are <literal>not paused > > instead of > > + Returns recovery pause state, the return values are <literal>not paused > > > > 3) I think it is 'get wal replay pause state', instead of { oid => > > '1137', descr => 'get wal replay is pause state', > > > > 4) can we just do this > > /* > > * If recovery pause is requested then set it paused. While we are in > > * the loop, user might resume and pause again so set this every time. > > */ > > if (((volatile XLogCtlData *) XLogCtl)->recoveryPauseState == > > RECOVERY_PAUSE_REQUESTED) > > SetRecoveryPause(RECOVERY_PAUSED); > > instead of > > /* > > * If recovery pause is requested then set it paused. While we are in > > * the loop, user might resume and pause again so set this every time. > > */ > > SpinLockAcquire(&XLogCtl->info_lck); > > if (XLogCtl->recoveryPauseState == RECOVERY_PAUSE_REQUESTED) > > XLogCtl->recoveryPauseState = RECOVERY_PAUSED; > > SpinLockRelease(&XLogCtl->info_lck); > > > > I think it's okay, since we take a spinlock anyways in > > GetRecoveryPauseState(). See the below comment and also a relevant > > commit 6ba4ecbf477e0b25dd7bde1b0c4e07fc2da19348 on why it's not > > necessary taking spinlock always: > > /* > > * Pause WAL replay, if requested by a hot-standby session via > > * SetRecoveryPause(). > > * > > * Note that we intentionally don't take the info_lck spinlock > > * here. We might therefore read a slightly stale value of > > * the recoveryPause flag, but it can't be very stale (no > > * worse than the last spinlock we did acquire). Since a > > * pause request is a pretty asynchronous thing anyway, > > * possibly responding to it one WAL record later than we > > * otherwise would is a minor issue, so it doesn't seem worth > > * adding another spinlock cycle to prevent that. > > */ > > How can we do that this is not a 1 byte flag this is enum so I don't > think we can read any atomic state without a spin lock here. I have fixed the other comments and the updated patch is attached. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
Attachment
On Thu, Feb 4, 2021 at 7:46 AM Dilip Kumar <dilipbalaut@gmail.com> wrote: > How can we do that this is not a 1 byte flag this is enum so I don't > think we can read any atomic state without a spin lock here. I think this discussion of atomics is confused. Let's talk about what atomic reads and writes mean. Imagine that you have a 64-bit value 0x0101010101010101. Somebody sets it to 0x0202020202020202. Imagine that just as they are doing that, someone else reads the value and gets 0x0202020201010101, because half of the value has been updated and the other half has not yet been updated yet. This kind of thing can actually happen on some platforms and what it means is that on those platforms 8-byte reads and writes are not atomic. The idea of an "atom" is that it can't be split into pieces but these reads and writes on some platforms are actually not "atomic" because they are split into two 4-byte pieces. But there's no such thing as a 1-byte read or write not being atomic. In theory you could imagine a computer where when you change 0x01 to 0x23 and read in the middle and see 0x21 or 0x13 or something, but no actual computers behave that way, or at least no mainstream ones that anybody cares about. So the idea that you somehow need a lock to prevent this is just wrong. Concurrent programs also suffer from another problem which is reordering of operations, which can happen either as the program is compiled or as the program is executed by the CPU. The CPU can see you set a->x = 1 and a->y = 2 and decide to update y first and then x even though you wrote it the other way around in the program text. To prevent this, we have barrier operations; see README.barrier in the source tree for a longer explanation. Atomic operations like compare-and-exchange are also full barriers, so that they not only prevent the torn read/write problem described above, but also enforce order of operations more strictly. Now I don't know whether a lock is needed here or not. Maybe it is; perhaps for consistency with other code, perhaps because the lock acquire and release is serving the function of a barrier; or perhaps to guard against some other hazard. But saying that it's because reading or writing a 1-byte value might not be atomic does not sound correct. -- Robert Haas EDB: http://www.enterprisedb.com
On Thu, Feb 4, 2021 at 7:20 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > How can we do that this is not a 1 byte flag this is enum so I don't > > think we can read any atomic state without a spin lock here. > > I have fixed the other comments and the updated patch is attached. Can we just do like below so that we could use the existing SetRecoveryPause instead of setting the state outside? /* loop until recoveryPauseState is set to RECOVERY_NOT_PAUSED */ while (1) { RecoveryPauseState state; state = GetRecoveryPauseState(); if (state == RECOVERY_NOT_PAUSED) break; HandleStartupProcInterrupts(); if (CheckForStandbyTrigger()) return; pgstat_report_wait_start(WAIT_EVENT_RECOVERY_PAUSE); /* * If recovery pause is requested then set it paused. While we are in * the loop, user might resume and pause again so set this every time. */ if (state == RECOVERY_PAUSE_REQUESTED) SetRecoveryPause(RECOVERY_PAUSED) And a typo - it's "pg_is_wal_replay_paused" not "pg_is_wal_repay_paused" + <function>pg_is_wal_repay_paused()</function> returns whether a request With Regards, Bharath Rupireddy. EnterpriseDB: http://www.enterprisedb.com
On Thu, Feb 4, 2021 at 10:19 PM Robert Haas <robertmhaas@gmail.com> wrote: > > On Thu, Feb 4, 2021 at 7:46 AM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > How can we do that this is not a 1 byte flag this is enum so I don't > > think we can read any atomic state without a spin lock here. > > I think this discussion of atomics is confused. Let's talk about what > atomic reads and writes mean. Imagine that you have a 64-bit value > 0x0101010101010101. Somebody sets it to 0x0202020202020202. Imagine > that just as they are doing that, someone else reads the value and > gets 0x0202020201010101, because half of the value has been updated > and the other half has not yet been updated yet. This kind of thing > can actually happen on some platforms and what it means is that on > those platforms 8-byte reads and writes are not atomic. The idea of an > "atom" is that it can't be split into pieces but these reads and > writes on some platforms are actually not "atomic" because they are > split into two 4-byte pieces. But there's no such thing as a 1-byte > read or write not being atomic. In theory you could imagine a computer > where when you change 0x01 to 0x23 and read in the middle and see 0x21 > or 0x13 or something, but no actual computers behave that way, or at > least no mainstream ones that anybody cares about. So the idea that > you somehow need a lock to prevent this is just wrong. > > Concurrent programs also suffer from another problem which is > reordering of operations, which can happen either as the program is > compiled or as the program is executed by the CPU. The CPU can see you > set a->x = 1 and a->y = 2 and decide to update y first and then x even > though you wrote it the other way around in the program text. To > prevent this, we have barrier operations; see README.barrier in the > source tree for a longer explanation. Atomic operations like > compare-and-exchange are also full barriers, so that they not only > prevent the torn read/write problem described above, but also enforce > order of operations more strictly. > > Now I don't know whether a lock is needed here or not. Maybe it is; > perhaps for consistency with other code, perhaps because the lock > acquire and release is serving the function of a barrier; or perhaps > to guard against some other hazard. But saying that it's because > reading or writing a 1-byte value might not be atomic does not sound > correct. I never told that reading /writing 1 byte is not atomic, of course, they are. I told that we can only guarantee that 1-byte read/write is atomic but this variable is not a bool or 1-byte value and the enum can take 32 bits on a 32-bit platform so we can not guarantee the atomic read/write on some processor so we need a lock. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
On Fri, Feb 5, 2021 at 6:22 AM Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote: > > On Thu, Feb 4, 2021 at 7:20 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > How can we do that this is not a 1 byte flag this is enum so I don't > > > think we can read any atomic state without a spin lock here. > > > > I have fixed the other comments and the updated patch is attached. > > Can we just do like below so that we could use the existing > SetRecoveryPause instead of setting the state outside? > > /* loop until recoveryPauseState is set to RECOVERY_NOT_PAUSED */ > while (1) > { > RecoveryPauseState state; > > state = GetRecoveryPauseState(); > > if (state == RECOVERY_NOT_PAUSED) > break; > > HandleStartupProcInterrupts(); > > if (CheckForStandbyTrigger()) > return; > pgstat_report_wait_start(WAIT_EVENT_RECOVERY_PAUSE); > > /* > * If recovery pause is requested then set it paused. While we are in > * the loop, user might resume and pause again so set this every time. > */ > if (state == RECOVERY_PAUSE_REQUESTED) > SetRecoveryPause(RECOVERY_PAUSED) We can not do that, basically, under one lock we need to check the state and set it to pause. Because by the time you release the lock someone might set it to RECOVERY_NOT_PAUSED then you don't want to set it to RECOVERY_PAUSED. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
On Fri, Feb 5, 2021 at 10:06 AM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > On Fri, Feb 5, 2021 at 6:22 AM Bharath Rupireddy > <bharath.rupireddyforpostgres@gmail.com> wrote: > > > > On Thu, Feb 4, 2021 at 7:20 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > > How can we do that this is not a 1 byte flag this is enum so I don't > > > > think we can read any atomic state without a spin lock here. > > > > > > I have fixed the other comments and the updated patch is attached. > > > > Can we just do like below so that we could use the existing > > SetRecoveryPause instead of setting the state outside? > > > > /* loop until recoveryPauseState is set to RECOVERY_NOT_PAUSED */ > > while (1) > > { > > RecoveryPauseState state; > > > > state = GetRecoveryPauseState(); > > > > if (state == RECOVERY_NOT_PAUSED) > > break; > > > > HandleStartupProcInterrupts(); > > > > if (CheckForStandbyTrigger()) > > return; > > pgstat_report_wait_start(WAIT_EVENT_RECOVERY_PAUSE); > > > > /* > > * If recovery pause is requested then set it paused. While we are in > > * the loop, user might resume and pause again so set this every time. > > */ > > if (state == RECOVERY_PAUSE_REQUESTED) > > SetRecoveryPause(RECOVERY_PAUSED) > > We can not do that, basically, under one lock we need to check the > state and set it to pause. Because by the time you release the lock > someone might set it to RECOVERY_NOT_PAUSED then you don't want to set > it to RECOVERY_PAUSED. Got it. Thanks. With Regards, Bharath Rupireddy. EnterpriseDB: http://www.enterprisedb.com
On Fri, Feb 5, 2021 at 10:14 AM Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote: > > We can not do that, basically, under one lock we need to check the > > state and set it to pause. Because by the time you release the lock > > someone might set it to RECOVERY_NOT_PAUSED then you don't want to set > > it to RECOVERY_PAUSED. > > Got it. Thanks. Hi Dilip, I have one more question: + /* test for recovery pause, if user has requested the pause */ + if (((volatile XLogCtlData *) XLogCtl)->recoveryPauseState == + RECOVERY_PAUSE_REQUESTED) + recoveryPausesHere(false); + + now = GetCurrentTimestamp(); + Do we need now = GetCurrentTimestamp(); here? Because, I see that whenever the variable now is used within the for loop in WaitForWALToBecomeAvailable, it's re-calculated anyways. It's being used within case XLOG_FROM_STREAM: Am I missing something? With Regards, Bharath Rupireddy. EnterpriseDB: http://www.enterprisedb.com
On Sun, Feb 7, 2021 at 6:44 PM Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote: > > On Fri, Feb 5, 2021 at 10:14 AM Bharath Rupireddy > <bharath.rupireddyforpostgres@gmail.com> wrote: > > > We can not do that, basically, under one lock we need to check the > > > state and set it to pause. Because by the time you release the lock > > > someone might set it to RECOVERY_NOT_PAUSED then you don't want to set > > > it to RECOVERY_PAUSED. > > > > Got it. Thanks. > > Hi Dilip, I have one more question: > > + /* test for recovery pause, if user has requested the pause */ > + if (((volatile XLogCtlData *) XLogCtl)->recoveryPauseState == > + RECOVERY_PAUSE_REQUESTED) > + recoveryPausesHere(false); > + > + now = GetCurrentTimestamp(); > + > > Do we need now = GetCurrentTimestamp(); here? Because, I see that > whenever the variable now is used within the for loop in > WaitForWALToBecomeAvailable, it's re-calculated anyways. It's being > used within case XLOG_FROM_STREAM: > > Am I missing something? Yeah, I don't see any reason for doing this, maybe it got copy pasted by mistake. Thanks for observing this. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
Attachment
Hi, On Sun, 7 Feb 2021 19:27:02 +0530 Dilip Kumar <dilipbalaut@gmail.com> wrote: > On Sun, Feb 7, 2021 at 6:44 PM Bharath Rupireddy > <bharath.rupireddyforpostgres@gmail.com> wrote: > > > > On Fri, Feb 5, 2021 at 10:14 AM Bharath Rupireddy > > <bharath.rupireddyforpostgres@gmail.com> wrote: > > > > We can not do that, basically, under one lock we need to check the > > > > state and set it to pause. Because by the time you release the lock > > > > someone might set it to RECOVERY_NOT_PAUSED then you don't want to set > > > > it to RECOVERY_PAUSED. > > > > > > Got it. Thanks. > > > > Hi Dilip, I have one more question: > > > > + /* test for recovery pause, if user has requested the pause */ > > + if (((volatile XLogCtlData *) XLogCtl)->recoveryPauseState == > > + RECOVERY_PAUSE_REQUESTED) > > + recoveryPausesHere(false); > > + > > + now = GetCurrentTimestamp(); > > + > > > > Do we need now = GetCurrentTimestamp(); here? Because, I see that > > whenever the variable now is used within the for loop in > > WaitForWALToBecomeAvailable, it's re-calculated anyways. It's being > > used within case XLOG_FROM_STREAM: > > > > Am I missing something? > > Yeah, I don't see any reason for doing this, maybe it got copy pasted > by mistake. Thanks for observing this. I also have a question: @@ -6270,14 +6291,14 @@ RecoveryRequiresIntParameter(const char *param_name, int currValue, int minValue currValue, minValue))); - SetRecoveryPause(true); + SetRecoveryPause(RECOVERY_PAUSED); ereport(LOG, (errmsg("recovery has paused"), errdetail("If recovery is unpaused, the server will shut down."), errhint("You can then restart the server after making the necessary configuration changes."))); - while (RecoveryIsPaused()) + while (GetRecoveryPauseState() != RECOVERY_NOT_PAUSED) { HandleStartupProcInterrupts(); If a user call pg_wal_replay_pause while waiting in RecoveryRequiresIntParameter, the state become 'pause requested' and this never returns to 'paused'. Should we check recoveryPauseState in this loop as in recoveryPausesHere? Regards, Yugo Nagata -- Yugo NAGATA <nagata@sraoss.co.jp>
On Mon, 8 Feb 2021 at 6:38 AM, Yugo NAGATA <nagata@sraoss.co.jp> wrote:
Hi,
On Sun, 7 Feb 2021 19:27:02 +0530
Dilip Kumar <dilipbalaut@gmail.com> wrote:
> On Sun, Feb 7, 2021 at 6:44 PM Bharath Rupireddy
> <bharath.rupireddyforpostgres@gmail.com> wrote:
> >
> > On Fri, Feb 5, 2021 at 10:14 AM Bharath Rupireddy
> > <bharath.rupireddyforpostgres@gmail.com> wrote:
> > > > We can not do that, basically, under one lock we need to check the
> > > > state and set it to pause. Because by the time you release the lock
> > > > someone might set it to RECOVERY_NOT_PAUSED then you don't want to set
> > > > it to RECOVERY_PAUSED.
> > >
> > > Got it. Thanks.
> >
> > Hi Dilip, I have one more question:
> >
> > + /* test for recovery pause, if user has requested the pause */
> > + if (((volatile XLogCtlData *) XLogCtl)->recoveryPauseState ==
> > + RECOVERY_PAUSE_REQUESTED)
> > + recoveryPausesHere(false);
> > +
> > + now = GetCurrentTimestamp();
> > +
> >
> > Do we need now = GetCurrentTimestamp(); here? Because, I see that
> > whenever the variable now is used within the for loop in
> > WaitForWALToBecomeAvailable, it's re-calculated anyways. It's being
> > used within case XLOG_FROM_STREAM:
> >
> > Am I missing something?
>
> Yeah, I don't see any reason for doing this, maybe it got copy pasted
> by mistake. Thanks for observing this.
I also have a question:
@@ -6270,14 +6291,14 @@ RecoveryRequiresIntParameter(const char *param_name, int currValue, int minValue
currValue,
minValue)));
- SetRecoveryPause(true);
+ SetRecoveryPause(RECOVERY_PAUSED);
ereport(LOG,
(errmsg("recovery has paused"),
errdetail("If recovery is unpaused, the server will shut down."),
errhint("You can then restart the server after making the necessary configuration changes.")));
- while (RecoveryIsPaused())
+ while (GetRecoveryPauseState() != RECOVERY_NOT_PAUSED)
{
HandleStartupProcInterrupts();
If a user call pg_wal_replay_pause while waiting in RecoveryRequiresIntParameter,
the state become 'pause requested' and this never returns to 'paused'.
Should we check recoveryPauseState in this loop as in
I think the right fix should be that the state should never go from ‘paused’ to ‘pause requested’ so I think pg_wal_replay_pause should take care of that.
On Mon, 8 Feb 2021 07:51:22 +0530 Dilip Kumar <dilipbalaut@gmail.com> wrote: > On Mon, 8 Feb 2021 at 6:38 AM, Yugo NAGATA <nagata@sraoss.co.jp> wrote: > > > Hi, > > > > On Sun, 7 Feb 2021 19:27:02 +0530 > > Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > > > On Sun, Feb 7, 2021 at 6:44 PM Bharath Rupireddy > > > <bharath.rupireddyforpostgres@gmail.com> wrote: > > > > > > > > On Fri, Feb 5, 2021 at 10:14 AM Bharath Rupireddy > > > > <bharath.rupireddyforpostgres@gmail.com> wrote: > > > > > > We can not do that, basically, under one lock we need to check the > > > > > > state and set it to pause. Because by the time you release the > > lock > > > > > > someone might set it to RECOVERY_NOT_PAUSED then you don't want to > > set > > > > > > it to RECOVERY_PAUSED. > > > > > > > > > > Got it. Thanks. > > > > > > > > Hi Dilip, I have one more question: > > > > > > > > + /* test for recovery pause, if user has requested the pause */ > > > > + if (((volatile XLogCtlData *) XLogCtl)->recoveryPauseState == > > > > + RECOVERY_PAUSE_REQUESTED) > > > > + recoveryPausesHere(false); > > > > + > > > > + now = GetCurrentTimestamp(); > > > > + > > > > > > > > Do we need now = GetCurrentTimestamp(); here? Because, I see that > > > > whenever the variable now is used within the for loop in > > > > WaitForWALToBecomeAvailable, it's re-calculated anyways. It's being > > > > used within case XLOG_FROM_STREAM: > > > > > > > > Am I missing something? > > > > > > Yeah, I don't see any reason for doing this, maybe it got copy pasted > > > by mistake. Thanks for observing this. > > > > I also have a question: > > > > @@ -6270,14 +6291,14 @@ RecoveryRequiresIntParameter(const char > > *param_name, int currValue, int minValue > > currValue, > > minValue))); > > > > - SetRecoveryPause(true); > > + SetRecoveryPause(RECOVERY_PAUSED); > > > > ereport(LOG, > > (errmsg("recovery has paused"), > > errdetail("If recovery is > > unpaused, the server will shut down."), > > errhint("You can then restart the > > server after making the necessary configuration changes."))); > > > > - while (RecoveryIsPaused()) > > + while (GetRecoveryPauseState() != > > RECOVERY_NOT_PAUSED) > > { > > HandleStartupProcInterrupts(); > > > > > > > > If a user call pg_wal_replay_pause while waiting in > > RecoveryRequiresIntParameter, > > the state become 'pause requested' and this never returns to 'paused'. > > Should we check recoveryPauseState in this loop as in > > > I think the right fix should be that the state should never go from > ‘paused’ to ‘pause requested’ so I think pg_wal_replay_pause should take > care of that. It makes sense to take care of this in pg_wal_replay_pause, but I wonder it can not handle the case that a user resume and pause again while a sleep. -- Yugo NAGATA <nagata@sraoss.co.jp>
On Mon, Feb 8, 2021 at 8:18 AM Yugo NAGATA <nagata@sraoss.co.jp> wrote: > > On Mon, 8 Feb 2021 07:51:22 +0530 > Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > On Mon, 8 Feb 2021 at 6:38 AM, Yugo NAGATA <nagata@sraoss.co.jp> wrote: > > > > > Hi, > > > > > > On Sun, 7 Feb 2021 19:27:02 +0530 > > > Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > > > > > On Sun, Feb 7, 2021 at 6:44 PM Bharath Rupireddy > > > > <bharath.rupireddyforpostgres@gmail.com> wrote: > > > > > > > > > > On Fri, Feb 5, 2021 at 10:14 AM Bharath Rupireddy > > > > > <bharath.rupireddyforpostgres@gmail.com> wrote: > > > > > > > We can not do that, basically, under one lock we need to check the > > > > > > > state and set it to pause. Because by the time you release the > > > lock > > > > > > > someone might set it to RECOVERY_NOT_PAUSED then you don't want to > > > set > > > > > > > it to RECOVERY_PAUSED. > > > > > > > > > > > > Got it. Thanks. > > > > > > > > > > Hi Dilip, I have one more question: > > > > > > > > > > + /* test for recovery pause, if user has requested the pause */ > > > > > + if (((volatile XLogCtlData *) XLogCtl)->recoveryPauseState == > > > > > + RECOVERY_PAUSE_REQUESTED) > > > > > + recoveryPausesHere(false); > > > > > + > > > > > + now = GetCurrentTimestamp(); > > > > > + > > > > > > > > > > Do we need now = GetCurrentTimestamp(); here? Because, I see that > > > > > whenever the variable now is used within the for loop in > > > > > WaitForWALToBecomeAvailable, it's re-calculated anyways. It's being > > > > > used within case XLOG_FROM_STREAM: > > > > > > > > > > Am I missing something? > > > > > > > > Yeah, I don't see any reason for doing this, maybe it got copy pasted > > > > by mistake. Thanks for observing this. > > > > > > I also have a question: > > > > > > @@ -6270,14 +6291,14 @@ RecoveryRequiresIntParameter(const char > > > *param_name, int currValue, int minValue > > > currValue, > > > minValue))); > > > > > > - SetRecoveryPause(true); > > > + SetRecoveryPause(RECOVERY_PAUSED); > > > > > > ereport(LOG, > > > (errmsg("recovery has paused"), > > > errdetail("If recovery is > > > unpaused, the server will shut down."), > > > errhint("You can then restart the > > > server after making the necessary configuration changes."))); > > > > > > - while (RecoveryIsPaused()) > > > + while (GetRecoveryPauseState() != > > > RECOVERY_NOT_PAUSED) > > > { > > > HandleStartupProcInterrupts(); > > > > > > > > > > > > If a user call pg_wal_replay_pause while waiting in > > > RecoveryRequiresIntParameter, > > > the state become 'pause requested' and this never returns to 'paused'. > > > Should we check recoveryPauseState in this loop as in > > > > > > I think the right fix should be that the state should never go from > > ‘paused’ to ‘pause requested’ so I think pg_wal_replay_pause should take > > care of that. > > It makes sense to take care of this in pg_wal_replay_pause, but I wonder > it can not handle the case that a user resume and pause again while a sleep. Right, we will have to check and set in the loop. But we should not allow the state to go from paused to pause requested irrespective of this. I will make these changes and send the updated patch. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
On Mon, Feb 8, 2021 at 9:35 AM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > > If a user call pg_wal_replay_pause while waiting in > > > > RecoveryRequiresIntParameter, > > > > the state become 'pause requested' and this never returns to 'paused'. > > > > Should we check recoveryPauseState in this loop as in > > > > > > > > > I think the right fix should be that the state should never go from > > > ‘paused’ to ‘pause requested’ so I think pg_wal_replay_pause should take > > > care of that. > > > > It makes sense to take care of this in pg_wal_replay_pause, but I wonder > > it can not handle the case that a user resume and pause again while a sleep. > > Right, we will have to check and set in the loop. But we should not > allow the state to go from paused to pause requested irrespective of > this. We can think of a state machine with the states "not paused", "pause requested", "paused". While we can go to "not paused" from any state, but cannot go to "pause requested" from "paused". So, will pg_wal_replay_pause throw an error or warning or silently return when it's called and the state is "paused" already? Maybe we should add better commenting in pg_wal_replay_pause why we don't set "pause requested" when the state is already "paused". And also, if we are adding below code in the RecoveryRequiresIntParameter loop, it's better to make it a function, like your earlier patch. /* * If recovery pause is requested then set it paused. While we are in * the loop, user might resume and pause again so set this every time. */ SpinLockAcquire(&XLogCtl->info_lck); if (XLogCtl->recoveryPauseState == RECOVERY_PAUSE_REQUESTED) XLogCtl->recoveryPauseState = RECOVERY_PAUSED; SpinLockRelease(&XLogCtl->info_lck); With Regards, Bharath Rupireddy. EnterpriseDB: http://www.enterprisedb.com
On Mon, Feb 8, 2021 at 9:49 AM Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote: > > On Mon, Feb 8, 2021 at 9:35 AM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > > > If a user call pg_wal_replay_pause while waiting in > > > > > RecoveryRequiresIntParameter, > > > > > the state become 'pause requested' and this never returns to 'paused'. > > > > > Should we check recoveryPauseState in this loop as in > > > > > > > > > > > > I think the right fix should be that the state should never go from > > > > ‘paused’ to ‘pause requested’ so I think pg_wal_replay_pause should take > > > > care of that. > > > > > > It makes sense to take care of this in pg_wal_replay_pause, but I wonder > > > it can not handle the case that a user resume and pause again while a sleep. > > > > Right, we will have to check and set in the loop. But we should not > > allow the state to go from paused to pause requested irrespective of > > this. > > We can think of a state machine with the states "not paused", "pause > requested", "paused". While we can go to "not paused" from any state, > but cannot go to "pause requested" from "paused". > > So, will pg_wal_replay_pause throw an error or warning or silently > return when it's called and the state is "paused" already? It should just silently return because pg_wal_replay_pause just claim it request to pause, but it not mean that it can not pause immediately. Maybe we > should add better commenting in pg_wal_replay_pause why we don't set > "pause requested" when the state is already "paused". > And also, if we are adding below code in the > RecoveryRequiresIntParameter loop, it's better to make it a function, > like your earlier patch. > > /* > * If recovery pause is requested then set it paused. While we are in > * the loop, user might resume and pause again so set this every time. > */ > SpinLockAcquire(&XLogCtl->info_lck); > if (XLogCtl->recoveryPauseState == RECOVERY_PAUSE_REQUESTED) > XLogCtl->recoveryPauseState = RECOVERY_PAUSED; > SpinLockRelease(&XLogCtl->info_lck); Yes, it should go back to function now as in the older versions. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
On Mon, 8 Feb 2021 09:35:00 +0530 Dilip Kumar <dilipbalaut@gmail.com> wrote: > On Mon, Feb 8, 2021 at 8:18 AM Yugo NAGATA <nagata@sraoss.co.jp> wrote: > > > > On Mon, 8 Feb 2021 07:51:22 +0530 > > Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > > > On Mon, 8 Feb 2021 at 6:38 AM, Yugo NAGATA <nagata@sraoss.co.jp> wrote: > > > > > > > Hi, > > > > > > > > On Sun, 7 Feb 2021 19:27:02 +0530 > > > > Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > > > > > > > On Sun, Feb 7, 2021 at 6:44 PM Bharath Rupireddy > > > > > <bharath.rupireddyforpostgres@gmail.com> wrote: > > > > > > > > > > > > On Fri, Feb 5, 2021 at 10:14 AM Bharath Rupireddy > > > > > > <bharath.rupireddyforpostgres@gmail.com> wrote: > > > > > > > > We can not do that, basically, under one lock we need to check the > > > > > > > > state and set it to pause. Because by the time you release the > > > > lock > > > > > > > > someone might set it to RECOVERY_NOT_PAUSED then you don't want to > > > > set > > > > > > > > it to RECOVERY_PAUSED. > > > > > > > > > > > > > > Got it. Thanks. > > > > > > > > > > > > Hi Dilip, I have one more question: > > > > > > > > > > > > + /* test for recovery pause, if user has requested the pause */ > > > > > > + if (((volatile XLogCtlData *) XLogCtl)->recoveryPauseState == > > > > > > + RECOVERY_PAUSE_REQUESTED) > > > > > > + recoveryPausesHere(false); > > > > > > + > > > > > > + now = GetCurrentTimestamp(); > > > > > > + > > > > > > > > > > > > Do we need now = GetCurrentTimestamp(); here? Because, I see that > > > > > > whenever the variable now is used within the for loop in > > > > > > WaitForWALToBecomeAvailable, it's re-calculated anyways. It's being > > > > > > used within case XLOG_FROM_STREAM: > > > > > > > > > > > > Am I missing something? > > > > > > > > > > Yeah, I don't see any reason for doing this, maybe it got copy pasted > > > > > by mistake. Thanks for observing this. > > > > > > > > I also have a question: > > > > > > > > @@ -6270,14 +6291,14 @@ RecoveryRequiresIntParameter(const char > > > > *param_name, int currValue, int minValue > > > > currValue, > > > > minValue))); > > > > > > > > - SetRecoveryPause(true); > > > > + SetRecoveryPause(RECOVERY_PAUSED); > > > > > > > > ereport(LOG, > > > > (errmsg("recovery has paused"), > > > > errdetail("If recovery is > > > > unpaused, the server will shut down."), > > > > errhint("You can then restart the > > > > server after making the necessary configuration changes."))); > > > > > > > > - while (RecoveryIsPaused()) > > > > + while (GetRecoveryPauseState() != > > > > RECOVERY_NOT_PAUSED) > > > > { > > > > HandleStartupProcInterrupts(); > > > > > > > > > > > > > > > > If a user call pg_wal_replay_pause while waiting in > > > > RecoveryRequiresIntParameter, > > > > the state become 'pause requested' and this never returns to 'paused'. > > > > Should we check recoveryPauseState in this loop as in > > > > > > > > > I think the right fix should be that the state should never go from > > > ‘paused’ to ‘pause requested’ so I think pg_wal_replay_pause should take > > > care of that. > > > > It makes sense to take care of this in pg_wal_replay_pause, but I wonder > > it can not handle the case that a user resume and pause again while a sleep. > > Right, we will have to check and set in the loop. But we should not > allow the state to go from paused to pause requested irrespective of > this. I agree with you. -- Yugo NAGATA <nagata@sraoss.co.jp>
At Mon, 8 Feb 2021 14:12:35 +0900, Yugo NAGATA <nagata@sraoss.co.jp> wrote in > > > > I think the right fix should be that the state should never go from > > > > ‘paused’ to ‘pause requested’ so I think pg_wal_replay_pause should take > > > > care of that. > > > > > > It makes sense to take care of this in pg_wal_replay_pause, but I wonder > > > it can not handle the case that a user resume and pause again while a sleep. > > > > Right, we will have to check and set in the loop. But we should not > > allow the state to go from paused to pause requested irrespective of > > this. > > I agree with you. Is there any actual harm if PAUSED returns to REQUESETED, assuming we immediately change the state to PAUSE always we see REQUESTED in the waiting loop, despite that we allow change the state from PAUSE to REQUESTED via NOT_PAUSED between two successive loop condition checks? regards. -- Kyotaro Horiguchi NTT Open Source Software Center
On Mon, 08 Feb 2021 17:32:46 +0900 (JST) Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote: > At Mon, 8 Feb 2021 14:12:35 +0900, Yugo NAGATA <nagata@sraoss.co.jp> wrote in > > > > > I think the right fix should be that the state should never go from > > > > > ‘paused’ to ‘pause requested’ so I think pg_wal_replay_pause should take > > > > > care of that. > > > > > > > > It makes sense to take care of this in pg_wal_replay_pause, but I wonder > > > > it can not handle the case that a user resume and pause again while a sleep. > > > > > > Right, we will have to check and set in the loop. But we should not > > > allow the state to go from paused to pause requested irrespective of > > > this. > > > > I agree with you. > > Is there any actual harm if PAUSED returns to REQUESETED, assuming we > immediately change the state to PAUSE always we see REQUESTED in the > waiting loop, despite that we allow change the state from PAUSE to > REQUESTED via NOT_PAUSED between two successive loop condition checks? If a user call pg_wal_replay_pause while recovery is paused, users can observe 'pause requested' during a sleep alghough the time window is short. It seems a bit odd that pg_wal_replay_pause changes the state like this because This state meeans that recovery may not be 'paused'. -- Yugo NAGATA <nagata@sraoss.co.jp>
On Mon, Feb 8, 2021 at 2:19 PM Yugo NAGATA <nagata@sraoss.co.jp> wrote: > > On Mon, 08 Feb 2021 17:32:46 +0900 (JST) > Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote: > > > At Mon, 8 Feb 2021 14:12:35 +0900, Yugo NAGATA <nagata@sraoss.co.jp> wrote in > > > > > > I think the right fix should be that the state should never go from > > > > > > ‘paused’ to ‘pause requested’ so I think pg_wal_replay_pause should take > > > > > > care of that. > > > > > > > > > > It makes sense to take care of this in pg_wal_replay_pause, but I wonder > > > > > it can not handle the case that a user resume and pause again while a sleep. > > > > > > > > Right, we will have to check and set in the loop. But we should not > > > > allow the state to go from paused to pause requested irrespective of > > > > this. > > > > > > I agree with you. > > > > Is there any actual harm if PAUSED returns to REQUESETED, assuming we > > immediately change the state to PAUSE always we see REQUESTED in the > > waiting loop, despite that we allow change the state from PAUSE to > > REQUESTED via NOT_PAUSED between two successive loop condition checks? > > If a user call pg_wal_replay_pause while recovery is paused, users can > observe 'pause requested' during a sleep alghough the time window is short. > It seems a bit odd that pg_wal_replay_pause changes the state like this > because This state meeans that recovery may not be 'paused'. Yeah, this appears wrong that after 'paused' we go back to 'pause requested'. the logical state transition should always be as below NOT PAUSED -> PAUSE REQUESTED or PAUSED (maybe we should always go to request and then paused but there is nothing wrong with going to paused) PAUSE REQUESTED -> NOT PAUSE or PAUSED (either cancel the request or get paused) PAUSED -> NOT PAUSED (from PAUSED we should not go to the PAUSE_REQUESTED without going to NOT PAUSED) -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
At Mon, 8 Feb 2021 17:05:52 +0530, Dilip Kumar <dilipbalaut@gmail.com> wrote in > On Mon, Feb 8, 2021 at 2:19 PM Yugo NAGATA <nagata@sraoss.co.jp> wrote: > > > > On Mon, 08 Feb 2021 17:32:46 +0900 (JST) > > Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote: > > > > > At Mon, 8 Feb 2021 14:12:35 +0900, Yugo NAGATA <nagata@sraoss.co.jp> wrote in > > > > > > > I think the right fix should be that the state should never go from > > > > > > > ‘paused’ to ‘pause requested’ so I think pg_wal_replay_pause should take > > > > > > > care of that. > > > > > > > > > > > > It makes sense to take care of this in pg_wal_replay_pause, but I wonder > > > > > > it can not handle the case that a user resume and pause again while a sleep. > > > > > > > > > > Right, we will have to check and set in the loop. But we should not > > > > > allow the state to go from paused to pause requested irrespective of > > > > > this. > > > > > > > > I agree with you. > > > > > > Is there any actual harm if PAUSED returns to REQUESETED, assuming we > > > immediately change the state to PAUSE always we see REQUESTED in the > > > waiting loop, despite that we allow change the state from PAUSE to > > > REQUESTED via NOT_PAUSED between two successive loop condition checks? > > > > If a user call pg_wal_replay_pause while recovery is paused, users can > > observe 'pause requested' during a sleep alghough the time window is short. > > It seems a bit odd that pg_wal_replay_pause changes the state like this > > because This state meeans that recovery may not be 'paused'. > > Yeah, this appears wrong that after 'paused' we go back to 'pause > requested'. the logical state transition should always be as below > > NOT PAUSED -> PAUSE REQUESTED or PAUSED (maybe we should always go to > request and then paused but there is nothing wrong with going to > paused) > PAUSE REQUESTED -> NOT PAUSE or PAUSED (either cancel the request or get paused) > PAUSED -> NOT PAUSED (from PAUSED we should not go to the > PAUSE_REQUESTED without going to NOT PAUSED) I didn't asked about the internal logical correctness, but asked about *actual harm* revealed to users. I don't see any actual harm in the "wrong" transition because: 1. It is not wrong nor strange that the invoker of pg_wal_replay_pause sees the state PAUSE_REQUESTED before it changes to PAUSED. Even if the previous state was PAUSED, it is no business of the requestors. 2. It is no harm in the recovery side since PAUSE_REQUESTED and PAUSED are effectively the same state. 3. After we inhibited the direct transition from PAUSED->PAUSE_REQUESTED, the effectively the same transition PAUSED->NOT_PAUSED->PAUSE_REQUESTED is still allowed. The inhibition of the former transition doesn't protect anything other than seeming correctness of the transition. If we are going to introduce that complexity, I'd like to re-propose to introduce interlocking between the recovery side and the pause-requestor side instead of introducing the intermediate state, which is the cause of the complexity. The problem is due to the looseness of checking for pause requests in the existing checkponts, and the window after the last checkpoint until calling rm_redo(). The attached PoC patch adds: - A solid checkpoint just before calling rm_redo. It doesn't add a info_lck since the check is done in the existing lock section. - Interlocking between the above and SetRecoveryPause without adding a shared variable. (This is what I called "synchronous" before.) There's a concern about pausing after updating XlogCtl->replayEndRecPtr but I don't see an issue yet.. regards. -- Kyotaro Horiguchi NTT Open Source Software Center diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c index 8e3b5df7dc..26aadb4178 100644 --- a/src/backend/access/transam/xlog.c +++ b/src/backend/access/transam/xlog.c @@ -6076,6 +6076,20 @@ void SetRecoveryPause(bool recoveryPause) { SpinLockAcquire(&XLogCtl->info_lck); + + /* + * Wait for the application of the record being applied to finish, so that + * no records will be applied after this function returns. We don't need to + * wait when ending a pause. Anyway we are requesting a recovery pause, we + * don't mind a possible slow down of recovery by the info_lck here. + */ + while(recoveryPause && !XLogCtl->recoveryPause && + XLogCtl->replayEndRecPtr != XLogCtl->lastReplayedEndRecPtr) + { + SpinLockRelease(&XLogCtl->info_lck); + pg_usleep(10000L); /* 10 ms */ + SpinLockAcquire(&XLogCtl->info_lck); + } XLogCtl->recoveryPause = recoveryPause; SpinLockRelease(&XLogCtl->info_lck); } @@ -7262,6 +7276,7 @@ StartupXLOG(void) do { bool switchedTLI = false; + bool pause_requested = false; #ifdef WAL_DEBUG if (XLOG_DEBUG || @@ -7292,11 +7307,9 @@ StartupXLOG(void) * Note that we intentionally don't take the info_lck spinlock * here. We might therefore read a slightly stale value of * the recoveryPause flag, but it can't be very stale (no - * worse than the last spinlock we did acquire). Since a - * pause request is a pretty asynchronous thing anyway, - * possibly responding to it one WAL record later than we - * otherwise would is a minor issue, so it doesn't seem worth - * adding another spinlock cycle to prevent that. + * worse than the last spinlock we did acquire). We eventually + * make sure catching the pause request if any just before + * applying this record. */ if (((volatile XLogCtlData *) XLogCtl)->recoveryPause) recoveryPausesHere(false); @@ -7385,12 +7398,19 @@ StartupXLOG(void) /* * Update shared replayEndRecPtr before replaying this record, * so that XLogFlush will update minRecoveryPoint correctly. + * Also we check for the correct value of the recoveryPause + * flag here not to have redo overrun during a pause. See + * SetRecoveryPuase() for details. */ SpinLockAcquire(&XLogCtl->info_lck); XLogCtl->replayEndRecPtr = EndRecPtr; XLogCtl->replayEndTLI = ThisTimeLineID; + pause_requested = XLogCtl->recoveryPause; SpinLockRelease(&XLogCtl->info_lck); + if (pause_requested) + recoveryPausesHere(false); + /* * If we are attempting to enter Hot Standby mode, process * XIDs we see
On Tue, 09 Feb 2021 10:58:04 +0900 (JST) Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote: > At Mon, 8 Feb 2021 17:05:52 +0530, Dilip Kumar <dilipbalaut@gmail.com> wrote in > > On Mon, Feb 8, 2021 at 2:19 PM Yugo NAGATA <nagata@sraoss.co.jp> wrote: > > > > > > On Mon, 08 Feb 2021 17:32:46 +0900 (JST) > > > Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote: > > > > > > > At Mon, 8 Feb 2021 14:12:35 +0900, Yugo NAGATA <nagata@sraoss.co.jp> wrote in > > > > > > > > I think the right fix should be that the state should never go from > > > > > > > > ‘paused’ to ‘pause requested’ so I think pg_wal_replay_pause should take > > > > > > > > care of that. > > > > > > > > > > > > > > It makes sense to take care of this in pg_wal_replay_pause, but I wonder > > > > > > > it can not handle the case that a user resume and pause again while a sleep. > > > > > > > > > > > > Right, we will have to check and set in the loop. But we should not > > > > > > allow the state to go from paused to pause requested irrespective of > > > > > > this. > > > > > > > > > > I agree with you. > > > > > > > > Is there any actual harm if PAUSED returns to REQUESETED, assuming we > > > > immediately change the state to PAUSE always we see REQUESTED in the > > > > waiting loop, despite that we allow change the state from PAUSE to > > > > REQUESTED via NOT_PAUSED between two successive loop condition checks? > > > > > > If a user call pg_wal_replay_pause while recovery is paused, users can > > > observe 'pause requested' during a sleep alghough the time window is short. > > > It seems a bit odd that pg_wal_replay_pause changes the state like this > > > because This state meeans that recovery may not be 'paused'. > > > > Yeah, this appears wrong that after 'paused' we go back to 'pause > > requested'. the logical state transition should always be as below > > > > NOT PAUSED -> PAUSE REQUESTED or PAUSED (maybe we should always go to > > request and then paused but there is nothing wrong with going to > > paused) > > PAUSE REQUESTED -> NOT PAUSE or PAUSED (either cancel the request or get paused) > > PAUSED -> NOT PAUSED (from PAUSED we should not go to the > > PAUSE_REQUESTED without going to NOT PAUSED) > > I didn't asked about the internal logical correctness, but asked about > *actual harm* revealed to users. I don't see any actual harm in the > "wrong" transition because: Actually, the incorrect state transition is not so harmful except that users can observe unnecessary state changes. However, I don't think any actual harm in prohibit the incorrect state transition. So, I think we can do it. > If we are going to introduce that complexity, I'd like to re-propose > to introduce interlocking between the recovery side and the > pause-requestor side instead of introducing the intermediate state, > which is the cause of the complexity. > > The attached PoC patch adds: > > - A solid checkpoint just before calling rm_redo. It doesn't add a > info_lck since the check is done in the existing lock section. > > - Interlocking between the above and SetRecoveryPause without adding a > shared variable. > (This is what I called "synchronous" before.) I think waiting in pg_wal_replay_pasue is a possible option, but this will also introduce other complexity to codes such as possibility of waiting for long or for ever. For example, waiting in SetRecoveryPause as in your POC patch appears to make recovery stuck in RecoveryRequiresIntParameter. By the way, attaching other patch to a thread without the original patch will make commitfest and cfbot APP confused... Regards, Yugo Nagata -- Yugo NAGATA <nagata@sraoss.co.jp>
On Tue, Feb 9, 2021 at 7:28 AM Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote: > > At Mon, 8 Feb 2021 17:05:52 +0530, Dilip Kumar <dilipbalaut@gmail.com> wrote in > > On Mon, Feb 8, 2021 at 2:19 PM Yugo NAGATA <nagata@sraoss.co.jp> wrote: > > > > > > On Mon, 08 Feb 2021 17:32:46 +0900 (JST) > > > Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote: > > > > > > > At Mon, 8 Feb 2021 14:12:35 +0900, Yugo NAGATA <nagata@sraoss.co.jp> wrote in > > > > > > > > I think the right fix should be that the state should never go from > > > > > > > > ‘paused’ to ‘pause requested’ so I think pg_wal_replay_pause should take > > > > > > > > care of that. > > > > > > > > > > > > > > It makes sense to take care of this in pg_wal_replay_pause, but I wonder > > > > > > > it can not handle the case that a user resume and pause again while a sleep. > > > > > > > > > > > > Right, we will have to check and set in the loop. But we should not > > > > > > allow the state to go from paused to pause requested irrespective of > > > > > > this. > > > > > > > > > > I agree with you. > > > > > > > > Is there any actual harm if PAUSED returns to REQUESETED, assuming we > > > > immediately change the state to PAUSE always we see REQUESTED in the > > > > waiting loop, despite that we allow change the state from PAUSE to > > > > REQUESTED via NOT_PAUSED between two successive loop condition checks? > > > > > > If a user call pg_wal_replay_pause while recovery is paused, users can > > > observe 'pause requested' during a sleep alghough the time window is short. > > > It seems a bit odd that pg_wal_replay_pause changes the state like this > > > because This state meeans that recovery may not be 'paused'. > > > > Yeah, this appears wrong that after 'paused' we go back to 'pause > > requested'. the logical state transition should always be as below > > > > NOT PAUSED -> PAUSE REQUESTED or PAUSED (maybe we should always go to > > request and then paused but there is nothing wrong with going to > > paused) > > PAUSE REQUESTED -> NOT PAUSE or PAUSED (either cancel the request or get paused) > > PAUSED -> NOT PAUSED (from PAUSED we should not go to the > > PAUSE_REQUESTED without going to NOT PAUSED) > > I didn't asked about the internal logical correctness, but asked about > *actual harm* revealed to users. I don't see any actual harm in the > "wrong" transition because: > > 1. It is not wrong nor strange that the invoker of pg_wal_replay_pause > sees the state PAUSE_REQUESTED before it changes to PAUSED. Even if > the previous state was PAUSED, it is no business of the requestors. The 'pg_wal_replay_pause' request to pause the recovery so it is fine to first change the state to PAUSE_REQUESTED and then to PAUSED. But if the recovery is already paused then what is the point in bringing the state back to PAUSE_REQUESTED. For example, suppose the tool want to raise the pause request and wait until recovery actually paused, so if it was already paused then if we bring it back to PAUSE_REQUESTED then it doesn't look correct and we might need to do an extra wait cycle in the tool until it reaches back to PAUSED, I don't think that is really a best design. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
On Tue, Feb 9, 2021 at 8:54 AM Yugo NAGATA <nagata@sraoss.co.jp> wrote: > > On Tue, 09 Feb 2021 10:58:04 +0900 (JST) > Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote: > > > At Mon, 8 Feb 2021 17:05:52 +0530, Dilip Kumar <dilipbalaut@gmail.com> wrote in > > > On Mon, Feb 8, 2021 at 2:19 PM Yugo NAGATA <nagata@sraoss.co.jp> wrote: > > > > > > > > On Mon, 08 Feb 2021 17:32:46 +0900 (JST) > > > > Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote: > > > > > > > > > At Mon, 8 Feb 2021 14:12:35 +0900, Yugo NAGATA <nagata@sraoss.co.jp> wrote in > > > > > > > > > I think the right fix should be that the state should never go from > > > > > > > > > ‘paused’ to ‘pause requested’ so I think pg_wal_replay_pause should take > > > > > > > > > care of that. > > > > > > > > > > > > > > > > It makes sense to take care of this in pg_wal_replay_pause, but I wonder > > > > > > > > it can not handle the case that a user resume and pause again while a sleep. > > > > > > > > > > > > > > Right, we will have to check and set in the loop. But we should not > > > > > > > allow the state to go from paused to pause requested irrespective of > > > > > > > this. > > > > > > > > > > > > I agree with you. > > > > > > > > > > Is there any actual harm if PAUSED returns to REQUESETED, assuming we > > > > > immediately change the state to PAUSE always we see REQUESTED in the > > > > > waiting loop, despite that we allow change the state from PAUSE to > > > > > REQUESTED via NOT_PAUSED between two successive loop condition checks? > > > > > > > > If a user call pg_wal_replay_pause while recovery is paused, users can > > > > observe 'pause requested' during a sleep alghough the time window is short. > > > > It seems a bit odd that pg_wal_replay_pause changes the state like this > > > > because This state meeans that recovery may not be 'paused'. > > > > > > Yeah, this appears wrong that after 'paused' we go back to 'pause > > > requested'. the logical state transition should always be as below > > > > > > NOT PAUSED -> PAUSE REQUESTED or PAUSED (maybe we should always go to > > > request and then paused but there is nothing wrong with going to > > > paused) > > > PAUSE REQUESTED -> NOT PAUSE or PAUSED (either cancel the request or get paused) > > > PAUSED -> NOT PAUSED (from PAUSED we should not go to the > > > PAUSE_REQUESTED without going to NOT PAUSED) > > > > I didn't asked about the internal logical correctness, but asked about > > *actual harm* revealed to users. I don't see any actual harm in the > > "wrong" transition because: > > Actually, the incorrect state transition is not so harmful except that > users can observe unnecessary state changes. However, I don't think any > actual harm in prohibit the incorrect state transition. So, I think we > can do it. > > > If we are going to introduce that complexity, I'd like to re-propose > > to introduce interlocking between the recovery side and the > > pause-requestor side instead of introducing the intermediate state, > > which is the cause of the complexity. > > > > The attached PoC patch adds: > > > > - A solid checkpoint just before calling rm_redo. It doesn't add a > > info_lck since the check is done in the existing lock section. > > > > - Interlocking between the above and SetRecoveryPause without adding a > > shared variable. > > (This is what I called "synchronous" before.) > > I think waiting in pg_wal_replay_pasue is a possible option, but this will > also introduce other complexity to codes such as possibility of waiting for > long or for ever. For example, waiting in SetRecoveryPause as in your POC > patch appears to make recovery stuck in RecoveryRequiresIntParameter. > I agree with this, I think we previously discussed these approaches where we can wait in pg_wal_replay_pasue() or pg_is_wal_replay_pasued(). In fact, we had an older version where we put the wait in pg_is_wal_replay_pasued(). But it appeared that doing so will add extra complexity as well as instead of waiting in these APIs the wait logic can be implemented in the application code which is actually using these APIs and IMHO that will give better control to the users. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
On Tue, Feb 9, 2021 at 9:48 AM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > On Tue, Feb 9, 2021 at 8:54 AM Yugo NAGATA <nagata@sraoss.co.jp> wrote: > > > > On Tue, 09 Feb 2021 10:58:04 +0900 (JST) > > Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote: > > > > > At Mon, 8 Feb 2021 17:05:52 +0530, Dilip Kumar <dilipbalaut@gmail.com> wrote in > > > > On Mon, Feb 8, 2021 at 2:19 PM Yugo NAGATA <nagata@sraoss.co.jp> wrote: > > > > > > > > > > On Mon, 08 Feb 2021 17:32:46 +0900 (JST) > > > > > Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote: > > > > > > > > > > > At Mon, 8 Feb 2021 14:12:35 +0900, Yugo NAGATA <nagata@sraoss.co.jp> wrote in > > > > > > > > > > I think the right fix should be that the state should never go from > > > > > > > > > > ‘paused’ to ‘pause requested’ so I think pg_wal_replay_pause should take > > > > > > > > > > care of that. > > > > > > > > > > > > > > > > > > It makes sense to take care of this in pg_wal_replay_pause, but I wonder > > > > > > > > > it can not handle the case that a user resume and pause again while a sleep. > > > > > > > > > > > > > > > > Right, we will have to check and set in the loop. But we should not > > > > > > > > allow the state to go from paused to pause requested irrespective of > > > > > > > > this. > > > > > > > > > > > > > > I agree with you. > > > > > > > > > > > > Is there any actual harm if PAUSED returns to REQUESETED, assuming we > > > > > > immediately change the state to PAUSE always we see REQUESTED in the > > > > > > waiting loop, despite that we allow change the state from PAUSE to > > > > > > REQUESTED via NOT_PAUSED between two successive loop condition checks? > > > > > > > > > > If a user call pg_wal_replay_pause while recovery is paused, users can > > > > > observe 'pause requested' during a sleep alghough the time window is short. > > > > > It seems a bit odd that pg_wal_replay_pause changes the state like this > > > > > because This state meeans that recovery may not be 'paused'. > > > > > > > > Yeah, this appears wrong that after 'paused' we go back to 'pause > > > > requested'. the logical state transition should always be as below > > > > > > > > NOT PAUSED -> PAUSE REQUESTED or PAUSED (maybe we should always go to > > > > request and then paused but there is nothing wrong with going to > > > > paused) > > > > PAUSE REQUESTED -> NOT PAUSE or PAUSED (either cancel the request or get paused) > > > > PAUSED -> NOT PAUSED (from PAUSED we should not go to the > > > > PAUSE_REQUESTED without going to NOT PAUSED) > > > > > > I didn't asked about the internal logical correctness, but asked about > > > *actual harm* revealed to users. I don't see any actual harm in the > > > "wrong" transition because: > > > > Actually, the incorrect state transition is not so harmful except that > > users can observe unnecessary state changes. However, I don't think any > > actual harm in prohibit the incorrect state transition. So, I think we > > can do it. > > > > > If we are going to introduce that complexity, I'd like to re-propose > > > to introduce interlocking between the recovery side and the > > > pause-requestor side instead of introducing the intermediate state, > > > which is the cause of the complexity. > > > > > > The attached PoC patch adds: > > > > > > - A solid checkpoint just before calling rm_redo. It doesn't add a > > > info_lck since the check is done in the existing lock section. > > > > > > - Interlocking between the above and SetRecoveryPause without adding a > > > shared variable. > > > (This is what I called "synchronous" before.) > > > > I think waiting in pg_wal_replay_pasue is a possible option, but this will > > also introduce other complexity to codes such as possibility of waiting for > > long or for ever. For example, waiting in SetRecoveryPause as in your POC > > patch appears to make recovery stuck in RecoveryRequiresIntParameter. > > > > I agree with this, I think we previously discussed these approaches > where we can wait in pg_wal_replay_pasue() or > pg_is_wal_replay_pasued(). In fact, we had an older version where we > put the wait in pg_is_wal_replay_pasued(). But it appeared that doing > so will add extra complexity as well as instead of waiting in these > APIs the wait logic can be implemented in the application code which > is actually using these APIs and IMHO that will give better control to > the users. And also, having waiting logic in pg_wal_replay_pasue() or pg_is_wal_replay_pasued() required changes to the existing API such as a timeout to not allow them infinitely waiting. With Regards, Bharath Rupireddy. EnterpriseDB: http://www.enterprisedb.com
At Tue, 9 Feb 2021 12:23:23 +0900, Yugo NAGATA <nagata@sraoss.co.jp> wrote in > On Tue, 09 Feb 2021 10:58:04 +0900 (JST) > Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote: > > I didn't asked about the internal logical correctness, but asked about > > *actual harm* revealed to users. I don't see any actual harm in the > > "wrong" transition because: > > Actually, the incorrect state transition is not so harmful except that > users can observe unnecessary state changes. However, I don't think any > actual harm in prohibit the incorrect state transition. So, I think we > can do it. I don't say that we cannot do that. Just it is needeless. > > If we are going to introduce that complexity, I'd like to re-propose > > to introduce interlocking between the recovery side and the > > pause-requestor side instead of introducing the intermediate state, > > which is the cause of the complexity. > > > > The attached PoC patch adds: > > > > - A solid checkpoint just before calling rm_redo. It doesn't add a > > info_lck since the check is done in the existing lock section. > > > > - Interlocking between the above and SetRecoveryPause without adding a > > shared variable. > > (This is what I called "synchronous" before.) > > I think waiting in pg_wal_replay_pasue is a possible option, but this will > also introduce other complexity to codes such as possibility of waiting for > long or for ever. For example, waiting in SetRecoveryPause as in your POC > patch appears to make recovery stuck in RecoveryRequiresIntParameter. That is easily avoidable CFI in the loop. > By the way, attaching other patch to a thread without the original patch > will make commitfest and cfbot APP confused... Oops! Sorry for that. I forgot to append .txt or such to the file name. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
At Tue, 9 Feb 2021 09:47:58 +0530, Dilip Kumar <dilipbalaut@gmail.com> wrote in > On Tue, Feb 9, 2021 at 8:54 AM Yugo NAGATA <nagata@sraoss.co.jp> wrote: > > > > On Tue, 09 Feb 2021 10:58:04 +0900 (JST) > > Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote: > > > > > At Mon, 8 Feb 2021 17:05:52 +0530, Dilip Kumar <dilipbalaut@gmail.com> wrote in > > > > On Mon, Feb 8, 2021 at 2:19 PM Yugo NAGATA <nagata@sraoss.co.jp> wrote: > > > > > > > > > > On Mon, 08 Feb 2021 17:32:46 +0900 (JST) > > > > > Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote: > > > > > > > > > > > At Mon, 8 Feb 2021 14:12:35 +0900, Yugo NAGATA <nagata@sraoss.co.jp> wrote in > > > > > > > > > > I think the right fix should be that the state should never go from > > > > > > > > > > ‘paused’ to ‘pause requested’ so I think pg_wal_replay_pause should take > > > > > > > > > > care of that. > > > > > > > > > > > > > > > > > > It makes sense to take care of this in pg_wal_replay_pause, but I wonder > > > > > > > > > it can not handle the case that a user resume and pause again while a sleep. > > > > > > > > > > > > > > > > Right, we will have to check and set in the loop. But we should not > > > > > > > > allow the state to go from paused to pause requested irrespective of > > > > > > > > this. > > > > > > > > > > > > > > I agree with you. > > > > > > > > > > > > Is there any actual harm if PAUSED returns to REQUESETED, assuming we > > > > > > immediately change the state to PAUSE always we see REQUESTED in the > > > > > > waiting loop, despite that we allow change the state from PAUSE to > > > > > > REQUESTED via NOT_PAUSED between two successive loop condition checks? > > > > > > > > > > If a user call pg_wal_replay_pause while recovery is paused, users can > > > > > observe 'pause requested' during a sleep alghough the time window is short. > > > > > It seems a bit odd that pg_wal_replay_pause changes the state like this > > > > > because This state meeans that recovery may not be 'paused'. > > > > > > > > Yeah, this appears wrong that after 'paused' we go back to 'pause > > > > requested'. the logical state transition should always be as below > > > > > > > > NOT PAUSED -> PAUSE REQUESTED or PAUSED (maybe we should always go to > > > > request and then paused but there is nothing wrong with going to > > > > paused) > > > > PAUSE REQUESTED -> NOT PAUSE or PAUSED (either cancel the request or get paused) > > > > PAUSED -> NOT PAUSED (from PAUSED we should not go to the > > > > PAUSE_REQUESTED without going to NOT PAUSED) > > > > > > I didn't asked about the internal logical correctness, but asked about > > > *actual harm* revealed to users. I don't see any actual harm in the > > > "wrong" transition because: > > > > Actually, the incorrect state transition is not so harmful except that > > users can observe unnecessary state changes. However, I don't think any > > actual harm in prohibit the incorrect state transition. So, I think we > > can do it. > > > > > If we are going to introduce that complexity, I'd like to re-propose > > > to introduce interlocking between the recovery side and the > > > pause-requestor side instead of introducing the intermediate state, > > > which is the cause of the complexity. > > > > > > The attached PoC patch adds: > > > > > > - A solid checkpoint just before calling rm_redo. It doesn't add a > > > info_lck since the check is done in the existing lock section. > > > > > > - Interlocking between the above and SetRecoveryPause without adding a > > > shared variable. > > > (This is what I called "synchronous" before.) > > > > I think waiting in pg_wal_replay_pasue is a possible option, but this will > > also introduce other complexity to codes such as possibility of waiting for > > long or for ever. For example, waiting in SetRecoveryPause as in your POC > > patch appears to make recovery stuck in RecoveryRequiresIntParameter. Ah. Yes, startup process does not need to wait. That is a bug of the patch. No other callers don't cause the self dead lock. > I agree with this, I think we previously discussed these approaches > where we can wait in pg_wal_replay_pasue() or > pg_is_wal_replay_pasued(). In fact, we had an older version where we > put the wait in pg_is_wal_replay_pasued(). But it appeared that doing Note that the expected waiting period is while calling rmgr_redo(). If it is stuck for a long time, that suggests something's going wrong. > so will add extra complexity as well as instead of waiting in these > APIs the wait logic can be implemented in the application code which > is actually using these APIs and IMHO that will give better control to > the users. Year, with the PoC pg_wal_replay_pause() can make a short wait as a side-effect but the tri-state patch also can add a function to wait for the state suffices. regards. -- Kyotaro Horiguchi NTT Open Source Software Center diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c index 8e3b5df7dc..194a2f9998 100644 --- a/src/backend/access/transam/xlog.c +++ b/src/backend/access/transam/xlog.c @@ -6076,6 +6076,23 @@ void SetRecoveryPause(bool recoveryPause) { SpinLockAcquire(&XLogCtl->info_lck); + + /* + * Wait for the application of the record being applied to finish, so that + * no records will be applied after this function returns. We don't need to + * wait when ending a pause. Anyway we are requesting a recovery pause, we + * don't mind a possible slow down of recovery by the info_lck here. + * We don't need to wait in the startup process. + */ + while(InRecovery && + recoveryPause && !XLogCtl->recoveryPause && + XLogCtl->replayEndRecPtr != XLogCtl->lastReplayedEndRecPtr) + { + SpinLockRelease(&XLogCtl->info_lck); + CHECK_FOR_INTERRUPTS(); + pg_usleep(10000L); /* 10 ms */ + SpinLockAcquire(&XLogCtl->info_lck); + } XLogCtl->recoveryPause = recoveryPause; SpinLockRelease(&XLogCtl->info_lck); } @@ -7262,6 +7279,7 @@ StartupXLOG(void) do { bool switchedTLI = false; + bool pause_requested = false; #ifdef WAL_DEBUG if (XLOG_DEBUG || @@ -7292,11 +7310,9 @@ StartupXLOG(void) * Note that we intentionally don't take the info_lck spinlock * here. We might therefore read a slightly stale value of * the recoveryPause flag, but it can't be very stale (no - * worse than the last spinlock we did acquire). Since a - * pause request is a pretty asynchronous thing anyway, - * possibly responding to it one WAL record later than we - * otherwise would is a minor issue, so it doesn't seem worth - * adding another spinlock cycle to prevent that. + * worse than the last spinlock we did acquire). We eventually + * make sure catching the pause request if any just before + * applying this record. */ if (((volatile XLogCtlData *) XLogCtl)->recoveryPause) recoveryPausesHere(false); @@ -7385,12 +7401,19 @@ StartupXLOG(void) /* * Update shared replayEndRecPtr before replaying this record, * so that XLogFlush will update minRecoveryPoint correctly. + * Also we check for the correct value of the recoveryPause + * flag here not to have redo overrun during a pause. See + * SetRecoveryPuase() for details. */ SpinLockAcquire(&XLogCtl->info_lck); XLogCtl->replayEndRecPtr = EndRecPtr; XLogCtl->replayEndTLI = ThisTimeLineID; + pause_requested = XLogCtl->recoveryPause; SpinLockRelease(&XLogCtl->info_lck); + if (pause_requested) + recoveryPausesHere(false); + /* * If we are attempting to enter Hot Standby mode, process * XIDs we see
At Tue, 9 Feb 2021 09:58:30 +0530, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote in > On Tue, Feb 9, 2021 at 9:48 AM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > > On Tue, Feb 9, 2021 at 8:54 AM Yugo NAGATA <nagata@sraoss.co.jp> wrote: > > > > > > On Tue, 09 Feb 2021 10:58:04 +0900 (JST) > > > Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote: > > > > If we are going to introduce that complexity, I'd like to re-propose > > > > to introduce interlocking between the recovery side and the > > > > pause-requestor side instead of introducing the intermediate state, > > > > which is the cause of the complexity. > > > > > > > > The attached PoC patch adds: > > > > > > > > - A solid checkpoint just before calling rm_redo. It doesn't add a > > > > info_lck since the check is done in the existing lock section. > > > > > > > > - Interlocking between the above and SetRecoveryPause without adding a > > > > shared variable. > > > > (This is what I called "synchronous" before.) > > > > > > I think waiting in pg_wal_replay_pasue is a possible option, but this will > > > also introduce other complexity to codes such as possibility of waiting for > > > long or for ever. For example, waiting in SetRecoveryPause as in your POC > > > patch appears to make recovery stuck in RecoveryRequiresIntParameter. > > > > > > > I agree with this, I think we previously discussed these approaches > > where we can wait in pg_wal_replay_pasue() or > > pg_is_wal_replay_pasued(). In fact, we had an older version where we > > put the wait in pg_is_wal_replay_pasued(). But it appeared that doing > > so will add extra complexity as well as instead of waiting in these > > APIs the wait logic can be implemented in the application code which > > is actually using these APIs and IMHO that will give better control to > > the users. > > And also, having waiting logic in pg_wal_replay_pasue() or > pg_is_wal_replay_pasued() required changes to the existing API such as > a timeout to not allow them infinitely waiting. I don't understand that. pg_wal_replay_pause() is defined as "pausees recovery". so it is the correct behavior to wait actual pause. pg_is_wal_replay_paused() doesn't wait for anything at all. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Sorry, I made a mistake here. At Tue, 09 Feb 2021 14:55:23 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in > At Tue, 9 Feb 2021 09:47:58 +0530, Dilip Kumar <dilipbalaut@gmail.com> wrote in > > APIs the wait logic can be implemented in the application code which > > is actually using these APIs and IMHO that will give better control to > > the users. > > Year, with the PoC pg_wal_replay_pause() can make a short wait as a > side-effect but the tri-state patch also can add a function to wait > for the state suffices. I said that it is surprising that pg_is_wal_replay_paused() waits for the state change. But I didn't say that pg_wal_replay_pause() shouldn't wait for the actual pause. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
On Tue, Feb 9, 2021 at 11:30 AM Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote: > > At Tue, 9 Feb 2021 09:58:30 +0530, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote in > > On Tue, Feb 9, 2021 at 9:48 AM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > > > > On Tue, Feb 9, 2021 at 8:54 AM Yugo NAGATA <nagata@sraoss.co.jp> wrote: > > > > > > > > On Tue, 09 Feb 2021 10:58:04 +0900 (JST) > > > > Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote: > > > > > If we are going to introduce that complexity, I'd like to re-propose > > > > > to introduce interlocking between the recovery side and the > > > > > pause-requestor side instead of introducing the intermediate state, > > > > > which is the cause of the complexity. > > > > > > > > > > The attached PoC patch adds: > > > > > > > > > > - A solid checkpoint just before calling rm_redo. It doesn't add a > > > > > info_lck since the check is done in the existing lock section. > > > > > > > > > > - Interlocking between the above and SetRecoveryPause without adding a > > > > > shared variable. > > > > > (This is what I called "synchronous" before.) > > > > > > > > I think waiting in pg_wal_replay_pasue is a possible option, but this will > > > > also introduce other complexity to codes such as possibility of waiting for > > > > long or for ever. For example, waiting in SetRecoveryPause as in your POC > > > > patch appears to make recovery stuck in RecoveryRequiresIntParameter. > > > > > > > > > > I agree with this, I think we previously discussed these approaches > > > where we can wait in pg_wal_replay_pasue() or > > > pg_is_wal_replay_pasued(). In fact, we had an older version where we > > > put the wait in pg_is_wal_replay_pasued(). But it appeared that doing > > > so will add extra complexity as well as instead of waiting in these > > > APIs the wait logic can be implemented in the application code which > > > is actually using these APIs and IMHO that will give better control to > > > the users. > > > > And also, having waiting logic in pg_wal_replay_pasue() or > > pg_is_wal_replay_pasued() required changes to the existing API such as > > a timeout to not allow them infinitely waiting. > > I don't understand that. pg_wal_replay_pause() is defined as "pausees > recovery". so it is the correct behavior to wait actual pause. > pg_is_wal_replay_paused() doesn't wait for anything at all. What I meant was that if we were to add waiting logic inside pg_wal_replay_pause, we should also have a timeout with some default value, to avoid pg_wal_replay_pause waiting forever in the waiting loop. Within that timeout, if the recovery isn't paused, pg_wal_replay_pause will return probably a warning and a false(this requires us to change the return value of the existing pg_wal_replay_pause)? To avoid changing the existing API and return type, a new function pg_get_wal_replay_pause_state is introduced. With Regards, Bharath Rupireddy. EnterpriseDB: http://www.enterprisedb.com
At Tue, 9 Feb 2021 12:27:21 +0530, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote in > What I meant was that if we were to add waiting logic inside > pg_wal_replay_pause, we should also have a timeout with some default > value, to avoid pg_wal_replay_pause waiting forever in the waiting > loop. Within that timeout, if the recovery isn't paused, > pg_wal_replay_pause will return probably a warning and a false(this > requires us to change the return value of the existing > pg_wal_replay_pause)? I thought that rm_redo finishes shortly unless any trouble happens. But on second thought, I found that I forgot a case of a recovery-conflict. So as you pointed out, pg_wal_replay_pause() needs a flag 'wait' to wait for a pause established. And the flag can be turned into "timeout". # And the prevous verision had another silly bug. > To avoid changing the existing API and return type, a new function > pg_get_wal_replay_pause_state is introduced. I mentioned about IN parameters, not OUTs. IN parameters can be optional to accept existing usage. pg_wal_replay_pause() is changed that way in the attached. If all of you still disagree with my proposal, I withdraw it. regards. -- Kyotaro Horiguchi NTT Open Source Software Center diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml index 1ab31a9056..7eb93f74dd 100644 --- a/doc/src/sgml/func.sgml +++ b/doc/src/sgml/func.sgml @@ -25320,14 +25320,19 @@ postgres=# SELECT * FROM pg_walfile_name_offset(pg_stop_backup()); <indexterm> <primary>pg_wal_replay_pause</primary> </indexterm> - <function>pg_wal_replay_pause</function> () + <function>pg_wal_replay_pause</function> ( + <optional> <parameter>timeout</parameter> <type>integer</type> + </optional> ) <returnvalue>void</returnvalue> </para> <para> Pauses recovery. While recovery is paused, no further database changes are applied. If hot standby is active, all new queries will see the same consistent snapshot of the database, and no further query - conflicts will be generated until recovery is resumed. + conflicts will be generated until recovery is resumed. Zero or + positive timeout value means the function errors out after that + milliseconds elapsed before recovery is paused (default is -1, wait + forever). </para> <para> This function is restricted to superusers by default, but other users diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c index 8e3b5df7dc..8fd614cded 100644 --- a/src/backend/access/transam/xlog.c +++ b/src/backend/access/transam/xlog.c @@ -6072,10 +6072,61 @@ RecoveryIsPaused(void) return recoveryPause; } +/* + * Pauses recovery. + * + * It is guaranteed that no WAL replay happens after this function returns. If + * timeout is zero or positive, emits ERROR when the timeout is reached before + * recovery is paused. + */ void -SetRecoveryPause(bool recoveryPause) +SetRecoveryPause(bool recoveryPause, int timeout) { + TimestampTz finish_time = 0; + TimestampTz now; + int sleep_ms; + SpinLockAcquire(&XLogCtl->info_lck); + + /* No need of timeout in the startup process */ + Assert(!InRecovery || timeout < 0); + + /* + * Wait for the concurrent rm_redo() to finish, so that no records will be + * applied after this function returns. No need to wait while resuming. + * Anyway we are requesting a recovery pause, we don't mind a possible slow + * down of recovery by the info_lck here. We don't need to wait in the + * startup process since no concurrent rm_redo() runs. + */ + while(!InRecovery && + recoveryPause && !XLogCtl->recoveryPause && + XLogCtl->replayEndRecPtr != XLogCtl->lastReplayedEndRecPtr) + { + SpinLockRelease(&XLogCtl->info_lck); + now = GetCurrentTimestamp(); + + if (timeout >= 0) + { + if (timeout > 0 && finish_time == 0) + finish_time = TimestampTzPlusMilliseconds(now, timeout); + + if (finish_time < now) + ereport(ERROR, + (errcode(ERRCODE_SQL_STATEMENT_NOT_YET_COMPLETE), + errmsg ("could not pause recovery: timed out"))); + } + + CHECK_FOR_INTERRUPTS(); + + sleep_ms = 10000L; /* 10 ms */ + + /* finish_time may be reached earlier than 10ms */ + if (finish_time > 0) + Min(sleep_ms, TimestampDifferenceMilliseconds(now, finish_time)); + + pg_usleep(sleep_ms); + SpinLockAcquire(&XLogCtl->info_lck); + } XLogCtl->recoveryPause = recoveryPause; SpinLockRelease(&XLogCtl->info_lck); } @@ -6270,7 +6321,7 @@ RecoveryRequiresIntParameter(const char *param_name, int currValue, int minValue currValue, minValue))); - SetRecoveryPause(true); + SetRecoveryPause(true, -1); ereport(LOG, (errmsg("recovery has paused"), @@ -7262,6 +7313,7 @@ StartupXLOG(void) do { bool switchedTLI = false; + bool pause_requested = false; #ifdef WAL_DEBUG if (XLOG_DEBUG || @@ -7292,11 +7344,9 @@ StartupXLOG(void) * Note that we intentionally don't take the info_lck spinlock * here. We might therefore read a slightly stale value of * the recoveryPause flag, but it can't be very stale (no - * worse than the last spinlock we did acquire). Since a - * pause request is a pretty asynchronous thing anyway, - * possibly responding to it one WAL record later than we - * otherwise would is a minor issue, so it doesn't seem worth - * adding another spinlock cycle to prevent that. + * worse than the last spinlock we did acquire). We eventually + * make sure catching the pause request if any just before + * applying this record. */ if (((volatile XLogCtlData *) XLogCtl)->recoveryPause) recoveryPausesHere(false); @@ -7385,12 +7435,19 @@ StartupXLOG(void) /* * Update shared replayEndRecPtr before replaying this record, * so that XLogFlush will update minRecoveryPoint correctly. + * Also we check for the correct value of the recoveryPause + * flag here not to have redo overrun during a pause. See + * SetRecoveryPuase() for details. */ SpinLockAcquire(&XLogCtl->info_lck); XLogCtl->replayEndRecPtr = EndRecPtr; XLogCtl->replayEndTLI = ThisTimeLineID; + pause_requested = XLogCtl->recoveryPause; SpinLockRelease(&XLogCtl->info_lck); + if (pause_requested) + recoveryPausesHere(false); + /* * If we are attempting to enter Hot Standby mode, process * XIDs we see @@ -7497,7 +7554,7 @@ StartupXLOG(void) proc_exit(3); case RECOVERY_TARGET_ACTION_PAUSE: - SetRecoveryPause(true); + SetRecoveryPause(true, -1); recoveryPausesHere(true); /* drop into promote */ diff --git a/src/backend/access/transam/xlogfuncs.c b/src/backend/access/transam/xlogfuncs.c index 5e1aab319d..4c8c41e0bc 100644 --- a/src/backend/access/transam/xlogfuncs.c +++ b/src/backend/access/transam/xlogfuncs.c @@ -538,7 +538,7 @@ pg_wal_replay_pause(PG_FUNCTION_ARGS) errhint("%s cannot be executed after promotion is triggered.", "pg_wal_replay_pause()"))); - SetRecoveryPause(true); + SetRecoveryPause(true, PG_GETARG_INT32(0)); PG_RETURN_VOID(); } @@ -565,7 +565,7 @@ pg_wal_replay_resume(PG_FUNCTION_ARGS) errhint("%s cannot be executed after promotion is triggered.", "pg_wal_replay_resume()"))); - SetRecoveryPause(false); + SetRecoveryPause(false, -1); PG_RETURN_VOID(); } diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql index fa58afd9d7..e03f22f350 100644 --- a/src/backend/catalog/system_views.sql +++ b/src/backend/catalog/system_views.sql @@ -1264,6 +1264,11 @@ CREATE OR REPLACE FUNCTION RETURNS boolean STRICT VOLATILE LANGUAGE INTERNAL AS 'pg_promote' PARALLEL SAFE; +CREATE OR REPLACE FUNCTION + pg_wal_replay_pause(timeout int4 DEFAULT -1) + RETURNS void VOLATILE LANGUAGE internal AS 'pg_wal_replay_pause' + PARALLEL SAFE; + -- legacy definition for compatibility with 9.3 CREATE OR REPLACE FUNCTION json_populate_record(base anyelement, from_json json, use_json_as_text boolean DEFAULT false) @@ -1473,7 +1478,7 @@ REVOKE EXECUTE ON FUNCTION pg_stop_backup() FROM public; REVOKE EXECUTE ON FUNCTION pg_stop_backup(boolean, boolean) FROM public; REVOKE EXECUTE ON FUNCTION pg_create_restore_point(text) FROM public; REVOKE EXECUTE ON FUNCTION pg_switch_wal() FROM public; -REVOKE EXECUTE ON FUNCTION pg_wal_replay_pause() FROM public; +REVOKE EXECUTE ON FUNCTION pg_wal_replay_pause(int) FROM public; REVOKE EXECUTE ON FUNCTION pg_wal_replay_resume() FROM public; REVOKE EXECUTE ON FUNCTION pg_rotate_logfile() FROM public; REVOKE EXECUTE ON FUNCTION pg_reload_conf() FROM public; diff --git a/src/include/access/xlog.h b/src/include/access/xlog.h index 75ec1073bd..397e206433 100644 --- a/src/include/access/xlog.h +++ b/src/include/access/xlog.h @@ -311,7 +311,7 @@ extern XLogRecPtr GetXLogReplayRecPtr(TimeLineID *replayTLI); extern XLogRecPtr GetXLogInsertRecPtr(void); extern XLogRecPtr GetXLogWriteRecPtr(void); extern bool RecoveryIsPaused(void); -extern void SetRecoveryPause(bool recoveryPause); +extern void SetRecoveryPause(bool recoveryPause, int timeout); extern TimestampTz GetLatestXTime(void); extern TimestampTz GetCurrentChunkReplayStartTime(void); diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat index 4e0c9be58c..a646721c3c 100644 --- a/src/include/catalog/pg_proc.dat +++ b/src/include/catalog/pg_proc.dat @@ -6222,7 +6222,7 @@ { oid => '3071', descr => 'pause wal replay', proname => 'pg_wal_replay_pause', provolatile => 'v', prorettype => 'void', - proargtypes => '', prosrc => 'pg_wal_replay_pause' }, + proargtypes => 'int4', prosrc => 'pg_wal_replay_pause' }, { oid => '3072', descr => 'resume wal replay, if it was paused', proname => 'pg_wal_replay_resume', provolatile => 'v', prorettype => 'void', proargtypes => '', prosrc => 'pg_wal_replay_resume' },
On Wed, Feb 10, 2021 at 8:19 AM Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote: > > At Tue, 9 Feb 2021 12:27:21 +0530, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote in > > What I meant was that if we were to add waiting logic inside > > pg_wal_replay_pause, we should also have a timeout with some default > > value, to avoid pg_wal_replay_pause waiting forever in the waiting > > loop. Within that timeout, if the recovery isn't paused, > > pg_wal_replay_pause will return probably a warning and a false(this > > requires us to change the return value of the existing > > pg_wal_replay_pause)? > > I thought that rm_redo finishes shortly unless any trouble > happens. But on second thought, I found that I forgot a case of a > recovery-conflict. So as you pointed out, pg_wal_replay_pause() needs > a flag 'wait' to wait for a pause established. And the flag can be > turned into "timeout". > > # And the prevous verision had another silly bug. > > > To avoid changing the existing API and return type, a new function > > pg_get_wal_replay_pause_state is introduced. > > I mentioned about IN parameters, not OUTs. IN parameters can be > optional to accept existing usage. pg_wal_replay_pause() is changed > that way in the attached. > > If all of you still disagree with my proposal, I withdraw it. I don't find any problem with this approach as well, but I personally feel that the other approach where we don't wait in any API and just return the recovery pause state is much simpler and more flexible. So I will make the pending changes in that patch and let's see what are the other opinion and based on that we can conclude. Thanks for the patch. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
On Wed, Feb 10, 2021 at 10:02 AM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > I don't find any problem with this approach as well, but I personally > feel that the other approach where we don't wait in any API and just > return the recovery pause state is much simpler and more flexible. So > I will make the pending changes in that patch and let's see what are > the other opinion and based on that we can conclude. Thanks for the > patch. Here is an updated version of the patch which fixes the last two open problems 1. In RecoveryRequiresIntParameter set the recovery pause state in the loop so that if recovery resumed and pause requested again we can set to pause again. 2. If the recovery state is already 'paused' then don't set it back to the 'pause requested'. One more point is that in 'pg_wal_replay_pause' even if we don't change the state because it was already set to the 'paused' then also we call the WakeupRecovery. But I don't think there is any problem with that, if we think that this should be changed then we can make SetRecoveryPause return a bool such that if it doesn't do state change then it returns false and in that case we can avoid calling WakeupRecovery, but I felt that is unnecessary. Any other thoughts on this? -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
Attachment
On Wed, Feb 10, 2021 at 8:39 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > On Wed, Feb 10, 2021 at 10:02 AM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > > I don't find any problem with this approach as well, but I personally > > feel that the other approach where we don't wait in any API and just > > return the recovery pause state is much simpler and more flexible. So > > I will make the pending changes in that patch and let's see what are > > the other opinion and based on that we can conclude. Thanks for the > > patch. > > Here is an updated version of the patch which fixes the last two open problems > 1. In RecoveryRequiresIntParameter set the recovery pause state in the > loop so that if recovery resumed and pause requested again we can set > to pause again. > 2. If the recovery state is already 'paused' then don't set it back to > the 'pause requested'. > > One more point is that in 'pg_wal_replay_pause' even if we don't > change the state because it was already set to the 'paused' then also > we call the WakeupRecovery. But I don't think there is any problem > with that, if we think that this should be changed then we can make > SetRecoveryPause return a bool such that if it doesn't do state change > then it returns false and in that case we can avoid calling > WakeupRecovery, but I felt that is unnecessary. Any other thoughts on > this? IMO, that WakeupRecovery should not be a problem, because even now, if we issue a simple select pg_reload_conf(); (without even changing any config parameter), WakeupRecovery gets called. Thanks for the patch. I tested the new function and it works as expected. I have no further comments on the v13 patch. With Regards, Bharath Rupireddy. EnterpriseDB: http://www.enterprisedb.com
On Thu, Feb 11, 2021 at 3:20 PM Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote: > > On Wed, Feb 10, 2021 at 8:39 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > > On Wed, Feb 10, 2021 at 10:02 AM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > > > > I don't find any problem with this approach as well, but I personally > > > feel that the other approach where we don't wait in any API and just > > > return the recovery pause state is much simpler and more flexible. So > > > I will make the pending changes in that patch and let's see what are > > > the other opinion and based on that we can conclude. Thanks for the > > > patch. > > > > Here is an updated version of the patch which fixes the last two open problems > > 1. In RecoveryRequiresIntParameter set the recovery pause state in the > > loop so that if recovery resumed and pause requested again we can set > > to pause again. > > 2. If the recovery state is already 'paused' then don't set it back to > > the 'pause requested'. > > > > One more point is that in 'pg_wal_replay_pause' even if we don't > > change the state because it was already set to the 'paused' then also > > we call the WakeupRecovery. But I don't think there is any problem > > with that, if we think that this should be changed then we can make > > SetRecoveryPause return a bool such that if it doesn't do state change > > then it returns false and in that case we can avoid calling > > WakeupRecovery, but I felt that is unnecessary. Any other thoughts on > > this? > > IMO, that WakeupRecovery should not be a problem, because even now, if > we issue a simple select pg_reload_conf(); (without even changing any > config parameter), WakeupRecovery gets called. > > Thanks for the patch. I tested the new function and it works as > expected. I have no further comments on the v13 patch. Thanks for the review and testing. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
On Thu, Feb 11, 2021 at 6:07 AM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > Thanks for the patch. I tested the new function and it works as > > expected. I have no further comments on the v13 patch. > > Thanks for the review and testing. I don't see a whole lot wrong with this patch, but I think there are some things that could make it a little clearer: - I suggest renaming CheckAndSetRecoveryPause() to ConfirmRecoveryPaused(). - I suggest moving the definition of that function to just after SetRecoveryPause(). - I suggest changing the argument to SetRecoveryPause() back to bool. In the one place where you call SetRecoveryPause(RECOVERY_PAUSED), just call SetRecoveryPause(true) and ConfirmRecoveryPaused() back to back. This in turn means that the "if" statement in SetRecoveryPaused() can be rewritten as if (!recoveryPaused) XLogCtl->recoveryPauseState = RECOVERY_NOT_PAUSED else if (XLogCtl->recoveryPauseState == RECOVERY_NOT_PAUSED) XLogCtl->recoveryPauseState = RECOVERY_PAUSE_REQUESTED(). This is slightly less efficient, but I don't think it matters, and I think it will be a lot more clear what's the job of SetRecoveryPause (say whether we're trying to pause or not) and what's the job of ConfirmRecoveryPaused (say whether we've succeeded in pausing). - Since the numeric values of RecoveryPauseState don't matter and the values are never visible to anything outside the server nor stored on disk, I would be inclined to (a) not specify particular values in xlog.h and (b) remove the test-and-elog in SetRecoveryPause(). - In the places where you say: - if (((volatile XLogCtlData *) XLogCtl)->recoveryPause) + if (((volatile XLogCtlData *) XLogCtl)->recoveryPauseState == + RECOVERY_PAUSE_REQUESTED) ...I would suggest instead testing for != RECOVERY_NOT_PAUSED. Perhaps we don't think RECOVERY_PAUSED can happen here. But if somehow it did, calling recoveryPausesHere() would be right. There might be some more to say here, but those are things I notice on a first read-through. -- Robert Haas EDB: http://www.enterprisedb.com
On Thu, 11 Feb 2021 16:36:55 +0530 Dilip Kumar <dilipbalaut@gmail.com> wrote: > On Thu, Feb 11, 2021 at 3:20 PM Bharath Rupireddy > <bharath.rupireddyforpostgres@gmail.com> wrote: > > > > On Wed, Feb 10, 2021 at 8:39 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > > > > On Wed, Feb 10, 2021 at 10:02 AM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > > > > > > I don't find any problem with this approach as well, but I personally > > > > feel that the other approach where we don't wait in any API and just > > > > return the recovery pause state is much simpler and more flexible. So > > > > I will make the pending changes in that patch and let's see what are > > > > the other opinion and based on that we can conclude. Thanks for the > > > > patch. I don't think that we need to include the waiting approach in pg_get_wal_replay_pause_state patch. However, Horiguchi-san's patch may be useful for some users who want pg_wal_replay_pause to wait until recovery gets paused instead of polling the state from applications. So, I shink we could discuss this patch in another thread as another commitfest entry independent from pg_get_wal_replay_pause_state. > > > Here is an updated version of the patch which fixes the last two open problems > > > 1. In RecoveryRequiresIntParameter set the recovery pause state in the > > > loop so that if recovery resumed and pause requested again we can set > > > to pause again. > > > 2. If the recovery state is already 'paused' then don't set it back to > > > the 'pause requested'. > > > > > > One more point is that in 'pg_wal_replay_pause' even if we don't > > > change the state because it was already set to the 'paused' then also > > > we call the WakeupRecovery. But I don't think there is any problem > > > with that, if we think that this should be changed then we can make > > > SetRecoveryPause return a bool such that if it doesn't do state change > > > then it returns false and in that case we can avoid calling > > > WakeupRecovery, but I felt that is unnecessary. Any other thoughts on > > > this? > > > > IMO, that WakeupRecovery should not be a problem, because even now, if > > we issue a simple select pg_reload_conf(); (without even changing any > > config parameter), WakeupRecovery gets called. > > > > Thanks for the patch. I tested the new function and it works as > > expected. I have no further comments on the v13 patch. > > Thanks for the review and testing. I have no futher comments on the v13 patch, too. Also, I agree with Robert Haas's suggestions. Regards, Yugo Nagata -- Yugo NAGATA <nagata@sraoss.co.jp>
At Fri, 12 Feb 2021 13:33:32 +0900, Yugo NAGATA <nagata@sraoss.co.jp> wrote in > I don't think that we need to include the waiting approach in pg_get_wal_replay_pause_state > patch. However, Horiguchi-san's patch may be useful for some users who want > pg_wal_replay_pause to wait until recovery gets paused instead of polling the > state from applications. So, I shink we could discuss this patch in another > thread as another commitfest entry independent from pg_get_wal_replay_pause_state. Since what I'm proposing is not making pg_wal_replay_pause() to wait, and no one seems on my side, I withdraw the proposal. > I have no futher comments on the v13 patch, too. Also, I agree with > Robert Haas's suggestions. Yeah, look reasonable. regards. -- Kyotaro Horiguchi NTT Open Source Software Center