Thread: Re: small pg_combinebackup improvements
Hi, On Wed, Oct 30, 2024 at 03:50:53PM -0400, Robert Haas wrote: > Hi, > > Here are two small patches to improve pg_combinebackup slightly. > > 0001: I noticed that there is some logic in reconstruct.c that > constructs a pathname of the form a/b//c instead of a/b/c. AFAICT, > this still works fine; it just looks ugly. It's possible to get one of > these pathnames to show up in an error message if you have a corrupted > backup, e.g.: > > pg_combinebackup: error: could not open file > "x/base/1//INCREMENTAL.6229": No such file or directory > pg_combinebackup: removing output directory "xy" > > So 0001 fixes this. Yeah, I don't think that was an issue per say but better to display a path of the form a/b/c (to not be "distracted" by the output at the first place). 0001 looks pretty straightforward and good to me. > 0002: If you try to do something like pg_combinebackup incr1 incr2 -o > result, you'll get an error saying that the first backup must be a > full backup. This is an implementation restriction that might be good > to lift, but right now that's how it is. However, if you do > pg_combinebackup full incr -o result, but the full backup happens to > contain an INCREMENTAL.* file that also exists in the incremental > backup, then you won't get an error. Instead you'll reconstruct a > completely bogus full file and write it to the result directory. Now, > this should really be impossible unless you're intentionally trying to > break pg_combinebackup, but it's also pretty lame, so 0002 fixes > things so that you get a proper error, and also adds a test case. > 1 === + * If we have only incremental files, and there's no full file at any + * point in the backup chain, something has gone wrong. Emit an error. + * * Otherwise, reconstruct. */ if (copy_source != NULL) copy_file(copy_source->filename, output_filename, &checksum_ctx, copy_method, dry_run); + else if (sidx == 0 && source[0]->header_length != 0) + { + pg_fatal("full backup contains unexpected incremental file \"%s\"", + source[0]->filename); + } What about moving the new comment just before the new "else if"? 2 === + copy("$backup2path/base/1/$iname", "$backup1path/base/1/$iname") Yeah, "copy" is needed. I tested to "just" create an empty incremental file and got: $ pg_combinebackup backup1 backup2 -o restored_full pg_combinebackup: error: could not read file "backup1/base/1/INCREMENTAL.6113": read 0 of 4 Which is not the error we want to produce. 3 === + qr/full backup contains unexpected incremental file/, + "pg_combinebackup fails"); s/pg_combinebackup fails/pg_combinebackup fails due to an unexpected incremental file/? > Review appreciated. My plan is to commit at least to master and > possibly back-patch. Opinions on whether to back-patch are also > appreciated. I'm not sure about 0001 but I think 0002 deserves a back patch as I think it fits into the "low-risk fixes" category [0]. [0]: https://www.postgresql.org/support/versioning/ Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
On Thu, Oct 31, 2024 at 11:41 AM Bertrand Drouvot <bertranddrouvot.pg@gmail.com> wrote: > 0001 looks pretty straightforward and good to me. Thanks for the review. > What about moving the new comment just before the new "else if"? Well, the block comment applies to the whole if-else if-else construction. If we get too many else-ifs here it may need restructuring, but I don't think it needs that now. > Yeah, "copy" is needed. I tested to "just" create an empty incremental file > and got: > > $ pg_combinebackup backup1 backup2 -o restored_full > pg_combinebackup: error: could not read file "backup1/base/1/INCREMENTAL.6113": read 0 of 4 > > Which is not the error we want to produce. Right, it needs to look like a valid incremental file. > s/pg_combinebackup fails/pg_combinebackup fails due to an unexpected incremental file/? OK > I'm not sure about 0001 but I think 0002 deserves a back patch as I think it fits > into the "low-risk fixes" category [0]. I'm inclined to back-patch both, then. We might have more small fixes and they'll be less risky to back-patch if we back-patch them all. -- Robert Haas EDB: http://www.enterprisedb.com
Hi, On Thu, Oct 31, 2024 at 12:06:25PM -0400, Robert Haas wrote: > On Thu, Oct 31, 2024 at 11:41 AM Bertrand Drouvot > <bertranddrouvot.pg@gmail.com> wrote: > > I'm not sure about 0001 but I think 0002 deserves a back patch as I think it fits > > into the "low-risk fixes" category [0]. > > I'm inclined to back-patch both, then. We might have more small fixes > and they'll be less risky to back-patch if we back-patch them all. Yeah, that makes fully sense. +1 to back-patch both then. Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
On Fri, Nov 1, 2024 at 1:31 PM Bertrand Drouvot <bertranddrouvot.pg@gmail.com> wrote: > > Hi, > > On Thu, Oct 31, 2024 at 12:06:25PM -0400, Robert Haas wrote: > > On Thu, Oct 31, 2024 at 11:41 AM Bertrand Drouvot > > <bertranddrouvot.pg@gmail.com> wrote: > > > I'm not sure about 0001 but I think 0002 deserves a back patch as I think it fits > > > into the "low-risk fixes" category [0]. > > > > I'm inclined to back-patch both, then. We might have more small fixes > > and they'll be less risky to back-patch if we back-patch them all. > > Yeah, that makes fully sense. +1 to back-patch both then. > +1 for the back-patching. For 0002, I think we could report the error a bit earlier — the better place might be in the else part of the following IF-block, IMO: /* * If s->header_length == 0, then this is a full file; otherwise, it's * an incremental file. */ if (s->header_length == 0) Regards, Amul
On Sun, Nov 3, 2024 at 11:40 PM Amul Sul <sulamul@gmail.com> wrote: > +1 for the back-patching. > > For 0002, I think we could report the error a bit earlier — the better > place might be in the else part of the following IF-block, IMO: > > /* > * If s->header_length == 0, then this is a full file; otherwise, it's > * an incremental file. > */ > if (s->header_length == 0) We could, but I have some follow-up patches adding some new functionality which I plan to post soon, and for those patches, it turns out to be more convenient to have the error check where I put it. So I'm sticking with that placement. Committed and back-patched both to v17; thanks to both of you for the review. -- Robert Haas EDB: http://www.enterprisedb.com