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(dot)linnakangas(at)iki(dot)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