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