On Thu, 2009-01-29 at 12:18 +0900, Fujii Masao wrote:
> Hi,
>
> On Wed, Jan 28, 2009 at 11:19 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
> >> I feel quite good about this patch now. Given the amount of code churn, it
> >> requires testing, and I'll read it through one more time after sleeping over
> >> it. Simon, do you see anything wrong with this?
> >
> > I also read this patch and found something odd. I apologize if I misread it..
>
> Sorry for my random reply.
>
> Though this is a matter of taste, I think that it's weird that bgwriter
> runs with ThisTimeLineID = 0 during recovery. This is because
> XLogCtl->ThisTimeLineID is set at the end of recovery. ISTM this will
> be a cause of bug in the near future, though this is harmless currently.
It doesn't. That's exactly what InitXLogAccess() was for.
> > + /*
> > + * XXX: Should we try to perform restartpoints if we're not in consistent
> > + * recovery? The bgwriter isn't doing it for us in that case.
> > + */
>
> I think yes. This is helpful for the system which it takes a long time to get
> a base backup, ie. it also takes a long time to reach a consistent recovery
> point.
The original patch did this.
> > +CreateRestartPoint(int flags)
> <snip>
> > + * We rely on this lock to ensure that the startup process doesn't exit
> > + * Recovery while we are half way through a restartpoint. XXX ?
> > */
> > + LWLockAcquire(CheckpointLock, LW_EXCLUSIVE);
>
> Is this comment correct? CheckpointLock cannot prevent the startup process
> from exiting recovery because the startup process doesn't acquire that lock.
The original patch acquired CheckpointLock during exitRecovery to prove
that a restartpoint was not in progress. It no longer does this, so not
sure if Heikki has found another way and the comment is wrong, or that
removing the lock request is a bug.
-- Simon Riggs www.2ndQuadrant.comPostgreSQL Training, Services and Support