Infrastructure changes for recovery

Simon Riggs <simon@2ndquadrant.com> writes:

I propose to
* have startup process signal postmaster when it starts Redo phase (if
it starts it)

Doesn't seem like "starts recovery" is the point at which you can start
letting clients into the DB. What you want is to reach a point at which
you're sure that the DB is internally consistent, though perhaps not
fully synced with the master. In a PITR recovery scenario this would
correspond to reaching the minimum safe stop point. In true crash
recovery I don't think you can let people in till you're done.

regards, tom lane

Simon Riggs wrote:

I propose to
* have startup process signal postmaster when it starts Redo phase (if
it starts it)

I think the first is a good idea -- at least, if you can get the startup
process to use the normal ReadBuffer code path instead of
XLogReadBuffer. I don't really know what's needed for this to work, but
there seem to be several layers of stuff that need to be up for this to
work.

* have startup process signal postmaster again when it has completed
recovery, so that the change of state is via explicit signal rather than
death of the child process

I'm not sure that this is very useful, because the startup process
cannot do anything much. It is an auxiliary process, which means it
can't connect to a database and it can't run transactions.

This other idea:

ii) have postmaster allow connections while Redo is taking place, as one
part of allowing query access to standby database

is interesting and I'm sure it would be very welcome. Of course, it is
first necessary to be able to run transactions in true "read only" mode,
which perhaps is nowadays not so difficult given the work with VXids.

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

On Thu, 2008-08-07 at 10:48 -0400, Tom Lane wrote:

Simon Riggs <simon@2ndquadrant.com> writes:

I propose to
* have startup process signal postmaster when it starts Redo phase (if
it starts it)

Doesn't seem like "starts recovery" is the point at which you can start
letting clients into the DB. What you want is to reach a point at which
you're sure that the DB is internally consistent, though perhaps not
fully synced with the master. In a PITR recovery scenario this would
correspond to reaching the minimum safe stop point. In true crash
recovery I don't think you can let people in till you're done.

Ack to both, no worries: just worded it a little too loosely.

For crash recovery we could let them in earlier, but I think its going
to recover faster if we don't. So, yes, only during archive recovery and
therefore only from min safe stopping point. That will mean bgwriter is
only active during archive recovery, but that's not important, since we
(almost) never perform restartpoints during crash recovery.

For other background I should also mention that this architecture
proposal is different from Florian's SoC proposals, which had a separate
recovery process to perform the work after the min safe stopping point.

--
Simon Riggs www.2ndQuadrant.com
PostgreSQL Training, Services and Support

On Thu, 2008-08-07 at 10:56 -0400, Alvaro Herrera wrote:

* have startup process signal postmaster again when it has completed
recovery, so that the change of state is via explicit signal rather than
death of the child process

I'm not sure that this is very useful, because the startup process
cannot do anything much. It is an auxiliary process, which means it
can't connect to a database and it can't run transactions.

Yes, but its going to be a lot easier to make it do transactions than it
will be to spawn something else that does? Or so I thought...

Specifically, I want to have a rmgr_post_cleanup() call to perform such
actions.

Who do you think should run those calls? They might need transactions,
or not, depending upon the rmgr. I envisage initially that rmgrs could
mark indexes as Invalid in pg_class, which requires a transaction (or
does it?).

If we did alter rmgrs to allow them to actually rebuild an index, then
it would be desirable to have them do it in parallel, just like AV
workers.

Would it be possible to use have startup to run rmgr_post_cleanup, then
provide a facility to have them run tasks through AV workers?

Sounds like a separate patch anyway.

--
Simon Riggs www.2ndQuadrant.com
PostgreSQL Training, Services and Support

On Thu, 2008-08-07 at 10:56 -0400, Alvaro Herrera wrote:

Simon Riggs wrote:

ii) have postmaster allow connections while Redo is taking place, as

one

part of allowing query access to standby database

is interesting and I'm sure it would be very welcome. Of course, it
is first necessary to be able to run transactions in true "read only"
mode, which perhaps is nowadays not so difficult given the work with
VXids.

