Re: backup manifests

On Wed, Sep 25, 2019 at 6:17 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:

> On Sat, Sep 21, 2019 at 12:25 AM Robert Haas <robertmhaas(at)gmail(dot)com>
> wrote:
> >
> Some comments:
>
> Manifest file will be in plain text format even if compression is
> specified, should we compress it?
> May be this is intended, just raised the point to make sure that it is
> intended.
> +static void
> +ReceiveBackupManifestChunk(size_t r, char *copybuf, void *callback_data)
> +{
> + WriteManifestState *state = callback_data;
> +
> + if (fwrite(copybuf, r, 1, state->file) != 1)
> + {
> + pg_log_error("could not write to file \"%s\": %m", state->filename);
> + exit(1);
> + }
> +}
>
> WALfile.done file gets added but wal file information is not included
> in the manifest file, should we include WAL file also?
> @@ -599,16 +618,20 @@ perform_base_backup(basebackup_options *opt)
> (errcode_for_file_access(),
> errmsg("could not stat file \"%s\": %m", pathbuf)));
>
> - sendFile(pathbuf, pathbuf, &statbuf, false, InvalidOid);
> + sendFile(pathbuf, pathbuf, &statbuf, false, InvalidOid, manifest,
> + NULL);
>
> /* unconditionally mark file as archived */
> StatusFilePath(pathbuf, fname, ".done");
> - sendFileWithContent(pathbuf, "");
> + sendFileWithContent(pathbuf, "", manifest);
>
> Should we add an option to make manifest generation configurable to
> reduce overhead during backup?
>
> Manifest file does not include directory information, should we include it?
>
> There is one warning:
> In file included from ../../../src/include/fe_utils/string_utils.h:20:0,
> from pg_basebackup.c:34:
> pg_basebackup.c: In function ‘ReceiveTarFile’:
> ../../../src/interfaces/libpq/pqexpbuffer.h:60:9: warning: the
> comparison will always evaluate as ‘false’ for the address of ‘buf’
> will never be NULL [-Waddress]
> ((str) == NULL || (str)->maxlen == 0)
> ^
> pg_basebackup.c:1203:7: note: in expansion of macro ‘PQExpBufferBroken’
> if (PQExpBufferBroken(&buf))
>
>
I also observed this warning. PFA to fix the same.

pg_gmtime can fail in case of malloc failure:
> + /*
> + * Convert time to a string. Since it's not clear what time zone to use
> + * and since time zone definitions can change, possibly causing confusion,
> + * use GMT always.
> + */
> + pg_strftime(timebuf, sizeof(timebuf), "%Y-%m-%d %H:%M:%S %Z",
> + pg_gmtime(&mtime));
>
>
Fixed that into attached patch.

Regards.
Rushabh Lathia
www.EnterpriseDB.com