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.
> + /*
> + * 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