Maybe I am assuming that part is easier than I thought. Florian's
contribution with the VXid work should not be understated.

I was going to spread the setting of the global variable InRedo to all
backends, so they would be able to answer the question IsInRedo().

Anybody attempting to assign a new TransactionId would check this and
then exit with an ERROR. There would also be a check in XLogInsert(), in
case the first case doesn't catch everything or everyone. Transactions
would also run as read-only transactions to keep the front door locked.

Anything else you think needs bolting down?

--
Simon Riggs www.2ndQuadrant.com
PostgreSQL Training, Services and Support

On Thu, 2008-08-07 at 12:44 +0100, Simon Riggs wrote:

I would like to propose some changes to the infrastructure for recovery.
These changes are beneficial in themselves, but also form the basis for
other work we might later contemplate.

Currently
* the startup process performs restartpoints during recovery
* the death of the startup process is tied directly to the change of
state in the postmaster following recovery

I propose to
* have startup process signal postmaster when it starts Redo phase (if
it starts it)

Decoupling things in this way allows us to
1. arrange for the bgwriter to start during Redo, so it can:
i) clean dirty blocks for the startup process
ii) perform restartpoints in background
Both of these aspects will increase performance of recovery

Taking into account comments from Tom and Alvaro

Included patch with the following changes:

* new postmaster mode known as consistent recovery, entered only when
recovery passes safe/consistent point. InRedo is now set in all
processes when started/connected in consistent recovery mode.

* bgwriter and stats process starts in consistent recovery mode.
bgwriter changes mode when startup process completes.

* bgwriter now performs restartpoints and also cleans shared_buffers
while the startup process performs redo apply

* recovery.conf parameter log_restartpoints is now deprecated, since
function overlaps with log_checkpoints too much. I've kept the
distinction between restartpoints and checkpoints in code, to avoid
convoluted code. Minor change, not critical.

[Replying to one of Alvaro's other comments: Startup process still uses
XLogReadBuffer. I'm not planning on changing that either, at least not
in this patch.]

Patch doesn't conflict with rmgr plugin patch.

Passes make check, but that's easy.
Various other tests all seem to be working.

--
Simon Riggs www.2ndQuadrant.com
PostgreSQL Training, Services and Support

Simon Riggs <simon@2ndQuadrant.com> writes:

Included patch with the following changes:

* new postmaster mode known as consistent recovery, entered only when
recovery passes safe/consistent point. InRedo is now set in all
processes when started/connected in consistent recovery mode.

I looked at this and didn't like the InRedo signaling at all. In the
first place, it looks like you're expecting the flag to be passed down
via fork(), but that won't work in EXEC_BACKEND mode. (Yes, easily
fixed, but it's wrong as-is.) In the second place, the method of
signaling it to the bgwriter looks to have race conditions: in
principle, at least, I think the startup process could try to clear
the shared-memory InRedo flag before the bgwriter has gotten as far as
setting it. (This might work if the startup process can't exit redo
mode without the bgwriter having finished a checkpoint, but such a
dependency is too rube goldbergian for me.)

ISTM that it would probably be better if there were exactly one InRedo
flag in shared memory, probably in xlog.c's shared state, with the
postmaster not being responsible for setting or clearing it; rather
the startup process should do those things.

* bgwriter and stats process starts in consistent recovery mode.
bgwriter changes mode when startup process completes.

I'm not sure about the interaction of this. In particular, what about
recovery restart points before we have reached the safe stop point?
I don't think we want to give up the capability of having those.

Also, it seems pretty bogus to update the in-memory ControlFile
checkpoint values before the restart point is actually done. It looks
to me like what you have done is to try to use those fields as signaling
for the restart request in addition to their existing purposes, which
I think is confusing and probably dangerous. I'd rather there were a
different signaling path and ControlFile maintains its currrent
definition.

I found the changes in bgwriter.c unreadable. You've evidently
moved blocks of code around, but exactly what did you mean to
change? Why is there so much duplicated code now?

I've kept the distinction between restartpoints and checkpoints in
code, to avoid convoluted code.

Hmm, I dunno, it seems like that might be a bad choice. Are you sure
it's not cleaner to just use the regular checkpoint code?

