From: Heikki Linnakangas <heikki.linnakangas@enterprisedb.com>
To: Simon Riggs <simon@2ndQuadrant.com>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: [PATCHES] Infrastructure changes for recovery (v8)
Date: Wed, 08 Oct 2008 14:43:01 +0300
Message-ID: <48EC9CC5.6060803@enterprisedb.com> (raw)
In-Reply-To: <1222815151.4445.1397.camel@ebony.2ndQuadrant>
References: <1222815151.4445.1397.camel@ebony.2ndQuadrant>
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-patches@postgresql.org
Cc: heikki.linnakangas@enterprisedb.com, simon@2ndQuadrant.com, pgsql-hackers@postgresql.org
Subject: Re: [PATCHES] Infrastructure changes for recovery (v8)
In-Reply-To: <48EC9CC5.6060803@enterprisedb.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox