Re: small pg_combinebackup improvements

On Thu, Oct 31, 2024 at 11:41 AM Bertrand Drouvot
<bertranddrouvot(dot)pg(at)gmail(dot)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