Horton
Menu

Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Improve assertion in mdwritev()

  2. Provide vectored variants of smgrread() and smgrwrite().

  1. Fix an incorrect assertion condition in mdwritev().

    Xing Guo <higuoxing@gmail.com> — 2024-05-25T15:52:22Z

    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

  2. Re: Fix an incorrect assertion condition in mdwritev().

    Michael Paquier <michael@paquier.xyz> — 2024-05-26T00:22:06Z

    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

  3. Re: Fix an incorrect assertion condition in mdwritev().

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-05-26T03:59:05Z

    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

  4. Re: Fix an incorrect assertion condition in mdwritev().

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-05-26T04:08:46Z

    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

  5. Re: Fix an incorrect assertion condition in mdwritev().

    Michael Paquier <michael@paquier.xyz> — 2024-05-26T11:07:45Z

    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

  6. Re: Fix an incorrect assertion condition in mdwritev().

    Michael Paquier <michael@paquier.xyz> — 2024-06-03T22:17:51Z

    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

  7. Re: Fix an incorrect assertion condition in mdwritev().

    Andres Freund <andres@anarazel.de> — 2024-06-03T22:24:07Z

    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

  8. Re: Fix an incorrect assertion condition in mdwritev().

    Michael Paquier <michael@paquier.xyz> — 2024-06-03T22:43:41Z

    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

  9. Re: Fix an incorrect assertion condition in mdwritev().

    Michael Paquier <michael@paquier.xyz> — 2024-06-05T04:10:41Z

    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