On Fri, Mar 06, 2020 at 05:22:16PM +0900, Michael Paquier wrote: > Thanks for the suggestion. Avoiding dead code makes sense as well > here. I'll think about this stuff a bit more first.
Okay, after pondering more on that, here is a first cut with a patch refactoring restore_command build to src/common/. One thing I have changed compared to the other versions is that BuildRestoreCommand() now returns a boolean to tell if the command was successfully built or not. A second thing. As of now the interface of BuildRestoreCommand() assumes that the resulting buffer has an allocation of MAXPGPATH. This should be fine because that's an assumption we rely on a lot in the code, be it frontend or backend. However, it could also be a trap for any caller of this routine if they allocate something smaller. Wouldn't it be cleaner to pass down the size of the result buffer directly to BuildRestoreCommand() and use the length given by the caller of the routine as a sanity check? Any thoughts? -- Michael
From bad2c04b0eb3a146f0c2719ed8360b3b255c3c47 Mon Sep 17 00:00:00 2001 From: Michael Paquier <mich...@paquier.xyz> Date: Mon, 9 Mar 2020 17:15:00 +0900 Subject: [PATCH] Move routine generating restore_command to src/common/ restore_command has only been used until now by the backend, but there is a pending patch for pg_rewind to make use of that in the frontend. Author: Alexey Kondratov Reviewed-by: Andrey Borodin, Andres Freund, Alvaro Herrera, Alexander Korotkov, Michael Paquier Discussion: https://postgr.es/m/a3acff50-5a0d-9a2c-b3b2-ee3616895...@postgrespro.ru --- src/include/common/archive.h | 28 ++++++ src/backend/access/transam/xlogarchive.c | 61 ++----------- src/common/Makefile | 1 + src/common/archive.c | 109 +++++++++++++++++++++++ src/tools/msvc/Mkvcbuild.pm | 4 +- 5 files changed, 146 insertions(+), 57 deletions(-) create mode 100644 src/include/common/archive.h create mode 100644 src/common/archive.c diff --git a/src/include/common/archive.h b/src/include/common/archive.h new file mode 100644 index 0000000000..bdcc49ffd3 --- /dev/null +++ b/src/include/common/archive.h @@ -0,0 +1,28 @@ +/*------------------------------------------------------------------------- + * + * archive.h + * Common WAL archive routines + * + * Portions Copyright (c) 1996-2020, PostgreSQL Global Development Group + * Portions Copyright (c) 1994, Regents of the University of California + * + * src/include/common/archive.h + * + *------------------------------------------------------------------------- + */ +#ifndef ARCHIVE_H +#define ARCHIVE_H + +/* + * Routine to build a restore command to retrieve WAL segments from a WAL + * archive. This uses the arguments given by the caller to replace the + * corresponding alias fields as defined in the GUC parameter + * restore_command. + */ +extern bool BuildRestoreCommand(const char *restoreCommand, + const char *xlogpath, /* %p */ + const char *xlogfname, /* %f */ + const char *lastRestartPointFname, /* %r */ + char *result); + +#endif /* ARCHIVE_H */ diff --git a/src/backend/access/transam/xlogarchive.c b/src/backend/access/transam/xlogarchive.c index 188b73e752..6e8cd88a5e 100644 --- a/src/backend/access/transam/xlogarchive.c +++ b/src/backend/access/transam/xlogarchive.c @@ -21,6 +21,7 @@ #include "access/xlog.h" #include "access/xlog_internal.h" +#include "common/archive.h" #include "miscadmin.h" #include "postmaster/startup.h" #include "replication/walsender.h" @@ -55,9 +56,6 @@ RestoreArchivedFile(char *path, const char *xlogfname, char xlogpath[MAXPGPATH]; char xlogRestoreCmd[MAXPGPATH]; char lastRestartPointFname[MAXPGPATH]; - char *dp; - char *endp; - const char *sp; int rc; struct stat stat_buf; XLogSegNo restartSegNo; @@ -149,58 +147,11 @@ RestoreArchivedFile(char *path, const char *xlogfname, else XLogFileName(lastRestartPointFname, 0, 0L, wal_segment_size); - /* - * construct the command to be executed - */ - dp = xlogRestoreCmd; - endp = xlogRestoreCmd + MAXPGPATH - 1; - *endp = '\0'; - - for (sp = recoveryRestoreCommand; *sp; sp++) - { - if (*sp == '%') - { - switch (sp[1]) - { - case 'p': - /* %p: relative path of target file */ - sp++; - StrNCpy(dp, xlogpath, endp - dp); - make_native_path(dp); - dp += strlen(dp); - break; - case 'f': - /* %f: filename of desired file */ - sp++; - StrNCpy(dp, xlogfname, endp - dp); - dp += strlen(dp); - break; - case 'r': - /* %r: filename of last restartpoint */ - sp++; - StrNCpy(dp, lastRestartPointFname, endp - dp); - dp += strlen(dp); - break; - case '%': - /* convert %% to a single % */ - sp++; - if (dp < endp) - *dp++ = *sp; - break; - default: - /* otherwise treat the % as not special */ - if (dp < endp) - *dp++ = *sp; - break; - } - } - else - { - if (dp < endp) - *dp++ = *sp; - } - } - *dp = '\0'; + /* Build the restore command to execute */ + if (!BuildRestoreCommand(recoveryRestoreCommand, xlogpath, xlogfname, + lastRestartPointFname, xlogRestoreCmd)) + elog(ERROR, "could not build restore command \"%s\"", + xlogRestoreCmd); ereport(DEBUG3, (errmsg_internal("executing restore command \"%s\"", diff --git a/src/common/Makefile b/src/common/Makefile index ce01df68b9..6939b9d087 100644 --- a/src/common/Makefile +++ b/src/common/Makefile @@ -46,6 +46,7 @@ LIBS += $(PTHREAD_LIBS) # If you add objects here, see also src/tools/msvc/Mkvcbuild.pm OBJS_COMMON = \ + archive.o \ base64.o \ config_info.o \ controldata_utils.o \ diff --git a/src/common/archive.c b/src/common/archive.c new file mode 100644 index 0000000000..574d56410d --- /dev/null +++ b/src/common/archive.c @@ -0,0 +1,109 @@ +/*------------------------------------------------------------------------- + * + * archive.c + * Common WAL archive routines + * + * Portions Copyright (c) 1996-2020, PostgreSQL Global Development Group + * Portions Copyright (c) 1994, Regents of the University of California + * + * + * IDENTIFICATION + * src/common/archive.c + * + *------------------------------------------------------------------------- + */ + +#ifndef FRONTEND +#include "postgres.h" +#else +#include "postgres_fe.h" +#endif + +#include "common/archive.h" + +/* + * BuildRestoreCommand + * + * Builds a restore command to retrieve a WAL segment from WAL archives, + * replacing the supported aliases with values supplied by the caller: + * xlogpath for %p, xlogfname for %f and lastRestartPointFname for %r. + * Returns true if restore_command was successfully built. + * + * If any of the required arguments is NULL and that the corresponding alias + * is found in the command given by the caller, then false is returned. + * + * The resulting restore command is stored in "result", which has to be + * a pre-allocated buffer of size MAXPGPATH. + */ +bool +BuildRestoreCommand(const char *restoreCommand, + const char *xlogpath, + const char *xlogfname, + const char *lastRestartPointFname, + char *result) +{ + char *dp, + *endp; + const char *sp; + + /* + * Build the command to be executed. + */ + dp = result; + endp = result + MAXPGPATH - 1; + *endp = '\0'; + + for (sp = restoreCommand; *sp; sp++) + { + if (*sp == '%') + { + switch (sp[1]) + { + case 'p': + /* %p: relative path of target file */ + if (xlogpath == NULL) + return false; + sp++; + StrNCpy(dp, xlogpath, endp - dp); + make_native_path(dp); + dp += strlen(dp); + break; + case 'f': + /* %f: filename of desired file */ + if (xlogfname == NULL) + return false; + sp++; + StrNCpy(dp, xlogfname, endp - dp); + dp += strlen(dp); + break; + case 'r': + /* %r: filename of last restartpoint */ + if (lastRestartPointFname == NULL) + return false; + sp++; + StrNCpy(dp, lastRestartPointFname, endp - dp); + dp += strlen(dp); + break; + case '%': + /* convert %% to a single % */ + sp++; + if (dp < endp) + *dp++ = *sp; + break; + default: + /* otherwise treat the % as not special */ + if (dp < endp) + *dp++ = *sp; + break; + } + } + else + { + if (dp < endp) + *dp++ = *sp; + } + } + *dp = '\0'; + + return true; +} diff --git a/src/tools/msvc/Mkvcbuild.pm b/src/tools/msvc/Mkvcbuild.pm index f89a8a4fdb..c648078e20 100644 --- a/src/tools/msvc/Mkvcbuild.pm +++ b/src/tools/msvc/Mkvcbuild.pm @@ -120,8 +120,8 @@ sub mkvcbuild } our @pgcommonallfiles = qw( - base64.c config_info.c controldata_utils.c d2s.c encnames.c exec.c - f2s.c file_perm.c hashfn.c ip.c jsonapi.c + archive.c base64.c config_info.c controldata_utils.c d2s.c encnames.c + exec.c f2s.c file_perm.c hashfn.c ip.c jsonapi.c keywords.c kwlookup.c link-canary.c md5.c pg_lzcompress.c pgfnames.c psprintf.c relpath.c rmtree.c saslprep.c scram-common.c string.c stringinfo.c unicode_norm.c username.c -- 2.25.1
signature.asc
Description: PGP signature
