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

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