Re: Split xlog.c

On 31/07/2021 02:11, Andres Freund wrote:
> Hi,
> 
> I think it'd make sense to apply the first few patches now, they seem
> uncontroversial and simple enough.

Pushed those, thanks!

>>  From da11050ca890ce0311d9e97d2832a6a61bc43e10 Mon Sep 17 00:00:00 2001
>> From: Heikki Linnakangas <heikki.linnakangas@iki.fi>
>> Date: Fri, 18 Jun 2021 12:15:04 +0300
>> Subject: [PATCH 5/7] Move code around in StartupXLOG().
>>
>> This is the order that things will happen with the next commit, this
>> makes it more explicit. To aid review, I added "BEGIN/END function"
>> comments to mark which blocks of code are moved to separate functions in
>> in the next commit.
> 
>> ---
>>   src/backend/access/transam/xlog.c | 605 ++++++++++++++++--------------
>>   1 file changed, 315 insertions(+), 290 deletions(-)
>>
>> diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
>> index efb3ca273ed..b9d96d6de26 100644
>> --- a/src/backend/access/transam/xlog.c
>> +++ b/src/backend/access/transam/xlog.c
>> @@ -882,7 +882,6 @@ static MemoryContext walDebugCxt = NULL;
>>   
>>   static void readRecoverySignalFile(void);
>>   static void validateRecoveryParameters(void);
>> -static void exitArchiveRecovery(TimeLineID endTLI, XLogRecPtr endOfLog);
>>   static bool recoveryStopsBefore(XLogReaderState *record);
>>   static bool recoveryStopsAfter(XLogReaderState *record);
>>   static char *getRecoveryStopReason(void);
>> @@ -5592,111 +5591,6 @@ validateRecoveryParameters(void)
>>       }
>>   }
>>   
>> -/*
>> - * Exit archive-recovery state
>> - */
>> -static void
>> -exitArchiveRecovery(TimeLineID endTLI, XLogRecPtr endOfLog)
>> -{
> 
> I don't really understand the motivation for this part of the change? This
> kind of seems to run counter to the stated goals of the patch series? Seems
> like it'd need a different commit message at last?

Hmm. Some parts of exitArchiveRecovery are being moved into 
xlogrecovery.c, so it becomes smaller than before. Maybe there's still 
enough code left there that a separate function makes sense. I'll try 
that differently.

>> +    /*---- BEGIN FreeWalRecovery ----*/
>> +
>>       /* Shut down xlogreader */
>>       if (readFile >= 0)
>>       {
> 
> FWIW, FreeWalRecovery() for something that closes and unlinks files among
> other things doesn't seem like a great name.

Rename to CloseWalRecovery(), maybe? I'll try that.

- Heikki