Re: pending patch: Re: [HACKERS] HS/SR and smart shutdown
On Thu, Apr 1, 2010 at 8:24 PM, Robert Haas robertmh...@gmail.com wrote: On Thu, Apr 1, 2010 at 7:18 AM, Simon Riggs si...@2ndquadrant.com wrote: I'm not willing to investigate this further myself at this stage. This looks like risk for little benefit. That's kind of what I figured. I'll see about fixing up Fujii-san's patch and documenting the behavior; but it won't happen before the weekend because I'm going to be out of town. I found the bug which makes smart shutdown get stuck for a while: If there is no WAL file available in the standby, walreceiver might be invoked before we have reached the PM_RECOVERY state. We switch to the PM_RECOVERY state after reading the checkpoint record pointed out in the pg_control file. If it's not found, we are in the PM_INIT or PM_START state and start walreceiver to read it from the primary. If smart shutdown is requested at that point, we cannot switch to the PM_WAIT_READONLY state because pmdie() doesn't allow that. So the SIGTERM is never sent to walreceiver, and smart shutdown would get stuck. If the current state is either PM_INIT or PM_START, it's guaranteed that there is no regular backend, so we should kill walreceiver as soon as smart shutdown is requested, I think. The attached patch does that. Regards, -- Fujii Masao NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center smart_shutdown_bugfix_v1.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: pending patch: Re: [HACKERS] HS/SR and smart shutdown
On Tue, Apr 13, 2010 at 9:18 AM, Fujii Masao masao.fu...@gmail.com wrote: On Thu, Apr 1, 2010 at 8:24 PM, Robert Haas robertmh...@gmail.com wrote: On Thu, Apr 1, 2010 at 7:18 AM, Simon Riggs si...@2ndquadrant.com wrote: I'm not willing to investigate this further myself at this stage. This looks like risk for little benefit. That's kind of what I figured. I'll see about fixing up Fujii-san's patch and documenting the behavior; but it won't happen before the weekend because I'm going to be out of town. I found the bug which makes smart shutdown get stuck for a while: If there is no WAL file available in the standby, walreceiver might be invoked before we have reached the PM_RECOVERY state. We switch to the PM_RECOVERY state after reading the checkpoint record pointed out in the pg_control file. If it's not found, we are in the PM_INIT or PM_START state and start walreceiver to read it from the primary. If smart shutdown is requested at that point, we cannot switch to the PM_WAIT_READONLY state because pmdie() doesn't allow that. So the SIGTERM is never sent to walreceiver, and smart shutdown would get stuck. If the current state is either PM_INIT or PM_START, it's guaranteed that there is no regular backend, so we should kill walreceiver as soon as smart shutdown is requested, I think. The attached patch does that. Can you explain how to recreate the problem that this patch fixes? ...Robert -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: pending patch: Re: [HACKERS] HS/SR and smart shutdown
On Tue, Apr 13, 2010 at 10:27 PM, Robert Haas robertmh...@gmail.com wrote: Can you explain how to recreate the problem that this patch fixes? 1. Configure and start the primary server. 2. Configure the standby server. 3. Remove all of the WAL files in pg_xlog of the standby. 4. Start the standby. 5. Request smart shutdown against the standby before walreceiver receives any WAL records. You would need to emulate the time-consuming authentication which usually requires the setting of authentication_timeout. I used the attached patch for the emulation. New GUC parameter wal_sender_sleep which the patch provides for the test specifies the sleep time during walsender's handshake processing. If you set it to 10s, walsender sleeps 10 secs before it sends the WAL records. Regards, -- Fujii Masao NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center test_smart_shutdown_bug_v1.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: pending patch: Re: [HACKERS] HS/SR and smart shutdown
On Thu, Apr 1, 2010 at 12:16 AM, Robert Haas robertmh...@gmail.com wrote: On Wed, Mar 31, 2010 at 5:02 AM, Simon Riggs si...@2ndquadrant.com wrote: From what I have seen, the comment about PM_WAIT_BACKENDS is incorrect. backends might be waiting for the WAL record that conflicts with their queries to be replayed. Recovery sometimes waits for backends, but backends never wait for recovery. Really? As Heikki explained before, backends might wait for the lock taken by the startup process. http://archives.postgresql.org/pgsql-hackers/2010-01/msg02984.php Backends wait for locks, yes, but they could be waiting for user locks also. That is not waiting for the WAL record, that concept does not exist. Hmm... this is a good point, on two levels. First, the comment is not as well-phrased as it could be. Second, I wonder why we can't kill the startup process and WAL receiver right away, and then wait for the backends to die off afterwards. I tested whether killing the startup process and walreceiver releases the lock which the backends are waiting for. Unfortunately it doesn't, and the backends have gotten stuck in my box. The behavior which the startup process shuts down without releasing the lock is a bug? BTW, I tested that by compiling postgres with the attached patch and doing the following operations. 1. Make the SR environment 2. Issue some SQLs to the primary psql -h primary server =# CREATE TABLE t(i int); =# BEGIN; =# DROP TABLE t; =# SELECT pg_switch_xlog(); (keep this session alive) 3. Issue some SQLs to the standby psql -h standby server =# BEGIN; =# SELECT * FROM t; -- waiting 4. Perform smart shutdown on the standby Then the startup process and walreceiver shut down, but the session created in #3 is still waiting. Regards, -- Fujii Masao NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center shutdown_test.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: pending patch: Re: [HACKERS] HS/SR and smart shutdown
On Thu, Apr 1, 2010 at 4:42 AM, Fujii Masao masao.fu...@gmail.com wrote: On Thu, Apr 1, 2010 at 12:16 AM, Robert Haas robertmh...@gmail.com wrote: On Wed, Mar 31, 2010 at 5:02 AM, Simon Riggs si...@2ndquadrant.com wrote: From what I have seen, the comment about PM_WAIT_BACKENDS is incorrect. backends might be waiting for the WAL record that conflicts with their queries to be replayed. Recovery sometimes waits for backends, but backends never wait for recovery. Really? As Heikki explained before, backends might wait for the lock taken by the startup process. http://archives.postgresql.org/pgsql-hackers/2010-01/msg02984.php Backends wait for locks, yes, but they could be waiting for user locks also. That is not waiting for the WAL record, that concept does not exist. Hmm... this is a good point, on two levels. First, the comment is not as well-phrased as it could be. Second, I wonder why we can't kill the startup process and WAL receiver right away, and then wait for the backends to die off afterwards. I tested whether killing the startup process and walreceiver releases the lock which the backends are waiting for. Unfortunately it doesn't, and the backends have gotten stuck in my box. The behavior which the startup process shuts down without releasing the lock is a bug? I think that what this shows is that the original design of Hot Standby didn't contemplate ever having Hot Standby up without the startup process running. In retrospect, maybe we want to allow that, because a smart shutdown would be more likely to complete in a timely fashion if we stopped replication first and then waited for the backends to die rather than waiting for the backends to die first and then stopping replication. That's because, for so long as replication continues, it may take new locks as well as releasing old ones, to say nothing of using other system resources like CPU and I/O bandwidth. But, for 9.0, I'm not sure we have any real choice, unless making the startup process release locks when it goes away is a very simple change. Assuming that's not the case, I think we should apply this patch with some updates to the comments, document how it works and that it may change in a future release, and add a TODO for 9.1. Thoughts? ...Robert -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: pending patch: Re: [HACKERS] HS/SR and smart shutdown
On Thu, 2010-04-01 at 06:48 -0400, Robert Haas wrote: On Thu, Apr 1, 2010 at 4:42 AM, Fujii Masao masao.fu...@gmail.com wrote: On Thu, Apr 1, 2010 at 12:16 AM, Robert Haas robertmh...@gmail.com wrote: On Wed, Mar 31, 2010 at 5:02 AM, Simon Riggs si...@2ndquadrant.com wrote: From what I have seen, the comment about PM_WAIT_BACKENDS is incorrect. backends might be waiting for the WAL record that conflicts with their queries to be replayed. Recovery sometimes waits for backends, but backends never wait for recovery. Really? As Heikki explained before, backends might wait for the lock taken by the startup process. http://archives.postgresql.org/pgsql-hackers/2010-01/msg02984.php Backends wait for locks, yes, but they could be waiting for user locks also. That is not waiting for the WAL record, that concept does not exist. Hmm... this is a good point, on two levels. First, the comment is not as well-phrased as it could be. Second, I wonder why we can't kill the startup process and WAL receiver right away, and then wait for the backends to die off afterwards. I tested whether killing the startup process and walreceiver releases the lock which the backends are waiting for. Unfortunately it doesn't, and the backends have gotten stuck in my box. The behavior which the startup process shuts down without releasing the lock is a bug? I think that what this shows is that the original design of Hot Standby didn't contemplate ever having Hot Standby up without the startup process running. In retrospect, maybe we want to allow that, because a smart shutdown would be more likely to complete in a timely fashion if we stopped replication first and then waited for the backends to die rather than waiting for the backends to die first and then stopping replication. That's because, for so long as replication continues, it may take new locks as well as releasing old ones, to say nothing of using other system resources like CPU and I/O bandwidth. But, for 9.0, I'm not sure we have any real choice, unless making the startup process release locks when it goes away is a very simple change. Assuming that's not the case, I think we should apply this patch with some updates to the comments, document how it works and that it may change in a future release, and add a TODO for 9.1. I'm not willing to investigate this further myself at this stage. This looks like risk for little benefit. -- Simon Riggs www.2ndQuadrant.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: pending patch: Re: [HACKERS] HS/SR and smart shutdown
On Thu, Apr 1, 2010 at 7:18 AM, Simon Riggs si...@2ndquadrant.com wrote: I'm not willing to investigate this further myself at this stage. This looks like risk for little benefit. That's kind of what I figured. I'll see about fixing up Fujii-san's patch and documenting the behavior; but it won't happen before the weekend because I'm going to be out of town. ...Robert -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: pending patch: Re: [HACKERS] HS/SR and smart shutdown
On Wed, 2010-03-31 at 10:48 +0900, Fujii Masao wrote: On Wed, Mar 31, 2010 at 9:47 AM, Robert Haas robertmh...@gmail.com wrote: On Tue, Mar 30, 2010 at 5:09 AM, Fujii Masao masao.fu...@gmail.com wrote: I rebased the patch to HEAD. Is the patch still required for 9.0? If not, I'd remove the open item of the smart shutdown during recovery. I am by no means an expert on this area of the code, but in the interest of moving things along I reviewed this patch tonight. Thanks a lot! 1. I wonder if there is a problem if we receive SIGINT while in the PM_WAIT_READONLY state? Seems to me that might need to be added to the if statement beginning at line 2212, in pmdie(). 2. It appears to me that HandleChildCrash() needs to switch to PM_WAIT_BACKENDS state if it's in PM_WAIT_READONLY when the child crash occurs - i.e. the if statement beginning at line 2772 needs updating. Thoughts? Oh, those are my oversights. You are right! I modified the patch as you pointed out. Please add some docs that a) explains what the patch does and b) notes any changes from behaviour in previous releases. ISTM this is a major change in behaviour. The reason for the lack of consensus on this issue is simply people aren't following what you're talking about and don't want to have to re-read a whole thread to do so. It might be that many would agree. From what I have seen, the comment about PM_WAIT_BACKENDS is incorrect. backends might be waiting for the WAL record that conflicts with their queries to be replayed. Recovery sometimes waits for backends, but backends never wait for recovery. -- Simon Riggs www.2ndQuadrant.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: pending patch: Re: [HACKERS] HS/SR and smart shutdown
On Wed, Mar 31, 2010 at 5:00 PM, Simon Riggs si...@2ndquadrant.com wrote: Please add some docs that a) explains what the patch does and b) notes any changes from behaviour in previous releases. ISTM this is a major change in behaviour. How about adding the following description into 17.5. Shutting Down the Server? If the server is in recovery, it waits for all of the read only connections to be closed. And, where should the note be put in? AFAIK, the previous behavior has not been documented anywhere. From what I have seen, the comment about PM_WAIT_BACKENDS is incorrect. backends might be waiting for the WAL record that conflicts with their queries to be replayed. Recovery sometimes waits for backends, but backends never wait for recovery. Really? As Heikki explained before, backends might wait for the lock taken by the startup process. http://archives.postgresql.org/pgsql-hackers/2010-01/msg02984.php Regards, -- Fujii Masao NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: pending patch: Re: [HACKERS] HS/SR and smart shutdown
On Wed, 2010-03-31 at 17:48 +0900, Fujii Masao wrote: On Wed, Mar 31, 2010 at 5:00 PM, Simon Riggs si...@2ndquadrant.com wrote: Please add some docs that a) explains what the patch does and b) notes any changes from behaviour in previous releases. ISTM this is a major change in behaviour. How about adding the following description into 17.5. Shutting Down the Server? If the server is in recovery, it waits for all of the read only connections to be closed. You need to explain which mode you're talking about. Adding minimal comments isn't my objective. We need good, useful documentation on every aspect that you add or change. And, where should the note be put in? AFAIK, the previous behavior has not been documented anywhere. From what I have seen, the comment about PM_WAIT_BACKENDS is incorrect. backends might be waiting for the WAL record that conflicts with their queries to be replayed. Recovery sometimes waits for backends, but backends never wait for recovery. Really? As Heikki explained before, backends might wait for the lock taken by the startup process. http://archives.postgresql.org/pgsql-hackers/2010-01/msg02984.php Backends wait for locks, yes, but they could be waiting for user locks also. That is not waiting for the WAL record, that concept does not exist. -- Simon Riggs www.2ndQuadrant.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: pending patch: Re: [HACKERS] HS/SR and smart shutdown
On Wed, Mar 31, 2010 at 6:02 PM, Simon Riggs si...@2ndquadrant.com wrote: On Wed, 2010-03-31 at 17:48 +0900, Fujii Masao wrote: On Wed, Mar 31, 2010 at 5:00 PM, Simon Riggs si...@2ndquadrant.com wrote: Please add some docs that a) explains what the patch does and b) notes any changes from behaviour in previous releases. ISTM this is a major change in behaviour. How about adding the following description into 17.5. Shutting Down the Server? If the server is in recovery, it waits for all of the read only connections to be closed. You need to explain which mode you're talking about. Smart Shutdown mode Adding minimal comments isn't my objective. We need good, useful documentation on every aspect that you add or change. But the patch doesn't provide anything beyond: If the server is in recovery, it waits for all of the read only connections to be closed. What additional explanation do you think is required? And, where should the note be put in? AFAIK, the previous behavior has not been documented anywhere. From what I have seen, the comment about PM_WAIT_BACKENDS is incorrect. backends might be waiting for the WAL record that conflicts with their queries to be replayed. Recovery sometimes waits for backends, but backends never wait for recovery. Really? As Heikki explained before, backends might wait for the lock taken by the startup process. http://archives.postgresql.org/pgsql-hackers/2010-01/msg02984.php Backends wait for locks, yes, but they could be waiting for user locks also. That is not waiting for the WAL record, that concept does not exist. How about the following change? -* waiting for the WAL record that conflicts with their queries to be -* replayed, recovery and replication need to remain until all read +* waiting until the startup process has released the lock that +* their queries are waiting for by replaying the WAL record, +* recovery and replication need to remain until all read Regards, -- Fujii Masao NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: pending patch: Re: [HACKERS] HS/SR and smart shutdown
On Wed, Mar 31, 2010 at 4:00 AM, Simon Riggs si...@2ndquadrant.com wrote: Please add some docs that a) explains what the patch does and b) notes any changes from behaviour in previous releases. ISTM this is a major change in behaviour. I guess I see this a little bit differently. If you do a smart shutdown on 8.4, the autovacuum launcher won't prevent s smart shutdown from completing successfully. Neither will the background writer. If they did, we'd consider that a bug and fix it. walreceiver is just one more system process that needs to get properly shut down when a smart shutdown is requested. So I don't think this is a major behavior change - I think it's preserving the behavior we've had all along. The current documentation reads: In stop mode, the server that is running in the specified data directory is shut down. Three different shutdown methods can be selected with the -m option: Smart mode waits for online backup mode to finish and all the clients to disconnect. This is the default. Fast mode does not wait for clients to disconnect and will terminate an online backup in progress. All active transactions are rolled back and clients are forcibly disconnected, then the server is shut down. Immediate mode will abort all server processes without a clean shutdown. This will lead to a recovery run on restart. That all still seems accurate after this patch. I'm not even sure what to add. I suppose we could add a sentence like If a smart shutdown is requested while the server is in recovery, recovery will stop and the server will shut down. ...but if we add that then why don't we have a similar sentence that says: If a smart shutdown is requested while the autovacuum launcher is running, the autovacuum launcher will be stopped and the server will shut down. I just don't see that we're adding any additional clarity here. I think what would require documentation is if we DIDN'T apply this patch. Then we'd need something like: Smart shutdown mode should not be used if streaming replication is in use. The server will begin to shut down but, because the streaming replication process is not automatically shut down, it will never actually finish shutting down unless the streaming replication process crashes. If a server using streaming replication is accidentally shut down using smart mode, the problem can be corrected by shutting down again using fast or immediate mode. ...Robert -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: pending patch: Re: [HACKERS] HS/SR and smart shutdown
On Wed, Mar 31, 2010 at 5:02 AM, Simon Riggs si...@2ndquadrant.com wrote: From what I have seen, the comment about PM_WAIT_BACKENDS is incorrect. backends might be waiting for the WAL record that conflicts with their queries to be replayed. Recovery sometimes waits for backends, but backends never wait for recovery. Really? As Heikki explained before, backends might wait for the lock taken by the startup process. http://archives.postgresql.org/pgsql-hackers/2010-01/msg02984.php Backends wait for locks, yes, but they could be waiting for user locks also. That is not waiting for the WAL record, that concept does not exist. Hmm... this is a good point, on two levels. First, the comment is not as well-phrased as it could be. Second, I wonder why we can't kill the startup process and WAL receiver right away, and then wait for the backends to die off afterwards. ...Robert -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: pending patch: Re: [HACKERS] HS/SR and smart shutdown
On Tue, Mar 30, 2010 at 5:09 AM, Fujii Masao masao.fu...@gmail.com wrote: I rebased the patch to HEAD. Is the patch still required for 9.0? If not, I'd remove the open item of the smart shutdown during recovery. I am by no means an expert on this area of the code, but in the interest of moving things along I reviewed this patch tonight. 1. I wonder if there is a problem if we receive SIGINT while in the PM_WAIT_READONLY state? Seems to me that might need to be added to the if statement beginning at line 2212, in pmdie(). 2. It appears to me that HandleChildCrash() needs to switch to PM_WAIT_BACKENDS state if it's in PM_WAIT_READONLY when the child crash occurs - i.e. the if statement beginning at line 2772 needs updating. Thoughts? ...Robert -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: pending patch: Re: [HACKERS] HS/SR and smart shutdown
On Wed, Mar 31, 2010 at 9:47 AM, Robert Haas robertmh...@gmail.com wrote: On Tue, Mar 30, 2010 at 5:09 AM, Fujii Masao masao.fu...@gmail.com wrote: I rebased the patch to HEAD. Is the patch still required for 9.0? If not, I'd remove the open item of the smart shutdown during recovery. I am by no means an expert on this area of the code, but in the interest of moving things along I reviewed this patch tonight. Thanks a lot! 1. I wonder if there is a problem if we receive SIGINT while in the PM_WAIT_READONLY state? Seems to me that might need to be added to the if statement beginning at line 2212, in pmdie(). 2. It appears to me that HandleChildCrash() needs to switch to PM_WAIT_BACKENDS state if it's in PM_WAIT_READONLY when the child crash occurs - i.e. the if statement beginning at line 2772 needs updating. Thoughts? Oh, those are my oversights. You are right! I modified the patch as you pointed out. Regards, -- Fujii Masao NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center new_smart_shutdown_20100331.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: pending patch: Re: [HACKERS] HS/SR and smart shutdown
On Tue, Mar 30, 2010 at 9:48 PM, Fujii Masao masao.fu...@gmail.com wrote: On Wed, Mar 31, 2010 at 9:47 AM, Robert Haas robertmh...@gmail.com wrote: On Tue, Mar 30, 2010 at 5:09 AM, Fujii Masao masao.fu...@gmail.com wrote: I rebased the patch to HEAD. Is the patch still required for 9.0? If not, I'd remove the open item of the smart shutdown during recovery. I am by no means an expert on this area of the code, but in the interest of moving things along I reviewed this patch tonight. Thanks a lot! 1. I wonder if there is a problem if we receive SIGINT while in the PM_WAIT_READONLY state? Seems to me that might need to be added to the if statement beginning at line 2212, in pmdie(). 2. It appears to me that HandleChildCrash() needs to switch to PM_WAIT_BACKENDS state if it's in PM_WAIT_READONLY when the child crash occurs - i.e. the if statement beginning at line 2772 needs updating. Thoughts? Oh, those are my oversights. You are right! I modified the patch as you pointed out. Cool. This looks good to me now and I also tested it. I will commit it unless (a) someone objects or (b) someone else does it first. ...Robert -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
