BUG #12885: The result of casting a double to an integer depends on the database version

rschaaf@commoninf.com writes:

The result returned by the following query appears to depend on the
PostgreSQL database version.

The query is:
select column1 AS double_value, cast(column1 AS INT) AS int_value
from (VALUES (-2.5::double precision),
(-1.5::double precision),
(-0.5::double precision),
(0.5::double precision),
(1.5::double precision),
(2.5::double precision)) t;

In PostgreSQL 9.3.5, the query returns:
double_value, int_value
-2.5, -3
-1.5, -2
-0.5, -1
0.5, 1
1.5, 2
2.5, 3

PostgreSQL 9.4.1, the query returns:
-2.5, -2
-1.5, -2
-0.5, 0
0.5, 0
1.5, 2
2.5, 2

FWIW, I get the latter behavior (round to nearest even) in all release
branches, and I would say that one is correct. Not real sure why your
9.3 installation is misbehaving.

regards, tom lane

--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

On Mon, Mar 23, 2015 at 7:48 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

rschaaf@commoninf.com writes:

The result returned by the following query appears to depend on the
PostgreSQL database version.

The query is:
select column1 AS double_value, cast(column1 AS INT) AS int_value
from (VALUES (-2.5::double precision),
(-1.5::double precision),
(-0.5::double precision),
(0.5::double precision),
(1.5::double precision),
(2.5::double precision)) t;

In PostgreSQL 9.3.5, the query returns:
double_value, int_value
-2.5, -3
-1.5, -2
-0.5, -1
0.5, 1
1.5, 2
2.5, 3

PostgreSQL 9.4.1, the query returns:
-2.5, -2
-1.5, -2
-0.5, 0
0.5, 0
1.5, 2
2.5, 2

FWIW, I get the latter behavior (round to nearest even) in all release
branches, and I would say that one is correct. Not real sure why your
9.3 installation is misbehaving.

On a Windows 7 box with code compiled with MSVC 2010 I am seeing the
same behavior as Rich. This looks like a bug in ~9.3 that meritates
some attention assuming that the latter behavior is legit.
--
Michael

--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

On Tue, Mar 24, 2015 at 11:27 AM, Michael Paquier wrote:

On a Windows 7 box with code compiled with MSVC 2010 I am seeing the
same behavior as Rich. This looks like a bug in ~9.3 that meritates
some attention assuming that the latter behavior is legit.

And MinGW outputs the latter, similarly to other platforms... I'll investigate.
--
Michael

--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

Michael Paquier <michael.paquier@gmail.com> writes:

On Tue, Mar 24, 2015 at 11:27 AM, Michael Paquier wrote:

On a Windows 7 box with code compiled with MSVC 2010 I am seeing the
same behavior as Rich. This looks like a bug in ~9.3 that meritates
some attention assuming that the latter behavior is legit.

And MinGW outputs the latter, similarly to other platforms... I'll investigate.

Look for something about setting the IEEE float rounding mode. "Round to
nearest even" is standard in most places, but it would not astonish me
to hear that Microsoft got that wrong.

If it is wrong I don't know that we'd want to back-patch a behavioral
change, but I'd definitely vote for conforming to the norm in 9.5
and later.

regards, tom lane

--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

On Mon, Mar 23, 2015 at 8:12 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Michael Paquier <michael.paquier@gmail.com> writes:

On Tue, Mar 24, 2015 at 11:27 AM, Michael Paquier wrote:

On a Windows 7 box with code compiled with MSVC 2010 I am seeing the
same behavior as Rich. This looks like a bug in ~9.3 that meritates
some attention assuming that the latter behavior is legit.

And MinGW outputs the latter, similarly to other platforms... I'll investigate.

Look for something about setting the IEEE float rounding mode. "Round to
nearest even" is standard in most places, but it would not astonish me
to hear that Microsoft got that wrong.

If it is wrong I don't know that we'd want to back-patch a behavioral
change, but I'd definitely vote for conforming to the norm in 9.5
and later.

