Re: backup manifests - Mailing list pgsql-hackers
| From | Suraj Kharage |
|---|---|
| Subject | Re: backup manifests |
| Date | |
| Msg-id | CAF1DzPWVhMUxZkiMJ+ffR7dXqdz_HhuYAswZrYrxY1KyCoeVrg@mail.gmail.com Whole thread Raw |
| In response to | Re: backup manifests (Robert Haas <robertmhaas@gmail.com>) |
| Responses |
Re: backup manifests
(Robert Haas <robertmhaas@gmail.com>)
|
| List | pgsql-hackers |
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.
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 <robertmhaas@gmail.com> wrote:
On Thu, Mar 5, 2020 at 7:05 AM tushar <tushar.ahuja@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.
pgsql-hackers by date: