Stefan's bug (was: max_standby_delay considered harmful)

Excerpts from Robert Haas's message of mié may 12 20:48:41 -0400 2010:

On Wed, May 12, 2010 at 3:55 PM, Robert Haas <robertmhaas@gmail.com> wrote:

I am wondering if we are not correctly handling the case where we get
a shutdown request while we are still in the PM_STARTUP state.  It
looks like we might go ahead and switch to PM_RECOVERY and then
PM_RECOVERY_CONSISTENT without noticing the shutdown.  There is some
logic to handle the shutdown when the startup process exits, but if
the startup process never exits it looks like we might get stuck.

I can reproduce the behavior Stefan is seeing consistently using the
attached patch.

W1: postgres -D ~/pgslave
W2: pg_ctl -D ~/pgslave stop

If there's anything to learn from this patch, is that sleep is
uninterruptible on some platforms. This is why sleeps elsewhere are
broken down in loops, sleeping in small increments and checking
interrupts each time. Maybe some of the sleeps in the new HS code need
to be handled this way?
--

On Wed, May 12, 2010 at 10:36 PM, Alvaro Herrera
<alvherre@alvh.no-ip.org> wrote:

Excerpts from Robert Haas's message of mié may 12 20:48:41 -0400 2010:

On Wed, May 12, 2010 at 3:55 PM, Robert Haas <robertmhaas@gmail.com> wrote:

I am wondering if we are not correctly handling the case where we get
a shutdown request while we are still in the PM_STARTUP state.  It
looks like we might go ahead and switch to PM_RECOVERY and then
PM_RECOVERY_CONSISTENT without noticing the shutdown.  There is some
logic to handle the shutdown when the startup process exits, but if
the startup process never exits it looks like we might get stuck.

I can reproduce the behavior Stefan is seeing consistently using the
attached patch.

W1: postgres -D ~/pgslave
W2: pg_ctl -D ~/pgslave stop

If there's anything to learn from this patch, is that sleep is
uninterruptible on some platforms.  This is why sleeps elsewhere are
broken down in loops, sleeping in small increments and checking
interrupts each time.  Maybe some of the sleeps in the new HS code need
to be handled this way?

I don't think the problem is that the sleep is uninterruptible. I
think the problem is that a smart shutdown request received while in
the PM_STARTUP state does not acted upon until we enter the PM_RUN
state. That is, there's a race condition between the SIGUSR1 that the
startup process sends to the postmaster to signal that recovery has
begun and the SIGTERM being sent by pg_ctl.

However, since I haven't succeeded in producing a fix yet, take that
with a grain of salt...

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

On Wed, May 12, 2010 at 10:41 PM, Robert Haas <robertmhaas@gmail.com> wrote:

On Wed, May 12, 2010 at 10:36 PM, Alvaro Herrera
<alvherre@alvh.no-ip.org> wrote:

Excerpts from Robert Haas's message of mié may 12 20:48:41 -0400 2010:

On Wed, May 12, 2010 at 3:55 PM, Robert Haas <robertmhaas@gmail.com> wrote:

I am wondering if we are not correctly handling the case where we get
a shutdown request while we are still in the PM_STARTUP state.  It
looks like we might go ahead and switch to PM_RECOVERY and then
PM_RECOVERY_CONSISTENT without noticing the shutdown.  There is some
logic to handle the shutdown when the startup process exits, but if
the startup process never exits it looks like we might get stuck.

I can reproduce the behavior Stefan is seeing consistently using the
attached patch.

W1: postgres -D ~/pgslave
W2: pg_ctl -D ~/pgslave stop

If there's anything to learn from this patch, is that sleep is
uninterruptible on some platforms.  This is why sleeps elsewhere are
broken down in loops, sleeping in small increments and checking
interrupts each time.  Maybe some of the sleeps in the new HS code need
to be handled this way?

I don't think the problem is that the sleep is uninterruptible.  I
think the problem is that a smart shutdown request received while in
the PM_STARTUP state does not acted upon until we enter the PM_RUN
state.  That is, there's a race condition between the SIGUSR1 that the
startup process sends to the postmaster to signal that recovery has
begun and the SIGTERM being sent by pg_ctl.

However, since I haven't succeeded in producing a fix yet, take that
with a grain of salt...

I'm replying to this thread rather than the max_standby_delay thread
because this line of discussion is not actually related to
max_standby_delay.

Fujii Masao previously reported this problem and proposed this fix:
http://archives.postgresql.org/pgsql-hackers/2010-04/msg00592.php

I responded by proposing that we instead simply adjust things so that
a smart shutdown is always handled at the end of recovery, just prior
to entering normal running. On further reflection, though, I've
concluded that was a dumb idea. Currently, when a smart shutdown
occurs during recovery, we handle it immediately UNLESS it happens
before we receive the signal that tells us it's OK to start the
background writer, in which case we get confused and lock everyone out
of the database until a fast or immediate shutdown request arrives.
So this is just a race condition, plain and simple. Therefore I think
Fujii Masao's original idea was the best, but I have what I believe is
an equivalent but simpler implementation, which is attached.

Thoughts? Should we try to fix this in 8.4 also, or just in HEAD?
8.3 and 8.2 never handle a smart shutdown prior to entering normal
running, and while that seems pretty useless, doing something
different would be a behavior change, so that seems like a
non-starter. 8.4 has the same behavior as HEAD, though it's not
documented in the release notes, so it's not clear how intentional the
change was.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

On Sun, 2010-05-16 at 21:25 -0400, Robert Haas wrote:

I have what I believe is
an equivalent but simpler implementation, which is attached.

There's no code comments to explain this, so without in-depth analysis
of the problem, Masao's patch and this one its not possible to say
anything.

Please explain in detail why its the right approach and put that in a
comment, so we'll understand now and in the future.

--
Simon Riggs www.2ndQuadrant.com

On Mon, May 17, 2010 at 10:25 AM, Robert Haas <robertmhaas@gmail.com> wrote:

Therefore I think
Fujii Masao's original idea was the best, but I have what I believe is
an equivalent but simpler implementation, which is attached.

Seems good.

I found another two problems related to shutdown in PM_STARTUP state:

(1)
Smart or fast shutdown requested in PM_STARTUP state always removes
the backup_label file if it exists. But it might be still required
for subsequent recovery. I changed your patch so that additionally
the postmaster skips deleting the backup_label in that case.

(2)
pg_ctl -ms stop emits the following warning whenever there is the
backup_label file in $PGDATA.

WARNING: online backup mode is active
Shutdown will not complete until pg_stop_backup() is called.

This warning doesn't fit in with the shutdown during recovery case.
Since smart shutdown might be requested by other than pg_ctl, the
warning should be emitted in server side rather than client, I think.
How about moving the warning to the server side?

Thoughts?  Should we try to fix this in 8.4 also, or just in HEAD?
8.3 and 8.2 never handle a smart shutdown prior to entering normal
running, and while that seems pretty useless, doing something
different would be a behavior change, so that seems like a
non-starter.  8.4 has the same behavior as HEAD, though it's not
documented in the release notes, so it's not clear how intentional the
change was.

In 8.4, smart shutdown during recovery waits until the startup process
has exited. So the backporting to 8.4 doesn't improve any situation,
I think.

Regards,

--
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

On Mon, May 17, 2010 at 3:13 AM, Simon Riggs <simon@2ndquadrant.com> wrote:

On Sun, 2010-05-16 at 21:25 -0400, Robert Haas wrote:

I have what I believe is
an equivalent but simpler implementation, which is attached.

There's no code comments to explain this, so without in-depth analysis
of the problem, Masao's patch and this one its not possible to say
anything.

Please explain in detail why its the right approach and put that in a
comment, so we'll understand now and in the future.

The explanation is what I wrote in my previous email: a smart shutdown
request during recovery should be treated the same way BEFORE the
postmaster has been asked to start the background writer and AFTER the
postmaster has been asked to start the background writer. I'll think
up a suitable comment.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

On Mon, 2010-05-17 at 06:30 -0400, Robert Haas wrote:

On Mon, May 17, 2010 at 3:13 AM, Simon Riggs <simon@2ndquadrant.com> wrote:

On Sun, 2010-05-16 at 21:25 -0400, Robert Haas wrote:

I have what I believe is
an equivalent but simpler implementation, which is attached.

There's no code comments to explain this, so without in-depth analysis
of the problem, Masao's patch and this one its not possible to say
anything.

