Message-ID: <48EC9CC5.6060803@enterprisedb.com>
Date: Wed, 08 Oct 2008 14:43:01 +0300
Organization: EnterpriseDB
User-Agent: Mozilla-Thunderbird 2.0.0.16 (X11/20080724)
MIME-Version: 1.0
To: Simon Riggs <simon@2ndQuadrant.com>
CC: pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: [PATCHES] Infrastructure changes for recovery (v8)
References: <1222815151.4445.1397.camel@ebony.2ndQuadrant>
In-Reply-To: <1222815151.4445.1397.camel@ebony.2ndQuadrant>
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit
From: Heikki Linnakangas <heikki.linnakangas@enterprisedb.com>

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