Обсуждение: relocating the server's backup manifest code
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
Вложения
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
Вложения
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
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
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.
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
Вложения
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