Infrastructure changes for recovery (v8)

Simon Riggs wrote:

* optional recovery_safe_start_location parameter now provided in
recovery.conf, to allow a consistency point to be manually defined if a
base backup was not taken using standard pg_start/stop backup functions

Do we want to cater for that? It only seems safe if you have
full_page_writes turned on, and you perform a checkpoint first. But if
you do that, why don't you just use pg_start_backup()?

Other Changes
* log_restartpoints removed, use log_checkpoints in postgresql.conf

Is this something that would make sense regardless of the rest of the
patch? If so, we could apply that separately, which would make this
patch a little less overwhelming to review.

* additional function signature for pg_start_backup('label', true |
false) to allow definition of immediate checkpoint/not

Wouldn't this need a new entry in pg_proc.h? Again, perhaps we should do
this as a separate patch.

* fixes bug discovered while other testing: if pg_stop_backup() is run
when xlogswitch has just occurred then we do not switch log files, yet
we return current filename even though nothing of value in it. If
archive_timeout not enabled we would wait forever for pg_stop_backup()
to return.
* Substantial comments throughout

These comments on CheckPointLock seem contradictory:

--- 247,256 ----
* ControlFileLock: must be held to read/update control file or create
* new log file.
*
!  * CheckpointLock: must be held to do a checkpoint or restartpoint, ensuring
!  * we get just one of those at any time. In 8.4+ recovery, both startup and
!  * bgwriter processes may take restartpoints, so this locking must be strict 
!  * to ensure there are no mistakes.
*
*----------
*/

and

--- 5901,5916 ----
XLogRecPtr	recptr;
XLogCtlInsert *Insert = &XLogCtl->Insert;
XLogRecData rdata;
uint32		_logId;
uint32		_logSeg;
TransactionId *inCommitXids;
int			nInCommit;
+ 	bool		leavingArchiveRecovery = false;

/*
* Acquire CheckpointLock to ensure only one checkpoint happens at a time.
! * That shouldn't be happening, but checkpoints are an important aspect
! * of our resilience, so we take no chances.
*/
LWLockAcquire(CheckpointLock, LW_EXCLUSIVE);

If I've understood the patch correctly, only bgwriter does checkpoints
and restart points now?

There's a trivial merge conflict in bgwriter.c, due to the FSM patch.

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

On Wed, 2008-10-08 at 14:43 +0300, Heikki Linnakangas wrote:

Simon Riggs wrote:

* optional recovery_safe_start_location parameter now provided in
recovery.conf, to allow a consistency point to be manually defined if a
base backup was not taken using standard pg_start/stop backup functions

Do we want to cater for that? It only seems safe if you have
full_page_writes turned on, and you perform a checkpoint first. But if
you do that, why don't you just use pg_start_backup()?

I'm easy on that one. It is a supported backup method, so without this
it would not be possible to utilise Hot Standby in conjunction with this
backup technique. Not many people use it, but I guess some do.

Other Changes
* log_restartpoints removed, use log_checkpoints in postgresql.conf

Is this something that would make sense regardless of the rest of the
patch? If so, we could apply that separately, which would make this
patch a little less overwhelming to review.

Maybe, it's fairly minor.

* additional function signature for pg_start_backup('label', true |
false) to allow definition of immediate checkpoint/not

Wouldn't this need a new entry in pg_proc.h? Again, perhaps we should do
this as a separate patch.

That's concerning. I remember adding the entry and assigning a new oid,
but it isn't in the patch. The multi-argument version was definitely
tested, that's how I discovered the bug also fixed in the patch.

* fixes bug discovered while other testing: if pg_stop_backup() is run
when xlogswitch has just occurred then we do not switch log files, yet
we return current filename even though nothing of value in it. If
archive_timeout not enabled we would wait forever for pg_stop_backup()
to return.

OK, I'll strip all of the above out, for separate consideration.

* Substantial comments throughout

These comments on CheckPointLock seem contradictory:

--- 247,256 ----
* ControlFileLock: must be held to read/update control file or create
* new log file.
*
!  * CheckpointLock: must be held to do a checkpoint or restartpoint, ensuring
!  * we get just one of those at any time. In 8.4+ recovery, both startup and
!  * bgwriter processes may take restartpoints, so this locking must be strict 
!  * to ensure there are no mistakes.
*
*----------
*/

