On Tue, 2006-12-05 at 17:26 -0500, Tom Lane wrote:
> "Simon Riggs" <simon@2ndquadrant.com> writes:
> > On Tue, 2006-12-05 at 16:24 -0500, Tom Lane wrote:
> >> Sure, what would happen is that every backend passing through this code
> >> would execute the several lines of computation needed to decide whether
> >> to call RequestCheckpoint.
>
> > Right, but the calculation uses RedoRecPtr, which may not be completely
> > up to date. So presumably you want to re-read the shared memory value
> > again to make sure we are exactly accurate and allow only one person to
> > call checkpoint? Either way we have to take a lock. Insert lock causes
> > deadlock, so we would need to use infolock.
>
> Not at all. It's highly unlikely that RedoRecPtr would be so out of
> date as to result in a false request for a checkpoint, and if it does,
> so what? Worst case is we perform an extra checkpoint.
On its own, I wouldn't normally agree...
> Also, given the current structure of the routine, this is probably not
> the best place for that code at all --- it'd make more sense for it to
> be in the just-finished-a-segment code stretch, which would ensure that
> it's only done by one backend once per segment.
But thats a much better plan since it requires no locking.
There's a lot more changes there for such a simple fix though and lots
more potential bugs, but I've coded it as you suggest and removed the
fields from pg_control.
Patch passes make check, applies cleanly on HEAD.
pg_resetxlog and pgcontroldata tested.
--
Simon Riggs
EnterpriseDB http://www.enterprisedb.com
