Thread: Re: [COMMITTERS] pgsql: Fast promote mode skips checkpoint at end of recovery.

On 31.01.2013 21:33, Simon Riggs wrote:
> If anyone really wants me to revert, pls start new hackers thread to
> discuss, or comment on changes.

Yes, I still think this needs fixing or reverting. Let me reiterate my
my complaints:

1. I don't like the check in ReadCheckPointRecord() that the WAL
containing last and previous checkpoint still exists. Several reasons
for that:

1.1. I don't think such a check is necessary to begin with. We replayed
that WAL record a while ago, so there's no reason to believe that it's
gone now. If there is a bug that causes that to happen, you're screwed
with or without this patch.

1.2. If we do that check, and it fails because the latest checkpoint is
not present, there should be a big fat warning in the log because
something's wrong. If you ask for fast promotion, and the system doesn't
do that, a log message is the least we can do.

1.3. Why check for the "prev" checkpoint? The latest checkpoint is
enough to recover, so why insist that also the previous one is present,
too? There are normal scenarios where it won't be, like just after
recovering from a base backup. I consider it a bug that fast promotion
doesn't work right after restoring from a base backup.

2. I don't like demoting the trigger file method to a second class
citizen. I think we should make all functionality available through both
methods. If there was a good reason for deprecating the trigger file
method, I could live with that, but this patch is not such a reason.

3. I don't like conflating the promotion modes and shutdown modes in the
pg_ctl option. Shutdown modes and promotion modes are separate concepts.
The "fast" option is pretty clear, but why does "smart" mean "create an
immediate checkpoint before promotion"? How is that smarter than the
fast mode?

The "pg_ctl --help" on that is a bit confusing too:

> Options for stop, restart or promote: -m, --mode=MODE        MODE can
> be "smart", "fast", or "immediate"

The "immediate" mode is not actually valid for "pg_ctl promote". That is
clarified later in the output by listing out what the modes mean, but
that above line is misleading,

4. I think fast promotion should be the default. Why not? There are
cases where you want the promotion to happen ASAP, and there are cases
where you don't care. But there are no scenarios where you want
promotion to be slow,

5. Docs changes are missing.


Here's what I think should be done:

1. Remove the check that prev checkpoint record exists.

2. Always do fast promotion if in standby mode. Remove the pg_ctl option.

3. Improve docs.

- Heikki

On 6 February 2013 16:36, Heikki Linnakangas <hlinnakangas@vmware.com> wrote:
> On 31.01.2013 21:33, Simon Riggs wrote:
>>
>> If anyone really wants me to revert, pls start new hackers thread to
>> discuss, or comment on changes.
>
>
> Yes, I still think this needs fixing or reverting. Let me reiterate my
> my complaints:

I'm sorry that they are complaints rather than just feedback, and will
work to address them.


> 1.3. Why check for the "prev" checkpoint? The latest checkpoint is
> enough to recover, so why insist that also the previous one is present,
> too?

That was there from Kyotaro's patch and I left it as it was since it
had been reviewed prior to me. I thought it was OK too, but now I
think your arguments are good and I'm now happy to change to just the
last checkpoint. That does bring into question what the value of the
prev checkpoint is in any situation, not just this one...

> There are normal scenarios where it won't be, like just after
> recovering from a base backup. I consider it a bug that fast promotion
> doesn't work right after restoring from a base backup.

OK

> 2. I don't like demoting the trigger file method to a second class
> citizen. I think we should make all functionality available through both
> methods. If there was a good reason for deprecating the trigger file
> method, I could live with that, but this patch is not such a reason.

I don't understand why we introduced a second method if they both will
continue to be used. I see no reason for that, other than backwards
compatibility. Enhancing both mechanisms suggests both will be
supported into the future. Please explain why the second mode exists?

> 3. I don't like conflating the promotion modes and shutdown modes in the
> pg_ctl option. Shutdown modes and promotion modes are separate concepts.
> The "fast" option is pretty clear, but why does "smart" mean "create an
> immediate checkpoint before promotion"? How is that smarter than the
> fast mode?

