Re: Add notes to pg_combinebackup docs

On Fri, Apr 12, 2024 at 3:01 PM Tomas Vondra <tomas(dot)vondra(at)enterprisedb(dot)com>
wrote:

>
>
> On 4/12/24 11:12, Magnus Hagander wrote:
> > On Tue, Apr 9, 2024 at 11:46 AM Tomas Vondra <
> tomas(dot)vondra(at)enterprisedb(dot)com>
> > wrote:
> >
> >>
> >>
> >> On 4/9/24 09:59, Martín Marqués wrote:
> >>> Hello,
> >>>
> >>> While doing some work/research on the new incremental backup feature
> >>> some limitations were not listed in the docs. Mainly the fact that
> >>> pg_combienbackup works with plain format and not tar.
> >>>
> >>
> >> Right. The docs mostly imply this by talking about output directory and
> >> backup directories, but making it more explicit would not hurt.
> >>
> >> FWIW it'd be great if we could make incremental backups work with tar
> >> format in the future too. People probably don't want to keep around the
> >> expanded data directory or extract everything before combining the
> >> backups is not very convenient. Reading and writing the tar would make
> >> this simpler.
> >>
> >>> Around the same time, Tomas Vondra tested incremental backups with a
> >>> cluster where he enabled checksums after taking the previous full
> >>> backup. After combining the backups the synthetic backup had pages
> >>> with checksums and other pages without checksums which ended in
> >>> checksum errors.
> >>>
> >>
> >> I'm not sure just documenting this limitation is sufficient. We can't
> >> make the incremental backups work in this case (it's as if someone
> >> messes with cluster without writing stuff into WAL), but I think we
> >> should do better than silently producing (seemingly) corrupted backups
> >
> >
> > +1. I think that should be an open item that needs to get sorted.
> >
> >
> > I say seemingly, because the backup is actually fine, the only problem
> >> is it has checksums enabled in the controlfile, but the pages from the
> >> full backup (and the early incremental backups) have no checksums.
> >>
> >> What we could do is detect this in pg_combinebackup, and either just
> >> disable checksums with a warning and hint to maybe enable them again. Or
> >> maybe just print that the user needs to disable them.
> >>
> >
> > I don't think either of these should be done automatically. Something
> like
> > pg_combinebackup simply failing and requiring you to say
> > "--disable-checksums" to have it do that would be the way to go, IMO.
> > (once we can reliably detect it of course)
> >
>
> You mean pg_combinebackup would have "--disable-checksums" switch? Yeah,
> that'd work, I think. It's probably better than producing a backup that
> would seem broken when the user tries to start the instance.
>
> Also, I realized the user probably can't disable the checksums without
> starting the instance to finish recovery. And if there are invalid
> checksums, I'm not sure that would actually work.
>
> >
> > I was thinking maybe we could detect this while taking the backups, and
> >> force taking a full backup if checksums got enabled since the last
> >> backup. But we can't do that because we only have the manifest from the
> >> last backup, and the manifest does not include info about checksums.
> >>
> >
> > Can we forcibly read and parse it out of pg_control?
> >
>
> You mean when taking the backup, or during pg_combinebackup?
>

Yes. That way combining the backups into something that doesn't have proper
checksums (either by actually turning them off or as today just breaking
them and forcing you to turn it off yourself) can only happen
intentionally. And if you weren't aware of the problem, it turns into a
hard error, so you will notice before it's too late.

During backup, it depends. For the instance we should be able to just
> get that from the instance, no need to get it from pg_control. But for
> the backup (used as a baseline for the increment) we can't read the
> pg_control - the only thing we have is the manifest.

> During pg_combinebackup we obviously can read pg_control for all the
> backups to combine, but at that point it feels a bit too late - it does
> not seem great to do backups, and then at recovery to tell the user the
> backups are actually not valid.

> I think it'd be better to detect this while taking the basebackup.
>

Agreed. In the end, we might want to do *both*, but the earlier the better.

But to do that what we'd need is to add a flag to the initial manifest that
says "this cluster is supposed to have checksum = <on/off>" and then refuse
to take an inc if it changes? It doesn't seem like the end of the world to
add that to it?

> > It's a bit unfortunate we don't have a way to enable checksums online.
> >> That'd not have this problem IIRC, because it writes proper WAL. Maybe
> >> it's time to revive that idea ... I recall there were some concerns
> >> about tracking progress to allow resuming stuff, but maybe not having
> >> anything because in some (rare?) cases it'd need to do more work does
> >> not seem like a great trade off.
> >>
> >>
> > For that one I still think it would be perfectly acceptable to have no
> > resume at all, but that's a whole different topic :)
> >
>
> I very much agree.
>
> It seems a bit strange that given three options to enable checksums
>
> 1) offline
> 2) online without resume
> 3) online with resume
>
> the initial argument was that we need to allow resuming the process
> because on large systems it might take a lot of time, and we'd lose all
> the work if the system restarts. But then we concluded that it's too
> complex and it's better if the large systems have to do an extended
> outage to enable checksums ...
>

Indeed. Or the workaround that still scares the crap out of me where you
use a switchover-and-switchback to a replica to "do the offline thing
almost online". To me that seems a lot scarier than the original option as
well.

--
Magnus Hagander
Me: https://www.hagander.net/ <http://www.hagander.net/>
Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/>