Horton

Thread

Commits

  1. pg_combinebackup: Error if incremental file exists in full backup.

  2. pg_combinebackup: When reconstructing, avoid double slash in filename.

  1. small pg_combinebackup improvements

    Robert Haas <robertmhaas@gmail.com> — 2024-10-30T19:50:53Z

    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.

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.

Review appreciated. My plan is to commit at least to master and
possibly back-patch. Opinions on whether to back-patch are also
appreciated.

Thanks,

-- 
Robert Haas
EDB: http://www.enterprisedb.com

  2. Re: small pg_combinebackup improvements

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2024-10-31T15:41:55Z

    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

  3. Re: small pg_combinebackup improvements

    Robert Haas <robertmhaas@gmail.com> — 2024-10-31T16:06:25Z

    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

  4. Re: small pg_combinebackup improvements

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2024-11-01T08:01:34Z

    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

  5. Re: small pg_combinebackup improvements

    Amul Sul <sulamul@gmail.com> — 2024-11-04T04:40:01Z

    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

  6. Re: small pg_combinebackup improvements

    Robert Haas <robertmhaas@gmail.com> — 2024-11-04T15:26:41Z

    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