regards, tom lane

On Mon, 2008-09-08 at 13:34 -0400, Tom Lane wrote:

Simon Riggs <simon@2ndQuadrant.com> writes:

Included patch with the following changes:

* new postmaster mode known as consistent recovery, entered only when
recovery passes safe/consistent point. InRedo is now set in all
processes when started/connected in consistent recovery mode.

I looked at this and didn't like the InRedo signaling at all. In the
first place, it looks like you're expecting the flag to be passed down
via fork(), but that won't work in EXEC_BACKEND mode. (Yes, easily
fixed, but it's wrong as-is.)

OK, thanks. I didn't check that.

In the second place, the method of
signaling it to the bgwriter looks to have race conditions: in
principle, at least, I think the startup process could try to clear
the shared-memory InRedo flag before the bgwriter has gotten as far as
setting it. (This might work if the startup process can't exit redo
mode without the bgwriter having finished a checkpoint, but such a
dependency is too rube goldbergian for me.)

We never interrupt what the bgwriter does. If it is in the middle of
doing a checkpoint when recovery finishes, then Startup process just
waits behind it and then issues a shutdown checkpoint. If there's loads
of blocks to be written it doesn't matter who writes them. There's only
one soup spoon, and it doesn't matter who stirs the soup.

ISTM that it would probably be better if there were exactly one InRedo
flag in shared memory, probably in xlog.c's shared state, with the
postmaster not being responsible for setting or clearing it; rather
the startup process should do those things.

So replace InRedo with an IsInRedo() function that accesses XLogCtl?

Sounds good

* bgwriter and stats process starts in consistent recovery mode.
bgwriter changes mode when startup process completes.

I'm not sure about the interaction of this. In particular, what about
recovery restart points before we have reached the safe stop point?
I don't think we want to give up the capability of having those.

Maybe. I felt it made the code cleaner to give that up. Realistically
its a fairly short window of time. But shouldn't be too hard to put
back.

Also, it seems pretty bogus to update the in-memory ControlFile
checkpoint values before the restart point is actually done. It looks
to me like what you have done is to try to use those fields as signaling
for the restart request in addition to their existing purposes, which
I think is confusing and probably dangerous. I'd rather there were a
different signaling path and ControlFile maintains its currrent
definition.

OK

I found the changes in bgwriter.c unreadable. You've evidently
moved blocks of code around, but exactly what did you mean to
change? Why is there so much duplicated code now?

The patch utility did that. Some code reuse confused it. It's real easy
to read though if you apply the patch and then read the main loop.
You'll see what I mean.

I've kept the distinction between restartpoints and checkpoints in
code, to avoid convoluted code.

Hmm, I dunno, it seems like that might be a bad choice. Are you sure
it's not cleaner to just use the regular checkpoint code?

When I tried to write it, it just looked to my eyes like every single
line had a caveat which looked ugly and multiplied the testing. You're
the code dude, always happy to structure things as you suggest. If
you're sure, that is.

Thanks for the review.

--
Simon Riggs www.2ndQuadrant.com
PostgreSQL Training, Services and Support

Simon Riggs <simon@2ndQuadrant.com> writes:

On Mon, 2008-09-08 at 13:34 -0400, Tom Lane wrote:

Hmm, I dunno, it seems like that might be a bad choice. Are you sure
it's not cleaner to just use the regular checkpoint code?

When I tried to write it, it just looked to my eyes like every single
line had a caveat which looked ugly and multiplied the testing. You're
the code dude, always happy to structure things as you suggest. If
you're sure, that is.

No, was just wondering if the other way would be better. If you're
sure not, that's fine.

regards, tom lane

On Mon, 2008-09-08 at 13:34 -0400, Tom Lane wrote:

ISTM that it would probably be better if there were exactly one InRedo
flag in shared memory, probably in xlog.c's shared state, with the
postmaster not being responsible for setting or clearing it; rather
the startup process should do those things.

Done

* bgwriter and stats process starts in consistent recovery mode.
bgwriter changes mode when startup process completes.

