Re: Re: [COMMITTERS] pgsql: Basic Recovery Control functions for use in Hot Standby. Pause,

Basic Recovery Control functions for use in Hot Standby. Pause, Resume,
Status check functions only. Also, new recovery.conf parameter to
pause_at_recovery_target, default on.

Simon Riggs, reviewed by Fujii Masao

Branch
------
master

Details
-------
http://git.postgresql.org/gitweb?p=postgresql.git;a=commitdiff;h=8c6e3adbf792c2bba448e88cbf2c8e03fb802e73

Modified Files
--------------
doc/src/sgml/func.sgml | 58 +++++++++++++++
doc/src/sgml/recovery-config.sgml | 25 +++++++
src/backend/access/transam/xlog.c | 137 ++++++++++++++++++++++++++++++++++++
src/include/access/xlog_internal.h | 3 +
src/include/catalog/pg_proc.h | 7 ++
5 files changed, 230 insertions(+), 0 deletions(-)

On Wed, Feb 9, 2011 at 3:30 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> Basic Recovery Control functions for use in Hot Standby. Pause, Resume,
> Status check functions only. Also, new recovery.conf parameter to
> pause_at_recovery_target, default on.

Why did you change the default to on? This would surprise people who are
used to PITR.

Regards,

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

On Tue, Feb 8, 2011 at 1:30 PM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> Basic Recovery Control functions for use in Hot Standby. Pause, Resume,
> Status check functions only. Also, new recovery.conf parameter to
> pause_at_recovery_target, default on.
>

I guess that new parameter should also be in recovery.conf.sample, right?

--
Jaime Casanova         www.2ndQuadrant.com
Professional PostgreSQL: Soporte y capacitación de PostgreSQL

On Wed, Feb 9, 2011 at 1:00 PM, Jaime Casanova <jaime(at)2ndquadrant(dot)com> wrote:
> On Tue, Feb 8, 2011 at 1:30 PM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>> Basic Recovery Control functions for use in Hot Standby. Pause, Resume,
>> Status check functions only. Also, new recovery.conf parameter to
>> pause_at_recovery_target, default on.
>>
>
> I guess that new parameter should also be in recovery.conf.sample, right?

Right.

Regards,

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

On Wed, 2011-02-09 at 12:50 +0900, Fujii Masao wrote:
> On Wed, Feb 9, 2011 at 3:30 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> > Basic Recovery Control functions for use in Hot Standby. Pause, Resume,
> > Status check functions only. Also, new recovery.conf parameter to
> > pause_at_recovery_target, default on.
>
> Why did you change the default to on? This would surprise people who are
> used to PITR.

You pointed out that the code did not match the documented default. So I
made them match according to the docs.

Making it pause at target by default is more natural behaviour, even if
it is a change of behaviour. It can waste a lot of time if it leaves
recovery at the wrong point so I don't see the change as a bad one? Only
PITR is affected, not replication or standalone operation.

--
Simon Riggs http://www.2ndQuadrant.com/books/
PostgreSQL Development, 24x7 Support, Training and Services

On Wed, Feb 9, 2011 at 2:02 PM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>> Why did you change the default to on? This would surprise people who are
>> used to PITR.
>
> You pointed out that the code did not match the documented default. So I
> made them match according to the docs.

Well, I meant changing the docs rather than the code.

> Making it pause at target by default is more natural behaviour, even if
> it is a change of behaviour. It can waste a lot of time if it leaves
> recovery at the wrong point so I don't see the change as a bad one? Only
> PITR is affected, not replication or standalone operation.

I agree that new option is useful to reduce the waste of time as you described.
But I'm still not sure that the change of default behavior is better.
Because I can
easily imagine the case where a user feels confused about the pause of PITR
when he starts PITR as he did in previous version. It would take some time for
him to learn what to do in that situation (i.e., execute pg_xlog_replay_resume).

On the second thought, I think it's useful to emit the NOTICE message when
recovery reaches the pause point, as follows.

NOTICE: Recovery will not complete until pg_xlog_replay_resume() is called.

Regards,

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

On Wed, Feb 9, 2011 at 07:22, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
> On Wed, Feb 9, 2011 at 2:02 PM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>>> Why did you change the default to on? This would surprise people who are
>>> used to PITR.
>>
>> You pointed out that the code did not match the documented default. So I
>> made them match according to the docs.
>
> Well, I meant changing the docs rather than the code.
>
>> Making it pause at target by default is more natural behaviour, even if
>> it is a change of behaviour. It can waste a lot of time if it leaves
>> recovery at the wrong point so I don't see the change as a bad one? Only
>> PITR is affected, not replication or standalone operation.
>
> I agree that new option is useful to reduce the waste of time as you described.
> But I'm still not sure that the change of default behavior is better.

FWIW, I like the change of behavior. We obviously need to put it
prominently in the release notes, but it makes life significantly
easier.

