Subject: Re: trying again to get incremental backup

From: Robert Haas <[email-masked]>
Date: Thu, 7 Dec 2023 10:14:45 -0500

On Thu, Dec 7, 2023 at 9:42 AM Jakub Wartak
<[email-masked]> wrote:
>     Comment: I was wondering if it wouldn't make some sense to teach
> pg_resetwal to actually delete all WAL summaries after any any
> WAL/controlfile alteration?

I thought that this was a good idea so I decided to go implement it,
only to discover that it was already part of the patch set ... did you
find some case where it doesn't work as expected? The code looks like
this:

    RewriteControlFile();
    KillExistingXLOG();
    KillExistingArchiveStatus();
    KillExistingWALSummaries();
    WriteEmptyXLOG();

> test_pending_2pc.sh - getting GOOD on most recent runs, but several
> times during early testing (probably due to my own mishaps), I've been
> hit by Abort/TRAP. I'm still investigating and trying to reproduce
> those ones. TRAP: failed Assert("summary_end_lsn >=
> WalSummarizerCtl->pending_lsn"), File: "walsummarizer.c", Line: 940

I have a fix for this locally, but I'm going to hold off on publishing
a new version until either there's a few more things I can address all
at once, or until Thomas commits the ubsan fix.

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

---

From: Jakub Wartak <[email-masked]>
Date: Fri, 8 Dec 2023 11:02:11 +0100

On Thu, Dec 7, 2023 at 4:15 PM Robert Haas <[email-masked]> wrote:

Hi Robert,

> find some case where it doesn't work as expected? The code looks like

Ah, my bad, with a fresh mind and coffee the error message makes it
clear and of course it did reset the summaries properly.

While we are at it, maybe around the below in PrepareForIncrementalBackup()

                if (tlep[i] == NULL)
                        ereport(ERROR,

(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
                                         errmsg("timeline %u found in
manifest, but not in this server's history",
                                                        range->tli)));

we could add

    errhint("You might need to start a new full backup instead of
incremental one")

?

> at once, or until Thomas commits the ubsan fix.

Great, I cannot get it to fail again today, it had to be some dirty
state of the testing env. BTW: Thomas has pushed that ubsan fix.

-J.

---

From: Robert Haas <[email-masked]>
Date: Mon, 11 Dec 2023 12:08:20 -0500

>     errhint("You might need to start a new full backup instead of

I can't exactly say that such a hint would be inaccurate, but I think
the impulse to add it here is misguided. One of my design goals for
this system is to make it so that you never have to take a new
incremental backup "just because," not even in case of an intervening
timeline switch. So, all of the errors in this function are warning
you that you've done something that you really should not have done.
In this particular case, you've either (1) manually removed the
timeline history file, and not just any timeline history file but the
one for a timeline for a backup that you still intend to use as the
basis for taking an incremental backup or (2) tried to use a full
backup taken from one server as the basis for an incremental backup on
a completely different server that happens to share the same system
identifier, e.g. because you promoted two standbys derived from the
same original primary and then tried to use a full backup taken on one
as the basis for an incremental backup taken on the other.

The scenario I was really concerned about when I wrote this test was
(2), because that could lead to a corrupt restore. This test isn't
strong enough to prevent that completely, because two unrelated
standbys can branch onto the same new timelines at the same LSNs, and
then these checks can't tell that something bad has happened. However,
they can detect a useful subset of problem cases. And the solution is
not so much "take a new full backup" as "keep straight which server is
which." Likewise, in case (1), the relevant hint would be "don't
manually remove timeline history files, and if you must, then at least
don't nuke timelines that you actually still care about."

> state of the testing env. BTW: Thomas has pushed that ubsan fix.

Huzzah, the cfbot likes the patch set now. Here's a new version with
the promised fix for your non-reproducible issue. Let's see whether
you and cfbot still like this version.

---

From: Jakub Wartak <[email-masked]>
Date: Wed, 13 Dec 2023 11:39:22 +0100

Hi Robert,

On Mon, Dec 11, 2023 at 6:08 PM Robert Haas <[email-masked]> wrote:
> incremental backup "just because,"

Did you mean take a new full backup here?

> not even in case of an intervening

Okay, but please consider two other possibilities:

(3) I had a corrupted DB where I've fixed it by running pg_resetwal
and some cronjob just a day later attempted to take incremental and
failed with that error.

(4) I had pg_upgraded (which calls pg_resetwal on fresh initdb
directory) the DB where I had cronjob that just failed with this error

I bet that (4) is going to happen more often than (1), (2) , which
might trigger users to complain on forums, support tickets.

> you and cfbot still like this version.

LGTM, all quick tests work from my end too. BTW: I have also scheduled
the long/large pgbench -s 14000 (~200GB?) - multiple day incremental
test. I'll let you know how it went.

-J.

---

From: Robert Haas <[email-masked]>
Date: Wed, 13 Dec 2023 08:16:20 -0500

> Did you mean take a new full backup here?

Yes, apologies for the typo.

> might trigger users to complain on forums, support tickets.

Hmm. In case (4), I was thinking that you'd get a complaint about the
database system identifier not matching. I'm not actually sure that's
what would happen, though, now that you mention it.

In case (3), I think you would get an error about missing WAL summary files.

> test. I'll let you know how it went.

Awesome, thank you so much.

---

From: Jakub Wartak <[email-masked]>
Date: Fri, 15 Dec 2023 11:36:26 +0100

Hi Robert,

> what would happen, though, now that you mention it.

I've played with with initdb/pg_upgrade (17->17) and i don't get DBID
mismatch (of course they do differ after initdb), but i get this
instead:

 $ pg_basebackup -c fast -D /tmp/incr2.after.upgrade -p 5432
--incremental /tmp/incr1.before.upgrade/backup_manifest
WARNING:  aborting backup due to backend exiting before pg_backup_stop
was called
pg_basebackup: error: could not initiate base backup: ERROR:  timeline
2 found in manifest, but not in this server's history
pg_basebackup: removing data directory "/tmp/incr2.after.upgrade"

Also in the manifest I don't see DBID ?
Maybe it's a nuisance and all I'm trying to see is that if an
automated cronjob with pg_basebackup --incremental hits a freshly
upgraded cluster, that error message without errhint() is going to
scare some Junior DBAs.

> Awesome, thank you so much.

OK, so pgbench -i -s 14440 and pgbench -P 1 -R 100 -c 8 -T 259200 did
generate pretty large incrementals (so I had to abort it due to lack
of space, I was expecting to see smaller incrementals so it took too
much space). I initally suspected that the problem lies in the normal
distribution of `\set aid random(1, 100000 * :scale)` for tpcbb that
UPDATEs on big pgbench_accounts.

$ du -sm /backups/backups/* /backups/archive/
216205  /backups/backups/full
215207  /backups/backups/incr.1
216706  /backups/backups/incr.2
102273  /backups/archive/

So I verified the recoverability yesterday anyway - the
pg_combinebackup "full incr.1 incr.2" took 44 minutes and later
archive wal recovery and promotion SUCCEED. The 8-way parallel seqscan
foir sum(abalance) on the pgbench_accounts and other tables worked
fine. The pg_combinebackup was using 15-20% CPU (mostly on %sys),
while performing mostly 60-80MB/s separately for both reads and writes
(it's slow, but it's due to maxed out sequence I/O of the Premium on a
small SSD on Azure).

So i've launched another improved test (to force more localized
UPDATEs) to see the more real-world space-effectiveness of the
incremental backup:

\set aid random_exponential(1, 100000 * :scale, 8)
\set bid random(1, 1 * :scale)
\set tid random(1, 10 * :scale)
\set delta random(-5000, 5000)
BEGIN;
UPDATE pgbench_accounts SET abalance = abalance + :delta WHERE aid = :aid;
INSERT INTO pgbench_history (tid
, bid, aid, delta, mtime) VALUES (:tid, :bid, :aid, :delta, CURRENT_TIMESTAMP);
END;

But then... (and i have verified the low-IDs for :aid above).. same
has happened:

backups/backups$ du -sm /backups/backups/*
210229  /backups/backups/full
208299  /backups/backups/incr.1
208351  /backups/backups/incr.2

# pgbench_accounts has relfilenodeid 16486
postgres@jw-test-1:/backups/backups$ for L in 5 10 15 30 100 161 173
174 175  ; do md5sum full/base/5/16486.$L ./incr.1/base/5/16486.$L
./incr.2/base/5/16486.$L /var/lib/postgres/17/data/base/5/16486.$L ;
echo; done
005c6bbb40fca3c1a0a819376ef0e793  full/base/5/16486.5
005c6bbb40fca3c1a0a819376ef0e793  ./incr.1/base/5/16486.5
005c6bbb40fca3c1a0a819376ef0e793  ./incr.2/base/5/16486.5
005c6bbb40fca3c1a0a819376ef0e793  /var/lib/postgres/17/data/base/5/16486.5

[.. all the checksums match (!) for the above $L..]

c5117a213253035da5e5ee8a80c3ee3d  full/base/5/16486.173
c5117a213253035da5e5ee8a80c3ee3d  ./incr.1/base/5/16486.173
c5117a213253035da5e5ee8a80c3ee3d  ./incr.2/base/5/16486.173
c5117a213253035da5e5ee8a80c3ee3d  /var/lib/postgres/17/data/base/5/16486.173

47ee6b18d7f8e40352598d194b9a3c8a  full/base/5/16486.174
47ee6b18d7f8e40352598d194b9a3c8a  ./incr.1/base/5/16486.174
47ee6b18d7f8e40352598d194b9a3c8a  ./incr.2/base/5/16486.174
47ee6b18d7f8e40352598d194b9a3c8a  /var/lib/postgres/17/data/base/5/16486.174

82dfeba58b4a1031ac12c23f9559a330  full/base/5/16486.175
21a8ac1e6fef3cf0b34546c41d59b2cc  ./incr.1/base/5/16486.175
2c3d89c612b2f97d575a55c6c0204d0b  ./incr.2/base/5/16486.175
73367d44d76e98276d3a6bbc14bb31f1  /var/lib/postgres/17/data/base/5/16486.175

So to me, it looks like it copied anyway 174 out of 175 files lowering
the effectiveness of that incremental backup to 0% .The commands to
generate those incr backups were:
pg_basebackup -v -P -c fast -D /backups/backups/incr.1
--incremental=/backups/backups/full/backup_manifest
sleep 4h
pg_basebackup -v -P -c fast -D /backups/backups/incr.2
--incremental=/backups/backups/incr1/backup_manifest

The incrementals are being generated , but just for the first (0)
segment of the relation?

/backups/backups$ ls -l incr.2/base/5 | grep INCR
-rw------- 1 postgres postgres         12 Dec 14 21:33 INCREMENTAL.112
-rw------- 1 postgres postgres         12 Dec 14 21:01 INCREMENTAL.113
-rw------- 1 postgres postgres         12 Dec 14 21:36 INCREMENTAL.1247
-rw------- 1 postgres postgres         12 Dec 14 21:38 INCREMENTAL.1247_vm
[..note, no INCREMENTAL.$int.$segment files]
-rw------- 1 postgres postgres         12 Dec 14 21:24 INCREMENTAL.6238
-rw------- 1 postgres postgres         12 Dec 14 21:17 INCREMENTAL.6239
-rw------- 1 postgres postgres         12 Dec 14 21:55 INCREMENTAL.827

# 16486 is pgbench_accounts
/backups/backups$ ls -l incr.2/base/5/*16486* | grep INCR
-rw------- 1 postgres postgres   14613480 Dec 14 21:00
incr.2/base/5/INCREMENTAL.16486
-rw------- 1 postgres postgres         12 Dec 14 21:52
incr.2/base/5/INCREMENTAL.16486_vm
/backups/backups$

/backups/backups$ find incr* -name INCREMENTAL.* | wc -l
1342
/backups/backups$ find incr* -name INCREMENTAL.*_* | wc -l # VM or FSM
236
/backups/backups$ find incr* -name INCREMENTAL.*.* | wc -l # not a
single >1GB single incremental relation
0

I'm quickly passing info and I haven't really looked at the code yet ,
but it should be somewhere around GetFileBackupMethod() and
reproducible easily with that configure --with-segsize-blocks=X
switch.

-J.

---

From: Peter Eisentraut <[email-masked]>
Date: Fri, 15 Dec 2023 12:53:40 +0100

I have a couple of quick fixes here.

The first fixes up some things in nls.mk related to a file move.  The 
second is some cleanup because some function you are using has been 
removed in the meantime; you probably found that yourself while rebasing.

The pg_walsummary patch doesn't have a nls.mk, but you also comment that 
it doesn't have tests yet, so I assume it's not considered complete yet 
anyway.
From 04aae4ee91ddd1d4ce061c36a99b0fa18bdd98ec Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <[email-masked]>
Date: Thu, 14 Dec 2023 12:50:33 +0100
Subject: [PATCH 2/6] fixup! Move src/bin/pg_verifybackup/parse_manifest.c into
 src/common.

---
 src/bin/pg_verifybackup/nls.mk | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/bin/pg_verifybackup/nls.mk b/src/bin/pg_verifybackup/nls.mk
index eba73a2c05..9e6a6049ba 100644
--- a/src/bin/pg_verifybackup/nls.mk
+++ b/src/bin/pg_verifybackup/nls.mk
@@ -1,10 +1,10 @@
 # src/bin/pg_verifybackup/nls.mk
 CATALOG_NAME     = pg_verifybackup
 GETTEXT_FILES    = $(FRONTEND_COMMON_GETTEXT_FILES) \
-                   parse_manifest.c \
                    pg_verifybackup.c \
                    ../../common/fe_memutils.c \
-                   ../../common/jsonapi.c
+                   ../../common/jsonapi.c \
+                   ../../common/parse_manifest.c
 GETTEXT_TRIGGERS = $(FRONTEND_COMMON_GETTEXT_TRIGGERS) \
                    json_manifest_parse_failure:2 \
                    error_cb:2 \
-- 
2.43.0

From 25211044687a629e632ef0a2bfad30acea337266 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <[email-masked]>
Date: Thu, 14 Dec 2023 18:32:29 +0100
Subject: [PATCH 4/6] fixup! Add a new WAL summarizer process.

---
 src/backend/backup/meson.build         | 2 +-
 src/backend/postmaster/walsummarizer.c | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/backend/backup/meson.build b/src/backend/backup/meson.build
index 0e2de91e9f..5d4ebe3ebe 100644
--- a/src/backend/backup/meson.build
+++ b/src/backend/backup/meson.build
@@ -13,5 +13,5 @@ backend_sources += files(
   'basebackup_throttle.c',
   'basebackup_zstd.c',
   'walsummary.c',
-  'walsummaryfuncs.c'
+  'walsummaryfuncs.c',
 )
diff --git a/src/backend/postmaster/walsummarizer.c b/src/backend/postmaster/walsummarizer.c
index 7c840c36b3..9fa155349e 100644
--- a/src/backend/postmaster/walsummarizer.c
+++ b/src/backend/postmaster/walsummarizer.c
@@ -290,7 +290,7 @@ WalSummarizerMain(void)
 		FlushErrorState();
 
 		/* Flush any leaked data in the top-level context */
-		MemoryContextResetAndDeleteChildren(context);
+		MemoryContextReset(context);
 
 		/* Now we can allow interrupts again */
 		RESUME_INTERRUPTS();
@@ -342,7 +342,7 @@ WalSummarizerMain(void)
 		XLogRecPtr	end_of_summary_lsn;
 
 		/* Flush any leaked data in the top-level context */
-		MemoryContextResetAndDeleteChildren(context);
+		MemoryContextReset(context);
 
 		/* Process any signals received recently. */
 		HandleWalSummarizerInterrupts();
-- 
2.43.0

---

From: Peter Eisentraut <[email-masked]>
Date: Fri, 15 Dec 2023 12:58:17 +0100

A separate bikeshedding topic: The GUC "summarize_wal", could that be 
"wal_something" instead?  (wal_summarize? wal_summarizer?)  It would be 
nice if these settings names group together a bit, both with existing 
wal_* ones and also with the new ones you are adding 
(wal_summary_keep_time).

---

From: Peter Eisentraut <[email-masked]>
Date: Mon, 18 Dec 2023 10:10:51 +0100

Another set of comments, about the patch that adds pg_combinebackup:

Make sure all the options are listed in a consistent order.  We have 
lately changed everything to be alphabetical.  This includes:

- reference page pg_combinebackup.sgml

- long_options listing

- getopt_long() argument

- subsequent switch

- (--help output, but it looks ok as is)

Also, in pg_combinebackup.sgml, the option --sync-method is listed as if 
it does not take an argument, but it does.

---

From: Robert Haas <[email-masked]>
Date: Mon, 18 Dec 2023 13:27:42 -0500

> removed in the meantime; you probably found that yourself while rebasing.

Incorporated these. As you guessed,
MemoryContextResetAndDeleteChildren -> MemoryContextReset had already
been done locally.

> it doesn't have tests yet, so I assume it's not considered complete yet

I think this was more of a case of me just not realizing that I should
add that. I'll add something simple to the next version, but I'm not
very good at this NLS stuff.

---

From: Robert Haas <[email-masked]>
Date: Mon, 18 Dec 2023 13:39:33 -0500

> wal_* ones and also with the new ones you are adding

Yeah, this is highly debatable, so bikeshed away. IMHO, the question
here is whether we care more about (1) having the name of the GUC
sound nice grammatically or (2) having the GUC begin with the same
string as other, related GUCs. I think that Tom Lane tends to prefer
the former, and probably some other people do too, while some other
people tend to prefer the latter. Ideally it would be possible to
satisfy both goals at once here, but everything I thought about that
started with "wal" sounded too awkward for me to like it; hence the
current choice of name. But if there's consensus on something else, so
be it.

---

From: Robert Haas <[email-masked]>
Date: Mon, 18 Dec 2023 13:58:17 -0500

> it does not take an argument, but it does.

I've attempted to clean this stuff up in the attached version. This
version also includes a fix for the bug found by Jakub that caused
things to not work properly for segment files beyond the first for any
particular relation, which turns out to be a really stupid mistake in
my earlier commit 025584a168a4b3002e193.

---

From: Robert Haas <[email-masked]>
Date: Tue, 19 Dec 2023 15:36:03 -0500

> upgraded cluster, that error message without errhint() is going to

Yeah. I think we should add the system identifier to the manifest, but
I think that should be left for a future project, as I don't think the
lack of it is a good reason to stop all progress here. When we have
that, we can give more reliable error messages about system mismatches
at an earlier stage. Unfortunately, I don't think that the timeline
messages you're seeing here are going to apply in every case: suppose
you have two unrelated servers that are both on timeline 1. I think
you could use a base backup from one of those servers and use it as
the basis for the incremental from the other, and I think that if you
did it right you might fail to hit any sanity check that would block
that. pg_combinebackup will realize there's a problem, because it has
the whole cluster to work with, not just the manifest, and will notice
the mismatching system identifiers, but that's kind of late to find
out that you made a big mistake. However, right now, it's the best we
can do.

> The incrementals are being generated , but just for the first (0)

I committed the first two patches from the series I posted yesterday.
The first should fix this, and the second relocates parse_manifest.c.
That patch hasn't changed in a while and seems unlikely to attract
major objections. There's no real reason to commit it until we're
ready to move forward with the main patches, but I think we're very
close to that now, so I did.

Here's a rebase for cfbot.