public inbox for [email protected]
help / color / mirror / Atom feedFrom: Anthonin Bonnefoy <[email protected]>
To: Amul Sul <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Subject: Re: Fix possible 'unexpected data beyond EOF' on replica restart
Date: Wed, 17 Dec 2025 09:40:28 +0100
Message-ID: <CAO6_XqojxYb=j_rqSi_MH-DB=tZ-OtMQHqEx6xRh9DQrr2uDuQ@mail.gmail.com> (raw)
In-Reply-To: <CAAJ_b94FEArRqXd4q+drOh=yS4_2sPrdHTGXTVH+wHRP0EPuCw@mail.gmail.com>
References: <CAO6_Xqrv-snNJNhbj1KjQmWiWHX3nYGDgAc=vxaZP3qc4g1Siw@mail.gmail.com>
<CAAJ_b94FEArRqXd4q+drOh=yS4_2sPrdHTGXTVH+wHRP0EPuCw@mail.gmail.com>
On Wed, Dec 17, 2025 at 8:26 AM Amul Sul <[email protected]> wrote:
> Thanks for detailed reproducible steps, I can see the reported issue
> and proposed patch fixes the same.
Thanks for the review!
> The deleted code you moved to mdtruncate() should be kept where it
> was. We cannot ensure that every extension using this interface will
> adhere to that requirement what the comment describes.
Yeah, I've overlooked the case of extensions. I've moved back the
InvalidBlockNumber assignment.
Furthermore, an extension's routine might miss updating
smgr_cached_nblocks. To ensure that updates smgr_cached_nblocks
properly, we should also add an assertion after the call, like this:
>
> /*
> * Ensure that the local smgr_cached_nblocks value is updated.
> */
> Assert(reln->smgr_cached_nblocks[forknum[i]] !=
> InvalidBlockNumber);
Good point. I've added the assertion.
I wonder how critical it is to have an up to date value of
smgr_cached_nblocks after smgr_truncate. Leaving InvalidBlockNumber was
also an option as the next user will ask the kernel for the real size.
There are some functions like DropRelationBuffers which rely only on the
cached value, so it's probably safer to keep the same behaviour.
Regards,
Anthonin Bonnefoy
Attachments:
[application/octet-stream] v2-0001-Fix-unexpected-data-beyond-EOF-on-replica-restart.patch (4.6K, ../CAO6_XqojxYb=j_rqSi_MH-DB=tZ-OtMQHqEx6xRh9DQrr2uDuQ@mail.gmail.com/3-v2-0001-Fix-unexpected-data-beyond-EOF-on-replica-restart.patch)
download | inline diff:
From cfde2a18b3806fac0a2e8721ac5916755d68a6a6 Mon Sep 17 00:00:00 2001
From: Anthonin Bonnefoy <[email protected]>
Date: Tue, 16 Dec 2025 10:48:12 +0100
Subject: Fix 'unexpected data beyond EOF' on replica restart
On restart, a replica can fail with an 'unexpected data beyond EOF in
block 200 of relation T/D/R' error. This can happen under the following
circumstances:
- A relation has a size of 400 blocks.
- Blocks 201 to 400 are empty.
- Block 200 has two rows.
- Blocks 100 to 199 are empty.
- A restartpoint is done
- Vacuum truncates the relation to 200 blocks
- A FPW deletes a row in block 200
- A checkpoint is done
- A FPW deletes the last row in block 200
- Vacuum truncates the relation to 100 blocks
- The replica restarts
When the replica restarts:
- The relation on disk is reduced to 100 blocks due to having applied
the truncate before restart.
- The first truncate to 200 blocks is replayed. It silently fails, but
it will still update the cache size to 200 blocks
- The first FPW on block 200 is applied, XLogReadBufferForRead will rely
on the cached size and incorrectly assume the page exists in file,
and thus won't extend the relation.
- The Checkpoint Online is replayed, calling smgrdestroyall which will
discard the cached size.
- The second FPW on block 200 is applied. This time, the detected size
is 100 blocks, an extend is attempted. However, the block 200 is
already present in the buffer table due to the first FPW. This
triggers the 'unexpected data beyond EOF' since the page isn't new.
This patch fixes the issue by moving smgr_cached_nblocks update in
mdtruncate. If truncate size > old size, we set the cache to the old
size. Otherwise, on successful truncate, the cached size is set to
truncate size.
---
src/backend/storage/smgr/md.c | 23 ++++++++++++++++++++++-
src/backend/storage/smgr/smgr.c | 12 ++++++------
2 files changed, 28 insertions(+), 7 deletions(-)
diff --git a/src/backend/storage/smgr/md.c b/src/backend/storage/smgr/md.c
index 2ccb0faceb5..78cf8980f6b 100644
--- a/src/backend/storage/smgr/md.c
+++ b/src/backend/storage/smgr/md.c
@@ -1282,16 +1282,28 @@ mdtruncate(SMgrRelation reln, ForkNumber forknum,
if (nblocks > curnblk)
{
- /* Bogus request ... but no complaint if InRecovery */
+ /*
+ * This can happen when a relation was truncated multiple times and
+ * the restartpoint is located before the truncates. On restart, the
+ * relation on disk will have the size of the second truncate. As the
+ * first truncate has a higher nblocks, mdtruncate will be called with
+ * nblocks > curnblk during startup.
+ */
if (InRecovery)
+ {
+ reln->smgr_cached_nblocks[forknum] = curnblk;
return;
+ }
ereport(ERROR,
(errmsg("could not truncate file \"%s\" to %u blocks: it's only %u blocks now",
relpath(reln->smgr_rlocator, forknum).str,
nblocks, curnblk)));
}
if (nblocks == curnblk)
+ {
+ reln->smgr_cached_nblocks[forknum] = curnblk;
return; /* no work */
+ }
/*
* Truncate segments, starting at the last one. Starting at the end makes
@@ -1357,6 +1369,15 @@ mdtruncate(SMgrRelation reln, ForkNumber forknum,
}
curopensegs--;
}
+
+ /*
+ * We might as well update the local smgr_cached_nblocks values. The smgr
+ * cache inval message that this function sent will cause other backends
+ * to invalidate their copies of smgr_cached_nblocks, and these ones too
+ * at the next command boundary. But ensure they aren't outright wrong
+ * until then.
+ */
+ reln->smgr_cached_nblocks[forknum] = nblocks;
}
/*
diff --git a/src/backend/storage/smgr/smgr.c b/src/backend/storage/smgr/smgr.c
index bce37a36d51..90d46b1ae10 100644
--- a/src/backend/storage/smgr/smgr.c
+++ b/src/backend/storage/smgr/smgr.c
@@ -905,13 +905,13 @@ smgrtruncate(SMgrRelation reln, ForkNumber *forknum, int nforks,
old_nblocks[i], nblocks[i]);
/*
- * We might as well update the local smgr_cached_nblocks values. The
- * smgr cache inval message that this function sent will cause other
- * backends to invalidate their copies of smgr_cached_nblocks, and
- * these ones too at the next command boundary. But ensure they aren't
- * outright wrong until then.
+ * smgr_truncate may do nothing, leaving the relation with a size of
+ * old_nblocks. However, this isn't reported to smgr_truncate's
+ * caller. Thus, it's expected that smgr_truncate's implementation
+ * update smgr_cached_nblocks' to nblocks on successful truncate, or
+ * old_nblocks if nothing was done.
*/
- reln->smgr_cached_nblocks[forknum[i]] = nblocks[i];
+ Assert(reln->smgr_cached_nblocks[forknum[i]] != InvalidBlockNumber);
}
}
--
2.51.0
view thread (5+ messages) latest in thread
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]
Subject: Re: Fix possible 'unexpected data beyond EOF' on replica restart
In-Reply-To: <CAO6_XqojxYb=j_rqSi_MH-DB=tZ-OtMQHqEx6xRh9DQrr2uDuQ@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