Re: Hot standby, recovery infra
От | Heikki Linnakangas |
---|---|
Тема | Re: Hot standby, recovery infra |
Дата | |
Msg-id | 49807C00.5020308@enterprisedb.com обсуждение исходный текст |
Ответ на | Re: Hot standby, recovery infra (Fujii Masao <masao.fujii@gmail.com>) |
Список | pgsql-hackers |
Fujii Masao wrote: > On Wed, Jan 28, 2009 at 7:04 PM, Heikki Linnakangas > <heikki.linnakangas@enterprisedb.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. Many thanks for looking into this! >> @@ -507,7 +550,7 @@ CheckArchiveTimeout(void) >> pg_time_t now; >> pg_time_t last_time; >> >> - if (XLogArchiveTimeout <= 0) >> + if (XLogArchiveTimeout <= 0 || !IsRecoveryProcessingMode()) > > The above change destroys archive_timeout because checking the timeout > is always skipped after recovery is done. Yep, good catch. That obviously needs to be "IsRecoveryProcessingMode()", without the exclamation mark. >> @@ -355,6 +359,27 @@ BackgroundWriterMain(void) >> */ >> PG_SETMASK(&UnBlockSig); >> >> + BgWriterRecoveryMode = IsRecoveryProcessingMode(); >> + >> + if (BgWriterRecoveryMode) >> + elog(DEBUG1, "bgwriter starting during recovery"); >> + else >> + InitXLOGAccess(); > > Why is InitXLOGAccess() called also here when bgwriter is started after > recovery? That is already called by AuxiliaryProcessMain(). Oh, I didn't realize that. Agreed, it's a bit disconcerting that InitXLOGAccess() is called twice (there was a 2nd call within the loop in the original patch as well). Looking at InitXLOGAccess, it's harmless to call it multiple times, but it seems better to remove the InitXLOGAccess call from AuxiliaryProcessMain(). InitXLOGAccess() needs to be called after seeing that IsRecoveryProcessingMode() == false, because it won't get the right timeline id before that. >> @@ -1302,7 +1314,7 @@ ServerLoop(void) >> * state that prevents it, start one. It doesn't matter if this >> * fails, we'll just try again later. >> */ >> - if (BgWriterPID == 0 && pmState == PM_RUN) >> + if (BgWriterPID == 0 && (pmState == PM_RUN || pmState == PM_RECOVERY)) >> BgWriterPID = StartBackgroundWriter(); > > Likewise, we should try to start also the stats collector during recovery? Hmm, there's not much point as this patch stands, but I guess we should in the hot standby patch, where we let backends in. >> @@ -2103,7 +2148,8 @@ XLogFileInit(uint32 log, uint32 seg, >> unlink(tmppath); >> } >> >> - elog(DEBUG2, "done creating and filling new WAL file"); >> + XLogFileName(tmppath, ThisTimeLineID, log, seg); >> + elog(DEBUG2, "done creating and filling new WAL file %s", tmppath); > > This debug message is somewhat confusing, because the WAL file > represented as "tmppath" might have been already created by > previous XLogFileInit() via InstallXLogFileSegment(). I don't quite understand what you're saying, but I think I'll just revert that. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com
В списке pgsql-hackers по дате отправления: