public inbox for [email protected]
help / color / mirror / Atom feedFrom: Chengpeng Yan <[email protected]>
To: Dean Rasheed <[email protected]>
Cc: PostgreSQL-development <[email protected]>
Cc: Tom Lane <[email protected]>
Subject: Re: [PATCH] Fix overflow and underflow in regr_r2()
Date: Sat, 23 May 2026 01:14:09 +0000
Message-ID: <[email protected]> (raw)
In-Reply-To: <CAEZATCUaV+qmBCh0zv0EPdBvhE2skHCzkRw78VNjBMCX9Z7h+w@mail.gmail.com>
References: <[email protected]>
<CAEZATCUaV+qmBCh0zv0EPdBvhE2skHCzkRw78VNjBMCX9Z7h+w@mail.gmail.com>
Hi,
> On May 16, 2026, at 17:39, Dean Rasheed <[email protected]> wrote:
>
>> corr() already has a stabilized calculation for the same Sxx * Syy
>> denominator scale. This patch factors that into a helper and lets
>> regr_r2() use it as a fallback when one of its direct products has
>> rounded to zero or infinity. Otherwise, regr_r2() keeps the existing
>> direct formula.
>
> The comments need work -- in particular float8_regr_r2() needs a
> comment explaining the new overflow/underflow checks, similar to the
> comment in float8_corr(). In fact, doing that, I think it's preferable
> to just keep this change local to float8_regr_r2(), rather than
> refactoring into a helper function for just a few lines of code.
>
> This new check in float8_regr_r2():
>
> + if (Sxy == 0 && !isnan(Sxx) && !isnan(Syy))
> + PG_RETURN_FLOAT8(0.0);
>
> seems pointless. It's optimising for a special case that will very
> rarely occur in practice, and which is handled fine by the general
> code. We don't want to slow down the common code path for such rare
> special cases.
>
> I noticed that this new overflow test case:
>
> +SELECT regr_r2(1e154::float8 * g, 1e154::float8 * g)
> + FROM generate_series(1, 2) g;
> + regr_r2
> +---------
> + 1
> +(1 row)
>
> only produces 1 because it's run with a reduced extra_float_digits
> value. I think it's better to use the test values "1e100 + g * 1e95",
> which still trigger the overflow on HEAD, but more reliably produce 1,
> regardless of the extra_float_digits setting, making it less likely
> that there will be variations between platforms. That's also more
> consistent with the other nearby test cases.
Thanks for the thorough review and feedback — I learned a lot from it!
> Attached is a v2 patch with those changes, plus a little more tidying
> up of the regression tests.
v2 LGTM. Thanks for the updates and test cleanup.
--
Best regards,
Chengpeng Yan
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected]
Subject: Re: [PATCH] Fix overflow and underflow in regr_r2()
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox