On Wed, Sep 25, 2019 at 6:17 PM vignesh C <[email protected]> wrote:
> On Sat, Sep 21, 2019 at 12:25 AM Robert Haas <[email protected]>
> 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
diff --git a/src/backend/replication/basebackup.c b/src/backend/replication/basebackup.c
index 5a2a7fc..990d758 100644
--- a/src/backend/replication/basebackup.c
+++ b/src/backend/replication/basebackup.c
@@ -903,6 +903,7 @@ AddFileToManifest(StringInfo manifest, const char *tsoid,
static char timebuf[128];
static char shatextbuf[PG_SHA256_DIGEST_LENGTH * 2 + 1];
int shatextlen;
+ struct pg_tm *tm;
/*
* If this file is part of a tablespace, the filename passed to this
@@ -924,8 +925,10 @@ AddFileToManifest(StringInfo manifest, const char *tsoid,
* 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));
+ tm = pg_gmtime(&mtime);
+ if (tm == NULL)
+ elog(ERROR, "could not convert epoch to timestamp: %m");
+ pg_strftime(timebuf, sizeof(timebuf), "%Y-%m-%d %H:%M:%S %Z", tm);
/* Convert checksum to hexadecimal. */
shatextlen =
diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index 2abe632..60d3a53 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -1200,7 +1200,7 @@ ReceiveTarFile(PGconn *conn, PGresult *res, int rownum)
initPQExpBuffer(&buf);
ReceiveBackupManifestInMemory(conn, &buf);
- if (PQExpBufferBroken(&buf))
+ if (PQExpBufferDataBroken(buf))
{
pg_log_error("out of memory");
exit(1);