> The "pg_ctl --help" on that is a bit confusing too:
>
>> Options for stop, restart or promote: -m, --mode=MODE        MODE can
>> be "smart", "fast", or "immediate"
>
>
> The "immediate" mode is not actually valid for "pg_ctl promote". That is
> clarified later in the output by listing out what the modes mean, but
> that above line is misleading,

We can always rename them, as you wish.

> 4. I think fast promotion should be the default. Why not? There are
> cases where you want the promotion to happen ASAP, and there are cases
> where you don't care. But there are no scenarios where you want
> promotion to be slow,

Not true. Slow means safe and stable, and there are many scenarios
where we want safe and stable. (Of course, nobody specifically
requests slow). My feeling is that this is an area of exposure that we
have no need and therefore no business touching. I will of course go
with what others think here, but I don't find the argument that we
should go fast always personally convincing. I am willing to relax it
over time once we get zero field problems as a result.

> 5. Docs changes are missing.

OK

> Here's what I think should be done:
>
> 1. Remove the check that prev checkpoint record exists.

Agreed

> 2. Always do fast promotion if in standby mode. Remove the pg_ctl option.

Disagreed, other viewpoints welcome.

> 3. Improve docs.

Agreed

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

On 02/06/2013 09:43 AM, Simon Riggs wrote:

>> 4. I think fast promotion should be the default. Why not? There are
>> cases where you want the promotion to happen ASAP, and there are cases
>> where you don't care. But there are no scenarios where you want
>> promotion to be slow,
>
> Not true. Slow means safe and stable, and there are many scenarios
> where we want safe and stable. (Of course, nobody specifically
> requests slow). My feeling is that this is an area of exposure that we
> have no need and therefore no business touching. I will of course go
> with what others think here, but I don't find the argument that we
> should go fast always personally convincing. I am willing to relax it
> over time once we get zero field problems as a result.

Promotion, should by default should take the most safe, stable route and 
only that route.

+1 On Simon's response.

JD


-- 
Command Prompt, Inc. - http://www.commandprompt.com/
PostgreSQL Support, Training, Professional Services and Development
High Availability, Oracle Conversion, Postgres-XC
@cmdpromptinc - 509-416-6579

On Wed, Feb 6, 2013 at 12:43 PM, Simon Riggs <simon@2ndquadrant.com> wrote:
>> 2. I don't like demoting the trigger file method to a second class
>> citizen. I think we should make all functionality available through both
>> methods. If there was a good reason for deprecating the trigger file
>> method, I could live with that, but this patch is not such a reason.
>
> I don't understand why we introduced a second method if they both will
> continue to be used. I see no reason for that, other than backwards
> compatibility. Enhancing both mechanisms suggests both will be
> supported into the future. Please explain why the second mode exists?

I agree that we should be pushing people towards pg_ctl promote.  I
have no strong opinion about whether backward-compatibility for the
trigger file method is a good idea or not.  It might be a little soon
to relegate that to second-class status, but I'm not sure.

>> 4. I think fast promotion should be the default. Why not? There are
>> cases where you want the promotion to happen ASAP, and there are cases
>> where you don't care. But there are no scenarios where you want
>> promotion to be slow,
>
> Not true. Slow means safe and stable, and there are many scenarios
> where we want safe and stable. (Of course, nobody specifically
> requests slow). My feeling is that this is an area of exposure that we
> have no need and therefore no business touching. I will of course go
> with what others think here, but I don't find the argument that we
> should go fast always personally convincing. I am willing to relax it
> over time once we get zero field problems as a result.

I'm skeptical of the idea that we shouldn't default to fast-promote
because the fast-promote code might be buggy.  We do sometimes default
new features to off on the grounds that they might be buggy - Hot
Standby got an on/off switch partly for that reason - but usually we
only add a knob if there's some plausible reason for wanting to change
the setting independently of the possibility of bugs.  For instance,
in the case of Hot Standby, another of the reasons for adding a knob
was that people wanted a way to make sure that they wouldn't
accidentally connect to the standby when they intended to connect to
the master.  That may or may not have been a sufficiently *good*
reason, but it was accepted as justification at the time.

