From: Tom Lane <tgl@sss.pgh.pa.us>
To: Dean Rasheed <dean.a.rasheed@gmail.com>
cc: Oleg Ivanov <o15611@gmail.com>, Laurenz Albe <laurenz.albe@cybertec.at>,
        pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #19340: Wrong result from CORR() function
In-reply-to: 
 <CAEZATCU8rnyKN3z_5-osk3Bn8dtzWf9nKjTr2E16-ExXiESNrQ@mail.gmail.com>
References: <19340-6fb9f6637f562092@postgresql.org>
 <4ab9867066e9545dd1a7e835a480bb0ecbe1a00d.camel@cybertec.at>
 <CAH1GMznwE=WGJYCZU1ou9iOmNR87NQFSRdYHqE3=JQ3as4P1hQ@mail.gmail.com>
 <375068.1764696127@sss.pgh.pa.us>
 <CAEZATCU8rnyKN3z_5-osk3Bn8dtzWf9nKjTr2E16-ExXiESNrQ@mail.gmail.com>
Comments: In-reply-to Dean Rasheed <dean.a.rasheed@gmail.com>
	message dated "Tue, 02 Dec 2025 18:54:20 +0000"
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0"
Content-ID: <434454.1764707182.0@sss.pgh.pa.us>
Date: Tue, 02 Dec 2025 15:26:43 -0500
Message-ID: <434484.1764707203@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/434484.1764707203%40sss.pgh.pa.us>
Precedence: bulk

------- =_aaaaaaaaaa0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <434454.1764707182.1@sss.pgh.pa.us>

Dean Rasheed <dean.a.rasheed@gmail.com> writes:
> On Tue, 2 Dec 2025 at 17:22, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I wonder whether it'd be worth carrying additional state to
>> check that explicitly (instead of assuming that "if (Sxx == 0 ||
>> Syy == 0)" will catch it).

> I wondered the same thing. It's not nice to have to do that, but
> clearly the existing test for constant inputs is no good. The question
> is, do we really want to spend extra cycles on every query just to
> catch this odd corner case?

I experimented with the attached patch, which is very incomplete;
I just carried it far enough to be able to run performance checks on
the modified code, and so all the binary statistics aggregates except
corr() are broken.  I observe about 2% slowdown on this test case:

SELECT corr( 0.09 , 0.09000001 ) FROM generate_series(1,100000000);

I think that any real-world usage is going to expend more effort
obtaining the input data than this test does, so 2% should be a
conservative upper bound on the cost.  Seems to me that getting
NULL-or-not right is probably worth a percent or so.

If anyone feels differently, another idea could be to use a
separate state transition function for corr() that skips the
accumulation steps that corr() doesn't use.  But I agree with
the pre-existing decision to use just one transition function
for all the binary aggregates.

If this seems like a reasonable approach, I'll see about finishing
out the patch.

			regards, tom lane


------- =_aaaaaaaaaa0
Content-Type: text/x-diff; name="wip-detect-constant-input-exactly.patch";
	charset="us-ascii"
Content-ID: <434454.1764707182.2@sss.pgh.pa.us>
Content-Description: wip-detect-constant-input-exactly.patch
Content-Transfer-Encoding: quoted-printable

diff --git a/src/backend/utils/adt/float.c b/src/backend/utils/adt/float.c
index 7b97d2be6ca..5d8954a34d0 100644
--- a/src/backend/utils/adt/float.c
+++ b/src/backend/utils/adt/float.c
@@ -3319,9 +3319,15 @@ float8_stddev_samp(PG_FUNCTION_ARGS)
  * As with the preceding aggregates, we use the Youngs-Cramer algorithm t=
o
  * reduce rounding errors in the aggregate final functions.
  *
- * The transition datatype for all these aggregates is a 6-element array =
of
+ * The transition datatype for all these aggregates is a 9-element array =
of
  * float8, holding the values N, Sx=3Dsum(X), Sxx=3Dsum((X-Sx/N)^2), Sy=3D=
sum(Y),
- * Syy=3Dsum((Y-Sy/N)^2), Sxy=3Dsum((X-Sx/N)*(Y-Sy/N)) in that order.
+ * Syy=3Dsum((Y-Sy/N)^2), Sxy=3Dsum((X-Sx/N)*(Y-Sy/N)), firstX, firstY, D=
IFF,
+ * in that order.
+ *
+ * DIFF, like N, is treated as an integer.  Bit 0 is set if we saw distin=
ct
+ * X inputs, and bit 1 is set if we saw distinct Y inputs.  This allows u=
s
+ * to detect constant inputs exactly, which is important for deciding whe=
ther
+ * some outputs should be NULL.
  *
  * Note that Y is the first argument to all these aggregates!
  *
@@ -3345,17 +3351,23 @@ float8_regr_accum(PG_FUNCTION_ARGS)
 				Sy,
 				Syy,
 				Sxy,
+				firstX,
+				firstY,
 				tmpX,
 				tmpY,
 				scale;
+	int			diff;
 =

-	transvalues =3D check_float8_array(transarray, "float8_regr_accum", 6);
+	transvalues =3D check_float8_array(transarray, "float8_regr_accum", 9);
 	N =3D transvalues[0];
 	Sx =3D transvalues[1];
 	Sxx =3D transvalues[2];
 	Sy =3D transvalues[3];
 	Syy =3D transvalues[4];
 	Sxy =3D transvalues[5];
+	firstX =3D transvalues[6];
+	firstY =3D transvalues[7];
+	diff =3D transvalues[8];
 =

 	/*
 	 * Use the Youngs-Cramer algorithm to incorporate the new values into th=
e
@@ -3373,6 +3385,19 @@ float8_regr_accum(PG_FUNCTION_ARGS)
 		Syy +=3D tmpY * tmpY * scale;
 		Sxy +=3D tmpX * tmpY * scale;
 =

+		/*
+		 * Check to see if we have seen distinct inputs.  In normal use, diff
+		 * will reach 3 very soon and then we can stop checking.
+		 */
+		if (diff !=3D 3)
+		{
+			/* Need SQL-style comparison of NaNs here */
+			if (float8_ne(newvalX, firstX))
+				diff |=3D 1;
+			if (float8_ne(newvalY, firstY))
+				diff |=3D 2;
+		}
+
 		/*
 		 * Overflow check.  We only report an overflow error when finite
 		 * inputs lead to infinite results.  Note also that Sxx, Syy and Sxy
@@ -3410,6 +3435,8 @@ float8_regr_accum(PG_FUNCTION_ARGS)
 			Sxx =3D Sxy =3D get_float8_nan();
 		if (isnan(newvalY) || isinf(newvalY))
 			Syy =3D Sxy =3D get_float8_nan();
+		firstX =3D newvalX;
+		firstY =3D newvalY;
 	}
 =

 	/*
@@ -3425,12 +3452,15 @@ float8_regr_accum(PG_FUNCTION_ARGS)
 		transvalues[3] =3D Sy;
 		transvalues[4] =3D Syy;
 		transvalues[5] =3D Sxy;
+		transvalues[6] =3D firstX;
+		transvalues[7] =3D firstY;
+		transvalues[8] =3D diff;
 =

 		PG_RETURN_ARRAYTYPE_P(transarray);
 	}
 	else
 	{
-		Datum		transdatums[6];
+		Datum		transdatums[9];
 		ArrayType  *result;
 =

 		transdatums[0] =3D Float8GetDatumFast(N);
@@ -3439,8 +3469,11 @@ float8_regr_accum(PG_FUNCTION_ARGS)
 		transdatums[3] =3D Float8GetDatumFast(Sy);
 		transdatums[4] =3D Float8GetDatumFast(Syy);
 		transdatums[5] =3D Float8GetDatumFast(Sxy);
+		transdatums[6] =3D Float8GetDatumFast(firstX);
+		transdatums[7] =3D Float8GetDatumFast(firstY);
+		transdatums[8] =3D Float8GetDatum(diff);
 =

-		result =3D construct_array_builtin(transdatums, 6, FLOAT8OID);
+		result =3D construct_array_builtin(transdatums, 9, FLOAT8OID);
 =

 		PG_RETURN_ARRAYTYPE_P(result);
 	}
@@ -3730,27 +3763,25 @@ float8_corr(PG_FUNCTION_ARGS)
 {
 	ArrayType  *transarray =3D PG_GETARG_ARRAYTYPE_P(0);
 	float8	   *transvalues;
-	float8		N,
-				Sxx,
+	float8		Sxx,
 				Syy,
 				Sxy;
+	int			diff;
 =

-	transvalues =3D check_float8_array(transarray, "float8_corr", 6);
-	N =3D transvalues[0];
+	transvalues =3D check_float8_array(transarray, "float8_corr", 9);
 	Sxx =3D transvalues[2];
 	Syy =3D transvalues[4];
 	Sxy =3D transvalues[5];
+	diff =3D transvalues[8];
 =

-	/* if N is 0 we should return NULL */
-	if (N < 1.0)
+	/*
+	 * Per spec, we must return NULL if N is zero, all X inputs are equal, o=
r
+	 * all Y inputs are equal.  Checking the diff mask covers all three case=
s.
+	 */
+	if (diff !=3D 3)
 		PG_RETURN_NULL();
 =

 	/* Note that Sxx and Syy are guaranteed to be non-negative */
-
-	/* per spec, return NULL for horizontal and vertical lines */
-	if (Sxx =3D=3D 0 || Syy =3D=3D 0)
-		PG_RETURN_NULL();
-
 	PG_RETURN_FLOAT8(Sxy / sqrt(Sxx * Syy));
 }
 =

diff --git a/src/include/catalog/pg_aggregate.dat b/src/include/catalog/pg=
_aggregate.dat
index 870769e8f14..68dc1329ea0 100644
--- a/src/include/catalog/pg_aggregate.dat
+++ b/src/include/catalog/pg_aggregate.dat
@@ -505,7 +505,7 @@
   aggtranstype =3D> '_float8', agginitval =3D> '{0,0,0,0,0,0}' },
 { aggfnoid =3D> 'corr', aggtransfn =3D> 'float8_regr_accum',
   aggfinalfn =3D> 'float8_corr', aggcombinefn =3D> 'float8_regr_combine',
-  aggtranstype =3D> '_float8', agginitval =3D> '{0,0,0,0,0,0}' },
+  aggtranstype =3D> '_float8', agginitval =3D> '{0,0,0,0,0,0,0,0,0}' },
 =

 # boolean-and and boolean-or
 { aggfnoid =3D> 'bool_and', aggtransfn =3D> 'booland_statefunc',

------- =_aaaaaaaaaa0--