Date: Tue, 10 Mar 2020 13:28:42 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Kuntal Ghosh <kuntalghosh.2007@gmail.com>
Cc: Alvaro Herrera <alvherre@2ndquadrant.com>,
	Alexey Kondratov <a.kondratov@postgrespro.ru>,
	Alexander Korotkov <a.korotkov@postgrespro.ru>,
	Liudmila Mantrova <l.mantrova@postgrespro.ru>,
	Thomas Munro <thomas.munro@gmail.com>,
	Andrey Borodin <x4mmm@yandex-team.ru>,
	David Steele <david@pgmasters.net>,
	Andres Freund <andres@anarazel.de>,
	PostgreSQL-development <pgsql-hackers@postgresql.org>,
	Dmitry Dolgov <9erthalion6@gmail.com>, vladimirlesk@yandex-team.ru,
	dsarafan@yandex-team.ru
Subject: Re: [Patch] pg_rewind: options to use restore_command from
 recovery.conf or command line
Message-ID: <20200310042842.GF4369@paquier.xyz>
References: <20200306012120.GD52814@paquier.xyz>
 <20200306020906.GA11931@alvherre.pgsql>
 <20200306082216.GA173911@paquier.xyz>
 <20200309081622.GF96055@paquier.xyz>
 <CAGz5QC+TF6DKFNNeYCRN41HSW7neF72Yb7aF4KCFjjk9_m-BAA@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="BXr400anF0jyguTS"
Content-Disposition: inline
In-Reply-To: 
 <CAGz5QC+TF6DKFNNeYCRN41HSW7neF72Yb7aF4KCFjjk9_m-BAA@mail.gmail.com>
Precedence: bulk


--BXr400anF0jyguTS
Content-Type: multipart/mixed; boundary="/Zw+/jwnNHcBRYYu"
Content-Disposition: inline


--/Zw+/jwnNHcBRYYu
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

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

--/Zw+/jwnNHcBRYYu
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v2-0001-Move-routine-generating-restore_command-to-src-co.patch"
Content-Transfer-Encoding: quoted-printable

=46rom 4e2b5b60e003ec67a4854f2625cc610f01428fe2 Mon Sep 17 00:00:00 2001
=46rom: Michael Paquier <michael@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-ee36168955c1@postgr=
espro.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 @@
=20
 #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);
=20
-	/*
-	 * construct the command to be executed
-	 */
-	dp =3D xlogRestoreCmd;
-	endp =3D xlogRestoreCmd + MAXPGPATH - 1;
-	*endp =3D '\0';
-
-	for (sp =3D recoveryRestoreCommand; *sp; sp++)
-	{
-		if (*sp =3D=3D '%')
-		{
-			switch (sp[1])
-			{
-				case 'p':
-					/* %p: relative path of target file */
-					sp++;
-					StrNCpy(dp, xlogpath, endp - dp);
-					make_native_path(dp);
-					dp +=3D strlen(dp);
-					break;
-				case 'f':
-					/* %f: filename of desired file */
-					sp++;
-					StrNCpy(dp, xlogfname, endp - dp);
-					dp +=3D strlen(dp);
-					break;
-				case 'r':
-					/* %r: filename of last restartpoint */
-					sp++;
-					StrNCpy(dp, lastRestartPointFname, endp - dp);
-					dp +=3D strlen(dp);
-					break;
-				case '%':
-					/* convert %% to a single % */
-					sp++;
-					if (dp < endp)
-						*dp++ =3D *sp;
-					break;
-				default:
-					/* otherwise treat the % as not special */
-					if (dp < endp)
-						*dp++ =3D *sp;
-					break;
-			}
-		}
-		else
-		{
-			if (dp < endp)
-				*dp++ =3D *sp;
-		}
-	}
-	*dp =3D '\0';
+	/* Build the restore command to execute */
+	if (!BuildRestoreCommand(recoveryRestoreCommand, xlogpath, xlogfname,
+							 lastRestartPointFname, xlogRestoreCmd,
+							 MAXPGPATH))
+		elog(ERROR, "could not build restore command \"%s\"",
+			 xlogRestoreCmd);
=20
 	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 +=3D $(PTHREAD_LIBS)
 # If you add objects here, see also src/tools/msvc/Mkvcbuild.pm
