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