> Because I can
> easily imagine the case where a user feels confused about the pause of PITR
> when he starts PITR as he did in previous version. It would take some time for
> him to learn what to do in that situation (i.e., execute pg_xlog_replay_resume).
>
> On the second thought, I think it's useful to emit the NOTICE message when
> recovery reaches the pause point, as follows.
>
>    NOTICE: Recovery will not complete until pg_xlog_replay_resume() is called.

Combined with this, yes.

I was also worried about the non-hot-standby case, but I see that the
patch makes sure you can't enable pause when not in hot standby mode.
Which in itself might be surprising - perhaps we need a NOTICE for
when that happens as well?

And it definitely needs to be mentioned in the docs for
pause_at_recovery_target that it only works in hot standby.

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

On Wed, 2011-02-09 at 15:22 +0900, Fujii Masao wrote:

> On the second thought, I think it's useful to emit the NOTICE message when
> recovery reaches the pause point, as follows.
>
> NOTICE: Recovery will not complete until pg_xlog_replay_resume() is called.

I'm OK with adding a message, but NOTICE is the wrong level.

My proposal is this message

LOG: Recovery has paused. Execute pg_xlog_replay_resume() to continue.

--
Simon Riggs http://www.2ndQuadrant.com/books/
PostgreSQL Development, 24x7 Support, Training and Services

On Tue, Feb 15, 2011 at 9:21 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> On Wed, 2011-02-09 at 15:22 +0900, Fujii Masao wrote:
>
>> On the second thought, I think it's useful to emit the NOTICE message when
>> recovery reaches the pause point, as follows.
>>
>>     NOTICE: Recovery will not complete until pg_xlog_replay_resume() is called.
>
> I'm OK with adding a message, but NOTICE is the wrong level.
>
> My proposal is this message
>
> LOG:  Recovery has paused. Execute pg_xlog_replay_resume() to continue.

I agree to use LOG level. But "Execute pg_xlog_replay_resume() to continue."
looks like a hint rather than log. So what about:

LOG: Recovery has paused.
HINT: Execute pg_xlog_replay_resume() to continue.

Regards,

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

On Wed, Feb 9, 2011 at 5:12 PM, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
> I was also worried about the non-hot-standby case, but I see that the
> patch makes sure you can't enable pause when not in hot standby mode.
> Which in itself might be surprising - perhaps we need a NOTICE for
> when that happens as well?
>
> And it definitely needs to be mentioned in the docs for
> pause_at_recovery_target that it only works in hot standby.

+1 with all the above.

Regards,

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

On Tue, Feb 8, 2011 at 11:00 PM, Jaime Casanova <jaime(at)2ndquadrant(dot)com> wrote:
> On Tue, Feb 8, 2011 at 1:30 PM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>> Basic Recovery Control functions for use in Hot Standby. Pause, Resume,
>> Status check functions only. Also, new recovery.conf parameter to
>> pause_at_recovery_target, default on.
>>
>
> I guess that new parameter should also be in recovery.conf.sample, right?
>

This one is still not in recovery.conf.sample

--
Jaime Casanova         www.2ndQuadrant.com
Professional PostgreSQL: Soporte y capacitación de PostgreSQL

On Tue, Mar 8, 2011 at 9:20 AM, Jaime Casanova <jaime(at)2ndquadrant(dot)com> wrote:
> On Tue, Feb 8, 2011 at 11:00 PM, Jaime Casanova <jaime(at)2ndquadrant(dot)com> wrote:
>> On Tue, Feb 8, 2011 at 1:30 PM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>>> Basic Recovery Control functions for use in Hot Standby. Pause, Resume,
>>> Status check functions only. Also, new recovery.conf parameter to
>>> pause_at_recovery_target, default on.
>>>
>>
>> I guess that new parameter should also be in recovery.conf.sample, right?
>>
>
> This one is still not in recovery.conf.sample

What about the attached patch?

>> And it definitely needs to be mentioned in the docs for
>> pause_at_recovery_target that it only works in hot standby.

Done.

>>> On the second thought, I think it's useful to emit the NOTICE message when
>>> recovery reaches the pause point, as follows.
>>>
>>> NOTICE: Recovery will not complete until pg_xlog_replay_resume() is called.
>>
>> I'm OK with adding a message, but NOTICE is the wrong level.
>>
>> My proposal is this message
>>
>> LOG: Recovery has paused. Execute pg_xlog_replay_resume() to continue.

Done.

And, I found that <indexterm> tag has not been added for recovery
control functions
yet. So I added that into func.sgml.

Regards,

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

Fujii Masao <masao(dot)fujii(at)gmail(dot)com> writes:
> What about the attached patch?

> + ereport(LOG,
> + (errmsg("Recovery has paused. Execute pg_xlog_replay_resume() to continue.")));

This isn't even close to following our message style guidelines ...

regards, tom lane

