Re: backup manifests

Thanks, Robert.

1: Getting below error while compiling 0002 patch.

edb(at)localhost:postgres$ mi > mi.log
basebackup.c: In function ‘AddFileToManifest’:
basebackup.c:1052:6: error: ‘pathname’ undeclared (first use in this
function)
pathname);
^
basebackup.c:1052:6: note: each undeclared identifier is reported only once
for each function it appears in
make[3]: *** [basebackup.o] Error 1
make[2]: *** [replication-recursive] Error 2
make[1]: *** [install-backend-recurse] Error 2
make: *** [install-src-recurse] Error 2

I can see you have renamed the filename argument of AddFileToManifest() to
pathname, but those changes are part of 0003 (validator patch).
I think the changes related to src/backend/replication/basebackup.c should
not be there in the validator patch (0003). We can move these changes to
backup manifest patch, either in 0002 or 0004 for better readability of
patch set.

2:

#define KW_MANIFEST_VERSION "PostgreSQL-Backup-Manifest-Version"
#define KW_MANIFEST_FILE "File"
#define KW_MANIFEST_CHECKSUM "Manifest-Checksum"
#define KWL_MANIFEST_VERSION (sizeof(KW_MANIFEST_VERSION)-1)
#define KWL_MANIFEST_FILE (sizeof(KW_MANIFEST_FILE)-1)
#define KWL_MANIFEST_CHECKSUM (sizeof(KW_MANIFEST_CHECKSUM)-1)

#define FIELDS_PER_FILE_LINE 4

Few macros defined in 0003 patch not used anywhere in 0005 patch. Either we
can replace these with hard-coded values or remove them.

On Thu, Mar 5, 2020 at 10:25 PM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> On Thu, Mar 5, 2020 at 7:05 AM tushar <tushar(dot)ahuja(at)enterprisedb(dot)com>
> wrote:
> > There is one small observation if we use slash (/) with option -i then
> not getting the desired result
>
> Here's an updated patch set responding to many of the comments
> received thus far. Since there are quite a few emails, let me
> consolidate my comments and responses here.
>
> Report: Segmentation fault if -m is used to point to a valid manifest,
> but actual backup directory is nonexistent.
> Response: Fixed; thanks for the report.
>
> Report: pg_validatebackup doesn't complain about problems within the
> pg_wal directory.
> Response: That's out of scope. The WAL files are fetched separately
> and are therefore not part of the manifest.
>
> Report: Inaccessible file in data directory being validated leads to a
> double free.
> Response: Fixed; thanks for the report.
>
> Report: Patch 0005 doesn't validate the manifest checksum.
> Response: I know. I mentioned that when posting the previous patch
> set. Fixed in this version, though.
>
> Report: Removing an empty directory doesn't make backup validation
> fail, even though it might cause problems for the server.
> Response: That's a little unfortunate, but I'm not sure it's really
> worth complicating the patch to deal with it. It's something of a
> corner case.
>
> Report: Negative file sizes in the backup manifest are interpreted as
> large integers.
> Response: That's also a little unfortunate, but I doubt it's worth
> adding code to catch it, since any such manifest is corrupt. Also,
> it's not like we're ignoring it; the error just isn't ideal.
>
> Report: If I take the backup label from backup #1 and stick it into
> otherwise-identical backup #2, validation succeeds but the server
> won't start.
> Response: That's because we can't validate the pg_wal directory. As
> noted above, that's out of scope.
>
> Report: Using --ignore with a slash-terminated pathname doesn't work
> as expected.
> Response: Fixed, thanks for the report.
>
> Off-List Report: You forgot a PG_BINARY flag.
> Response: Fixed. I thought I'd done this before but there were two
> places and I'd only fixed one of them.
>
> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>

--
--

Thanks & Regards,
Suraj kharage,
EnterpriseDB Corporation,
The Postgres Database Company.