Please explain in detail why its the right approach and put that in a
comment, so we'll understand now and in the future.

The explanation is what I wrote in my previous email: a smart shutdown
request during recovery should be treated the same way BEFORE the
postmaster has been asked to start the background writer and AFTER the
postmaster has been asked to start the background writer. I'll think
up a suitable comment.

I think we should review Masao's patch and ask him to make any changes
we think are appropriate. There's no benefit to have multiple patch
authors at one time.

--
Simon Riggs www.2ndQuadrant.com

On Mon, May 17, 2010 at 6:41 AM, Simon Riggs <simon@2ndquadrant.com> wrote:

On Mon, 2010-05-17 at 06:30 -0400, Robert Haas wrote:

On Mon, May 17, 2010 at 3:13 AM, Simon Riggs <simon@2ndquadrant.com> wrote:

On Sun, 2010-05-16 at 21:25 -0400, Robert Haas wrote:

I have what I believe is
an equivalent but simpler implementation, which is attached.

There's no code comments to explain this, so without in-depth analysis
of the problem, Masao's patch and this one its not possible to say
anything.

Please explain in detail why its the right approach and put that in a
comment, so we'll understand now and in the future.

The explanation is what I wrote in my previous email: a smart shutdown
request during recovery should be treated the same way BEFORE the
postmaster has been asked to start the background writer and AFTER the
postmaster has been asked to start the background writer.  I'll think
up a suitable comment.

I think we should review Masao's patch and ask him to make any changes
we think are appropriate. There's no benefit to have multiple patch
authors at one time.

I did review his patch. It duplicates a few lines of logic and I
found a way to avoid that, so I proposed it. That seems totally
normal to me and I'm not sure what you're concerned about.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

On Mon, May 17, 2010 at 3:38 AM, Fujii Masao <masao.fujii@gmail.com> wrote:

On Mon, May 17, 2010 at 10:25 AM, Robert Haas <robertmhaas@gmail.com> wrote:

Therefore I think
Fujii Masao's original idea was the best, but I have what I believe is
an equivalent but simpler implementation, which is attached.

Seems good.

I found another two problems related to shutdown in PM_STARTUP state:

(1)
Smart or fast shutdown requested in PM_STARTUP state always removes
the backup_label file if it exists. But it might be still required
for subsequent recovery. I changed your patch so that additionally
the postmaster skips deleting the backup_label in that case.

Can you explain in a little more detail how this can cause a problem?
I'm not very familiar with how the backup label is used.

Also, why is this different in PM_STARTUP than in PM_RECOVERY?
PM_RECOVERY doesn't guarantee that we've reached consistency.

(2)
pg_ctl -ms stop emits the following warning whenever there is the
backup_label file in $PGDATA.

     WARNING: online backup mode is active
     Shutdown will not complete until pg_stop_backup() is called.

This warning doesn't fit in with the shutdown during recovery case.
Since smart shutdown might be requested by other than pg_ctl, the
warning should be emitted in server side rather than client, I think.
How about moving the warning to the server side?

Hmm, I'm not sure whether that's a good idea or not. Perhaps we
should discuss for 9.1?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

On Mon, 2010-05-17 at 06:55 -0400, Robert Haas wrote:

I think we should review Masao's patch and ask him to make any changes
we think are appropriate. There's no benefit to have multiple patch
authors at one time.

I did review his patch. It duplicates a few lines of logic and I
found a way to avoid that, so I proposed it. That seems totally
normal to me and I'm not sure what you're concerned about.

I think we should concentrate efforts on just one patch: Masao's.

--
Simon Riggs www.2ndQuadrant.com

On Mon, May 17, 2010 at 7:14 AM, Simon Riggs <simon@2ndquadrant.com> wrote:

On Mon, 2010-05-17 at 06:55 -0400, Robert Haas wrote:

I think we should review Masao's patch and ask him to make any changes
we think are appropriate. There's no benefit to have multiple patch
authors at one time.

I did review his patch.  It duplicates a few lines of logic and I
found a way to avoid that, so I proposed it.  That seems totally
normal to me and I'm not sure what you're concerned about.

I think we should concentrate efforts on just one patch: Masao's.

