Fix an incorrect assertion condition in mdwritev().
Hackorum builds and tests every patch posted to the lists, not only commitfest submissions. This is Hackorum's own CI rather than the PostgreSQL project's, and it is still under testing - please report anything that looks wrong.
You can run a PostgreSQL built from this patch straight from Docker, with no checkout and no build:
docker run --rm -p 5432:5432 ghcr.io/hackorum-dev/postgres-patch:t49689psql -h localhost -U postgresBuilt from patchset v9 (message #9), September 19, 2026 at 02:57 PM.
Every patchset is also pushed to a branch of our PostgreSQL fork, so you can check out the same tree CI built. Without a PostgreSQL checkout:
git clone --branch t49689_9 https://github.com/hackorum-dev/postgres.gitIn a checkout you already have, add the fork once:
git remote add hackorum https://github.com/hackorum-dev/postgres.gitthen, for this patchset and every later one:
git fetch hackorum t49689_9 && git checkout t49689_9Patchset v9 (message #9) is on t49689_9
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
Attachments:
v1-0001-Fix-an-incorrect-assertion-in-mdwritev.patchtext/x-patch; charset=US-ASCII; name=v1-0001-Fix-an-incorrect-assertion-in-mdwritev.patchDownload+1-2
On Sat, May 25, 2024 at 11:52:22PM +0800, Xing Guo wrote:
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./* 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
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
Michael Paquier <michael@paquier.xyz> writes:
On Sat, May 25, 2024 at 11:52:22PM +0800, Xing Guo wrote:
#ifdef CHECK_WRITE_VS_EXTEND - Assert(blocknum < mdnblocks(reln, forknum)); + Assert(blocknum + nblocks <= mdnblocks(reln, forknum)); #endif
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).
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
I wrote:
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?
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
On Sun, May 26, 2024 at 12:08:46AM -0400, Tom Lane wrote:
After a few minutes' thought, how about:
Assert((uint64) blocknum + (uint64) nblocks <= (uint64) mdnblocks(reln, forknum));
LGTM. Yeah that should be OK this way.
--
Michael
On Sun, May 26, 2024 at 12:08:46AM -0400, Tom Lane wrote:
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.)
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
Hi,
On 2024-06-04 07:17:51 +0900, Michael Paquier wrote:
On Sun, May 26, 2024 at 12:08:46AM -0400, Tom Lane wrote:
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.)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.
I'm confused - isn't using common/int.h entirely sufficient for that? Nearly
all architectures have more efficient ways to check for 64bit overflows than
doing actual 128 bit math.
Greetings,
Andres Freund
On Mon, Jun 03, 2024 at 03:24:07PM -0700, Andres Freund wrote:
I'm confused - isn't using common/int.h entirely sufficient for that? Nearly
all architectures have more efficient ways to check for 64bit overflows than
doing actual 128 bit math.
Oh, right. We could just plug in pg_add_u32_overflow here. Funny
thing is that I'm the one who committed these toys with
__builtin_add_overflow(), still nobody has found a case where this one
would be useful. At least until now.
--
Michael
On Mon, Jun 03, 2024 at 03:24:07PM -0700, Andres Freund wrote:
I'm confused - isn't using common/int.h entirely sufficient for that? Nearly
all architectures have more efficient ways to check for 64bit overflows than
doing actual 128 bit math.
One simple way to change the assertion would be something like that, I
assume. Andres, does it answer your concerns?
--
Michael