Обсуждение: Fix an incorrect assertion condition in mdwritev().

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

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