relocating the server's backup manifest code

Started by Robert Haasover 5 years ago7 messages
#1Robert Haas
robertmhaas@gmail.com
1 attachment(s)

Hi,

The backup manifest patch added a bunch of new code to
src/backend/replication/basebackup.c, and while lamenting the
complexity of that source file yesterday, I suddenly realized that I'd
unwittingly contributed to making that problem worse, and that it
would be quite easy to move the code added by that patch into a
separate file. Attached is a patch to do that.

Despite the fact that we are after feature freeze, I think it would be
a good idea to commit this to PG 13. It could be saved for PG 14, but
that will make future back-patching substantially harder. Also, a
patch that's just moving code is pretty low-risk.

Thoughts?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Attachments:

v1-0001-Move-the-server-s-backup-manifest-code-to-a-separ.patchapplication/octet-stream; name=v1-0001-Move-the-server-s-backup-manifest-code-to-a-separ.patchDownload
From a9386c0c1059cbf567005a6606956ecf808e1071 Mon Sep 17 00:00:00 2001
From: Robert Haas <rhaas@postgresql.org>
Date: Sat, 18 Apr 2020 08:28:50 -0400
Subject: [PATCH v1] Move the server's backup manifest code to a separate file.

basebackup.c is already a pretty big and complicated file, so it
makes more sense to keep the backup manifest support routines
in a separate file, for clarity and ease of maintenance.
---
 src/backend/replication/Makefile          |   1 +
 src/backend/replication/backup_manifest.c | 362 ++++++++++++++++++++
 src/backend/replication/basebackup.c      | 391 +---------------------
 src/include/replication/backup_manifest.h |  64 ++++
 4 files changed, 429 insertions(+), 389 deletions(-)
 create mode 100644 src/backend/replication/backup_manifest.c
 create mode 100644 src/include/replication/backup_manifest.h

diff --git a/src/backend/replication/Makefile b/src/backend/replication/Makefile
index fd08f093e1..a0381e52f3 100644
--- a/src/backend/replication/Makefile
+++ b/src/backend/replication/Makefile
@@ -15,6 +15,7 @@ include $(top_builddir)/src/Makefile.global
 override CPPFLAGS := -I. -I$(srcdir) $(CPPFLAGS)
 
 OBJS = \
+	backup_manifest.o \
 	basebackup.o \
 	repl_gram.o \
 	slot.o \
