On Mon, Mar 09, 2020 at 03:38:29PM +0530, Kuntal Ghosh wrote: > That's a good suggestion. But, it's unlikely that a caller would pass > something longer than MAXPGPATH and we indeed use that value a lot in > the code. IMHO, it looks okay to me to have that assumption here as > well.
Well, a more serious problem would be to allocate something smaller than MAXPGPATH. This reminds me a bit of 09ec55b9 where we did not correctly design from the start the base64 encode and decode routines for SCRAM, so I'd rather design this one correctly from the start as per the attached. Alexey, Alexander, what do you think? -- Michael
From 4e2b5b60e003ec67a4854f2625cc610f01428fe2 Mon Sep 17 00:00:00 2001 From: Michael Paquier <mich...@paquier.xyz> Date: Mon, 9 Mar 2020 17:15:00 +0900 Subject: [PATCH v2] 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 | 29 ++++++ src/backend/access/transam/xlogarchive.c | 62 ++----------- src/common/Makefile | 1 + src/common/archive.c | 110 +++++++++++++++++++++++ src/tools/msvc/Mkvcbuild.pm | 4 +- 5 files changed, 149 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..805930490b --- /dev/null +++ b/src/include/common/archive.h @@ -0,0 +1,29 @@ +/*------------------------------------------------------------------------- + * + * 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 *commandResult, + int commandResultLen); + +#endif /* ARCHIVE_H */ diff --git a/src/backend/access/transam/xlogarchive.c b/src/backend/access/transam/xlogarchive.c index 188b73e752..e1d6df89e1 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,12 @@ 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, + MAXPGPATH)) + 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..74c71e0f4c --- /dev/null +++ b/src/common/archive.c @@ -0,0 +1,110 @@ +/*------------------------------------------------------------------------- + * + * 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 "commandResult", pre-allocated + * buffer of size "restoreCommandLen". + */ +bool +BuildRestoreCommand(const char *restoreCommand, + const char *xlogpath, + const char *xlogfname, + const char *lastRestartPointFname, + char *commandResult, + int commandResultLen) +{ + char *dp, + *endp; + const char *sp; + + /* + * Build the command to be executed. + */ + dp = commandResult; + endp = commandResult + commandResultLen - 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