I have done more testing (done only 9.3 with MinGW and MSVC before)
and the failure can be reproduced on master and REL9_4_STABLE as well
because visibly with MSVC 2010 the version of rint used is the one of
src/port, which is defined like that:
return (x >= 0.0) ? floor(x + 0.5) : ceil(x - 0.5);
For example by passing 2.5, we would get 3.0, and not 2.0 (nearest
even number). So isn't the problem here?
--
Michael

--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

Michael Paquier <michael.paquier@gmail.com> writes:

On Mon, Mar 23, 2015 at 8:12 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Look for something about setting the IEEE float rounding mode. "Round to
nearest even" is standard in most places, but it would not astonish me
to hear that Microsoft got that wrong.

I have done more testing (done only 9.3 with MinGW and MSVC before)
and the failure can be reproduced on master and REL9_4_STABLE as well
because visibly with MSVC 2010 the version of rint used is the one of
src/port, which is defined like that:
return (x >= 0.0) ? floor(x + 0.5) : ceil(x - 0.5);
For example by passing 2.5, we would get 3.0, and not 2.0 (nearest
even number). So isn't the problem here?

Hmm ... why are we using src/port's version? rint() has been required
by POSIX for decades, surely MSVC has got it?

(IOW, I'd rather fix this by removing src/port's version than by
trying to upgrade it to full IEEE spec.)

regards, tom lane

--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

On Tue, Mar 24, 2015 at 10:37 PM, Tom Lane wrote:

Michael Paquier <michael.paquier@gmail.com> writes:

For example by passing 2.5, we would get 3.0, and not 2.0 (nearest
even number). So isn't the problem here?

Hmm ... why are we using src/port's version? rint() has been required
by POSIX for decades, surely MSVC has got it?

(IOW, I'd rather fix this by removing src/port's version than by
trying to upgrade it to full IEEE spec.)

rint() has been added in MSVC 2013, per se for example this thing
introduced by cec8394:
src/tools/msvc/Mkvcbuild.pm: push(@pgportfiles, 'rint.c') if
($vsVersion < '12.00');

So I would imagine that in a majority of cases the version of rint()
used by an MSVC build on Windows is src/port/rint.c, not to mention
that on platforms where rint() is not available (the world is full of
surprises), the behavior would be incorrect. It is possible to blame
Redmond's folks about missing rint() from MSVC for more than 10 years,
but the blame could be put on PG-side as well.

Hence I'd rather think that patching src/port is the way to go, with
for example something like the patch attached. That's a behavioral
change, so maybe a backpatch is not welcome. See for example Rich's
email meaning that 9.4 behavior is broken as I understand it, but
that's actually the other way around, and I suspect that the version
of 9.4 Rich has used was built with at least MSVC 2013, while the
version of 9.3 he used was built with src/port/rint.c.
--
Michael

Michael Paquier wrote, On 2015-03-25 01:19:

Hence I'd rather think that patching src/port is the way to go, with
for example something like the patch attached.

