On Tue, Mar 10, 2020 at 12:39:53PM -0300, Alvaro Herrera wrote: > Another option is to return the command as a palloc'ed string (per > psprintf), instead of using a caller-stack-allocated variable. Passing > the buffer len is widely used, but more error prone (and I think getting > this one wrong might be more catastrophic than a mistake elsewhere.) > This is not a performance-critical path enough that we *need* the > optimization that avoids the palloc is important. (Failure can be > reported by returning NULL.)
That's a better approach here. > Also, I think the function comment could stand some more detailing. What kind of additional information would you like to add on top of what the updated version attached does? > Also, I think Msvcbuild.pm could follow Makefile's ideas of one line per > file. Maybe no need to fix all of that in this patch, but let's start > by adding the new file it its own line rather than reflowing two > adjacent lines (oh wait ... does perltidy put it that way? if so, > nevermind.) Good idea. It happens that perltidy does not care about that, but I would rather keep that stuff for a separate patch/thread. -- Michael
From 33d9a6837f2435966006947dad18c21e40e0c271 Mon Sep 17 00:00:00 2001 From: Michael Paquier <mich...@paquier.xyz> Date: Wed, 11 Mar 2020 12:25:18 +0900 Subject: [PATCH v3] 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 | 27 +++++ src/backend/access/transam/xlogarchive.c | 66 ++----------- src/common/Makefile | 1 + src/common/archive.c | 119 +++++++++++++++++++++++ src/tools/msvc/Mkvcbuild.pm | 4 +- 5 files changed, 159 insertions(+), 58 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..8af0b4566f --- /dev/null +++ b/src/include/common/archive.h @@ -0,0 +1,27 @@ +/*------------------------------------------------------------------------- + * + * 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 char *BuildRestoreCommand(const char *restoreCommand, + const char *xlogpath, /* %p */ + const char *xlogfname, /* %f */ + const char *lastRestartPointFname); /* %r */ + +#endif /* ARCHIVE_H */ diff --git a/src/backend/access/transam/xlogarchive.c b/src/backend/access/transam/xlogarchive.c index 188b73e752..914ad340ea 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" @@ -53,11 +54,8 @@ RestoreArchivedFile(char *path, const char *xlogfname, bool cleanupEnabled) { char xlogpath[MAXPGPATH]; - char xlogRestoreCmd[MAXPGPATH]; + char *xlogRestoreCmd; char lastRestartPointFname[MAXPGPATH]; - char *dp; - char *endp; - const char *sp; int rc; struct stat stat_buf; XLogSegNo restartSegNo; @@ -149,58 +147,13 @@ 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 */ + xlogRestoreCmd = BuildRestoreCommand(recoveryRestoreCommand, + xlogpath, xlogfname, + lastRestartPointFname); + if (xlogRestoreCmd == NULL) + elog(ERROR, "could not build restore command \"%s\"", + recoveryRestoreCommand); ereport(DEBUG3, (errmsg_internal("executing restore command \"%s\"", @@ -217,6 +170,7 @@ RestoreArchivedFile(char *path, const char *xlogfname, rc = system(xlogRestoreCmd); PostRestoreCommand(); + pfree(xlogRestoreCmd); if (rc == 0) { 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..ef0ce532ed --- /dev/null +++ b/src/common/archive.c @@ -0,0 +1,119 @@ +/*------------------------------------------------------------------------- + * + * 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 as + * defined by the GUC parameter restore_command: xlogpath for %p, + * xlogfname for %f and lastRestartPointFname for %r. + * + * The result is a palloc'd string for the restore command built. The + * caller is responsible for freeing it. If any of the required arguments + * is NULL and that the corresponding alias is found in the command given + * by the caller, then NULL is returned. + */ +char * +BuildRestoreCommand(const char *restoreCommand, + const char *xlogpath, + const char *xlogfname, + const char *lastRestartPointFname) +{ + char *dp; + char *endp; + char *result; + const char *sp; + + result = palloc0(MAXPGPATH); + + /* + * 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) + { + pfree(result); + return NULL; + } + sp++; + StrNCpy(dp, xlogpath, endp - dp); + make_native_path(dp); + dp += strlen(dp); + break; + case 'f': + /* %f: filename of desired file */ + if (xlogfname == NULL) + { + pfree(result); + return NULL; + } + sp++; + StrNCpy(dp, xlogfname, endp - dp); + dp += strlen(dp); + break; + case 'r': + /* %r: filename of last restartpoint */ + if (lastRestartPointFname == NULL) + { + pfree(result); + return NULL; + } + 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 result; +} 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