So I would ask this question: why would someone want to turn off
fast-promote mode, assuming for the sake of argument that it isn't
buggy?  I think there might be good reasons to do that, but I'm not
sure what they are.  I doubt it will be a common thing to want.  I
think most people are going to want fast-promote, but many may not
know enough to request it, which means that if it isn't the default,
the code may not get much testing anyway.

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

On 06.02.2013 20:02, Robert Haas wrote:
> On Wed, Feb 6, 2013 at 12:43 PM, Simon Riggs<simon@2ndquadrant.com>  wrote:
>>> 2. I don't like demoting the trigger file method to a second class
>>> citizen. I think we should make all functionality available through both
>>> methods. If there was a good reason for deprecating the trigger file
>>> method, I could live with that, but this patch is not such a reason.
>>
>> I don't understand why we introduced a second method if they both will
>> continue to be used. I see no reason for that, other than backwards
>> compatibility. Enhancing both mechanisms suggests both will be
>> supported into the future. Please explain why the second mode exists?
>
> I agree that we should be pushing people towards pg_ctl promote.  I
> have no strong opinion about whether backward-compatibility for the
> trigger file method is a good idea or not.  It might be a little soon
> to relegate that to second-class status, but I'm not sure.

Both the trigger file and pg_ctl promote methods are useful in different 
setups. If you point the trigger file on an NFS mount or similar, that 
allows triggering promotion from a different host without providing 
shell access. You might want to put the trigger file on an NFS mount 
that also contains the WAL archive, for example. A promotion script that 
also controls the network routers to redirect traffic and STONITH the 
dead node, can then simply "touch /mnt/.../trigger" to promote. Sure, it 
could also ssh to the server and run "pg_ctl promote", but that requires 
more setup.

- Heikki

On Wed, Feb 6, 2013 at 1:47 PM, Heikki Linnakangas
<hlinnakangas@vmware.com> wrote:
> On 06.02.2013 20:02, Robert Haas wrote:
>>
>> On Wed, Feb 6, 2013 at 12:43 PM, Simon Riggs<simon@2ndquadrant.com>
>> wrote:
>>>>
>>>> 2. I don't like demoting the trigger file method to a second class
>>>> citizen. I think we should make all functionality available through both
>>>> methods. If there was a good reason for deprecating the trigger file
>>>> method, I could live with that, but this patch is not such a reason.
>>>
>>>
>>> I don't understand why we introduced a second method if they both will
>>> continue to be used. I see no reason for that, other than backwards
>>> compatibility. Enhancing both mechanisms suggests both will be
>>> supported into the future. Please explain why the second mode exists?
>>
>>
>> I agree that we should be pushing people towards pg_ctl promote.  I
>> have no strong opinion about whether backward-compatibility for the
>> trigger file method is a good idea or not.  It might be a little soon
>> to relegate that to second-class status, but I'm not sure.
>
>
> Both the trigger file and pg_ctl promote methods are useful in different
> setups. If you point the trigger file on an NFS mount or similar, that
> allows triggering promotion from a different host without providing shell
> access. You might want to put the trigger file on an NFS mount that also
> contains the WAL archive, for example. A promotion script that also controls
> the network routers to redirect traffic and STONITH the dead node, can then
> simply "touch /mnt/.../trigger" to promote. Sure, it could also ssh to the
> server and run "pg_ctl promote", but that requires more setup.

Good point.  I hadn't thought about that.

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

On 6 February 2013 18:02, Robert Haas <robertmhaas@gmail.com> wrote:

> So I would ask this question: why would someone want to turn off
> fast-promote mode, assuming for the sake of argument that it isn't
> buggy?

You can write a question many ways, and lead people towards a
conclusion as a result.

Why would someone want to turn off safe-promote mode, assuming it was
fast enough?

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