I understand that's your opinion, but you haven't explained why. My
approach is simpler and Fujii Masao has already endorsed it. I would
prefer that we focus on the technical issues here rather than who
wrote the patch. I believe that my approach is better because it
avoids duplicating code, which should reduce the chance of future
bugs, since someone might conceivably update one chunk of code but not
the other.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

On Mon, 2010-05-17 at 07:33 -0400, Robert Haas wrote:

I would
prefer that we focus on the technical issues here rather than who
wrote the patch.

There are three patches now from 2 authors. I agree we should focus on
the technical issues, but which issues, in which patch? If Masao had
accepted your version he would not have written another, would he?

--
Simon Riggs www.2ndQuadrant.com

On Mon, May 17, 2010 at 8:02 PM, Robert Haas <robertmhaas@gmail.com> wrote:

(1)
Smart or fast shutdown requested in PM_STARTUP state always removes
the backup_label file if it exists. But it might be still required
for subsequent recovery. I changed your patch so that additionally
the postmaster skips deleting the backup_label in that case.

Can you explain in a little more detail how this can cause a problem?
I'm not very familiar with how the backup label is used.

Also, why is this different in PM_STARTUP than in PM_RECOVERY?
PM_RECOVERY doesn't guarantee that we've reached consistency.

Before the startup process sends the PMSIGNAL_RECOVERY_STARTED signal
(i.e., when the postmaster is in PM_STARTUP state), it reads the
backup_label file to know the recovery starting WAL location, saves
that information in pg_control file, and rename the file "backup_label"
to "backup_label.old".

If the backup_label file is removed before pg_control is updated,
subsequent recovery cannot get the right recovery starting location.
This is the problem that I'm concerned.

The smart shutdown during recovery and the fast shutdown might call
CancelBackup() and remove the backup_label file. So if shutdown is
requested in PM_STARTUP state, the problem would happen.

In the patch, if shutdown is requested in PM_STARTUP, the postmaster
skips calling CancelBackup() since the backup_label file might be
required.

(2)
pg_ctl -ms stop emits the following warning whenever there is the
backup_label file in $PGDATA.

     WARNING: online backup mode is active
     Shutdown will not complete until pg_stop_backup() is called.

This warning doesn't fit in with the shutdown during recovery case.
Since smart shutdown might be requested by other than pg_ctl, the
warning should be emitted in server side rather than client, I think.
How about moving the warning to the server side?

Hmm, I'm not sure whether that's a good idea or not.  Perhaps we
should discuss for 9.1?

Okay, this is not a critical problem.

Regards,

--
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

On Mon, May 17, 2010 at 7:38 AM, Simon Riggs <simon@2ndquadrant.com> wrote:

On Mon, 2010-05-17 at 07:33 -0400, Robert Haas wrote:

I would
prefer that we focus on the technical issues here rather than who
wrote the patch.

There are three patches now from 2 authors. I agree we should focus on
the technical issues, but which issues, in which patch? If Masao had
accepted your version he would not have written another, would he?

He wrote his version first. I reviewed it and submitted a simplified
version. He reviewed mine and submitted an updated version that
addresses an additional possible issue, which is currently under
discussion.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

On Mon, 2010-05-17 at 16:38 +0900, Fujii Masao wrote:

On Mon, May 17, 2010 at 10:25 AM, Robert Haas <robertmhaas@gmail.com> wrote:

Therefore I think
Fujii Masao's original idea was the best, but I have what I believe is
an equivalent but simpler implementation, which is attached.

Seems good.

I found another two problems related to shutdown in PM_STARTUP state:

(1)
Smart or fast shutdown requested in PM_STARTUP state always removes
the backup_label file if it exists. But it might be still required
for subsequent recovery. I changed your patch so that additionally
the postmaster skips deleting the backup_label in that case.

Don't like the name NeedBackupLabel seems too specific. That really
corresponds to "we were in recovery". We should have a couple of
super-states that correspond to am in recovery/am not in recovery so we
can drive it from that.

(2)
pg_ctl -ms stop emits the following warning whenever there is the
backup_label file in $PGDATA.

WARNING: online backup mode is active
Shutdown will not complete until pg_stop_backup() is called.

This warning doesn't fit in with the shutdown during recovery case.
Since smart shutdown might be requested by other than pg_ctl, the
warning should be emitted in server side rather than client, I think.
How about moving the warning to the server side?

