Thanks, Robert. 1: Getting below error while compiling 0002 patch.
edb@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 <robertmh...@gmail.com> wrote: > On Thu, Mar 5, 2020 at 7:05 AM tushar <tushar.ah...@enterprisedb.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.