diff --git a/src/backend/replication/backup_manifest.c b/src/backend/replication/backup_manifest.c
new file mode 100644
index 0000000000..1f11e91a3f
--- /dev/null
+++ b/src/backend/replication/backup_manifest.c
@@ -0,0 +1,362 @@
+/*-------------------------------------------------------------------------
+ *
+ * backup_manifest.c
+ *	  code for generating and sending a backup manifest
+ *
+ * Portions Copyright (c) 2010-2020, PostgreSQL Global Development Group
+ *
+ * IDENTIFICATION
+ *	  src/backend/replication/backup_manifest.c
+ *
+ *-------------------------------------------------------------------------
+ */
+#include "postgres.h"
+
+#include "access/timeline.h"
+#include "libpq/libpq.h"
+#include "libpq/pqformat.h"
+#include "mb/pg_wchar.h"
+#include "replication/backup_manifest.h"
+#include "utils/builtins.h"
+#include "utils/json.h"
+
+/*
+ * Convenience macro for appending data to the backup manifest.
+ */
+#define AppendToManifest(manifest, ...) \
+	{ \
+		char *_manifest_s = psprintf(__VA_ARGS__);	\
+		AppendStringToManifest(manifest, _manifest_s);	\
+		pfree(_manifest_s);	\
+	}
+
+/*
+ * Initialize state so that we can construct a backup manifest.
+ *
+ * NB: Although the checksum type for the data files is configurable, the
+ * checksum for the manifest itself always uses SHA-256. See comments in
+ * SendBackupManifest.
+ */
+void
+InitializeManifest(manifest_info *manifest, manifest_option want_manifest,
+				   pg_checksum_type manifest_checksum_type)
+{
+	if (want_manifest == MANIFEST_OPTION_NO)
+		manifest->buffile = NULL;
+	else
+		manifest->buffile = BufFileCreateTemp(false);
+	manifest->checksum_type = manifest_checksum_type;
+	pg_sha256_init(&manifest->manifest_ctx);
+	manifest->manifest_size = UINT64CONST(0);
+	manifest->force_encode = (want_manifest == MANIFEST_OPTION_FORCE_ENCODE);
+	manifest->first_file = true;
+	manifest->still_checksumming = true;
+
+	if (want_manifest != MANIFEST_OPTION_NO)
+		AppendToManifest(manifest,
+						 "{ \"PostgreSQL-Backup-Manifest-Version\": 1,\n"
+						 "\"Files\": [");
+}
+
+/*
+ * Append a cstring to the manifest.
+ */
+void
+AppendStringToManifest(manifest_info *manifest, char *s)
+{
+	int			len = strlen(s);
+	size_t		written;
+
+	Assert(manifest != NULL);
+	if (manifest->still_checksumming)
+		pg_sha256_update(&manifest->manifest_ctx, (uint8 *) s, len);
+	written = BufFileWrite(manifest->buffile, s, len);
+	if (written != len)
+		ereport(ERROR,
+				(errcode_for_file_access(),
+				 errmsg("could not write to temporary file: %m")));
+	manifest->manifest_size += len;
+}
+
+/*
+ * Add an entry to the backup manifest for a file.
+ */
+void
+AddFileToManifest(manifest_info *manifest, const char *spcoid,
+				  const char *pathname, size_t size, pg_time_t mtime,
+				  pg_checksum_context *checksum_ctx)
+{
+	char		pathbuf[MAXPGPATH];
+	int			pathlen;
+	StringInfoData buf;
+
+	if (!IsManifestEnabled(manifest))
+		return;
+
+	/*
+	 * If this file is part of a tablespace, the pathname passed to this
+	 * function will be relative to the tar file that contains it. We want the
+	 * pathname relative to the data directory (ignoring the intermediate
+	 * symlink traversal).
+	 */
+	if (spcoid != NULL)
+	{
+		snprintf(pathbuf, sizeof(pathbuf), "pg_tblspc/%s/%s", spcoid,
+				 pathname);
+		pathname = pathbuf;
+	}
+
+	/*
+	 * Each file's entry needs to be separated from any entry that follows by a
+	 * comma, but there's no comma before the first one or after the last one.
+	 * To make that work, adding a file to the manifest starts by terminating
+	 * the most recently added line, with a comma if appropriate, but does not
+	 * terminate the line inserted for this file.
+	 */
+	initStringInfo(&buf);
+	if (manifest->first_file)
+	{
+		appendStringInfoString(&buf, "\n");
+		manifest->first_file = false;
+	}
+	else
+		appendStringInfoString(&buf, ",\n");
+
+	/*
+	 * Write the relative pathname to this file out to the manifest. The
+	 * manifest is always stored in UTF-8, so we have to encode paths that are
+	 * not valid in that encoding.
+	 */
+	pathlen = strlen(pathname);
+	if (!manifest->force_encode &&
+		pg_verify_mbstr(PG_UTF8, pathname, pathlen, true))
+	{
+		appendStringInfoString(&buf, "{ \"Path\": ");
+		escape_json(&buf, pathname);
+		appendStringInfoString(&buf, ", ");
+	}
+	else
+	{
+		appendStringInfoString(&buf, "{ \"Encoded-Path\": \"");
+		enlargeStringInfo(&buf, 2 * pathlen);
+		buf.len += hex_encode((char *) pathname, pathlen,
+							  &buf.data[buf.len]);
+		appendStringInfoString(&buf, "\", ");
+	}
+
+	appendStringInfo(&buf, "\"Size\": %zu, ", size);
+
+	/*
+	 * Convert last modification time to a string and append it to the
+	 * manifest. Since it's not clear what time zone to use and since time
+	 * zone definitions can change, possibly causing confusion, use GMT
+	 * always.
+	 */
+	appendStringInfoString(&buf, "\"Last-Modified\": \"");
+	enlargeStringInfo(&buf, 128);
+	buf.len += pg_strftime(&buf.data[buf.len], 128, "%Y-%m-%d %H:%M:%S %Z",
+						   pg_gmtime(&mtime));
+	appendStringInfoString(&buf, "\"");
+
+	/* Add checksum information. */
+	if (checksum_ctx->type != CHECKSUM_TYPE_NONE)
+	{
+		uint8		checksumbuf[PG_CHECKSUM_MAX_LENGTH];
+		int			checksumlen;
+
+		checksumlen = pg_checksum_final(checksum_ctx, checksumbuf);
+
+		appendStringInfo(&buf,
+						 ", \"Checksum-Algorithm\": \"%s\", \"Checksum\": \"",
+						 pg_checksum_type_name(checksum_ctx->type));
+		enlargeStringInfo(&buf, 2 * checksumlen);
+		buf.len += hex_encode((char *) checksumbuf, checksumlen,
+							  &buf.data[buf.len]);
+		appendStringInfoString(&buf, "\"");
+	}
+
+	/* Close out the object. */
+	appendStringInfoString(&buf, " }");
+
+	/* OK, add it to the manifest. */
+	AppendStringToManifest(manifest, buf.data);
+
+	/* Avoid leaking memory. */
+	pfree(buf.data);
+}
+
+/*
+ * Add information about the WAL that will need to be replayed when restoring
+ * this backup to the manifest.
+ */
+void
+AddWALInfoToManifest(manifest_info *manifest, XLogRecPtr startptr,
+					 TimeLineID starttli, XLogRecPtr endptr, TimeLineID endtli)
+{
+	List	   *timelines;
+	ListCell   *lc;
+	bool		first_wal_range = true;
+	bool		found_start_timeline = false;
+
+	if (!IsManifestEnabled(manifest))
+		return;
+
+	/* Terminate the list of files. */
+	AppendStringToManifest(manifest, "\n],\n");
+
+	/* Read the timeline history for the ending timeline. */
+	timelines = readTimeLineHistory(endtli);
+
+	/* Start a list of LSN ranges. */
+	AppendStringToManifest(manifest, "\"WAL-Ranges\": [\n");
+
+	foreach(lc, timelines)
+	{
+		TimeLineHistoryEntry *entry = lfirst(lc);
+		XLogRecPtr	tl_beginptr;
+
+		/*
+		 * We only care about timelines that were active during the backup.
+		 * Skip any that ended before the backup started. (Note that if
+		 * entry->end is InvalidXLogRecPtr, it means that the timeline has not
+		 * yet ended.)
+		 */
+		if (!XLogRecPtrIsInvalid(entry->end) && entry->end < startptr)
+			continue;
+
+		/*
+		 * Because the timeline history file lists newer timelines before
+		 * older ones, the first timeline we encounter that is new enough to
+		 * matter ought to match the ending timeline of the backup.
+		 */
+		if (first_wal_range && endtli != entry->tli)
+			ereport(ERROR,
+					errmsg("expected end timeline %u but found timeline %u",
+						   starttli, entry->tli));
+
+		if (!XLogRecPtrIsInvalid(entry->begin))
+			tl_beginptr = entry->begin;
+		else
+		{
+			tl_beginptr = startptr;
+
+			/*
+			 * If we reach a TLI that has no valid beginning LSN, there can't
+			 * be any more timelines in the history after this point, so we'd
+			 * better have arrived at the expected starting TLI. If not,
+			 * something's gone horribly wrong.
+			 */
+			if (starttli != entry->tli)
+				ereport(ERROR,
+						errmsg("expected start timeline %u but found timeline %u",
+							   starttli, entry->tli));
+		}
+
+		AppendToManifest(manifest,
+						 "%s{ \"Timeline\": %u, \"Start-LSN\": \"%X/%X\", \"End-LSN\": \"%X/%X\" }",
+						 first_wal_range ? "" : ",\n",
+						 entry->tli,
+						 (uint32) (tl_beginptr >> 32), (uint32) tl_beginptr,
+						 (uint32) (endptr >> 32), (uint32) endptr);
+
+		if (starttli == entry->tli)
+		{
+			found_start_timeline = true;
+			break;
+		}
+
+		endptr = entry->begin;
+		first_wal_range = false;
+	}
+
+	/*
+	 * The last entry in the timeline history for the ending timeline should
+	 * be the ending timeline itself. Verify that this is what we observed.
+	 */
+	if (!found_start_timeline)
+		ereport(ERROR,
+				errmsg("start timeline %u not found history of timeline %u",
+					   starttli, endtli));
+
+	/* Terminate the list of WAL ranges. */
+	AppendStringToManifest(manifest, "\n],\n");
+}
+
+/*
+ * Finalize the backup manifest, and send it to the client.
+ */
+void
+SendBackupManifest(manifest_info *manifest)
+{
+	StringInfoData protobuf;
+	uint8		checksumbuf[PG_SHA256_DIGEST_LENGTH];
+	char		checksumstringbuf[PG_SHA256_DIGEST_STRING_LENGTH];
+	size_t		manifest_bytes_done = 0;
+
+	if (!IsManifestEnabled(manifest))
+		return;
+
+	/*
+	 * Append manifest checksum, so that the problems with the manifest itself
+	 * can be detected.
+	 *
+	 * We always use SHA-256 for this, regardless of what algorithm is chosen
+	 * for checksumming the files.  If we ever want to make the checksum
+	 * algorithm used for the manifest file variable, the client will need a
+	 * way to figure out which algorithm to use as close to the beginning of
+	 * the manifest file as possible, to avoid having to read the whole thing
+	 * twice.
+	 */
+	manifest->still_checksumming = false;
+	pg_sha256_final(&manifest->manifest_ctx, checksumbuf);
+	AppendStringToManifest(manifest, "\"Manifest-Checksum\": \"");
+	hex_encode((char *) checksumbuf, sizeof checksumbuf, checksumstringbuf);
+	checksumstringbuf[PG_SHA256_DIGEST_STRING_LENGTH - 1] = '\0';
+	AppendStringToManifest(manifest, checksumstringbuf);
+	AppendStringToManifest(manifest, "\"}\n");
+
+	/*
+	 * We've written all the data to the manifest file.  Rewind the file so
+	 * that we can read it all back.
+	 */
+	if (BufFileSeek(manifest->buffile, 0, 0L, SEEK_SET))
+		ereport(ERROR,
+				(errcode_for_file_access(),
+				 errmsg("could not rewind temporary file: %m")));
+
+	/* Send CopyOutResponse message */
+	pq_beginmessage(&protobuf, 'H');
+	pq_sendbyte(&protobuf, 0);	/* overall format */
+	pq_sendint16(&protobuf, 0); /* natts */
+	pq_endmessage(&protobuf);
+
+	/*
+	 * Send CopyData messages.
+	 *
+	 * We choose to read back the data from the temporary file in chunks of
+	 * size BLCKSZ; this isn't necessary, but buffile.c uses that as the I/O
+	 * size, so it seems to make sense to match that value here.
+	 */
+	while (manifest_bytes_done < manifest->manifest_size)
+	{
+		char		manifestbuf[BLCKSZ];
+		size_t		bytes_to_read;
+		size_t		rc;
+
+		bytes_to_read = Min(sizeof(manifestbuf),
+							manifest->manifest_size - manifest_bytes_done);
+		rc = BufFileRead(manifest->buffile, manifestbuf, bytes_to_read);
+		if (rc != bytes_to_read)
+			ereport(ERROR,
+					(errcode_for_file_access(),
+					 errmsg("could not read from temporary file: %m")));
+		pq_putmessage('d', manifestbuf, bytes_to_read);
+		manifest_bytes_done += bytes_to_read;
+	}
+
+	/* No more data, so send CopyDone message */
+	pq_putemptymessage('c');
+
+	/* Release resources */
+	BufFileClose(manifest->buffile);
+}
diff --git a/src/backend/replication/basebackup.c b/src/backend/replication/basebackup.c
index f5b2411d54..f3fb5716a4 100644
--- a/src/backend/replication/basebackup.c
+++ b/src/backend/replication/basebackup.c
@@ -16,10 +16,8 @@
 #include <unistd.h>
 #include <time.h>
 
