From: Tom Lane <tgl@sss.pgh.pa.us>
To: Andrew Gierth <andrew@tao11.riddles.org.uk>
cc: victor@magic.io, pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #15519: Casting float4 into int4 gets the wrong sign instead
 of "integer out of range" error
In-reply-to: <874lc7sb1a.fsf@news-spur.riddles.org.uk>
References: <15519-4fc785b483201ff1@postgresql.org>
 <874lc7sb1a.fsf@news-spur.riddles.org.uk>
Comments: In-reply-to Andrew Gierth <andrew@tao11.riddles.org.uk>
	message dated "Fri, 23 Nov 2018 22:21:07 +0000"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <13161.1543014227.1@sss.pgh.pa.us>
Content-Transfer-Encoding: quoted-printable
Date: Fri, 23 Nov 2018 18:03:47 -0500
Message-ID: <13162.1543014227@sss.pgh.pa.us>
Precedence: bulk

Andrew Gierth <andrew@tao11.riddles.org.uk> writes:
> "PG" =3D=3D PG Bug reporting form <noreply@postgresql.org> writes:
>  PG> Offending examples:
>  PG> SELECT ((2147483647::float4) - 1.0::float4)::int4;
>  PG> SELECT ((2147483590::float4) - 1.0::float4)::int4;
>  PG> SELECT ((2147483647::float4) + 1.0::float4)::int4;
>  PG> They all produce the same result: -2147483648

> The bug here is that the ftoi4 conversion function thinks it can do
> this:
>     if (num < INT_MIN || num > INT_MAX || isnan(num))
> Probably this should be changed to use ((float8) INT_MAX) etc. instead?

After thinking about this for awhile, there's another class of undesirable
behavior here, which can be illustrated by

regression=3D# select '32766.1'::float4::int2;
 int2  =

-------
 32766
(1 row)

regression=3D# select '32767.1'::float4;
 float4  =

---------
 32767.1
(1 row)

regression=3D# select '32767.1'::float4::int2;
ERROR:  smallint out of range

It doesn't seem very nice that that fails rather than producing 32767.

I think we could fix that by doing the rint() first, before the
comparisons, which should be safe enough: as the Linux man page for it
points out, it really can't ever fail.

I'm inclined to write ftoi4 as

    /*
     * Get rid of any fractional part in the input.  This is so we don't
     * fail on just-out-of-range values that would round into range.
     */
    num =3D rint(num);

    /*
     * Range check.  We must be careful here that the boundary values are
     * expressed exactly in the appropriate float domain; we assume float4
     * is going to round off INT_MAX to a power of 2.  Also note assumptio=
n
     * that rint() will pass through a NaN or Inf unchanged.
     */
     if (unlikely(num < (float4) INT_MIN || num >=3D (float4) INT_MAX || i=
snan(num)))
        ereport(...);

    PG_RETURN_INT32((int32) num);

We could use the same pattern for the other five functions at issue,
but "num >=3D" would be "num >" in cases where we expect the comparison
value to be represented without roundoff.

Obviously, some regression test cases to verify all these assumptions
would be a good idea.

			regards, tom lane