Re: relocating the server's backup manifest code

On Sat, Apr 18, 2020 at 8:57 AM Michael Paquier <michael(at)paquier(dot)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