Re: Split xlog.c - Mailing list pgsql-hackers

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.linnakangas@iki.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.linnakangas@iki.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.linnakangas@iki.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.linnakangas@iki.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.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?


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