Hi, On Wed, Jan 28, 2009 at 11:19 PM, Fujii Masao <masao.fu...@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. > + /* > + * 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. > +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. Regards, -- Fujii Masao NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
