Content-Type: multipart/mixed; boundary="------------R5UIhwD7yzT9kBUybJ0Giya3"
Message-ID: <02e720a8-761b-4309-b26f-f41c288f7990@iki.fi>
Date: Thu, 18 Dec 2025 16:18:46 +0200
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: Fix possible 'unexpected data beyond EOF' on replica restart
To: Anthonin Bonnefoy <anthonin.bonnefoy@datadoghq.com>,
 Amul Sul <sulamul@gmail.com>
Cc: PostgreSQL Hackers <pgsql-hackers@postgresql.org>
References: 
 <CAO6_Xqrv-snNJNhbj1KjQmWiWHX3nYGDgAc=vxaZP3qc4g1Siw@mail.gmail.com>
 <CAAJ_b94FEArRqXd4q+drOh=yS4_2sPrdHTGXTVH+wHRP0EPuCw@mail.gmail.com>
 <CAO6_XqojxYb=j_rqSi_MH-DB=tZ-OtMQHqEx6xRh9DQrr2uDuQ@mail.gmail.com>
Content-Language: en-US
From: Heikki Linnakangas <hlinnaka@iki.fi>
In-Reply-To: 
 <CAO6_XqojxYb=j_rqSi_MH-DB=tZ-OtMQHqEx6xRh9DQrr2uDuQ@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/02e720a8-761b-4309-b26f-f41c288f7990%40iki.fi>
Precedence: bulk

This is a multi-part message in MIME format.
--------------R5UIhwD7yzT9kBUybJ0Giya3
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit

On 17/12/2025 10:40, Anthonin Bonnefoy wrote:
> On Wed, Dec 17, 2025 at 8:26 AM Amul Sul <sulamul@gmail.com 
> <mailto:sulamul@gmail.com>> wrote:
> 
>> 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.

The smgr interface isn't really an extension point, so we don't need to 
worry about that. (I wish it was, but that's a different story [1])

I don't think mdtruncate() should modify smgr_cached_nblocks, we should 
keep that in smgrtruncate(). smgr.c is responsible for all other updates 
of smgr_cached_nblocks, so doing it in mdtruncate() would be a layering 
violation.

I'm thinking that we should do the attached. Untested, and we should 
also add a comment to smgrtruncate() and mdtruncate() to explain how 
they behave if nblocks > curnblk.

I wonder if we should move the whole "if (nblocks > curnblk)" check and 
ereport() from mdtruncate() to smgrtruncate(). That logic doesn't really 
depend on anything specific to md.c. If you'd imagine a different smgr 
implementation, it'd need to just copy-paste that check. It's the 
caller's mistake if it passes nblocks > curnblk, when not in recovery. 
Then again, we do have other places in md.c too that behave differently 
when InRecovery.

What do you think?

> 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.

Leaving it invalid should work. But as the comment says, we might as 
well update the cached value since we have the value at hand. It's just 
that we were doing it wrong.

[1] 
https://www.postgresql.org/message-id/CAEze2WgMySu2suO_TLvFyGY3URa4mAx22WeoEicnK%3DPCNWEMrA%40mail.gmail.com

- Heikki

--------------R5UIhwD7yzT9kBUybJ0Giya3
Content-Type: text/x-patch; charset=UTF-8;
 name="v3-Fix-unexpected-data-beyond-EOF-on-replica-restart.patch"
Content-Disposition: attachment;
 filename*0="v3-Fix-unexpected-data-beyond-EOF-on-replica-restart.patch"
Content-Transfer-Encoding: base64

ZGlmZiAtLWdpdCBhL3NyYy9iYWNrZW5kL3N0b3JhZ2Uvc21nci9zbWdyLmMgYi9zcmMvYmFj
a2VuZC9zdG9yYWdlL3NtZ3Ivc21nci5jCmluZGV4IGY5MDY2YWI4YzQ5Li5hYmI1MWYwYTBi
YiAxMDA2NDQKLS0tIGEvc3JjL2JhY2tlbmQvc3RvcmFnZS9zbWdyL3NtZ3IuYworKysgYi9z
cmMvYmFja2VuZC9zdG9yYWdlL3NtZ3Ivc21nci5jCkBAIC05MTEsNyArOTExLDggQEAgc21n
cnRydW5jYXRlKFNNZ3JSZWxhdGlvbiByZWxuLCBGb3JrTnVtYmVyICpmb3JrbnVtLCBpbnQg
bmZvcmtzLAogCQkgKiB0aGVzZSBvbmVzIHRvbyBhdCB0aGUgbmV4dCBjb21tYW5kIGJvdW5k
YXJ5LiBCdXQgZW5zdXJlIHRoZXkgYXJlbid0CiAJCSAqIG91dHJpZ2h0IHdyb25nIHVudGls
IHRoZW4uCiAJCSAqLwotCQlyZWxuLT5zbWdyX2NhY2hlZF9uYmxvY2tzW2ZvcmtudW1baV1d
ID0gbmJsb2Nrc1tpXTsKKwkJcmVsbi0+c21ncl9jYWNoZWRfbmJsb2Nrc1tmb3JrbnVtW2ld
XSA9CisJCQluYmxvY2tzW2ldID4gb2xkX25ibG9ja3NbaV0gPyBvbGRfbmJsb2Nrc1tpXSA6
IG5ibG9ja3NbaV07CiAJfQogfQogCg==

--------------R5UIhwD7yzT9kBUybJ0Giya3--