Re: Split xlog.c

Hi,

I think it'd make sense to apply the first few patches now, they seem
uncontroversial and simple enough.

On 2021-07-31 00:33:34 +0300, Heikki Linnakangas wrote:
> From 0cfb852e320bd8fe83c588d25306d5b4c57b9da6 Mon Sep 17 00:00:00 2001
> From: Heikki Linnakangas <heikki(dot)linnakangas(at)iki(dot)fi>
> Date: Mon, 21 Jun 2021 22:14:58 +0300
> Subject: [PATCH 1/7] Don't use O_SYNC or similar when opening signal file to
> fsync it.

+1

> From 83f00e90bb818ed21bb14580f19f58c4ade87ef7 Mon Sep 17 00:00:00 2001
> From: Heikki Linnakangas <heikki(dot)linnakangas(at)iki(dot)fi>
> Date: Wed, 9 Jun 2021 12:05:53 +0300
> Subject: [PATCH 2/7] Remove unnecessary 'restoredFromArchive' global variable.
>
> It might've been useful for debugging purposes, but meh. There's
> 'readSource' which does almost the same thing.

+1

> From ec53470c8d271c01b8d2e12b92863501c3a9b4cf Mon Sep 17 00:00:00 2001
> From: Heikki Linnakangas <heikki(dot)linnakangas(at)iki(dot)fi>
> Date: Mon, 21 Jun 2021 16:12:50 +0300
> Subject: [PATCH 3/7] Extract code to get reason that recovery was stopped to a
> function.

+1

> +/*
> + * Create a comment for the history file to explain why and where
> + * timeline changed.
> + */
> +static char *
> +getRecoveryStopReason(void)
> +{
> + char reason[200];
> +
> + if (recoveryTarget == RECOVERY_TARGET_XID)
> + snprintf(reason, sizeof(reason),
> + "%s transaction %u",
> + recoveryStopAfter ? "after" : "before",
> + recoveryStopXid);
> + else if (recoveryTarget == RECOVERY_TARGET_TIME)
> + snprintf(reason, sizeof(reason),
> + "%s %s\n",
> + recoveryStopAfter ? "after" : "before",
> + timestamptz_to_str(recoveryStopTime));
> + else if (recoveryTarget == RECOVERY_TARGET_LSN)
> + snprintf(reason, sizeof(reason),
> + "%s LSN %X/%X\n",
> + recoveryStopAfter ? "after" : "before",
> + LSN_FORMAT_ARGS(recoveryStopLSN));
> + else if (recoveryTarget == RECOVERY_TARGET_NAME)
> + snprintf(reason, sizeof(reason),
> + "at restore point \"%s\"",
> + recoveryStopName);
> + else if (recoveryTarget == RECOVERY_TARGET_IMMEDIATE)
> + snprintf(reason, sizeof(reason), "reached consistency");
> + else
> + snprintf(reason, sizeof(reason), "no recovery target specified");
> +
> + return pstrdup(reason);
> +}

I guess it would make sense to change this over to a switch at some
point, so we can get warnings if a new type of target is added...

> From 70f688f9576b7939d18321444fd59c51c402ce23 Mon Sep 17 00:00:00 2001
> From: Heikki Linnakangas <heikki(dot)linnakangas(at)iki(dot)fi>
> Date: Mon, 21 Jun 2021 21:25:37 +0300
> Subject: [PATCH 4/7] Move InRecovery and standbyState global vars to
> xlogutils.c.
>
> They are used in code that is sometimes called from a redo routine,
> so xlogutils.c seems more appropriate. That's where we have other helper
> functions used by redo routines.

FWIW, with some compilers on some linux distributions there is an efficiency
difference between accessing a variable (or calling a function) defined in the
current translation unit or a separate one (with the separate TU going through
the GOT). I don't think it's a problem here, but it's worth keeping in mind
while moving things around. We should probably adjust our compiler settings
to address that at some point :(

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

> + /*---- 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.

Greetings,

Andres Freund