On Wed, Feb 9, 2011 at 5:12 PM, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
> I was also worried about the non-hot-standby case, but I see that the
> patch makes sure you can't enable pause when not in hot standby mode.
> Which in itself might be surprising - perhaps we need a NOTICE for
> when that happens as well?

I didn't include this fix in the patch because I prefer FATAL to
NOTICE for that.
NOTICE doesn't stop recovery. So we might be unable to notice such a NOTICE
message and stop the recovery before it's too late, i.e., the recovery has
completed at the undesirable point. So I think that emitting FATAL is safer.

Regards,

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

On Tue, Mar 8, 2011 at 12:04 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Fujii Masao <masao(dot)fujii(at)gmail(dot)com> writes:
>> What about the attached patch?
>
>> +                                             ereport(LOG,
>> +                                                             (errmsg("Recovery has paused. Execute pg_xlog_replay_resume() to continue.")));
>
> This isn't even close to following our message style guidelines ...

Thanks! I lower-cased the leading character of the first sentence, and treated
the second as HINT. I attached the updated patch.

Regards,

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

On Tue, Mar 8, 2011 at 12:04 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
> On Wed, Feb 9, 2011 at 5:12 PM, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
>> I was also worried about the non-hot-standby case, but I see that the
>> patch makes sure you can't enable pause when not in hot standby mode.
>> Which in itself might be surprising - perhaps we need a NOTICE for
>> when that happens as well?
>
> I didn't include this fix in the patch because I prefer FATAL to
> NOTICE for that.
> NOTICE doesn't stop recovery. So we might be unable to notice such a NOTICE
> message and stop the recovery before it's too late, i.e., the recovery has
> completed at the undesirable point. So I think that emitting FATAL is safer.

I included this fix in the patch, which emits FATAL if pause_at_recovery_target
is enabled while hot standby is disabled and the recovery target is set.

Regards,

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

On Tue, Mar 8, 2011 at 5:48 AM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
> On Tue, Mar 8, 2011 at 12:04 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
>> On Wed, Feb 9, 2011 at 5:12 PM, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
>>> I was also worried about the non-hot-standby case, but I see that the
>>> patch makes sure you can't enable pause when not in hot standby mode.
>>> Which in itself might be surprising - perhaps we need a NOTICE for
>>> when that happens as well?
>>
>> I didn't include this fix in the patch because I prefer FATAL to
>> NOTICE for that.
>> NOTICE doesn't stop recovery. So we might be unable to notice such a NOTICE
>> message and stop the recovery before it's too late, i.e., the recovery has
>> completed at the undesirable point. So I think that emitting FATAL is safer.
>
> I included this fix in the patch, which emits FATAL if pause_at_recovery_target
> is enabled while hot standby is disabled and the recovery target is set.

Eh, this is problematic, because you can't claim in the documentation
(and the comments in recovery.conf.sample) that the parameter has no
effect when Hot Standby is not enabled, and then at the same time make
that combination a FATAL error. I don't have a strong opinion on
whether to change the docs or make it not FATAL, but the two have to
match.

Committing the rest.

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

On Fri, Mar 11, 2011 at 4:36 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Tue, Mar 8, 2011 at 5:48 AM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
>> On Tue, Mar 8, 2011 at 12:04 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
>>> On Wed, Feb 9, 2011 at 5:12 PM, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
>>>> I was also worried about the non-hot-standby case, but I see that the
>>>> patch makes sure you can't enable pause when not in hot standby mode.
>>>> Which in itself might be surprising - perhaps we need a NOTICE for
>>>> when that happens as well?
>>>
>>> I didn't include this fix in the patch because I prefer FATAL to
>>> NOTICE for that.
>>> NOTICE doesn't stop recovery. So we might be unable to notice such a NOTICE
>>> message and stop the recovery before it's too late, i.e., the recovery has
>>> completed at the undesirable point. So I think that emitting FATAL is safer.
>>
>> I included this fix in the patch, which emits FATAL if pause_at_recovery_target
>> is enabled while hot standby is disabled and the recovery target is set.
>
> Eh, this is problematic, because you can't claim in the documentation
> (and the comments in recovery.conf.sample) that the parameter has no
> effect when Hot Standby is not enabled, and then at the same time make
> that combination a FATAL error.  I don't have a strong opinion on
> whether to change the docs or make it not FATAL, but the two have to
> match.

Yeah, since I like the former, I changed the wordings in the doc and
recovery.conf.sample. What about the attached patch?

> Committing the rest.

Thanks!

Regards,

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

Fujii Masao <masao(dot)fujii(at)gmail(dot)com> writes:
> Yeah, since I like the former, I changed the wordings in the doc and
> recovery.conf.sample. What about the attached patch?

Please stop plastering the code with elog(FATAL) calls. Those are
hardly ever appropriate. In contexts where it might be reasonable
to do that, the error handler will treat ERROR like FATAL anyway.

regards, tom lane

