Re: pg_combinebackup does not detect missing files

From: Robert Haas <robertmhaas(at)gmail(dot)com>
To: David Steele <david(at)pgmasters(dot)net>
Cc: Pg Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: pg_combinebackup does not detect missing files
Date: 2024-04-17 15:03:55
Message-ID: CA+TgmoZmuhkjGx=97kEniMRNC9Qaifjh0zMFZnHne72JGFNdKA@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On Tue, Apr 16, 2024 at 7:25 PM David Steele <david(at)pgmasters(dot)net> wrote:
> Except that we are running pg_combinebackup on the incremental, which
> the user might reasonably expect to check backup integrity. It actually
> does a bunch of integrity checks -- but not this one.

I think it's a bad idea to duplicate all of the checks from
pg_verifybackup into pg_combinebackup. I did consider this exact issue
during development. These are separate tools with separate purposes.
I think that what a user should expect is that each tool has some job
and tries to do that job well, while leaving other jobs to other
tools.

And I think if you think about it that way, it explains a lot about
which checks pg_combinebackup does and which checks it does not do.
pg_combinebackup tries to check that it is valid to combine backup A
with backup B (and maybe C, D, E ...), and it checks a lot of stuff to
try to make sure that such an operation appears to be sensible. Those
are checks that pg_verifybackup cannot do, because pg_verifybackup
only looks at one backup in isolation. If pg_combinebackup does not do
those checks, nobody does. Aside from that, it will also report errors
that it can't avoid noticing, even if those are things that
pg_verifybackup would also have found, such as, say, the control file
not existing.

But it will not go out of its way to perform checks that are unrelated
to its documented purpose. And I don't think it should, especially if
we have another tool that already does that.

> > I'm not averse to having some kind of statement in the documentation
> > along the lines of "Note that pg_combinebackup does not attempt to
> > verify that the individual backups are intact; for that, use
> > pg_verifybackup."
>
> I think we should do this at a minimum.

Here is a patch to do that.

> Especially given how pg_combinebackup works, backups are going to
> undergo a lot of user manipulation (pushing to and pull from storage,
> decompressing, untaring, etc.) and I think that means we should take
> extra care.

We are in agreement on that point, if nothing else. I am terrified of
users having problems with pg_combinebackup and me not being able to
tell whether those problems are due to user error, Robert error, or
something else. I put a lot of effort into detecting dumb things that
I thought a user might do, and a lot of effort into debugging output
so that when things do go wrong anyway, we have a reasonable chance of
figuring out exactly where they went wrong. We do seem to have a
philosophical difference about what the scope of those checks ought to
be, and I don't really expect what I wrote above to convince you that
my position is correct, but perhaps it will convince you that I have a
thoughtful position, as opposed to just having done stuff at random.

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

Attachment Content-Type Size
v1-0001-docs-Mention-that-pg_combinebackup-does-not-verif.patch application/octet-stream 2.1 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Pavel Borisov 2024-04-17 15:41:10 Re: [PATCH] Improve amcheck to also check UNIQUE constraint in btree index.
Previous Message Jelte Fennema-Nio 2024-04-17 14:51:49 Re: Speed up clean meson builds by ~25%