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: <513345.1764717860@sss.pgh.pa.us>
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>
 <434484.1764707203@sss.pgh.pa.us>
 <CAEZATCUo6skdTfRP1XM1sbMveRo1PgpjzOMZEps69Bbz6-OnKA@mail.gmail.com>
 <513345.1764717860@sss.pgh.pa.us>
Comments: In-reply-to Tom Lane <tgl@sss.pgh.pa.us>
	message dated "Tue, 02 Dec 2025 18:24:20 -0500"
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0"
Content-ID: <531509.1764721047.0@sss.pgh.pa.us>
Date: Tue, 02 Dec 2025 19:17:32 -0500
Message-ID: <531516.1764721052@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/531516.1764721052%40sss.pgh.pa.us>
Precedence: bulk

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

I tried the attached realization of your idea, and it does seem
very marginally faster than what I did; but it's like a 1.8%
slowdown instead of 2%.  Might be different on a different
machine of course.  But I think we should choose on the basis
of which semantics we like better, rather than such a tiny
performance difference.

I'm coming around to the conclusion that your way is better,
though.  It seems good that "any NaN in the input results in
NaN output", which your way does and mine doesn't.

			regards, tom lane


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

diff --git a/src/backend/utils/adt/float.c b/src/backend/utils/adt/float.c
index 7b97d2be6ca..c2173f378bf 100644
--- a/src/backend/utils/adt/float.c
+++ b/src/backend/utils/adt/float.c
@@ -3319,9 +3319,16 @@ 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 an 8-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)), commonX, and comm=
onY
+ * in that order.
+ *
+ * commonX is defined as the common X value if all the X values were the
+ * same, else NaN; likewise for commonY.  This is useful for deciding
+ * whether corr() should return NULL.  Note that this representation cann=
ot
+ * distinguish all-the-values-were-NaN from the-values-weren't-all-the-sa=
me,
+ * but that's okay because we want to return NaN for all-NaN input.
  *
  * Note that Y is the first argument to all these aggregates!
  *
@@ -3345,17 +3352,21 @@ float8_regr_accum(PG_FUNCTION_ARGS)
 				Sy,
 				Syy,
 				Sxy,
+				commonX,
+				commonY,
 				tmpX,
 				tmpY,
 				scale;
 =

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

 	/*
 	 * Use the Youngs-Cramer algorithm to incorporate the new values into th=
e
@@ -3373,6 +3384,16 @@ 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.  We can use a test
+		 * that's a bit cheaper than float8_ne() because if commonX is already
+		 * NaN, it does not matter whether the !=3D test returns true or not.
+		 */
+		if (newvalX !=3D commonX || isnan(newvalX))
+			commonX =3D get_float8_nan();
+		if (newvalY !=3D commonY || isnan(newvalY))
+			commonY =3D get_float8_nan();
+
 		/*
 		 * Overflow check.  We only report an overflow error when finite
 		 * inputs lead to infinite results.  Note also that Sxx, Syy and Sxy
@@ -3410,6 +3431,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();
+		commonX =3D newvalX;
+		commonY =3D newvalY;
 	}
 =

 	/*
@@ -3425,12 +3448,14 @@ float8_regr_accum(PG_FUNCTION_ARGS)
 		transvalues[3] =3D Sy;
 		transvalues[4] =3D Syy;
 		transvalues[5] =3D Sxy;
+		transvalues[6] =3D commonX;
+		transvalues[7] =3D commonY;
 =

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

 		transdatums[0] =3D Float8GetDatumFast(N);
@@ -3439,8 +3464,10 @@ 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(commonX);
+		transdatums[7] =3D Float8GetDatumFast(commonY);
 =

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

 		PG_RETURN_ARRAYTYPE_P(result);
 	}
@@ -3733,24 +3760,27 @@ float8_corr(PG_FUNCTION_ARGS)
 	float8		N,
 				Sxx,
 				Syy,
-				Sxy;
+				Sxy,
+				commonX,
+				commonY;
 =

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

 	/* if N is 0 we should return NULL */
 	if (N < 1.0)
 		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)
+	if (!isnan(commonX) || !isnan(commonY))
 		PG_RETURN_NULL();
 =

+	/* at this point, Sxx and Syy cannot be zero or negative */
 	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..d1342f7bb94 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}' },
 =

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

------- =_aaaaaaaaaa0--