Re: BUG #19826: corr()/regr_r2() of constant input: NULL serially, 1 with partial aggregation

On Thu, Nov 12, 2026 at 1:53 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> I studied this a bit, and remained unhappy about one point: it's
> not clear that float8_regr_combine() has the same behavior for
> overflow cases as float8_regr_accum(). Maybe it does, but I don't
> think readers should have to spend time convincing themselves of that.
> I suggest adjusting it to use the same block of overflow-handling
> logic that float8_regr_accum() has, as in v3 attached. Does that
> seem sane to you?

Yes. I compared it with float8_regr_accum() case by case: Sx1 and
Sx2 play the roles of transvalues[1] and newvalX there, so we still
only raise an error when finite sums give an infinite result, and
dropping float8_pl() for Sx and Sy loses nothing because the new block
catches that overflow. Testing Sx/Sy for the Inf/NaN case is right
too, since one Inf or NaN anywhere in a state makes its sum Inf or NaN.

> I suspect that the last few lines in my v3, [...] are dead code for
> the same reason, but again that doesn't seem like something to
> depend on

Agreed. A state built by float8_regr_accum() never has an infinite
Sxx, Syy or Sxy, so those lines only matter for hand-made states, but
they keep the two functions in step.

> Yeah, it seems clear that that test case is just made-up data,
> probably put there just so that float8_regr_combine() shows as having
> some coverage in our coverage tests. I wonder whether the new test
> doesn't render it useless even for that. It might be better to change
> it to exercise something your new test still doesn't reach, like an
> overflow error.

The first six entries aren't made up: they are the transition states
for the regr_test rows with x IN (10,20,30) and x IN (80,100), which
the queries just above print. Only the commonX/commonY entries (7,8
and 7,9) don't fit those rows, which have distinct X and Y values.
With NaN in both, the test goes through the path where both
correction terms are applied, which regr_part doesn't reach since its
X is constant, and the result is again
{5,240,6280,1490,95080,8680,NaN,NaN}, the sums over the whole table.

I also added two overflow cases (Sx itself, and the Sxx correction
term alone) and one showing that an infinite input is not reported
as an overflow. These give the same output with and without the
float.c change, so they are coverage for the combine function rather
than a test of this fix.

v4 attached: your v3 plus those test changes and a paragraph about
them in the commit message. make check passes on master (b0496e3)
with assertions enabled.

Regards,
Venu

On Thu, Nov 12, 2026 at 1:53 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> Venu K <venu2805k(at)gmail(dot)com> writes:
> > v2 attached; make check passes on master.
>
> I studied this a bit, and remained unhappy about one point: it's
> not clear that float8_regr_combine() has the same behavior for
> overflow cases as float8_regr_accum(). Maybe it does, but I don't
> think readers should have to spend time convincing themselves of that.
> I suggest adjusting it to use the same block of overflow-handling
> logic that float8_regr_accum() has, as in v3 attached. Does that
> seem sane to you?
>
> > In practice a partial state that has seen an Inf or NaN already has
> > Sxy = NaN, so I couldn't get a different result from SQL either way, but
> > I don't think the combine function should depend on that.
>
> Right, I'm with you on that. I suspect that the last few lines in
> my v3,
>
> + if (isinf(Sxx))
> + Sxx = get_float8_nan();
> + if (isinf(Syy))
> + Syy = get_float8_nan();
> + if (isinf(Sxy))
> + Sxy = get_float8_nan();
>
> are dead code for the same reason, but again that doesn't seem
> like something to depend on, especially in a spot like this
> that can't be at all performance-critical.
>
> > One existing expected output changes: the direct float8_regr_combine()
> > call in aggregates.sql passes two states with the same common X (7) but
> > different means, so Sxx is now 400 rather than 6280. Such states can't
> > come from real input; I can change the test input instead if you prefer.
>
> Yeah, it seems clear that that test case is just made-up data,
> probably put there just so that float8_regr_combine() shows as having
> some coverage in our coverage tests. I wonder whether the new test
> doesn't render it useless even for that. It might be better to change
> it to exercise something your new test still doesn't reach, like an
> overflow error. I didn't do anything about that in v3 though.
>
> regards, tom lane
>