+1

Thoughts? Should we try to fix this in 8.4 also, or just in HEAD?
8.3 and 8.2 never handle a smart shutdown prior to entering normal
running, and while that seems pretty useless, doing something
different would be a behavior change, so that seems like a
non-starter. 8.4 has the same behavior as HEAD, though it's not
documented in the release notes, so it's not clear how intentional the
change was.

In 8.4, smart shutdown during recovery waits until the startup process
has exited. So the backporting to 8.4 doesn't improve any situation,
I think.

We shouldn't be discussing backporting a behaviour change.

--
Simon Riggs www.2ndQuadrant.com

On Mon, May 17, 2010 at 7:44 AM, Fujii Masao <masao.fujii@gmail.com> wrote:

On Mon, May 17, 2010 at 8:02 PM, Robert Haas <robertmhaas@gmail.com> wrote:

(1)
Smart or fast shutdown requested in PM_STARTUP state always removes
the backup_label file if it exists. But it might be still required
for subsequent recovery. I changed your patch so that additionally
the postmaster skips deleting the backup_label in that case.

Can you explain in a little more detail how this can cause a problem?
I'm not very familiar with how the backup label is used.

Also, why is this different in PM_STARTUP than in PM_RECOVERY?
PM_RECOVERY doesn't guarantee that we've reached consistency.

Before the startup process sends the PMSIGNAL_RECOVERY_STARTED signal
(i.e., when the postmaster is in PM_STARTUP state), it reads the
backup_label file to know the recovery starting WAL location, saves
that information in pg_control file, and rename the file "backup_label"
to "backup_label.old".

If the backup_label file is removed before pg_control is updated,
subsequent recovery cannot get the right recovery starting location.
This is the problem that I'm concerned.

The smart shutdown during recovery and the fast shutdown might call
CancelBackup() and remove the backup_label file. So if shutdown is
requested in PM_STARTUP state, the problem would happen.

OK, I think I understand now. But, the SIGTERM sent by the postmaster
doesn't kill the recovery process unconditionally. It will invoke
StartupProcShutdownHandler(), which will set set shutdown_requested =
true. That gets checked by RestoreArchivedFile() and
HandleStartupProcInterrupts(), and I think that neither of those can
get invoked until after the control file has been updated. Do you see
a way it can happen?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

On Mon, May 17, 2010 at 10:20 PM, Robert Haas <robertmhaas@gmail.com> wrote:

OK, I think I understand now.  But, the SIGTERM sent by the postmaster
doesn't kill the recovery process unconditionally.  It will invoke
StartupProcShutdownHandler(), which will set set shutdown_requested =
true.  That gets checked by RestoreArchivedFile() and
HandleStartupProcInterrupts(), and I think that neither of those can
get invoked until after the control file has been updated.  Do you see
a way it can happen?

Yeah, the way is:
StartupXLOG() --> ReadCheckpointRecord() --> ReadRecord() -->
XLogPageRead() --> XLogFileReadAnyTLI() --> XLogFileRead() -->
RestoreArchivedFile()

ReadCheckpointRecord() is called before pg_control is updated.

ISTM that walreceiver might be invoked even after shutdown is requested.
We should prevent the postmaster from starting up walreceiver if
Shutdown > NoShutdown?

Regards,

--
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

On Mon, May 17, 2010 at 9:01 PM, Simon Riggs <simon@2ndquadrant.com> wrote:

(1)
Smart or fast shutdown requested in PM_STARTUP state always removes
the backup_label file if it exists. But it might be still required
for subsequent recovery. I changed your patch so that additionally
the postmaster skips deleting the backup_label in that case.

Don't like the name NeedBackupLabel seems too specific. That really
corresponds to "we were in recovery". We should have a couple of
super-states that correspond to am in recovery/am not in recovery so we
can drive it from that.

ISTM that we can use XLogCtl->SharedRecoveryInProgress for that.
Is this OK?

Regards,

--
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

On Tue, 2010-05-18 at 11:40 +0900, Fujii Masao wrote:

ISTM that walreceiver might be invoked even after shutdown is
requested. We should prevent the postmaster from starting up
walreceiver if Shutdown > NoShutdown?

+1

--
Simon Riggs www.2ndQuadrant.com