Subject: Fix an incorrect assertion condition in mdwritev().

From: Xing Guo <[email-masked]>
Date: Sat, 25 May 2024 23:52:22 +0800

Hi hackers,

In commit 4908c58[^1], a vectored variant of smgrwrite() is added and
the assertion condition in mdwritev() no longer matches the comment.
This patch helps fix it.

[^1]: https://github.com/postgres/postgres/commit/4908c5872059c409aa647bcde758dfeffe07996e

Best Regards,
Xing
From c77d1810ac6f13e9b58da5cd3ded5e47d44d03af Mon Sep 17 00:00:00 2001
From: Xing Guo <[email-masked]>
Date: Sat, 25 May 2024 23:36:29 +0800
Subject: [PATCH v1] Fix an incorrect assertion in mdwritev().

In commit 4908c58[^1], we added smgrwritev() which is a vectored variant
of smgrwrite().  Since mdwritev() is to be used only for updating
already-existing blocks of a relation, the assertion condition shall be
updated accordingly.

[^1]: https://github.com/postgres/postgres/commit/4908c5872059c409aa647bcde758dfeffe07996e
---
 src/backend/storage/smgr/md.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/backend/storage/smgr/md.c b/src/backend/storage/smgr/md.c
index bf0f3ca76d..2abb5133a6 100644
--- a/src/backend/storage/smgr/md.c
+++ b/src/backend/storage/smgr/md.c
@@ -930,7 +930,7 @@ mdwritev(SMgrRelation reln, ForkNumber forknum, BlockNumber blocknum,
 {
 	/* This assert is too expensive to have on normally ... */
 #ifdef CHECK_WRITE_VS_EXTEND
-	Assert(blocknum < mdnblocks(reln, forknum));
+	Assert(blocknum + nblocks <= mdnblocks(reln, forknum));
 #endif
 
 	while (nblocks > 0)
-- 
2.45.1

---

From: Michael Paquier <[email-masked]>
Date: Sun, 26 May 2024 09:22:06 +0900

> +	Assert(blocknum + nblocks <= mdnblocks(reln, forknum));

Yes, it looks like you're right that this can be made stricter,
computing the number of blocks we're adding in the number calculated
(aka adding one block to this number fails immediately at initdb).
--
Michael

---

From: Tom Lane <[email-masked]>
Date: Sat, 25 May 2024 23:59:05 -0400

> (aka adding one block to this number fails immediately at initdb).

Hmm ... I agree that this is better normally.  But there's an
edge case where it would fail to notice a problem that the
existing code does notice: if blocknum is close to UINT32_MAX
and adding nblocks causes it to wrap around to a small value.
Is there an inexpensive way to catch that?  (If not, it's
not a reason to block this patch; but let's think about it
while we're here.)

			regards, tom lane

---

From: Tom Lane <[email-masked]>
Date: Sun, 26 May 2024 00:08:46 -0400

I wrote:
> Is there an inexpensive way to catch that?

After a few minutes' thought, how about:

	Assert((uint64) blocknum + (uint64) nblocks <= (uint64) mdnblocks(reln, forknum));

This'd stop being helpful if we ever widen BlockNumber to 64 bits,
but I think that's unlikely.  (Partitioning seems like a better answer
for giant tables.)

			regards, tom lane

---

From: Michael Paquier <[email-masked]>
Date: Tue, 4 Jun 2024 07:17:51 +0900

> but I think that's unlikely.  (Partitioning seems like a better answer

No idea if this will happen or not, but that's not the only area where
we are going to need a native uint128 implementation to control the
overflows with uint64.

What you are suggesting is good enough for me, so I've applied on HEAD
a version using that.
--
Michael