I'm not sure about the interaction of this. In particular, what about
recovery restart points before we have reached the safe stop point?
I don't think we want to give up the capability of having those.

Also, it seems pretty bogus to update the in-memory ControlFile
checkpoint values before the restart point is actually done. It looks
to me like what you have done is to try to use those fields as signaling
for the restart request in addition to their existing purposes, which
I think is confusing and probably dangerous. I'd rather there were a
different signaling path and ControlFile maintains its currrent
definition.

Done

Testing takes a while on this, I probably won't complete it until
Friday. So enclosed patch is for eyeballs only at this stage.

I added in the XLogCtl padding we've discussed before, while I'm there.

--
Simon Riggs www.2ndQuadrant.com
PostgreSQL Training, Services and Support

Simon Riggs wrote:

--- 5716,5725 ----
CheckpointStats.ckpt_sync_end_t,
&sync_secs, &sync_usecs);

! elog(LOG, "%s complete: wrote %d buffers (%.1f%%); "
"%d transaction log file(s) added, %d removed, %d recycled; "
"write=%ld.%03d s, sync=%ld.%03d s, total=%ld.%03d s",
+ (checkpoint ? " checkpoint" : "restartpoint"),
CheckpointStats.ckpt_bufs_written,
(double) CheckpointStats.ckpt_bufs_written * 100 / NBuffers,
CheckpointStats.ckpt_segs_added,

Very minor nit: this really needs a rework. It is relatively OK in the
previous code, but it was already stuffing too much in a single message.
Maybe

ereport(LOG,
(errmsg(checkpoint ? "checkpoint complete" : "restartpoint complete"),
errdetail("Wrote %d buffers (%.1f%%); "
"%d transaction log file(s) added, %d removed, %d recycled; "
"write=%ld.%03d s, sync=%ld.%03d s, total=%ld.%03d s.",
...
)))

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

On Fri, 2008-09-12 at 14:14 -0400, Alvaro Herrera wrote:

Simon Riggs wrote:

--- 5716,5725 ----
CheckpointStats.ckpt_sync_end_t,
&sync_secs, &sync_usecs);

! elog(LOG, "%s complete: wrote %d buffers (%.1f%%); "
"%d transaction log file(s) added, %d removed, %d recycled; "
"write=%ld.%03d s, sync=%ld.%03d s, total=%ld.%03d s",
+ (checkpoint ? " checkpoint" : "restartpoint"),
CheckpointStats.ckpt_bufs_written,
(double) CheckpointStats.ckpt_bufs_written * 100 / NBuffers,
CheckpointStats.ckpt_segs_added,

Very minor nit: this really needs a rework.

All I changed was the word "restartpoint"... its otherwise identical to
existing message. I'd rather not change that.

--
Simon Riggs www.2ndQuadrant.com
PostgreSQL Training, Services and Support

Simon Riggs wrote:

On Fri, 2008-09-12 at 14:14 -0400, Alvaro Herrera wrote:

Simon Riggs wrote:

--- 5716,5725 ----
CheckpointStats.ckpt_sync_end_t,
&sync_secs, &sync_usecs);

