On Tue, Jul 16, 2024 at 09:23:27PM -0400, Joseph Koshakow wrote:
> On Tue, Jul 16, 2024 at 1:57 PM Nathan Bossart <nathandbossart@gmail.com>
> wrote:
>> The reason I suggested this is so that we could omit all the prerequisite
>> isinf(), isnan(), etc. checks in the cash_mul_float8() and friends. The
>> checks are slighly different, but from a quick glance it just looks like
> we
>> might end up relying on the FLOAT8_FITS_IN_INT64 check in more cases.
>
> I don't think we can omit the prerequisite isnan() checks. Neither
> float8_mul() nor float8_div() reject nan inputs/result, and
> FLOAT8_FITS_IN_INT64 has the following to say about inf and nan
>
> These macros will do the right thing for Inf, but not necessarily for
> NaN,
> so check isnan(num) first if that's a possibility.
My instinct would be to just let float8_mul(), float8_div(), etc. handle
the inputs and then to add an additional isnan() check for the result.
This is how it's done elsewhere (e.g., dtoi8() in int8.c). If something
about the float8 helper functions is inadequate, let's go fix those
instead.
> Though, I think you're right that we can remove the isinf() check from
> cash_mul_float8(). That check is fully covered by FLOAT8_FITS_IN_INT64,
> since all infinite inputs will result in an infinite output. That also
> makes the infinite result check in float8_mul() redundant.
> Additionally, I believe that the underflow check in float8_mul() is
> unnecessary. val1 is an int64 casted to a float8, so it can never be
> -1 < val < 1, so it can never cause an underflow to 0. So I went ahead
> and removed float8_mul() since all of its checks are redundant.
TBH I'd rather keep this stuff super simple by farming out the corner case
handling whenever we can, even if it is redundant in a few cases. That
keeps it centralized and consistent with the rest of the tree so that any
fixes apply everywhere. If there's a performance angle, then maybe it's
worth considering the added complexity, though...
> For cash_div_float8() we have a choice. The isinf() input check
> protects against the following, which is not rejected by any of
> the other checks.
>
> test=# SELECT '5'::money / 'inf'::float8;
> ?column?
> ----------
> $0.00
> (1 row)
>
> For now, I've kept the isinf() input check to reject the above query,
> let me know if you think we should allow this.
We appear to allow this for other data types, so I see no reason to
disallow it for the money type.
I've attached an editorialized version of 0002 based on my thoughts above.
--
nathan