-#include "access/timeline.h"
 #include "access/xlog_internal.h"	/* for pg_start/stop_backup */
 #include "catalog/pg_type.h"
-#include "common/checksum_helper.h"
 #include "common/file_perm.h"
 #include "commands/progress.h"
 #include "lib/stringinfo.h"
@@ -32,9 +30,9 @@
 #include "port.h"
 #include "postmaster/syslogger.h"
 #include "replication/basebackup.h"
+#include "replication/backup_manifest.h"
 #include "replication/walsender.h"
 #include "replication/walsender_private.h"
-#include "storage/buffile.h"
 #include "storage/bufpage.h"
 #include "storage/checksum.h"
 #include "storage/dsm_impl.h"
@@ -42,19 +40,11 @@
 #include "storage/ipc.h"
 #include "storage/reinit.h"
 #include "utils/builtins.h"
-#include "utils/json.h"
 #include "utils/ps_status.h"
 #include "utils/relcache.h"
 #include "utils/resowner.h"
 #include "utils/timestamp.h"
 
-typedef enum manifest_option
-{
-	MANIFEST_OPTION_YES,
-	MANIFEST_OPTION_NO,
-	MANIFEST_OPTION_FORCE_ENCODE
-} manifest_option;
-
 typedef struct
 {
 	const char *label;
@@ -68,18 +58,6 @@ typedef struct
 	pg_checksum_type manifest_checksum_type;
 } basebackup_options;
 
-struct manifest_info
-{
-	BufFile    *buffile;
-	pg_checksum_type checksum_type;
-	pg_sha256_ctx manifest_ctx;
-	uint64		manifest_size;
-	bool		force_encode;
-	bool		first_file;
-	bool		still_checksumming;
-};
-
-
 static int64 sendDir(const char *path, int basepathlen, bool sizeonly,
 					 List *tablespaces, bool sendtblspclinks,
 					 manifest_info *manifest, const char *spcoid);
@@ -94,18 +72,6 @@ static int64 _tarWriteDir(const char *pathbuf, int basepathlen, struct stat *sta
 						  bool sizeonly);
 static void send_int8_string(StringInfoData *buf, int64 intval);
 static void SendBackupHeader(List *tablespaces);
-static bool IsManifestEnabled(manifest_info *manifest);
-static void InitializeManifest(manifest_info *manifest,
-							   basebackup_options *opt);
-static void AppendStringToManifest(manifest_info *manifest, char *s);
-static void AddFileToManifest(manifest_info *manifest, const char *spcoid,
-							  const char *pathname, size_t size,
-							  pg_time_t mtime,
-							  pg_checksum_context *checksum_ctx);
-static void AddWALInfoToManifest(manifest_info *manifest, XLogRecPtr startptr,
-								 TimeLineID starttli, XLogRecPtr endptr,
-								 TimeLineID endtli);
-static void SendBackupManifest(manifest_info *manifest);
 static void perform_base_backup(basebackup_options *opt);
 static void parse_basebackup_options(List *options, basebackup_options *opt);
 static void SendXlogRecPtrResult(XLogRecPtr ptr, TimeLineID tli);
@@ -142,16 +108,6 @@ do { \
 				(errmsg("could not read from file \"%s\"", filename))); \
 } while (0)
 
-/*
- * Convenience macro for appending data to the backup manifest.
- */
-#define AppendToManifest(manifest, ...) \
-	{ \
-		char *_manifest_s = psprintf(__VA_ARGS__);	\
-		AppendStringToManifest(manifest, _manifest_s);	\
-		pfree(_manifest_s);	\
-	}
-
 /* The actual number of bytes, transfer of which may cause sleep. */
 static uint64 throttling_sample;
 
@@ -342,7 +298,7 @@ perform_base_backup(basebackup_options *opt)
 
 	labelfile = makeStringInfo();
 	tblspc_map_file = makeStringInfo();