and

--- 5901,5916 ----
XLogRecPtr	recptr;
XLogCtlInsert *Insert = &XLogCtl->Insert;
XLogRecData rdata;
uint32		_logId;
uint32		_logSeg;
TransactionId *inCommitXids;
int			nInCommit;
+ 	bool		leavingArchiveRecovery = false;

/*
* Acquire CheckpointLock to ensure only one checkpoint happens at a time.
! * That shouldn't be happening, but checkpoints are an important aspect
! * of our resilience, so we take no chances.
*/
LWLockAcquire(CheckpointLock, LW_EXCLUSIVE);

If I've understood the patch correctly, only bgwriter does checkpoints
and restart points now?

Tom requested that we retain the ability for Startup process to perform
restartpoints up until the point that bgwriter spawns, then after that
bgwriter performs them.

The form is this

PM_START startup process performs restartpoints
transition when database is consistent state
PM_RECOVERY bgwriter process performs restartpoints
delicate transition between two states
PM_RUN bgwriter process performs checkpoints

There's a trivial merge conflict in bgwriter.c, due to the FSM patch.

OK, will look.

Thanks for looking.

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

On Wed, 2008-10-08 at 13:24 +0100, Simon Riggs wrote:

On Wed, 2008-10-08 at 14:43 +0300, Heikki Linnakangas wrote:

Again, perhaps we should do
this as a separate patch.

New patch enclosed with other stuff stripped out. 15% lighter...

New thread spawned for bug fix patch. Will resubmit others when we can
be sure they'll not cause patch conflicts.

OK, I'll strip all of the above out, for separate consideration.
The form is this

PM_START startup process performs restartpoints
transition when database is consistent state
PM_RECOVERY bgwriter process performs restartpoints
delicate transition between two states
PM_RUN bgwriter process performs checkpoints

Above added as comments in patch.

The patch agonises over the two state transitions above. First
transition needs to be exactly correct otherwise we might be using an
inconsistent database during recovery. Second transition is harder
because it isn't just the startup process working alone any more.

The key to understanding it is all in concurrent behaviour. Startup and
bgwriter chat together through bgwriter shared memory and call functions
back and forth between xlog.c and bgwriter.c.

I haven't retested the patch yet, but it passes make check. I'll be
rechecking it later today, starting in about 2 hours time.

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

On Tue, 2008-09-30 at 23:52 +0100, Simon Riggs wrote:

* optional recovery_safe_start_location parameter now provided in
recovery.conf, to allow a consistency point to be manually defined if a
base backup was not taken using standard pg_start/stop backup functions

If using synchronous replication, it seems like this may be useful. For
instance, if the primary server A fails (let's assume power off
failure), then you make the secondary server B the new primary and start
committing transactions, and then you want to bring A back up as a
secondary to B.

Will server A know where to start recovering from, even if many
checkpoints have happened on server B in the meantime? Is there a way to
avoid wiping A and making a new base backup?

Are the safety issues that Heikki brought up potentially solvable, or am
I asking for the impossible?

And also, what if server A is shut down cleanly? Is there any way at all
to get it into recovery mode to catch up with B, or would it require a
new base backup?

I haven't read through the entire thread, so I apologize if this
question has been answered elsewhere.

Regards,
Jeff Davis

On Fri, 2008-11-07 at 15:44 -0800, Jeff Davis wrote:

Is there a way to avoid wiping A and making a new base backup?

rsync

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

Simon Riggs wrote:

diff --git a/src/backend/access/transam/subtrans.c b/src/backend/access/transam/
index 063b366..5e64cb4 100644
--- a/src/backend/access/transam/subtrans.c
+++ b/src/backend/access/transam/subtrans.c
@@ -226,6 +226,9 @@ ZeroSUBTRANSPage(int pageno)
*
* oldestActiveXID is the oldest XID of any prepared transaction, or nextXid
* if there are none.
+ *
+ * Note that this is not atomic and is not yet safe to perform while other
+ * processes might access subtrans.
*/
void
StartupSUBTRANS(TransactionId oldestActiveXID)