! elog(LOG, "%s complete: wrote %d buffers (%.1f%%); "
"%d transaction log file(s) added, %d removed, %d recycled; "
"write=%ld.%03d s, sync=%ld.%03d s, total=%ld.%03d s",
+ (checkpoint ? " checkpoint" : "restartpoint"),
CheckpointStats.ckpt_bufs_written,
(double) CheckpointStats.ckpt_bufs_written * 100 / NBuffers,
CheckpointStats.ckpt_segs_added,

Very minor nit: this really needs a rework.

All I changed was the word "restartpoint"... its otherwise identical to
existing message. I'd rather not change that.

The new message is not translatable, the original was.

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

On Fri, 2008-09-12 at 15:31 -0400, Alvaro Herrera wrote:

Simon Riggs wrote:

All I changed was the word "restartpoint"... its otherwise identical to
existing message. I'd rather not change that.

The new message is not translatable, the original was.

OK, perhaps the word "restartpoint" is redundant anyhow. Anybody looking
at the log can see we're in recovery. So I'll switch it back to say just
checkpoint whether we do restartpoint or checkpoints.

--
Simon Riggs www.2ndQuadrant.com
PostgreSQL Training, Services and Support

Simon Riggs <simon@2ndQuadrant.com> wrote:

Testing takes a while on this, I probably won't complete it until
Friday. So enclosed patch is for eyeballs only at this stage.

What's the status on that patch?

regards, tom lane

On Tue, 2008-09-16 at 15:45 -0400, Tom Lane wrote:

Simon Riggs <simon@2ndQuadrant.com> wrote:

Testing takes a while on this, I probably won't complete it until
Friday. So enclosed patch is for eyeballs only at this stage.

What's the status on that patch?

Checking EXEC_BACKEND case and that reporting back later today.

--
Simon Riggs www.2ndQuadrant.com
PostgreSQL Training, Services and Support

On Tue, 2008-09-16 at 15:45 -0400, Tom Lane wrote:

Simon Riggs <simon@2ndQuadrant.com> wrote:

Testing takes a while on this, I probably won't complete it until
Friday. So enclosed patch is for eyeballs only at this stage.

What's the status on that patch?

Having some trouble trying to get a clean state change from recovery to
normal mode.

Startup needs to be able to write WAL at the end of recovery so it can
write a ShutdownCheckpoint, yet must not be allowed to write WAL before
that. Other services are still not fully up yet. So every other process
apart from Startup musn't write WAL until Startup has fully completed
its actions, which is sometime later.

bgwriter needs to know about the impending state change so it can finish
off any checkpoint its currently working on. But then can't start doing
normal processing yet either. So it must have a third state that is
between recovery and normal processing. When normal processing is
reached, it *must* write WAL immediately from that point onwards, yet
using the new timeline that startup decides upon.

So the idea of a single database-wide boolean state seems impossible. We
need a local canInsertWAL flag that is set at different times in
different processes.

Global states changes are now

not started
started - postmaster process then startup process
safestoppingpoint - bgwriter starts
startup_does_shutdown_checkpoint - bgwriter finishes up, just waits,
startup is now allowed to insert WAL for checkpoint record
startup completes StartupXLog - bgwriter begins normal processing
startup exits - postmaster in normal state

We *might* solve this by making the final checkpoint that Startup writes
into an online checkpoint. That would then allow a straight and safe
transition for bgwriter from just one state to the other. But that
leaves some other ugliness that I don't want to deal with, 'cos there's
other fish frying.

Feels like I should shutdown the bgwriter after recovery and then allow
it to be cranked up again after normal processing starts, and do all of
this through postmaster state changes. That way bgwriter doesn't need to
do a dynamic state change.

Comments anyone?

--
Simon Riggs www.2ndQuadrant.com
PostgreSQL Training, Services and Support

On Thu, 2008-09-18 at 09:05 +0100, Simon Riggs wrote:

Feels like I should shutdown the bgwriter after recovery and then
allow it to be cranked up again after normal processing starts, and do
all of this through postmaster state changes. That way bgwriter
doesn't need to do a dynamic state change.

This approach appears to be working nicely so far. Some ugly bits of
former patch removed.

Patch passes basic tests and changes state cleanly.

Restarting test cycle on this patch now, confirm tomorrow.

--
Simon Riggs www.2ndQuadrant.com
PostgreSQL Training, Services and Support

Simon Riggs <simon@2ndQuadrant.com> writes:

Having some trouble trying to get a clean state change from recovery to
normal mode.

Startup needs to be able to write WAL at the end of recovery so it can
write a ShutdownCheckpoint, yet must not be allowed to write WAL before
that. Other services are still not fully up yet. So every other process
apart from Startup musn't write WAL until Startup has fully completed
its actions, which is sometime later.
...
We *might* solve this by making the final checkpoint that Startup writes
into an online checkpoint.

Do we really need a checkpoint there at all? I can't recall right now
if there was a good reason why Vadim made it do that. I have a feeling
that it might have had to do with an assumption that the recovery
environment was completely distinct from live environment; which is a
statement that's certainly not going to be true in a query-answering
slave.

regards, tom lane