On Fri, Mar 11, 2011 at 9:31 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Fujii Masao <masao(dot)fujii(at)gmail(dot)com> writes:
>> Yeah, since I like the former, I changed the wordings in the doc and
>> recovery.conf.sample. What about the attached patch?
>
> Please stop plastering the code with elog(FATAL) calls.  Those are
> hardly ever appropriate.  In contexts where it might be reasonable
> to do that, the error handler will treat ERROR like FATAL anyway.

Another problem here is that we are defaulting to hot_standby=off and
pause_at_recovery_target=on. So AIUI, with this patch, if someone
sets a recovery target without making any other changes to the
configuration, their database won't start up. That seems poor.

Even without the FATAL error, this whole pause_at_recovery_target
thing is a little weird. If someone sets a recovery target without
making any other configuration changes, and Hot Standby is not
enabled, then we will enter normal running, but if Hot Standby *is*
enabled, then we'll replay to that point and pause recovery. That
seems a bit confusing.

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

On Sat, Mar 12, 2011 at 1:59 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Fri, Mar 11, 2011 at 9:31 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Fujii Masao <masao(dot)fujii(at)gmail(dot)com> writes:
>>> Yeah, since I like the former, I changed the wordings in the doc and
>>> recovery.conf.sample. What about the attached patch?
>>
>> Please stop plastering the code with elog(FATAL) calls.  Those are
>> hardly ever appropriate.  In contexts where it might be reasonable
>> to do that, the error handler will treat ERROR like FATAL anyway.
>
> Another problem here is that we are defaulting to hot_standby=off and
> pause_at_recovery_target=on.  So AIUI, with this patch, if someone
> sets a recovery target without making any other changes to the
> configuration, their database won't start up.  That seems poor.

We should flip the default value of pause_at_recovery_target?

> Even without the FATAL error, this whole pause_at_recovery_target
> thing is a little weird.  If someone sets a recovery target without
> making any other configuration changes, and Hot Standby is not
> enabled, then we will enter normal running, but if Hot Standby *is*
> enabled, then we'll replay to that point and pause recovery.  That
> seems a bit confusing.

That's because there is no way to resume recovery which was
paused by pause_at_recovery_target when hot standby is disabled,
i.e., in that case we cannot call pg_xlog_replay_resume() to resume
the recovery.

How should recovery work when pause_at_recovery_target is
enabled but hot standby is disabled? We have three choices:

1. Forbit those settings, i.e., throw FATAL error. Tom dislikes this
idea.
2. Ignore pause_at_recovery_target. When recovery reaches the
target, it ends without pausing, and then the server gets into
normal processing mode. This would be unexpected behavior
from DBA's point of view because he or she expects that
recovery is paused at the target. To retry recovery, he or she
needs to restore the backup again.
3. Pause recovery even if hot standby is disabled. Since there
is no way to resume recovery, recovery would pause until
shutdown is requested.

For me, #1 looks like the most harmless in them. But, better
ideas? Votes?

Regards,

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

On Wed, 2011-03-16 at 16:29 +0900, Fujii Masao wrote:
> On Sat, Mar 12, 2011 at 1:59 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> > On Fri, Mar 11, 2011 at 9:31 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> >> Fujii Masao <masao(dot)fujii(at)gmail(dot)com> writes:
> >>> Yeah, since I like the former, I changed the wordings in the doc and
> >>> recovery.conf.sample. What about the attached patch?
> >>
> >> Please stop plastering the code with elog(FATAL) calls. Those are
> >> hardly ever appropriate. In contexts where it might be reasonable
> >> to do that, the error handler will treat ERROR like FATAL anyway.
> >
> > Another problem here is that we are defaulting to hot_standby=off and
> > pause_at_recovery_target=on. So AIUI, with this patch, if someone
> > sets a recovery target without making any other changes to the
> > configuration, their database won't start up. That seems poor.
>
> We should flip the default value of pause_at_recovery_target?

No, we shouldn't. Robert's comments are wrong and he shouldn't post such
things without testing them or reading the code.

> > Even without the FATAL error, this whole pause_at_recovery_target
> > thing is a little weird. If someone sets a recovery target without
> > making any other configuration changes, and Hot Standby is not
> > enabled, then we will enter normal running, but if Hot Standby *is*
> > enabled, then we'll replay to that point and pause recovery. That
> > seems a bit confusing.
>
> That's because there is no way to resume recovery which was
> paused by pause_at_recovery_target when hot standby is disabled,
> i.e., in that case we cannot call pg_xlog_replay_resume() to resume
> the recovery.
>
> How should recovery work when pause_at_recovery_target is
> enabled but hot standby is disabled? We have three choices:
>
> 1. Forbit those settings, i.e., throw FATAL error. Tom dislikes this
> idea.
> 2. Ignore pause_at_recovery_target. When recovery reaches the
> target, it ends without pausing, and then the server gets into
> normal processing mode. This would be unexpected behavior
> from DBA's point of view because he or she expects that
> recovery is paused at the target. To retry recovery, he or she
> needs to restore the backup again.
> 3. Pause recovery even if hot standby is disabled. Since there
> is no way to resume recovery, recovery would pause until
> shutdown is requested.
>
> For me, #1 looks like the most harmless in them. But, better
> ideas? Votes?

