Re: [PATCHES] 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