I'm a bit confused by that comment. Does that need to be fixed? It
sounds like it does, because other processes might access subtrans when
StartupSUBTRANS is called, with the patch to allow read-only queries
during recovery. Or is that done in the hot standby patch?

However, I don't see why that isn't safe. StartupSUBTRANS takes the
SubtransControlLock in exclusive mode while it zeroes out subtrans.

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

Simon Riggs wrote:

@@ -3845,6 +3850,52 @@ sigusr1_handler(SIGNAL_ARGS)

PG_SETMASK(&BlockSig);

+       if (CheckPostmasterSignal(PMSIGNAL_RECOVERY_START))
+       {
+               Assert(pmState == PM_STARTUP);
+
+               /*
+                * Go to shutdown mode if a shutdown request was pending.
+                */
+               if (Shutdown > NoShutdown)
+               {
+                       pmState = PM_WAIT_BACKENDS;
+                       /* PostmasterStateMachine logic does the rest */
+               }
+               else
+               {
+                       /*
+                        * Startup process has entered recovery
+                        */
+                       pmState = PM_RECOVERY;

Hmm, I smell a race condition here:

1. Startup process goes into consistent state, and signals postmaster
2. Startup process finishes WAL replay and dies
3. Postmaster wakes up in reaper(), noting that the startup process
dies, and goes into PM_RUN mode.
4. The above signal handler for postmaster is run, causing an assertion
failure, or putting postmaster back into PM_RECOVERY mode if assertions
are disabled.

Highly unlikely in practice, given how much code needs to run in the
startup process between signaling the postmaster and exiting, but it
seems theoretically possible. Do we care, and if we do, how can we fix it?

+
+                       /*
+                        * Load the flat authorization file into postmaster's ca
+                        * startup process won't have recomputed this from the d
+                        * yet, so it may change following recovery.
+                        */
+                       load_role();

Is there a race condition here too, if the startup process is writing
the auth file at the same time? I guess we'd have the same problem with
flat files in general, so maybe there's something else that mitigates
the problem?

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

This comment in XLogFlush is no longer accurate:

* The current approach is to ERROR under normal conditions, but only
* WARNING during recovery, so that the system can be brought up even if
* there's a corrupt LSN. Note that for calls from xact.c, the ERROR will
* be promoted to PANIC since xact.c calls this routine inside a critical
* section. However, calls from bufmgr.c are not within critical sections
* and so we will not force a restart for a bad LSN on a data page.
*/
if (XLByteLT(LogwrtResult.Flush, record))
elog(ERROR,
"xlog flush request %X/%X is not satisfied --- flushed only to %X/%X",
record.xlogid, record.xrecoff,
LogwrtResult.Flush.xlogid, LogwrtResult.Flush.xrecoff);

Because of this hunk:

***************
*** 1822,1828 **** XLogFlush(XLogRecPtr record)
* and so we will not force a restart for a bad LSN on a data page.
*/
if (XLByteLT(LogwrtResult.Flush, record))
!               elog(InRecovery ? WARNING : ERROR,
"xlog flush request %X/%X is not satisfied --- flushed only to %X/%X",
record.xlogid, record.xrecoff,
LogwrtResult.Flush.xlogid, LogwrtResult.Flush.xrecoff);
--- 1874,1880 ----
* and so we will not force a restart for a bad LSN on a data page.
*/
if (XLByteLT(LogwrtResult.Flush, record))
!               elog(ERROR,
"xlog flush request %X/%X is not satisfied --- flushed only to %X/%X",
record.xlogid, record.xrecoff,
LogwrtResult.Flush.xlogid, LogwrtResult.Flush.xrecoff);

I'm not sure what the most robust behavior would be.

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

On Mon, 2008-11-17 at 15:51 +0200, Heikki Linnakangas wrote:

Simon Riggs wrote:

diff --git a/src/backend/access/transam/subtrans.c b/src/backend/access/transam/
index 063b366..5e64cb4 100644
--- a/src/backend/access/transam/subtrans.c
+++ b/src/backend/access/transam/subtrans.c
@@ -226,6 +226,9 @@ ZeroSUBTRANSPage(int pageno)
*
* oldestActiveXID is the oldest XID of any prepared transaction, or nextXid
* if there are none.
+ *
+ * Note that this is not atomic and is not yet safe to perform while other
+ * processes might access subtrans.
*/
void
StartupSUBTRANS(TransactionId oldestActiveXID)

I'm a bit confused by that comment. Does that need to be fixed?

It is, in a later version. Apologies if you're reviewing the wrong one.

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

On Mon, 2008-11-17 at 16:18 +0200, Heikki Linnakangas wrote:

+
+                       /*
+                        * Load the flat authorization file into

postmaster's ca

+ * startup process won't have recomputed

this from the d

+                        * yet, so it may change following recovery.
+                        */
+                       load_role();

Is there a race condition here too, if the startup process is writing
the auth file at the same time? I guess we'd have the same problem
with flat files in general, so maybe there's something else that
mitigates the problem?

The flat file is not race condition proof. When the file is read it is
just a guide and the real data is re-accessed from catalog. So the
problem you see does exist, but is handled elsewhere - not in this
patch.

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

On Mon, 2008-11-17 at 16:18 +0200, Heikki Linnakangas wrote:

Simon Riggs wrote:

@@ -3845,6 +3850,52 @@ sigusr1_handler(SIGNAL_ARGS)

PG_SETMASK(&BlockSig);

+       if (CheckPostmasterSignal(PMSIGNAL_RECOVERY_START))
+       {
+               Assert(pmState == PM_STARTUP);
+
+               /*
+                * Go to shutdown mode if a shutdown request was pending.
+                */
+               if (Shutdown > NoShutdown)
+               {
+                       pmState = PM_WAIT_BACKENDS;
+                       /* PostmasterStateMachine logic does the rest */
+               }
+               else
+               {
+                       /*
+                        * Startup process has entered recovery
+                        */
+                       pmState = PM_RECOVERY;

Hmm, I smell a race condition here:

1. Startup process goes into consistent state, and signals postmaster
2. Startup process finishes WAL replay and dies
3. Postmaster wakes up in reaper(), noting that the startup process
dies, and goes into PM_RUN mode.
4. The above signal handler for postmaster is run, causing an assertion
failure, or putting postmaster back into PM_RECOVERY mode if assertions
are disabled.

Highly unlikely in practice, given how much code needs to run in the
startup process between signaling the postmaster and exiting, but it
seems theoretically possible. Do we care, and if we do, how can we fix it?

Might be possible - it does depend on the sequence of actions its true.
Agree not likely to happen, except as the result of another bug.

I'll change it to a test for

if (pmState == PM_STARTUP)
pmState = PM_RECOVERY;

The assertion was mainly for documentation, its not protecting anything
critical (IIRC).

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

On Tue, Nov 18, 2008 at 12:39 AM, Simon Riggs <simon@2ndquadrant.com> wrote:

On Mon, 2008-11-17 at 16:18 +0200, Heikki Linnakangas wrote:

Simon Riggs wrote:

@@ -3845,6 +3850,52 @@ sigusr1_handler(SIGNAL_ARGS)

PG_SETMASK(&BlockSig);

+       if (CheckPostmasterSignal(PMSIGNAL_RECOVERY_START))
+       {
+               Assert(pmState == PM_STARTUP);
+
+               /*
+                * Go to shutdown mode if a shutdown request was pending.
+                */
+               if (Shutdown > NoShutdown)
+               {
+                       pmState = PM_WAIT_BACKENDS;
+                       /* PostmasterStateMachine logic does the rest */
+               }
+               else
+               {
+                       /*
+                        * Startup process has entered recovery
+                        */
+                       pmState = PM_RECOVERY;

Hmm, I smell a race condition here:

1. Startup process goes into consistent state, and signals postmaster
2. Startup process finishes WAL replay and dies
3. Postmaster wakes up in reaper(), noting that the startup process
dies, and goes into PM_RUN mode.
4. The above signal handler for postmaster is run, causing an assertion
failure, or putting postmaster back into PM_RECOVERY mode if assertions
are disabled.

Highly unlikely in practice, given how much code needs to run in the
startup process between signaling the postmaster and exiting, but it
seems theoretically possible. Do we care, and if we do, how can we fix it?

Might be possible - it does depend on the sequence of actions its true.
Agree not likely to happen, except as the result of another bug.

I'll change it to a test for

if (pmState == PM_STARTUP)
pmState = PM_RECOVERY;

Likewise, should we also change the assertion against the pid of the
background process (BgWriterPID, PgStatPID)?

Regards,

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

On Mon, Nov 17, 2008 at 9:01 PM, Simon Riggs <simon@2ndquadrant.com> wrote:

It is, in a later version. Apologies if you're reviewing the wrong one.

The most recent version I can find is v9, but I remember you mentioned v10
somewhere else.
Can you please confirm if v9 is the latest version and point to the latest
version if not ? I've some free cycles and would like to help with the
review process.

Thanks,
Pavan

--

Pavan Deolasee
EnterpriseDB http://www.enterprisedb.com

On Thu, 2008-11-20 at 11:06 +0530, Pavan Deolasee wrote:

On Mon, Nov 17, 2008 at 9:01 PM, Simon Riggs <simon@2ndquadrant.com>
wrote:

It is, in a later version. Apologies if you're reviewing the
wrong one.

The most recent version I can find is v9, but I remember you mentioned
v10 somewhere else.
Can you please confirm if v9 is the latest version and point to the
latest version if not ? I've some free cycles and would like to help
with the review process.

The latest Hot Standby patch includes the latest version of
"infrastructure changes" patch. Thanks for reviewing.

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

On Thu, Nov 20, 2008 at 3:12 PM, Simon Riggs <simon@2ndquadrant.com> wrote:

The latest Hot Standby patch includes the latest version of
"infrastructure changes" patch. Thanks for reviewing.

Do you intend to split the patch into smaller pieces ? The latest hot
standby patch is almost 10K+ lines. Splitting that would definitely help the
review process.

Thanks,
Pavan

--
Pavan Deolasee
EnterpriseDB http://www.enterprisedb.com

On Thu, 2008-11-20 at 15:19 +0530, Pavan Deolasee wrote:

Do you intend to split the patch into smaller pieces ? The latest hot
standby patch is almost 10K+ lines. Splitting that would definitely
help the review process.

If it helps you, then I'll do it. Hang on an hour or so.

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

On Thu, 2008-11-20 at 10:10 +0000, Simon Riggs wrote:

On Thu, 2008-11-20 at 15:19 +0530, Pavan Deolasee wrote:

Do you intend to split the patch into smaller pieces ? The latest hot
standby patch is almost 10K+ lines. Splitting that would definitely
help the review process.

If it helps you, then I'll do it. Hang on an hour or so.

I've posted a slightly subdivided patch now via Wiki.

Putting "infrastructure" and "hot standby" together was fairly easy, but
splitting them apart has not been and I was unable to complete that
after a lot of hacking.

If you wouldn't mind looking at the major subsystems some more, I'm
happy to attempt some further parceling to make it easier for you to
review. I'm not completely certain the "infra" v "hot standby" is a good
split point anyway.

Please let me know how I can make the reviewer's job easier. Diagrams,
writeups, whatever. Thanks,

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

Simon Riggs escribi�:

Please let me know how I can make the reviewer's job easier. Diagrams,
writeups, whatever. Thanks,

A link perhaps?

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

On Wed, 2008-12-17 at 23:32 -0300, Alvaro Herrera wrote:

Simon Riggs escribió:

Please let me know how I can make the reviewer's job easier. Diagrams,
writeups, whatever. Thanks,

A link perhaps?

There is much confusion on this point for which I'm very sorry.

I originally wrote "infra" patch to allow it to be committed separately
in the Sept commitfest, to reduce size of the forthcoming hotstandby
patch. That didn't happen (no moans there) so the eventual "hotstandby"
patch includes all of what was the infra patch, plus the new code.

So currently there is no separate "infra" patch. The two line items on
the CommitFest page are really just one large project. I would be in
favour of removing the "infra" lines from the CommitFest page.

Of course we can consider "hotstandby" patch in parts, but
deconstructing it into wholly separate patches doesn't make much sense
now and would raise many questions about why certain code exists with no
apparent function or why certain design choices made.

If you were to review a part of this, I might ask that you look at the
changes to XidInMVCCSnapshot(), GetSnapshotData() and
AssignTransactionId(), which relate specifically to subtransaction
handling. Comments explain the new approach.

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