Re: relocating the server's backup manifest code - Mailing list pgsql-hackers

From Michael Paquier
Subject Re: relocating the server's backup manifest code
Date
Msg-id 20200418125713.GG350229@paquier.xyz
Whole thread Raw
In response to relocating the server's backup manifest code  (Robert Haas <robertmhaas@gmail.com>)
Responses Re: relocating the server's backup manifest code  (Robert Haas <robertmhaas@gmail.com>)
List pgsql-hackers
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

Attachment

pgsql-hackers by date:

Previous
From: Erik Rijkers
Date:
Subject: Re: PATCH: logical_work_mem and logical streaming of largein-progress transactions
Next
From: Andrew Dunstan
Date:
Subject: valgrind error