Re: Hot standby, recovery infra - Mailing list 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