(2) is how it works now.

(3) doesn't sound very sensible. Why would that be better than (2)

There's lots of ways to misconfigure things, so I'm not too concerned
about this minor point.

--
Simon Riggs http://www.2ndQuadrant.com/books/
PostgreSQL Development, 24x7 Support, Training and Services

On Wed, Mar 16, 2011 at 4:41 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>> > Another problem here is that we are defaulting to hot_standby=off and
>> > pause_at_recovery_target=on.  So AIUI, with this patch, if someone
>> > sets a recovery target without making any other changes to the
>> > configuration, their database won't start up.  That seems poor.
>>
>> We should flip the default value of pause_at_recovery_target?
>
> No, we shouldn't. Robert's comments are wrong and he shouldn't post such
> things without testing them or reading the code.

Did you miss the part where I said "with this patch"? Because my
description of what happens with Fujii-san's patch applied does in
fact match the behavior of the code he wrote. It doesn't match the
current behavior, nor was it intended to describe the current
behavior.

>> > Even without the FATAL error, this whole pause_at_recovery_target
>> > thing is a little weird.  If someone sets a recovery target without
>> > making any other configuration changes, and Hot Standby is not
>> > enabled, then we will enter normal running, but if Hot Standby *is*
>> > enabled, then we'll replay to that point and pause recovery.  That
>> > seems a bit confusing.
>>
>> That's because there is no way to resume recovery which was
>> paused by pause_at_recovery_target when hot standby is disabled,
>> i.e., in that case we cannot call pg_xlog_replay_resume() to resume
>> the recovery.
>>
>> How should recovery work when pause_at_recovery_target is
>> enabled but hot standby is disabled? We have three choices:
>>
>> 1. Forbit those settings, i.e., throw FATAL error. Tom dislikes this
>>     idea.
>> 2. Ignore pause_at_recovery_target. When recovery reaches the
>>     target, it ends without pausing, and then the server gets into
>>     normal processing mode. This would be unexpected behavior
>>     from DBA's point of view because he or she expects that
>>     recovery is paused at the target. To retry recovery, he or she
>>     needs to restore the backup again.
>> 3. Pause recovery even if hot standby is disabled. Since there
>>     is no way to resume recovery, recovery would pause until
>>     shutdown is requested.
>>
>> For me, #1 looks like the most harmless in them. But, better
>> ideas? Votes?
>
> (2) is how it works now.
>
> (3) doesn't sound very sensible. Why would that be better than (2)
>
> There's lots of ways to misconfigure things, so I'm not too concerned
> about this minor point.

I agree that (3) is not very sensible. I think there's a reasonable
debate to be had about whether (1) or (2) is better. Like you, I
prefer #2 (the current behavior) to #1 (the proposed patch); but for
my money it would be a little less confusing if the default were
pause_at_recovery_target=false.

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

Fujii Masao <masao(dot)fujii(at)gmail(dot)com> writes:
> How should recovery work when pause_at_recovery_target is
> enabled but hot standby is disabled? We have three choices:

> 1. Forbit those settings, i.e., throw FATAL error. Tom dislikes this
> idea.

No, I didn't say that. I said not to write elog(FATAL). If the
combination is nonsensical then it's fine to forbid it, but you don't
need FATAL for that. In particular, attempting to change to a
disallowed setting after system startup should not result in crashing
the postmaster. And it won't, if you just use the normal error level
for complaining about an invalid GUC setting.

regards, tom lane

On Wed, Mar 16, 2011 at 11:27 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Fujii Masao <masao(dot)fujii(at)gmail(dot)com> writes:
>> How should recovery work when pause_at_recovery_target is
>> enabled but hot standby is disabled? We have three choices:
>
>> 1. Forbit those settings, i.e., throw FATAL error. Tom dislikes this
>>     idea.
>
> No, I didn't say that.  I said not to write elog(FATAL).

Oh, sorry.

>  If the
> combination is nonsensical then it's fine to forbid it, but you don't
> need FATAL for that.  In particular, attempting to change to a
> disallowed setting after system startup should not result in crashing
> the postmaster.  And it won't, if you just use the normal error level
> for complaining about an invalid GUC setting.

Sorry, I've not been able to understand the point well yet. We should
just use elog(ERROR) instead? But since ERROR in startup process
is treated as FATAL, I'm not sure whether it's worth using ERROR
instead. Or you meant another things?

Only startup process is able to notice that nonsensical settings since
pause_at_recovery_target is a recovery.conf parameter. So I'm not
sure there is another way to forbid that other than elog(ERROR) and
elog(FATAL).