-	InitializeManifest(&manifest, opt);
+	InitializeManifest(&manifest, opt->manifest, opt->manifest_checksum_type);
 
 	total_checksum_failures = 0;
 
@@ -1068,349 +1024,6 @@ SendBackupHeader(List *tablespaces)
 	pq_puttextmessage('C', "SELECT");
 }
 
-/*
- * Does the user want a backup manifest?
- *
- * It's simplest to always have a manifest_info object, so that we don't need
- * checks for NULL pointers in too many places. However, if the user doesn't
- * want a manifest, we set manifest->buffile to NULL.
- */
-static bool
-IsManifestEnabled(manifest_info *manifest)
-{
-	return (manifest->buffile != NULL);
-}
-
-/*
- * Initialize state so that we can construct a backup manifest.
- *
- * NB: Although the checksum type for the data files is configurable, the
- * checksum for the manifest itself always uses SHA-256. See comments in
- * SendBackupManifest.
- */
-static void
-InitializeManifest(manifest_info *manifest, basebackup_options *opt)
-{
-	if (opt->manifest == MANIFEST_OPTION_NO)
-		manifest->buffile = NULL;
-	else
-		manifest->buffile = BufFileCreateTemp(false);
-	manifest->checksum_type = opt->manifest_checksum_type;
-	pg_sha256_init(&manifest->manifest_ctx);
-	manifest->manifest_size = UINT64CONST(0);
-	manifest->force_encode = (opt->manifest == MANIFEST_OPTION_FORCE_ENCODE);
-	manifest->first_file = true;
-	manifest->still_checksumming = true;
-
-	if (opt->manifest != MANIFEST_OPTION_NO)
-		AppendToManifest(manifest,
-						 "{ \"PostgreSQL-Backup-Manifest-Version\": 1,\n"
-						 "\"Files\": [");
-}
-
-/*
- * Append a cstring to the manifest.
- */
-static void
-AppendStringToManifest(manifest_info *manifest, char *s)
-{
-	int			len = strlen(s);
-	size_t		written;
-
-	Assert(manifest != NULL);
-	if (manifest->still_checksumming)
-		pg_sha256_update(&manifest->manifest_ctx, (uint8 *) s, len);
-	written = BufFileWrite(manifest->buffile, s, len);
-	if (written != len)
-		ereport(ERROR,
-				(errcode_for_file_access(),
-				 errmsg("could not write to temporary file: %m")));
-	manifest->manifest_size += len;
-}
-
-/*
- * Add an entry to the backup manifest for a file.
- */
-static void
-AddFileToManifest(manifest_info *manifest, const char *spcoid,
-				  const char *pathname, size_t size, pg_time_t mtime,
-				  pg_checksum_context *checksum_ctx)
-{
-	char		pathbuf[MAXPGPATH];
-	int			pathlen;
-	StringInfoData buf;
-
-	if (!IsManifestEnabled(manifest))
-		return;
-
-	/*
-	 * If this file is part of a tablespace, the pathname passed to this
-	 * function will be relative to the tar file that contains it. We want the
-	 * pathname relative to the data directory (ignoring the intermediate
-	 * symlink traversal).
-	 */
-	if (spcoid != NULL)
-	{
-		snprintf(pathbuf, sizeof(pathbuf), "pg_tblspc/%s/%s", spcoid,
-				 pathname);
-		pathname = pathbuf;
-	}
-
-	/*
-	 * Each file's entry needs to be separated from any entry that follows by a
-	 * comma, but there's no comma before the first one or after the last one.
-	 * To make that work, adding a file to the manifest starts by terminating
-	 * the most recently added line, with a comma if appropriate, but does not
-	 * terminate the line inserted for this file.
-	 */
-	initStringInfo(&buf);
-	if (manifest->first_file)
-	{
-		appendStringInfoString(&buf, "\n");
-		manifest->first_file = false;
-	}
-	else
-		appendStringInfoString(&buf, ",\n");
-
-	/*
-	 * Write the relative pathname to this file out to the manifest. The
-	 * manifest is always stored in UTF-8, so we have to encode paths that are
-	 * not valid in that encoding.
-	 */
-	pathlen = strlen(pathname);
-	if (!manifest->force_encode &&
-		pg_verify_mbstr(PG_UTF8, pathname, pathlen, true))
-	{
-		appendStringInfoString(&buf, "{ \"Path\": ");
-		escape_json(&buf, pathname);
-		appendStringInfoString(&buf, ", ");
-	}
-	else
-	{
-		appendStringInfoString(&buf, "{ \"Encoded-Path\": \"");
-		enlargeStringInfo(&buf, 2 * pathlen);
-		buf.len += hex_encode((char *) pathname, pathlen,
-							  &buf.data[buf.len]);
-		appendStringInfoString(&buf, "\", ");
-	}
-
-	appendStringInfo(&buf, "\"Size\": %zu, ", size);
-
-	/*
-	 * Convert last modification time to a string and append it to the
-	 * manifest. Since it's not clear what time zone to use and since time
-	 * zone definitions can change, possibly causing confusion, use GMT
-	 * always.
-	 */
-	appendStringInfoString(&buf, "\"Last-Modified\": \"");
-	enlargeStringInfo(&buf, 128);
-	buf.len += pg_strftime(&buf.data[buf.len], 128, "%Y-%m-%d %H:%M:%S %Z",
-						   pg_gmtime(&mtime));
-	appendStringInfoString(&buf, "\"");
-
-	/* Add checksum information. */
-	if (checksum_ctx->type != CHECKSUM_TYPE_NONE)
-	{
-		uint8		checksumbuf[PG_CHECKSUM_MAX_LENGTH];
-		int			checksumlen;
-
-		checksumlen = pg_checksum_final(checksum_ctx, checksumbuf);
-
-		appendStringInfo(&buf,
-						 ", \"Checksum-Algorithm\": \"%s\", \"Checksum\": \"",
-						 pg_checksum_type_name(checksum_ctx->type));
-		enlargeStringInfo(&buf, 2 * checksumlen);
-		buf.len += hex_encode((char *) checksumbuf, checksumlen,
-							  &buf.data[buf.len]);
-		appendStringInfoString(&buf, "\"");
-	}
-
-	/* Close out the object. */
-	appendStringInfoString(&buf, " }");
-
-	/* OK, add it to the manifest. */
-	AppendStringToManifest(manifest, buf.data);
-
-	/* Avoid leaking memory. */
-	pfree(buf.data);
-}
-
-/*
- * Add information about the WAL that will need to be replayed when restoring
- * this backup to the manifest.
- */
-static void
-AddWALInfoToManifest(manifest_info *manifest, XLogRecPtr startptr,
-					 TimeLineID starttli, XLogRecPtr endptr, TimeLineID endtli)
-{
-	List	   *timelines;
-	ListCell   *lc;
-	bool		first_wal_range = true;
-	bool		found_start_timeline = false;
-
-	if (!IsManifestEnabled(manifest))
-		return;
-
-	/* Terminate the list of files. */
-	AppendStringToManifest(manifest, "\n],\n");
-
-	/* Read the timeline history for the ending timeline. */
-	timelines = readTimeLineHistory(endtli);
-
-	/* Start a list of LSN ranges. */
-	AppendStringToManifest(manifest, "\"WAL-Ranges\": [\n");
-
-	foreach(lc, timelines)
-	{
-		TimeLineHistoryEntry *entry = lfirst(lc);
-		XLogRecPtr	tl_beginptr;
-
-		/*
-		 * We only care about timelines that were active during the backup.
-		 * Skip any that ended before the backup started. (Note that if
-		 * entry->end is InvalidXLogRecPtr, it means that the timeline has not
-		 * yet ended.)
-		 */
-		if (!XLogRecPtrIsInvalid(entry->end) && entry->end < startptr)
-			continue;
-
-		/*
-		 * Because the timeline history file lists newer timelines before
-		 * older ones, the first timeline we encounter that is new enough to
-		 * matter ought to match the ending timeline of the backup.
-		 */
-		if (first_wal_range && endtli != entry->tli)
-			ereport(ERROR,
-					errmsg("expected end timeline %u but found timeline %u",
-						   starttli, entry->tli));
-
-		if (!XLogRecPtrIsInvalid(entry->begin))
-			tl_beginptr = entry->begin;
-		else
-		{
-			tl_beginptr = startptr;
-
-			/*
-			 * If we reach a TLI that has no valid beginning LSN, there can't
-			 * be any more timelines in the history after this point, so we'd
-			 * better have arrived at the expected starting TLI. If not,
-			 * something's gone horribly wrong.
-			 */
-			if (starttli != entry->tli)
-				ereport(ERROR,
-						errmsg("expected start timeline %u but found timeline %u",
-							   starttli, entry->tli));
-		}
-
-		AppendToManifest(manifest,
-						 "%s{ \"Timeline\": %u, \"Start-LSN\": \"%X/%X\", \"End-LSN\": \"%X/%X\" }",
-						 first_wal_range ? "" : ",\n",
-						 entry->tli,
-						 (uint32) (tl_beginptr >> 32), (uint32) tl_beginptr,
-						 (uint32) (endptr >> 32), (uint32) endptr);
-
-		if (starttli == entry->tli)
-		{
-			found_start_timeline = true;
-			break;
-		}
-
-		endptr = entry->begin;
-		first_wal_range = false;
-	}
-
-	/*
-	 * The last entry in the timeline history for the ending timeline should
-	 * be the ending timeline itself. Verify that this is what we observed.
-	 */
-	if (!found_start_timeline)
-		ereport(ERROR,
-				errmsg("start timeline %u not found history of timeline %u",
-					   starttli, endtli));
-
-	/* Terminate the list of WAL ranges. */
-	AppendStringToManifest(manifest, "\n],\n");
-}
-
-/*
- * Finalize the backup manifest, and send it to the client.
- */
-static void
-SendBackupManifest(manifest_info *manifest)
-{
-	StringInfoData protobuf;
-	uint8		checksumbuf[PG_SHA256_DIGEST_LENGTH];
-	char		checksumstringbuf[PG_SHA256_DIGEST_STRING_LENGTH];
-	size_t		manifest_bytes_done = 0;
-
-	if (!IsManifestEnabled(manifest))
-		return;
-
-	/*
-	 * Append manifest checksum, so that the problems with the manifest itself
-	 * can be detected.
-	 *
-	 * We always use SHA-256 for this, regardless of what algorithm is chosen
-	 * for checksumming the files.  If we ever want to make the checksum
-	 * algorithm used for the manifest file variable, the client will need a
-	 * way to figure out which algorithm to use as close to the beginning of
-	 * the manifest file as possible, to avoid having to read the whole thing
-	 * twice.
-	 */
-	manifest->still_checksumming = false;
-	pg_sha256_final(&manifest->manifest_ctx, checksumbuf);
-	AppendStringToManifest(manifest, "\"Manifest-Checksum\": \"");
-	hex_encode((char *) checksumbuf, sizeof checksumbuf, checksumstringbuf);
-	checksumstringbuf[PG_SHA256_DIGEST_STRING_LENGTH - 1] = '\0';
-	AppendStringToManifest(manifest, checksumstringbuf);
-	AppendStringToManifest(manifest, "\"}\n");
-
-	/*
-	 * We've written all the data to the manifest file.  Rewind the file so
-	 * that we can read it all back.
-	 */
-	if (BufFileSeek(manifest->buffile, 0, 0L, SEEK_SET))
-		ereport(ERROR,
-				(errcode_for_file_access(),
-				 errmsg("could not rewind temporary file: %m")));
-
-	/* Send CopyOutResponse message */
-	pq_beginmessage(&protobuf, 'H');
-	pq_sendbyte(&protobuf, 0);	/* overall format */
-	pq_sendint16(&protobuf, 0); /* natts */
-	pq_endmessage(&protobuf);
-
-	/*
-	 * Send CopyData messages.
-	 *
-	 * We choose to read back the data from the temporary file in chunks of
-	 * size BLCKSZ; this isn't necessary, but buffile.c uses that as the I/O
-	 * size, so it seems to make sense to match that value here.
-	 */
-	while (manifest_bytes_done < manifest->manifest_size)
-	{
-		char		manifestbuf[BLCKSZ];
-		size_t		bytes_to_read;
-		size_t		rc;
-
-		bytes_to_read = Min(sizeof(manifestbuf),
-							manifest->manifest_size - manifest_bytes_done);
-		rc = BufFileRead(manifest->buffile, manifestbuf, bytes_to_read);
-		if (rc != bytes_to_read)
-			ereport(ERROR,
-					(errcode_for_file_access(),
-					 errmsg("could not read from temporary file: %m")));
-		pq_putmessage('d', manifestbuf, bytes_to_read);
-		manifest_bytes_done += bytes_to_read;
-	}
-
-	/* No more data, so send CopyDone message */
-	pq_putemptymessage('c');
-
-	/* Release resources */
-	BufFileClose(manifest->buffile);
-}
-
 /*
  * Send a single resultset containing just a single
  * XLogRecPtr record (in text format)
diff --git a/src/include/replication/backup_manifest.h b/src/include/replication/backup_manifest.h
new file mode 100644
index 0000000000..4f7ee82dd6
--- /dev/null
+++ b/src/include/replication/backup_manifest.h
@@ -0,0 +1,64 @@
+/*-------------------------------------------------------------------------
+ *
+ * backup_manifest.h
+ *	  Routines for generating a backup manifest.
+ *
+ * Portions Copyright (c) 2010-2020, PostgreSQL Global Development Group
+ *
+ * src/include/replication/backup_manifest.h
+ *
+ *-------------------------------------------------------------------------
+ */
+#ifndef BACKUP_MANIFEST_H
+#define BACKUP_MANIFEST_H
+
+#include "access/xlogdefs.h"
+#include "common/checksum_helper.h"
+#include "pgtime.h"
+#include "storage/buffile.h"
+
+typedef enum manifest_option
+{
+	MANIFEST_OPTION_YES,
+	MANIFEST_OPTION_NO,
+	MANIFEST_OPTION_FORCE_ENCODE
+} manifest_option;
+
+typedef struct manifest_info
+{
+	BufFile    *buffile;
+	pg_checksum_type checksum_type;
+	pg_sha256_ctx manifest_ctx;
+	uint64		manifest_size;
+	bool		force_encode;
+	bool		first_file;
+	bool		still_checksumming;
+} manifest_info;
+
+/*
+ * Does the user want a backup manifest?
+ *
+ * It's simplest to always have a manifest_info object, so that we don't need
+ * checks for NULL pointers in too many places. However, if the user doesn't
+ * want a manifest, we set manifest->buffile to NULL.
+ */
+static inline bool
+IsManifestEnabled(manifest_info *manifest)
+{
+	return (manifest->buffile != NULL);
+}
+
+extern void InitializeManifest(manifest_info *manifest,
+							   manifest_option want_manifest,
+							   pg_checksum_type manifest_checksum_type);
+extern void AppendStringToManifest(manifest_info *manifest, char *s);
+extern void AddFileToManifest(manifest_info *manifest, const char *spcoid,
+							  const char *pathname, size_t size,
+							  pg_time_t mtime,
+							  pg_checksum_context *checksum_ctx);
+extern void AddWALInfoToManifest(manifest_info *manifest, XLogRecPtr startptr,
+								 TimeLineID starttli, XLogRecPtr endptr,
+								 TimeLineID endtli);
+extern void SendBackupManifest(manifest_info *manifest);
+
+#endif
-- 
2.17.2 (Apple Git-113)