=20
 OBJS_COMMON =3D \
+	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 ali=
as
+ * is found in the command given by the caller, then false is returned.
+ *
+ * The resulting restore command is stored in "commandResult", pre-allocat=
ed
+ * 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 =3D commandResult;
+	endp =3D commandResult + commandResultLen - 1;
+	*endp =3D '\0';
+
+	for (sp =3D restoreCommand; *sp; sp++)
+	{
+		if (*sp =3D=3D '%')
+		{
+			switch (sp[1])
+			{
+				case 'p':
+					/* %p: relative path of target file */
+					if (xlogpath =3D=3D NULL)
+						return false;
+					sp++;
+					StrNCpy(dp, xlogpath, endp - dp);
+					make_native_path(dp);
+					dp +=3D strlen(dp);
+					break;
+				case 'f':
+					/* %f: filename of desired file */
+					if (xlogfname =3D=3D NULL)
+						return false;
+					sp++;
+					StrNCpy(dp, xlogfname, endp - dp);
+					dp +=3D strlen(dp);
+					break;
+				case 'r':
+					/* %r: filename of last restartpoint */
+					if (lastRestartPointFname =3D=3D NULL)
+						return false;
+					sp++;
+					StrNCpy(dp, lastRestartPointFname, endp - dp);
+					dp +=3D strlen(dp);
+					break;
+				case '%':
+					/* convert %% to a single % */
+					sp++;
+					if (dp < endp)
+						*dp++ =3D *sp;
+					break;
+				default:
+					/* otherwise treat the % as not special */
+					if (dp < endp)
+						*dp++ =3D *sp;
+					break;
+			}
+		}
+		else
+		{
+			if (dp < endp)
+				*dp++ =3D *sp;
+		}
+	}
+	*dp =3D '\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
 	}
=20
 	our @pgcommonallfiles =3D 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=
=2Ec
--=20
2.25.1


--/Zw+/jwnNHcBRYYu--

--BXr400anF0jyguTS
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAl5nF3oACgkQnvQgOdby
QH35EQ//duEMIFmlxwLK8sWSY5dQTOiu1I8xoUxhecrtopLFfhXQyex6TIfRN2ie
SIDw23VpLBadRq4HLuqoPzWxJRy4kuCx26aOrooCJY8MZzJA57/S/M26+faHMTBe
7VpFtYTUoy/WPOJOJj5dKLbyzYeNS3ewZiwo5cuqW2Fpz46oVL6IO17Gczj3tEz7
5RElGu4IslhNxFdcbJHRqd/kURaBA7qLuwelQytMxVb5WhWVsKIuZ81WVUTA1MyI
ykmFU92RipeDzuki3tndgw+/9+C2nP4aC2m0NK6S88cwRHUCiWxuHyirbNgUAoWk
5utg9UWhydodtIXss9kTsQdl1d26HPH95r5ZK6ygvxX3qX4YgBkcyh/T97lseU3d
F7KMNt5bcm6FGgxHeExL1HXxUU0xjPIBtcNnm3mIqFimqJPdKHxJEHnN/q1lhZBI
P7TDgDrGBqJbezRenru15PsCfkvGt11prtX0RxvdGwytpp1ONv54YbrEEQRXH44A
WUxgCPERw0WJW95Bt1FplIwPznZvSzrdaE/eQdiTvzl6JkddzbmdjN7LqmcXQ+j8
AGfkilSKtAdVe+ZU4JoLYdL1viAGbbqJfTKihECavAwImFJKUtLPfXRWXqMs3PFW
GiehRAruHQhsl0D8zr1Wcc4e3W4HZ6ou9kRwTnvLLKD6fsVDY1s=
=69u/
-----END PGP SIGNATURE-----

--BXr400anF0jyguTS--