Regards,

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

On Thu, Mar 17, 2011 at 2:47 AM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
> On Wed, Mar 16, 2011 at 11:27 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Fujii Masao <masao(dot)fujii(at)gmail(dot)com> writes:
>>> How should recovery work when pause_at_recovery_target is
>>> enabled but hot standby is disabled? We have three choices:
>>
>>> 1. Forbit those settings, i.e., throw FATAL error. Tom dislikes this
>>>     idea.
>>
>> No, I didn't say that.  I said not to write elog(FATAL).
>
> Oh, sorry.
>
>>  If the
>> combination is nonsensical then it's fine to forbid it, but you don't
>> need FATAL for that.  In particular, attempting to change to a
>> disallowed setting after system startup should not result in crashing
>> the postmaster.  And it won't, if you just use the normal error level
>> for complaining about an invalid GUC setting.
>
> Sorry, I've not been able to understand the point well yet. We should
> just use elog(ERROR) instead? But since ERROR in startup process
> is treated as FATAL, I'm not sure whether it's worth using ERROR
> instead. Or you meant another things?

Yeah, I think he's saying that an ERROR in the startup process is
better than a FATAL, even though the effect is the same.

On the substantive issue, I don't think we have any consensus that
forbidding this combination of parameters is the right thing to do
anyway. Both Simon and I voted against that, and Tom's point has to
do only with style. Similarly, I voted for flipping the default for
pause_at_recovery_target to off, rather than on, but no one else has
bought into that suggestion either. Unless we get some more votes in
favor of doing one of those things, I think we should focus on the
actual must-fix issue here, which is properly documenting the way it
works now (i.e. adding the parameter to recovery.conf.sample with
appropriate documentation of the current behavior).

One thing I'm not quite clear on is what happens if we reach the
recovery target before we reach consistency. i.e. create restore
point, flush xlog, abnormal shutdown, try to recover to named restore
point. Is there any possibility that we can end up paused before Hot
Standby has actually started up. Because that would be fairly useless
and annoying.

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

On Fri, Mar 18, 2011 at 1:17 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> Sorry, I've not been able to understand the point well yet. We should
>> just use elog(ERROR) instead? But since ERROR in startup process
>> is treated as FATAL, I'm not sure whether it's worth using ERROR
>> instead. Or you meant another things?
>
> Yeah, I think he's saying that an ERROR in the startup process is
> better than a FATAL, even though the effect is the same.

We've already been using FATAL all over the recovery code. We should
s/FATAL/ERROR/g there (at least readRecoveryCommandFile)?

> On the substantive issue, I don't think we have any consensus that
> forbidding this combination of parameters is the right thing to do
> anyway.  Both Simon and I voted against that, and Tom's point has to
> do only with style.  Similarly, I voted for flipping the default for
> pause_at_recovery_target to off, rather than on, but no one else has
> bought into that suggestion either.  Unless we get some more votes in
> favor of doing one of those things, I think we should focus on the
> actual must-fix issue here, which is properly documenting the way it
> works now (i.e. adding the parameter to recovery.conf.sample with
> appropriate documentation of the current behavior).

I agree to flip the default to false, whether we forbid that combination
of settings.

> One thing I'm not quite clear on is what happens if we reach the
> recovery target before we reach consistency.  i.e. create restore
> point, flush xlog, abnormal shutdown, try to recover to named restore
> point.  Is there any possibility that we can end up paused before Hot
> Standby has actually started up.  Because that would be fairly useless
> and annoying.

Good catch! In that case, the same situation as (3) would happen.
I think that recovery should ignore pause_at_recovery_target until
it reaches consistent point. If we do so, when recovery target is
ahead of consistent point, recovery just ends in inconsistent point
and throws FATAL error.

Regards,

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

On 18.03.2011 07:13, Fujii Masao wrote:
> On Fri, Mar 18, 2011 at 1:17 AM, Robert Haas<robertmhaas(at)gmail(dot)com> wrote:
>> One thing I'm not quite clear on is what happens if we reach the
>> recovery target before we reach consistency. i.e. create restore
>> point, flush xlog, abnormal shutdown, try to recover to named restore
>> point. Is there any possibility that we can end up paused before Hot
>> Standby has actually started up. Because that would be fairly useless
>> and annoying.
>
> Good catch! In that case, the same situation as (3) would happen.
> I think that recovery should ignore pause_at_recovery_target until
> it reaches consistent point. If we do so, when recovery target is
> ahead of consistent point, recovery just ends in inconsistent point
> and throws FATAL error.

If recovery target is set to before its consistent, ie. before
minRecoveryPoint, we should throw an error before recovery even starts.
I'm not sure if we check that at the moment.