There are some corner cases that that patch does not handle properly,
most notably doubles that would overflow an integer. I suggest the
attached implementation (it's not as a patch, though). I've included a
test suite. The function can deal with non-IEEE floats too, but some of
the tests assume IEEE doubles.

On Tue, Mar 24, 2015 at 7:11 PM, Pedro Gimeno
<pgsql-004@personal.formauri.es> wrote:

Michael Paquier wrote, On 2015-03-25 01:19:

Hence I'd rather think that patching src/port is the way to go, with
for example something like the patch attached.

There are some corner cases that that patch does not handle properly,
most notably doubles that would overflow an integer. I suggest the
attached implementation (it's not as a patch, though). I've included a
test suite. The function can deal with non-IEEE floats too, but some of
the tests assume IEEE doubles.

copysign is not that portable, at least it is not in the MSVC world.
So as a patch you would get something like the attached with what you
wrote.
--
Michael

Michael Paquier wrote, On 2015-03-25 04:26:

copysign is not that portable, at least it is not in the MSVC world.
So as a patch you would get something like the attached with what you
wrote.

Here's a version without copysign.

Michael Paquier wrote, On 2015-03-25 04:26:

copysign is not that portable, at least it is not in the MSVC world.
So as a patch you would get something like the attached with what you
wrote.

The only point of using copysign there was to deal with a negative zero
corner case. The attached removes copysign from the test program too,
relying instead on sprintf outputting the sign of minus zero when available.

On Wed, Mar 25, 2015 at 8:18 PM, Pedro Gimeno
<pgsql-004@personal.formauri.es> wrote:

Michael Paquier wrote, On 2015-03-25 04:26:

copysign is not that portable, at least it is not in the MSVC world.
So as a patch you would get something like the attached with what you
wrote.

The only point of using copysign there was to deal with a negative zero
corner case. The attached removes copysign from the test program too,
relying instead on sprintf outputting the sign of minus zero when available.

Cool, thanks. Applied to the code tree of Postgres, it gives the patch attached.
--
Michael

Michael Paquier <michael.paquier@gmail.com> writes:

On Wed, Mar 25, 2015 at 8:18 PM, Pedro Gimeno
<pgsql-004@personal.formauri.es> wrote:

The only point of using copysign there was to deal with a negative zero
corner case. The attached removes copysign from the test program too,
relying instead on sprintf outputting the sign of minus zero when available.

Cool, thanks. Applied to the code tree of Postgres, it gives the patch attached.

This is pretty desperately in need of some comments, but I'll see what
I can do with it.

I assume the consensus is that we should not back-patch this? It seems
like the kind of behavioral change that people might get annoyed about.

regards, tom lane

--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

Tom Lane wrote, On 2015-03-25 15:37:

This is pretty desperately in need of some comments, but I'll see what
I can do with it.

Sorry about that. I hope the attached helps.

Pedro Gimeno wrote, On 2015-03-25 17:22:

Sorry about that. I hope the attached helps.

The previous version left out a corner case. Attached is a function and
test suite to deal with it.

Pedro Gimeno <pgsql-004@personal.formauri.es> writes:

The previous version left out a corner case. Attached is a function and
test suite to deal with it.

Hmm ... I'm thinking we probably should explicitly check for inf and NaN,
no?

if (isnan(x) || isinf(x))
return x;

It's possible the given coding would return this result anyway by
accident, but that seems rather fragile.

regards, tom lane

--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

Tom Lane wrote, On 2015-03-25 18:57:

Hmm ... I'm thinking we probably should explicitly check for inf and NaN,
no?

if (isnan(x) || isinf(x))
return x;

It's possible the given coding would return this result anyway by
accident, but that seems rather fragile.

I agree about NaN; it worked but mostly by accident. The big number
detection catches infinity unambiguously. Added to a comment in the
attached (plus corresponding tests).

Pedro Gimeno <pgsql-004@personal.formauri.es> writes:

Tom Lane wrote, On 2015-03-25 18:57:

Hmm ... I'm thinking we probably should explicitly check for inf and NaN,
no?

I agree about NaN; it worked but mostly by accident. The big number
detection catches infinity unambiguously. Added to a comment in the
attached (plus corresponding tests).

I fooled around with this some more for clarity and committed it.
Thanks for your work!

BTW, if memory serves we also have a pretty lazy rounding implementation
for the numeric datatype. I wonder if now would be a good time to upgrade
that to be round-to-nearest-even as well.

regards, tom lane

--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

On Thu, Mar 26, 2015 at 4:57 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Pedro Gimeno <pgsql-004@personal.formauri.es> writes:

Tom Lane wrote, On 2015-03-25 18:57:

Hmm ... I'm thinking we probably should explicitly check for inf and NaN,
no?

I agree about NaN; it worked but mostly by accident. The big number
detection catches infinity unambiguously. Added to a comment in the
attached (plus corresponding tests).

I fooled around with this some more for clarity and committed it.
Thanks for your work!

Thanks Tom for wrapping up stuff, and Pedro for the patch.
--
Michael

--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs