Skip site navigation (1) Skip section navigation (2)

Re: [PATCHES] Infrastructure changes for recovery (v8)

From: Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)com>
To: Simon Riggs <simon(at)2ndQuadrant(dot)com>
Cc: pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: [PATCHES] Infrastructure changes for recovery (v8)
Date: 2008-10-08 11:43:01
Message-ID: 48EC9CC5.6060803@enterprisedb.com (view raw or flat)
Thread:
Lists: pgsql-hackerspgsql-patches
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

In response to

Responses

pgsql-hackers by date

Next:From: Tom LaneDate: 2008-10-08 12:20:14
Subject: Re: Reducing some DDL Locks to ShareLock
Previous:From: Pavel StehuleDate: 2008-10-08 10:33:54
Subject: problem with compilation on fedora core 10 64 bit

pgsql-patches by date

Next:From: Simon RiggsDate: 2008-10-08 12:24:20
Subject: Re: [PATCHES] Infrastructure changes for recovery (v8)
Previous:From: Simon RiggsDate: 2008-10-07 16:07:46
Subject: Re: [PATCHES] Infrastructure changes for recovery

Privacy Policy | About PostgreSQL
Copyright © 1996-2014 The PostgreSQL Global Development Group