#2Michael Paquier
michael@paquier.xyz
In reply to: Robert Haas (#1)
Re: relocating the server's backup manifest code

On Sat, Apr 18, 2020 at 08:37:58AM -0400, Robert Haas wrote:

Despite the fact that we are after feature freeze, I think it would be
a good idea to commit this to PG 13. It could be saved for PG 14, but
that will make future back-patching substantially harder. Also, a
patch that's just moving code is pretty low-risk.

Makes sense to move this code around, so that's fine by me to do it
even after the feature freeze as it means less back-patching pain in
the future.

+static inline bool
+IsManifestEnabled(manifest_info *manifest)
+{
+   return (manifest->buffile != NULL);
+}
I would keep this one static and located within backup_manifest.c as
it is only used there.
+extern void InitializeManifest(manifest_info *manifest,
+                              manifest_option want_manifest,
+                              pg_checksum_type manifest_checksum_type);
+extern void AppendStringToManifest(manifest_info *manifest, char *s);
+extern void AddFileToManifest(manifest_info *manifest, const char *spcoid,
+                             const char *pathname, size_t size,
+                             pg_time_t mtime,
+                             pg_checksum_context *checksum_ctx);
+extern void AddWALInfoToManifest(manifest_info *manifest, XLogRecPtr startptr,
+                                TimeLineID starttli, XLogRecPtr endptr,
+                                TimeLineID endtli);
+extern void SendBackupManifest(manifest_info *manifest);

Now the names of those routines is not really consistent if you wish
to make one single family. Here is a suggestion:
- BackupManifestInit()
- BackupManifestAppendString()
- BackupManifestAddFile()
- BackupManifestAddWALInfo()
- BackupManifestSend()

+ * Portions Copyright (c) 2010-2020, PostgreSQL Global Development Group
I would vote for some more consistency. Personally I just use that
all the time:
* Portions Copyright (c) 1996-2020, PostgreSQL Global Development Group
* Portions Copyright (c) 1994, Regents of the University of California

+typedef enum manifest_option
+{
[...]
+typedef struct manifest_info
+{
These ought to be named backup_manifest_info and backup_manifest_option?
--
Michael
#3Robert Haas
robertmhaas@gmail.com
In reply to: Michael Paquier (#2)
Re: relocating the server's backup manifest code

On Sat, Apr 18, 2020 at 8:57 AM Michael Paquier <michael@paquier.xyz> wrote:

+static inline bool
+IsManifestEnabled(manifest_info *manifest)
+{
+   return (manifest->buffile != NULL);
+}
I would keep this one static and located within backup_manifest.c as
it is only used there.

Oh, OK.

+extern void InitializeManifest(manifest_info *manifest,
+                              manifest_option want_manifest,
+                              pg_checksum_type manifest_checksum_type);
+extern void AppendStringToManifest(manifest_info *manifest, char *s);
+extern void AddFileToManifest(manifest_info *manifest, const char *spcoid,
+                             const char *pathname, size_t size,
+                             pg_time_t mtime,
+                             pg_checksum_context *checksum_ctx);
+extern void AddWALInfoToManifest(manifest_info *manifest, XLogRecPtr startptr,
+                                TimeLineID starttli, XLogRecPtr endptr,
+                                TimeLineID endtli);
+extern void SendBackupManifest(manifest_info *manifest);

Now the names of those routines is not really consistent if you wish
to make one single family. Here is a suggestion:
- BackupManifestInit()
- BackupManifestAppendString()
- BackupManifestAddFile()
- BackupManifestAddWALInfo()
- BackupManifestSend()

I'm not in favor of this renaming. Different people have different
preferences, of course, but my impression is that the general project
style is to choose names that follow English word ordering, i.e.
appendStringInfo rather than stringInfoAppend; add_paths_to_joinrel
rather than joinrel_append_paths; etc. There are many exceptions, but
I tend to lean toward English word ordering unless it seems to create
a large amount of awkwardness in a particular case. At any rate, it
seems a separate question from moving the code.

+ * Portions Copyright (c) 2010-2020, PostgreSQL Global Development Group
I would vote for some more consistency. Personally I just use that
all the time:
* Portions Copyright (c) 1996-2020, PostgreSQL Global Development Group
* Portions Copyright (c) 1994, Regents of the University of California

Sure, that's fine.

+typedef enum manifest_option
+{
[...]
+typedef struct manifest_info
+{
These ought to be named backup_manifest_info and backup_manifest_option?

I'm OK with that. I don't think it's a big deal because "manifest"
isn't a widely-used PostgreSQL term already, but it doesn't bother me
to rename it.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

#4Tom Lane
tgl@sss.pgh.pa.us
In reply to: Robert Haas (#1)
Re: relocating the server's backup manifest code

Robert Haas <robertmhaas@gmail.com> writes:

Despite the fact that we are after feature freeze, I think it would be
a good idea to commit this to PG 13. It could be saved for PG 14, but
that will make future back-patching substantially harder. Also, a
patch that's just moving code is pretty low-risk.

+1 in principle --- I didn't read the patch though.

regards, tom lane

#5Magnus Hagander
magnus@hagander.net
In reply to: Tom Lane (#4)
Re: relocating the server's backup manifest code

On Sat, Apr 18, 2020 at 5:43 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Robert Haas <robertmhaas@gmail.com> writes:

Despite the fact that we are after feature freeze, I think it would be
a good idea to commit this to PG 13. It could be saved for PG 14, but
that will make future back-patching substantially harder. Also, a
patch that's just moving code is pretty low-risk.

+1 in principle --- I didn't read the patch though.

Same here, +1 in principle. This is not a new feature, this is "polishing a
feature that was added in 13", and doing so now will save a lot of work
down the road vs doing it later.

--
Magnus Hagander
Me: https://www.hagander.net/ <http://www.hagander.net/&gt;
Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/&gt;

#6Michael Paquier
michael@paquier.xyz
In reply to: Robert Haas (#3)
Re: relocating the server's backup manifest code

On Sat, Apr 18, 2020 at 10:43:52AM -0400, Robert Haas wrote:

I'm not in favor of this renaming. Different people have different
preferences, of course, but my impression is that the general project
style is to choose names that follow English word ordering, i.e.
appendStringInfo rather than stringInfoAppend; add_paths_to_joinrel
rather than joinrel_append_paths; etc. There are many exceptions, but
I tend to lean toward English word ordering unless it seems to create
a large amount of awkwardness in a particular case. At any rate, it
seems a separate question from moving the code.

Both of us have rather different views on the matter then. I still
prefer my suggestion because that's more consistent and easier to
grep, but I'll be fine with your call at the end. I would suggest to
still use BackupManifest instead of Manifest in those functions and
structures though, as we cannot really know if the concept of manifest
would apply to other parts of the system. A recent example of API
name conflict I have in mind is index AM vs table AM. Index AMs have
been using rather generic names, causing issues when table AMs have
been introduced.

I'm OK with that. I don't think it's a big deal because "manifest"
isn't a widely-used PostgreSQL term already, but it doesn't bother me
to rename it.

Thanks.
--
Michael

#7Robert Haas
robertmhaas@gmail.com
In reply to: Michael Paquier (#6)
1 attachment(s)
Re: relocating the server's backup manifest code

On Sat, Apr 18, 2020 at 8:12 PM Michael Paquier <michael@paquier.xyz> wrote:

I would suggest to
still use BackupManifest instead of Manifest in those functions and
structures though, ...

Done in the attached, which also adds "backup_" to the type names.

After further examination, I think the Copyright header issue is
entirely separate. If someone wants to standardize that across the
source tree, cool, but this patch just duplicated the header from the
file out of which it was moving code.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Attachments:

0001-Rename-exposed-identifiers-to-say-backup-manifest.patchapplication/octet-stream; name=0001-Rename-exposed-identifiers-to-say-backup-manifest.patchDownload
From b76a316d1baa80dcc70c81b539f6d14f04556983 Mon Sep 17 00:00:00 2001
From: Robert Haas <rhaas@postgresql.org>
Date: Mon, 20 Apr 2020 14:51:05 -0400
Subject: [PATCH] Rename exposed identifiers to say "backup manifest".

Function names declared "extern" now use BackupManifest in the name
rather than just Manifest, and data types use backup_manifest
rather than just manifest.

Per note from Michael Paquier.

Discussion: http://postgr.es/m/20200418125713.GG350229@paquier.xyz
---
 src/backend/replication/backup_manifest.c | 62 ++++++++++++-----------
 src/backend/replication/basebackup.c      | 16 +++---
 src/include/replication/backup_manifest.h | 29 ++++++-----
 3 files changed, 56 insertions(+), 51 deletions(-)

diff --git a/src/backend/replication/backup_manifest.c b/src/backend/replication/backup_manifest.c
index 8aead70726..bef02113d9 100644
--- a/src/backend/replication/backup_manifest.c
+++ b/src/backend/replication/backup_manifest.c
@@ -20,6 +20,8 @@
 #include "utils/builtins.h"
 #include "utils/json.h"
 
+static void AppendStringToManifest(backup_manifest_info *manifest, char *s);
+
 /*
  * Does the user want a backup manifest?
  *
@@ -28,7 +30,7 @@
  * want a manifest, we set manifest->buffile to NULL.
  */
 static inline bool
-IsManifestEnabled(manifest_info *manifest)
+IsManifestEnabled(backup_manifest_info *manifest)
 {
 	return (manifest->buffile != NULL);
 }
@@ -51,8 +53,9 @@ IsManifestEnabled(manifest_info *manifest)
  * SendBackupManifest.
  */
 void
-InitializeManifest(manifest_info *manifest, manifest_option want_manifest,
-				   pg_checksum_type manifest_checksum_type)
+InitializeBackupManifest(backup_manifest_info *manifest,
+						 backup_manifest_option want_manifest,
+						 pg_checksum_type manifest_checksum_type)
 {
 	if (want_manifest == MANIFEST_OPTION_NO)
 		manifest->buffile = NULL;
@@ -71,33 +74,13 @@ InitializeManifest(manifest_info *manifest, manifest_option want_manifest,
 						 "\"Files\": [");
 }
 
-/*
- * Append a cstring to the manifest.
- */
-void
-AppendStringToManifest(manifest_info *manifest, char *s)
-{
-	int			len = strlen(s);
-	size_t		written;
-
-	Assert(manifest != NULL);
-	if (manifest->still_checksumming)
-		pg_sha256_update(&manifest->manifest_ctx, (uint8 *) s, len);
-	written = BufFileWrite(manifest->buffile, s, len);
-	if (written != len)
-		ereport(ERROR,
-				(errcode_for_file_access(),
-				 errmsg("could not write to temporary file: %m")));
-	manifest->manifest_size += len;
-}
-
 /*
  * Add an entry to the backup manifest for a file.
  */
 void
-AddFileToManifest(manifest_info *manifest, const char *spcoid,
-				  const char *pathname, size_t size, pg_time_t mtime,
-				  pg_checksum_context *checksum_ctx)
+AddFileToBackupManifest(backup_manifest_info *manifest, const char *spcoid,
+						const char *pathname, size_t size, pg_time_t mtime,
+						pg_checksum_context * checksum_ctx)
 {
 	char		pathbuf[MAXPGPATH];
 	int			pathlen;
@@ -203,8 +186,9 @@ AddFileToManifest(manifest_info *manifest, const char *spcoid,
  * this backup to the manifest.
  */
 void
-AddWALInfoToManifest(manifest_info *manifest, XLogRecPtr startptr,
-					 TimeLineID starttli, XLogRecPtr endptr, TimeLineID endtli)
+AddWALInfoToBackupManifest(backup_manifest_info *manifest, XLogRecPtr startptr,
+						   TimeLineID starttli, XLogRecPtr endptr,
+						   TimeLineID endtli)
 {
 	List	   *timelines;
 	ListCell   *lc;
@@ -299,7 +283,7 @@ AddWALInfoToManifest(manifest_info *manifest, XLogRecPtr startptr,
  * Finalize the backup manifest, and send it to the client.
  */
 void
-SendBackupManifest(manifest_info *manifest)
+SendBackupManifest(backup_manifest_info *manifest)
 {
 	StringInfoData protobuf;
 	uint8		checksumbuf[PG_SHA256_DIGEST_LENGTH];
@@ -373,3 +357,23 @@ SendBackupManifest(manifest_info *manifest)
 	/* Release resources */
 	BufFileClose(manifest->buffile);
 }
+
+/*
+ * Append a cstring to the manifest.
+ */
+static void
+AppendStringToManifest(manifest_info * manifest, char *s)
+{
+	int			len = strlen(s);
+	size_t		written;
+
+	Assert(manifest != NULL);
+	if (manifest->still_checksumming)
+		pg_sha256_update(&manifest->manifest_ctx, (uint8 *) s, len);
+	written = BufFileWrite(manifest->buffile, s, len);
+	if (written != len)
+		ereport(ERROR,
+				(errcode_for_file_access(),
+				 errmsg("could not write to temporary file: %m")));
+	manifest->manifest_size += len;
+}
diff --git a/src/backend/replication/basebackup.c b/src/backend/replication/basebackup.c
index f3fb5716a4..b59542d56b 100644
--- a/src/backend/replication/basebackup.c
+++ b/src/backend/replication/basebackup.c
@@ -54,7 +54,7 @@ typedef struct
 	bool		includewal;
 	uint32		maxrate;
 	bool		sendtblspcmapfile;
-	manifest_option manifest;
+	backup_manifest_option manifest;
 	pg_checksum_type manifest_checksum_type;
 } basebackup_options;
 
@@ -298,7 +298,8 @@ perform_base_backup(basebackup_options *opt)
 
 	labelfile = makeStringInfo();
 	tblspc_map_file = makeStringInfo();
-	InitializeManifest(&manifest, opt->manifest, opt->manifest_checksum_type);
+	InitializeBackupManifest(&manifest, opt->manifest,
+							 opt->manifest_checksum_type);
 
 	total_checksum_failures = 0;
 
@@ -710,7 +711,7 @@ perform_base_backup(basebackup_options *opt)
 		pq_putemptymessage('c');
 	}
 
-	AddWALInfoToManifest(&manifest, startptr, starttli, endptr, endtli);
+	AddWALInfoToBackupManifest(&manifest, startptr, starttli, endptr, endtli);
 
 	SendBackupManifest(&manifest);
 
@@ -1129,9 +1130,8 @@ sendFileWithContent(const char *filename, const char *content,
 	}
 
 	pg_checksum_update(&checksum_ctx, (uint8 *) content, len);
-	AddFileToManifest(manifest, NULL, filename, len,
-					  (pg_time_t) statbuf.st_mtime,
-					  &checksum_ctx);
+	AddFileToBackupManifest(manifest, NULL, filename, len,
+							(pg_time_t) statbuf.st_mtime, &checksum_ctx);
 }
 
 /*
@@ -1810,8 +1810,8 @@ sendFile(const char *readfilename, const char *tarfilename,
 
 	total_checksum_failures += checksum_failures;
 
-	AddFileToManifest(manifest, spcoid, tarfilename, statbuf->st_size,
-					  (pg_time_t) statbuf->st_mtime, &checksum_ctx);
+	AddFileToBackupManifest(manifest, spcoid, tarfilename, statbuf->st_size,
+							(pg_time_t) statbuf->st_mtime, &checksum_ctx);
 
 	return true;
 }
diff --git a/src/include/replication/backup_manifest.h b/src/include/replication/backup_manifest.h
index e7fccddd0d..fd614202b3 100644
--- a/src/include/replication/backup_manifest.h
+++ b/src/include/replication/backup_manifest.h
@@ -22,7 +22,7 @@ typedef enum manifest_option
 	MANIFEST_OPTION_YES,
 	MANIFEST_OPTION_NO,
 	MANIFEST_OPTION_FORCE_ENCODE
-} manifest_option;
+} backup_manifest_option;
 
 typedef struct manifest_info
 {
@@ -33,19 +33,20 @@ typedef struct manifest_info
 	bool		force_encode;
 	bool		first_file;
 	bool		still_checksumming;
-} manifest_info;
+} backup_manifest_info;
 
-extern void InitializeManifest(manifest_info *manifest,
-							   manifest_option want_manifest,
-							   pg_checksum_type manifest_checksum_type);
-extern void AppendStringToManifest(manifest_info *manifest, char *s);
-extern void AddFileToManifest(manifest_info *manifest, const char *spcoid,
-							  const char *pathname, size_t size,
-							  pg_time_t mtime,
-							  pg_checksum_context *checksum_ctx);
-extern void AddWALInfoToManifest(manifest_info *manifest, XLogRecPtr startptr,
-								 TimeLineID starttli, XLogRecPtr endptr,
-								 TimeLineID endtli);
-extern void SendBackupManifest(manifest_info *manifest);
+extern void InitializeBackupManifest(backup_manifest_info *manifest,
+									 backup_manifest_option want_manifest,
+									 pg_checksum_type manifest_checksum_type);
+extern void AddFileToBackupManifest(backup_manifest_info *manifest,
+									const char *spcoid,
+									const char *pathname, size_t size,
+									pg_time_t mtime,
+									pg_checksum_context * checksum_ctx);
+extern void AddWALInfoToBackupManifest(backup_manifest_info *manifest,
+									   XLogRecPtr startptr,
+									   TimeLineID starttli, XLogRecPtr endptr,
+									   TimeLineID endtli);
+extern void SendBackupManifest(backup_manifest_info *manifest);
 
 #endif
-- 
2.24.2 (Apple Git-127)