Hi,

On Wed, Jan 28, 2009 at 7:04 PM, Heikki Linnakangas
<heikki.linnakan...@enterprisedb.com> wrote:
> I've been reviewing and massaging the so called recovery infra patch.

Great!

> 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..

> @@ -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.

> @@ -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().

> @@ -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?

> @@ -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().

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

Reply via email to