public inbox for [email protected]
help / color / mirror / Atom feedFrom: Robert Haas <[email protected]>
To: [email protected] <[email protected]>
Cc: Jakub Wartak <[email protected]>
Cc: Tomas Vondra <[email protected]>
Subject: incremental backup breakage in BlockRefTableEntryGetBlocks
Date: Thu, 4 Apr 2024 13:38:09 -0400
Message-ID: <CA+TgmoYwy_KHp1-5GYNmVa=zdeJWhNH1T0SBmEuvqQNJEHj1Lw@mail.gmail.com> (raw)
Hi,
Yesterday, Tomas Vondra reported to me off-list that he was seeing
what appeared to be data corruption after taking and restoring an
incremental backup. Overnight, Jakub Wartak further experimented with
Tomas's test case, did some initial analysis, and made it very easy to
reproduce. I spent this morning tracking down the problem, for which I
attach a patch.
It doesn't take a whole lot to hit this bug. I think all you need is a
relation that is more than 1GB in size, plus a modification to the
second half of some 1GB file between the full backup and the
incremental backup, plus some way of realizing after you do a restore
that you've gotten the wrong answer. It's obviously quite embarrassing
that this wasn't caught sooner, and to be honest I'm not sure why we
didn't. I think the test cases pass because we avoid committing test
cases that create large relations, but the failure to catch it during
manual testing must be because I never worked hard enough to verify
that the results were fully correct. Ouch.
The actual bug is here:
if (chunkno == stop_chunkno - 1)
stop_offset = stop_blkno % BLOCKS_PER_CHUNK;
Each chunk covers 64k block numbers. The caller specifies a range of
block numbers of interest via start_blkno (inclusive) and stop_blkno
(non-inclusive). We need to translate those start and stop values for
the overall function call into start and stop values for each
particular chunk. When we're on any chunk but the last one of
interest, the stop offset is BLOCKS_PER_CHUNK i.e. we care about
blocks all the way through the end of the chunk. The existing code
handles that fine. If stop_blkno is somewhere in the middle of the
last chunk, the existing code also handles that fine. But the code is
wrong when stop_blkno is a multiple of BLOCKS_PER_CHUNK, because it
then calculates a stop_offset of 0 (which is never right, because that
would mean that we thought the chunk was relevant when it isn't)
rather than BLOCKS_PER_CHUNK. So this forgets about 64k * BLCKSZ =
512MB of potentially modified blocks whenever the size of a single
relation file is exactly 512MB or exactly 1GB, which our users would
probably find more than slightly distressing.
I'm not posting Jakub's reproducer here because it was sent to me
privately and, although I presume he would have no issue with me
posting it, I can't actually confirm that right at the moment.
Hopefully he'll reply with it, and double-check that this does indeed
fix the issue. My thanks to both Tomas and Jakub.
Regards,
--
Robert Haas
EDB: http://www.enterprisedb.com
Attachments:
[application/octet-stream] v1-0001-Fix-incorrect-calculation-in-BlockRefTableEntryGe.patch (1.5K, ../CA+TgmoYwy_KHp1-5GYNmVa=zdeJWhNH1T0SBmEuvqQNJEHj1Lw@mail.gmail.com/2-v1-0001-Fix-incorrect-calculation-in-BlockRefTableEntryGe.patch)
download | inline diff:
From d2b7db90f9d1489c8e299ad64176e17ddf070168 Mon Sep 17 00:00:00 2001
From: Robert Haas <[email protected]>
Date: Thu, 4 Apr 2024 12:38:44 -0400
Subject: [PATCH v1] Fix incorrect calculation in BlockRefTableEntryGetBlocks.
The previous formula was incorrect in the case where the function's
nblocks argument was a multiple of BLOCKS_PER_CHUNK, which happens
whenever a relation segment file is exactly 512MB or exactly 1GB in
length. In such cases, the formula would calculate a stop_offset of
0 rather than 65536, resulting in modified blocks in the second half
of a 1GB file, or all the modified blocks in a 512MB file, being
omitted from the incremental backup.
Reported off-list by Tomas Vondra and Jakub Wartak.
---
src/common/blkreftable.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/src/common/blkreftable.c b/src/common/blkreftable.c
index bfa6f7ab5d..845b5d1dc4 100644
--- a/src/common/blkreftable.c
+++ b/src/common/blkreftable.c
@@ -410,7 +410,11 @@ BlockRefTableEntryGetBlocks(BlockRefTableEntry *entry,
if (chunkno == start_chunkno)
start_offset = start_blkno % BLOCKS_PER_CHUNK;
if (chunkno == stop_chunkno - 1)
- stop_offset = stop_blkno % BLOCKS_PER_CHUNK;
+ {
+ Assert(stop_blkno > chunkno * BLOCKS_PER_CHUNK);
+ stop_offset = stop_blkno - (chunkno * BLOCKS_PER_CHUNK);
+ Assert(stop_offset <= BLOCKS_PER_CHUNK);
+ }
/*
* Handling differs depending on whether this is an array of offsets
--
2.39.3 (Apple Git-145)
view thread (2+ messages)
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected]
Subject: Re: incremental backup breakage in BlockRefTableEntryGetBlocks
In-Reply-To: <CA+TgmoYwy_KHp1-5GYNmVa=zdeJWhNH1T0SBmEuvqQNJEHj1Lw@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox