From: Simon Riggs <simon@2ndQuadrant.com>
To: Heikki Linnakangas <heikki.linnakangas@enterprisedb.com>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: [PATCHES] Infrastructure changes for recovery (v8)
Date: Wed, 08 Oct 2008 13:24:20 +0100
Message-ID: <1223468660.4747.294.camel@ebony.2ndQuadrant> (raw)
In-Reply-To: <48EC9CC5.6060803@enterprisedb.com>
References: <1222815151.4445.1397.camel@ebony.2ndQuadrant>
<48EC9CC5.6060803@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
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: simon@2ndQuadrant.com, heikki.linnakangas@enterprisedb.com, pgsql-hackers@postgresql.org
Subject: Re: [PATCHES] Infrastructure changes for recovery (v8)
In-Reply-To: <1223468660.4747.294.camel@ebony.2ndQuadrant>
* 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