BUG #17172: NaN compare error in hash agg

PG Bug reporting form <noreply@postgresql.org> writes:

postgres=# select '-NaN'::float union select ('Infinity'::float +
'-Infinity') union select 'NaN';
float8
--------
NaN
NaN
(2 rows)

Hm. The reason for that is that NaN and -NaN are different bitpatterns
(0x7ff8000000000000 vs 0xfff8000000000000) so they produce different
hash values. (On my machine, Inf plus -Inf produces -NaN, which is
not what I'd have expected, but that's what I see.)

Since float8_eq considers all NaNs equal, this is clearly a bug in the
float hash functions. It's simple enough to fix, along the same lines
that we special-case minus zero: check isnan() and return some fixed
value if so. I'm inclined to do that like this:

Datum
hashfloat8(PG_FUNCTION_ARGS)
{
float8 key = PG_GETARG_FLOAT8(0);

/*
* On IEEE-float machines, minus zero and zero have different bit patterns
* but should compare as equal. We must ensure that they have the same
* hash value, which is most reliably done this way:
*/
if (key == (float8) 0)
PG_RETURN_UINT32(0);

+   /*
+    * Similarly, NaNs can have different bit patterns but should compare
+    * as equal.  For backwards-compatibility reasons we force them all to
+    * have the hash value of a standard NaN.
+    */
+    if (isnan(key))
+       key = get_float8_nan();
+
     return hash_any((unsigned char *) &key, sizeof(key));
}

This has been broken for a very long time. Do we dare back-patch
the fix? Given the lack of complaints, maybe fixing it in HEAD/v14
is enough. OTOH, it's not likely that many people have hash indexes
containing minus NaNs, so maybe it's okay to back-patch.

regards, tom lane

I wrote:

This has been broken for a very long time. Do we dare back-patch
the fix? Given the lack of complaints, maybe fixing it in HEAD/v14
is enough. OTOH, it's not likely that many people have hash indexes
containing minus NaNs, so maybe it's okay to back-patch.

After further thought I concluded that there's little reason
not to back-patch. If someone has -NaN in a hash index,
they'd need to re-index if they ever want to find that entry
again ... but it was already true that many queries would not
find that entry. Hence, pushed to all branches.

regards, tom lane