Not sure what to to do recovery target is beyond minRecoveryPoint and
pause_at_recovery_target=true, but the server hasn't been opened for hot
standby yet (because it hasn't seen a running-xacts record yet). I agree
it's pretty useless and annoying to stop there.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

On Fri, Mar 18, 2011 at 3:22 AM, Heikki Linnakangas
<heikki(dot)linnakangas(at)enterprisedb(dot)com> wrote:
> If recovery target is set to before its consistent, ie. before
> minRecoveryPoint, we should throw an error before recovery even starts. I'm
> not sure if we check that at the moment.

I don't see how you could check that anyway. How do you know where
you're going to see the given XID/timestamp/named restore point until
you actually get there?

> Not sure what to to do recovery target is beyond minRecoveryPoint and
> pause_at_recovery_target=true, but the server hasn't been opened for hot
> standby yet (because it hasn't seen a running-xacts record yet). I agree
> it's pretty useless and annoying to stop there.

I think the reasonable options are "enter normal running" and "shut down".

In any event, it sounds like someone needs to fix this, and I don't
know enough to do it. Can you or Fujii Masao do it?

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

On 18.03.2011 14:14, Robert Haas wrote:
> On Fri, Mar 18, 2011 at 3:22 AM, Heikki Linnakangas
> <heikki(dot)linnakangas(at)enterprisedb(dot)com> wrote:
>> If recovery target is set to before its consistent, ie. before
>> minRecoveryPoint, we should throw an error before recovery even starts. I'm
>> not sure if we check that at the moment.
>
> I don't see how you could check that anyway. How do you know where
> you're going to see the given XID/timestamp/named restore point until
> you actually get there?

Oh, good point. I was thinking that the recovery target is a particular
LSN, but clearly it's not.

>> Not sure what to to do recovery target is beyond minRecoveryPoint and
>> pause_at_recovery_target=true, but the server hasn't been opened for hot
>> standby yet (because it hasn't seen a running-xacts record yet). I agree
>> it's pretty useless and annoying to stop there.
>
> I think the reasonable options are "enter normal running" and "shut down".
>
> In any event, it sounds like someone needs to fix this, and I don't
> know enough to do it. Can you or Fujii Masao do it?

You could also argue for "log a warning, continue until we can open for
Hot standby, then pause".

I can write the patch once we know what we want. All of those options
sound reasonable to me. This is such a corner-case that it doesn't make
sense to make it user-configurable, though.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

On Fri, Mar 18, 2011 at 8:27 AM, Heikki Linnakangas
<heikki(dot)linnakangas(at)enterprisedb(dot)com> wrote:
> You could also argue for "log a warning, continue until we can open for Hot
> standby, then pause".

I don't like that one much.

> I can write the patch once we know what we want. All of those options sound
> reasonable to me. This is such a corner-case that it doesn't make sense to
> make it user-configurable, though.

I agree. Since pause_at_recovery_target is ignored when
hot_standby=off, I think it would be consistent to treat the case
where hot_standby=on but can't actually be initiated the same way -
just ignore the pause request and enter normal running. However, I
don't have a super-strong feeling that that's the only sensible way to
go, so count me as +0.5 for that approach.

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

Fujii Masao <masao(dot)fujii(at)gmail(dot)com> writes:
> On Fri, Mar 18, 2011 at 1:17 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>>> Sorry, I've not been able to understand the point well yet. We should
>>> just use elog(ERROR) instead? But since ERROR in startup process
>>> is treated as FATAL, I'm not sure whether it's worth using ERROR
>>> instead. Or you meant another things?

>> Yeah, I think he's saying that an ERROR in the startup process is
>> better than a FATAL, even though the effect is the same.

> We've already been using FATAL all over the recovery code. We should
> s/FATAL/ERROR/g there (at least readRecoveryCommandFile)?

Possibly, but as you say, it doesn't make that much difference in the
startup process. What is bothering me is the prospect of elog(FATAL)
in the postmaster. Code associated with GUC validity checking is likely
to get executed in the postmaster, which is why it should not throw
anything stronger than the normal GUC complaint levels. Even if the
patch as proposed is for code that could only be reached in the startup
process today, somebody might decide to rearrange it ...

regards, tom lane

On Fri, Mar 18, 2011 at 9:31 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Fri, Mar 18, 2011 at 8:27 AM, Heikki Linnakangas
> <heikki(dot)linnakangas(at)enterprisedb(dot)com> wrote:
>> You could also argue for "log a warning, continue until we can open for Hot
>> standby, then pause".
>
> I don't like that one much.
>
>> I can write the patch once we know what we want. All of those options sound
>> reasonable to me. This is such a corner-case that it doesn't make sense to
>> make it user-configurable, though.
>
> I agree.  Since pause_at_recovery_target is ignored when
> hot_standby=off, I think it would be consistent to treat the case
> where hot_standby=on but can't actually be initiated the same way -
> just ignore the pause request and enter normal running.

When hot_standby = on and the recovery target is ahead of the consistent point,
the server doesn't enter normal running since FATAL error happens. So I think
that it's more consistent to prevent the server from entering normal
running also
when hot_standby = off.

Regards,

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

On Wed, Mar 23, 2011 at 4:51 AM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
> On Fri, Mar 18, 2011 at 9:31 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> On Fri, Mar 18, 2011 at 8:27 AM, Heikki Linnakangas
>> <heikki(dot)linnakangas(at)enterprisedb(dot)com> wrote:
>>> You could also argue for "log a warning, continue until we can open for Hot
>>> standby, then pause".
>>
>> I don't like that one much.
>>
>>> I can write the patch once we know what we want. All of those options sound
>>> reasonable to me. This is such a corner-case that it doesn't make sense to
>>> make it user-configurable, though.
>>
>> I agree.  Since pause_at_recovery_target is ignored when
>> hot_standby=off, I think it would be consistent to treat the case
>> where hot_standby=on but can't actually be initiated the same way -
>> just ignore the pause request and enter normal running.
>
> When hot_standby = on and the recovery target is ahead of the consistent point,
> the server doesn't enter normal running since FATAL error happens. So I think
> that it's more consistent to prevent the server from entering normal
> running also
> when hot_standby = off.

Actually, my previous email was all nonsense, wasn't it? If we don't
reach the consistency point, we can't enter normal running anyway -
shut down is the only option no matter what.

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

On Wed, Mar 23, 2011 at 11:50 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Wed, Mar 23, 2011 at 4:51 AM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
>> On Fri, Mar 18, 2011 at 9:31 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>>> On Fri, Mar 18, 2011 at 8:27 AM, Heikki Linnakangas
>>> <heikki(dot)linnakangas(at)enterprisedb(dot)com> wrote:
>>>> You could also argue for "log a warning, continue until we can open for Hot
>>>> standby, then pause".
>>>
>>> I don't like that one much.
>>>
>>>> I can write the patch once we know what we want. All of those options sound
>>>> reasonable to me. This is such a corner-case that it doesn't make sense to
>>>> make it user-configurable, though.
>>>
>>> I agree.  Since pause_at_recovery_target is ignored when
>>> hot_standby=off, I think it would be consistent to treat the case
>>> where hot_standby=on but can't actually be initiated the same way -
>>> just ignore the pause request and enter normal running.
>>
>> When hot_standby = on and the recovery target is ahead of the consistent point,
>> the server doesn't enter normal running since FATAL error happens. So I think
>> that it's more consistent to prevent the server from entering normal
>> running also
>> when hot_standby = off.
>
> Actually, my previous email was all nonsense, wasn't it?  If we don't
> reach the consistency point, we can't enter normal running anyway -
> shut down is the only option no matter what.

Presumably you mean that the way its currently coded is the way it should stay?

--
 Simon Riggs                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

On Wed, Mar 23, 2011 at 9:38 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>> Actually, my previous email was all nonsense, wasn't it?  If we don't
>> reach the consistency point, we can't enter normal running anyway -
>> shut down is the only option no matter what.
>
> Presumably you mean that the way its currently coded is the way it should stay?

Uh, maybe, but it's not obvious to me that it actually is coded that
way. I don't see any safeguard that prevents recovery from pausing
before consistency is released. Is there one? Where?

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

On Thu, Mar 24, 2011 at 12:19 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Wed, Mar 23, 2011 at 9:38 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>>> Actually, my previous email was all nonsense, wasn't it?  If we don't
>>> reach the consistency point, we can't enter normal running anyway -
>>> shut down is the only option no matter what.
>>
>> Presumably you mean that the way its currently coded is the way it should stay?
>
> Uh, maybe, but it's not obvious to me that it actually is coded that
> way.  I don't see any safeguard that prevents recovery from pausing
> before consistency is released.  Is there one?  Where?

Oh, sorry for my poor explanation.

My explanation is true if we'll just change the code so that it ignores
pause_at_recovery_target until recovery reaches the consistency point.
Simon changed the code in that way yesterday.

Regards,

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

On Thu, Mar 24, 2011 at 7:02 AM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
> On Thu, Mar 24, 2011 at 12:19 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> On Wed, Mar 23, 2011 at 9:38 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>>>> Actually, my previous email was all nonsense, wasn't it?  If we don't
>>>> reach the consistency point, we can't enter normal running anyway -
>>>> shut down is the only option no matter what.
>>>
>>> Presumably you mean that the way its currently coded is the way it should stay?
>>
>> Uh, maybe, but it's not obvious to me that it actually is coded that
>> way.  I don't see any safeguard that prevents recovery from pausing
>> before consistency is released.  Is there one?  Where?
>
> Oh, sorry for my poor explanation.
>
> My explanation is true if we'll just change the code so that it ignores
> pause_at_recovery_target until recovery reaches the consistency point.
> Simon changed the code in that way yesterday.

Yep, I think we're good on this one now.

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