Subject: Re: trying again to get incremental backup

From: David Steele <[email-masked]>
Date: Mon, 23 Oct 2023 19:56:51 -0400

On 10/23/23 11:44, Robert Haas wrote:
> On Fri, Oct 20, 2023 at 11:30 AM David Steele <[email-masked]> wrote:
>>
>> I don't plan to stand in your way on this feature. I'm reviewing what
>> patches I can out of courtesy and to be sure that nothing adjacent to
>> your work is being affected. My apologies if my reviews are not meeting
>> your expectations, but I am contributing as my time constraints allow.
> 
> Sorry, I realize reading this response that I probably didn't do a
> very good job writing that email and came across sounding like a jerk.
> Possibly, I actually am a jerk. Whether it just sounded like it or I
> actually am, I apologize. 

That was the way it came across, though I prefer to think it was 
unintentional. I certainly understand how frustrating dealing with a 
large and uncertain patch can be. Either way, I appreciate the apology.

Now onward...

> But your last paragraph here gets at my real
> question, which is whether you were going to try to block the feature.
> I recognize that we have different priorities when it comes to what
> would make most sense to implement in PostgreSQL, and that's OK, or at
> least, it's OK with me. 

This seem perfectly natural to me.

> I also don't have any particular expectation
> about how much you should review the patch or in what level of detail,
> and I have sincerely appreciated your feedback thus far. If you are
> able to continue to provide more, that's great, and if that's not,
> well, you're not obligated. What I was concerned about was whether
> that review was a precursor to a vigorous attempt to keep the main
> patch from getting committed, because if that was going to be the
> case, then I'd like to surface that conflict sooner rather than later.
> It sounds like that's not an issue, which is great.

Overall I would say I'm not strongly for or against the patch. I think 
it will be very difficult to use in a manual fashion, but automation is 
they way to go in general so that's not necessarily and argument against.

However, this is an area of great interest to me so I do want to at 
least make sure nothing is being impacted adjacent to the main goal of 
this patch. So far I have seen no sign of that, but that has been a 
primary goal of my reviews.

> At the risk of drifting into the fraught question of what I *should*
> be implementing rather than the hopefully-less-fraught question of
> whether what I am actually implementing is any good, I see incremental
> backup as a way of removing some of the use cases for the low-level
> backup API. If you said "but people still will have lots of reasons to
> use it," I would agree; and if you said "people can still screw things
> up with pg_basebackup," I would also agree. Nonetheless, most of the
> disasters I've personally seen have stemmed from the use of the
> low-level API rather than from the use of pg_basebackup, though there
> are exceptions. 

This all makes sense to me.

> I also think a lot of the use of the low-level API is
> driven by it being just too darn slow to copy the whole database, and
> incremental backup can help with that in some circumstances. 

I would argue that restore performance is *more* important than backup 
performance and this patch is a step backward in that regard. Backups 
will be faster and less space will be used in the repository, but 
restore performance is going to suffer. If the deltas are very small the 
difference will probably be negligible, but as the deltas get large (and 
especially if there are a lot of them) the penalty will be more noticeable.

> Also, I
> have worked fairly hard to try to make sure that if you misuse
> pg_combinebackup, or fail to use it altogether, you'll get an error
> rather than silent data corruption. I would be interested to hear
> about scenarios where the checks that I've implemented can be defeated
> by something that is plausibly described as stupidity rather than
> malice. I'm not sure we can fix all such cases, but I'm very alert to
> the horror that will befall me if user error looks exactly like a bug
> in the code. For my own sanity, we have to be able to distinguish
> those cases. 

I was concerned with the difficulty of trying to stage the correct 
backups for pg_combinebackup, not whether it would recognize that the 
needed data was not available and then error appropriately. The latter 
is surmountable within pg_combinebackup but the former is left up to the 
user.

> Moreover, we also need to be able to distinguish
> backup-time bugs from reassembly-time bugs, which is why I've got the
> pg_walsummary tool, and why pg_combinebackup has the ability to emit
> fairly detailed debugging output. I anticipate those things being
> useful in investigating bug reports when they show up. I won't be too
> surprised if it turns out that more work on sanity-checking and/or
> debugging tools is needed, but I think your concern about people
> misusing stuff is bang on target and I really want to do whatever we
> can to avoid that when possible and detect it when it happens.

The ability of users to misuse tools is, of course, legendary, so that 
all sounds good to me.

One note regarding the patches. I feel like 
v5-0005-Prototype-patch-for-incremental-backup should be split to have 
the WAL summarizer as one patch and the changes to base backup as a 
separate patch.

It might not be useful to commit one without the other, but it would 
make for an easier read. Just my 2c.

Regards,
-David

---

From: Robert Haas <[email-masked]>
Date: Tue, 24 Oct 2023 08:29:49 -0400

> especially if there are a lot of them) the penalty will be more noticeable.

I think an awful lot depends here on whether the repository is local
or remote. If you have filesystem access to wherever the backups are
stored anyway, I don't think that using pg_combinebackup to write out
a new data directory is going to be much slower than copying one data
directory from the repository to wherever you'd actually use the
backup. It may be somewhat slower because we do need to access some
data in every involved backup, but I don't think it should be vastly
slower because we don't have to read every backup in its entirety. For
each file, we read the (small) header of the newest incremental file
and every incremental file that precedes it until we find a full file.
Then, we construct a map of which blocks need to be read from which
sources and read only the required blocks from each source. If all the
blocks are coming from a single file (because there are no incremental
for a certain file, or they contain no blocks) then we just copy the
entire source file in one shot, which can be optimized using the same
tricks we use elsewhere. Inevitably, this is going to read more data
and do more random I/O than just a flat copy of a directory, but it's
not terrible. The overall amount of I/O should be a lot closer to the
size of the output directory than to the sum of the sizes of the input
directories.

Now, if the repository is remote, and you have to download all of
those backups first, and then run pg_combinebackup on them afterward,
that is going to be unpleasant, unless the incremental backups are all
quite small. Possibly this could be addressed by teaching
pg_combinebackup to do things like accessing data over HTTP and SSH,
and relatedly, looking inside tarfiles without needing them unpacked.
For now, I've left those as ideas for future improvement, but I think
potentially they could address some of your concerns here. A
difficulty is that there are a lot of protocols that people might want
to use to push bytes around, and it might be hard to keep up with the
march of progress.

I do agree, though, that there's no such thing as a free lunch. I
wouldn't recommend to anyone that they plan to restore from a chain of
100 incremental backups. Not only might it be slow, but the
opportunities for something to go wrong are magnified. Even if you've
automated everything well enough that there's no human error involved,
what if you've got a corrupted file somewhere? Maybe that's not likely
in absolute terms, but the more files you've got, the more likely it
becomes. What I'd suggest someone do instead is periodically do
pg_combinebackup full_reference_backup oldest_incremental -o
new_full_reference_backup; rm -rf full_reference_backup; mv
new_full_reference_backup full_reference_backup. The new full
reference backup is intended to still be usable for restoring
incrementals based on the incremental it replaced. I hope that, if
people use the feature well, this should limit the need for really
long backup chains. I am sure, though, that some people will use it
poorly. Maybe there's room for more documentation on this topic.

> is surmountable within pg_combinebackup but the former is left up to the

Indeed.

> make for an easier read. Just my 2c.

Yeah, maybe so. I'm not quite ready to commit to doing that split as
of this writing but I will think about it and possibly do it.

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

---

From: Robert Haas <[email-masked]>
Date: Wed, 25 Oct 2023 13:38:25 -0400

> of this writing but I will think about it and possibly do it.

I have done this. Here's v7.

This version also includes several new TAP tests for the main patch,
some of which were inspired by our discussion. It also includes SGML
documentation for pg_walsummary.

New tests:
003_timeline.pl tests the case where the prior backup for an
incremental backup was taken on an earlier timeline.
004_manifest.pl tests the manifest-related options for pg_combinebackup.
005_integrity.pl tests the sanity checks that prevent combining a
backup with the wrong prior backup.

Overview of the new organization of the patch set:
0001 - preparatory refactoring of basebackup.c, changing the algorithm
that we use to decide which files have checksums
0002 - code movement only. makes it possible to reuse parse_manifest.c
0003 - add the WAL summarizer process, but useless on its own
0004 - add incremental backup, making use of 0003
0005 - add pg_walsummary debugging tool

Notes:
- I suspect that 0003 is the most likely to have serious bugs, followed by 0004.
- See XXX comments in the commit messages for some known open issues.
- Still looking for more comments on
http://postgr.es/m/[email-masked]
and other recent emails where design questions came up