On 07.02.2013 10:41, Simon Riggs wrote:
> On 6 February 2013 18:02, Robert Haas<robertmhaas@gmail.com>  wrote:
>
>> So I would ask this question: why would someone want to turn off
>> fast-promote mode, assuming for the sake of argument that it isn't
>> buggy?
>
> You can write a question many ways, and lead people towards a
> conclusion as a result.
>
> Why would someone want to turn off safe-promote mode, assuming it was
> fast enough?

Okay, I'll bite..

Because in some of your servers, the safe/slow promotion is not fast 
enough, and you want to use the same promotion script in both scenarios, 
to keep things simple.

Because you're not sure if it's fast enough, and want to play it safe.

Because faster is nicer, even if the slow mode would be "fast enough".


It makes me uncomfortable that we're adding switches to pg_ctl promote 
just because we're worried there might be bugs in our code. If we don't 
trust the code as it is, it needs more testing. We can analyze the code 
more thoroughly, to make an educated guess on what's likely to happen if 
it's broken, and consider adding some sanity checks etc. to make the 
consequences less severe. We should not put the burden on our users to 
decide if the code is trustworthy enough to use.

Note that we still wouldn't do fast promotion in crash recovery, so 
there's that escape hatch if there is indeed a bug in our code and fast 
promotion fails.

- Heikki

On 7 February 2013 09:04, Heikki Linnakangas <hlinnakangas@vmware.com> wrote:

> It makes me uncomfortable that we're adding switches to pg_ctl promote just
> because we're worried there might be bugs in our code. If we don't trust the
> code as it is, it needs more testing. We can analyze the code more
> thoroughly, to make an educated guess on what's likely to happen if it's
> broken, and consider adding some sanity checks etc. to make the consequences
> less severe. We should not put the burden on our users to decide if the code
> is trustworthy enough to use.

I don't think I said I was worried about bugs in code, did I? The
point is that this has been a proven mechanism for many years and
we're now discussing turning that off completely with no user option
to put it back, which has considerable risk with it.

Acknowledging risks and taking risk mitigating actions is a normal
part of any IT project. If we start getting unexplained errors it
could take a long time to trace that back to the lack of a shutdown
checkpoint.

I don't mind saying openly this worries me and its why I took months
to commit it. If there was no risk here and its all so easy, why
didn't we commit this last year, or why didn't you override me and
commit this earlier in this cycle?

I have to say I care very little for the beauty or lack of command
switches, in such a case. The "cost" there is low.

Tell me you understand the risk I am discussing, tell me in your
opinion we're safe and I'm being unnecessarily cautious, maybe even
foolishly so, and I'll relent. I'll stand by that and take the flak.
But saying you don't like a switch is like telling me you don't like
the colour of my car safety belt.

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

On Thu, Feb 7, 2013 at 4:04 AM, Heikki Linnakangas
<hlinnakangas@vmware.com> wrote:
> It makes me uncomfortable that we're adding switches to pg_ctl promote just
> because we're worried there might be bugs in our code. If we don't trust the
> code as it is, it needs more testing. We can analyze the code more
> thoroughly, to make an educated guess on what's likely to happen if it's
> broken, and consider adding some sanity checks etc. to make the consequences
> less severe. We should not put the burden on our users to decide if the code
> is trustworthy enough to use.

+1

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

Heikki Linnakangas <hlinnakangas@vmware.com> wrote:
> On 07.02.2013 10:41, Simon Riggs wrote:


>>  Why would someone want to turn off safe-promote mode, assuming it was
>>  fast enough?

> Because faster is nicer, even if the slow mode would be "fast enough".

http://www.youtube.com/watch?v=H3R-rtWPyJY

-Kevin

On 6 February 2013 17:43, Simon Riggs <simon@2ndquadrant.com> wrote:

>> Here's what I think should be done:
>>
>> 1. Remove the check that prev checkpoint record exists.
>
> Agreed

Done

>> 2. Always do fast promotion if in standby mode. Remove the pg_ctl option.
>
> Disagreed, other viewpoints welcome.

Waiting for further comments.

>> 3. Improve docs.
>
> Agreed

Pending.

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