Re: Infinities in type numeric

On Tue, 16 Jun 2020 at 18:24, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> The attached v3 patch fixes these things and also takes care of an
> oversight in v2: I'd made numeric() apply typmod restrictions to Inf,
> but not numeric_in() or numeric_recv().  I believe the patch itself
> is in pretty good shape now, though there are still some issues
> elsewhere as noted in the first message in this thread.
>

I had a look at this, and I think it's mostly in good shape. It looks
like everything from the first message in this thread has been
resolved, except I don't know about the jsonpath stuff, because I
haven't been following that.

I tried to go over all the edge cases and I think they all make sense,
except for a couple of cases which I've listed below, along with a few
other minor comments:

1). I don't think that the way in_range() handles infinities is quite
right. For example:

SELECT in_range('inf'::numeric, 10::numeric, 'inf'::numeric, false, false);
 in_range
----------
 f
(1 row)

But I think that should return "val >= base + offset", which is "Inf
>= Inf", which should be true.

Similarly, I think this should return true:

SELECT in_range('-inf'::numeric, 10::numeric, 'inf'::numeric, true, true);
 in_range
----------
 f
(1 row)

I think this could use some test coverage.

2). I think numeric_pg_lsn() needs updating -- this should probably be an error:

SELECT pg_lsn('inf'::numeric);
 pg_lsn
--------
 0/0
(1 row)

3). In the bottom half of numeric.c, there's a section header comment
saying "Local functions follow ... In general, these do not support
NaNs ...". That should probably also mention infinities. There are
also now more functions to mention that are exceptions to that comment
about not handling NaN/Inf, but I think that some of the new
exceptions can be avoided.

4). The comment for set_var_from_str() mentions that it doesn't handle
"NaN", so on the face of it, it ought to also mention that it doesn't
handle "Infinity" either. However, this is only a few lines down from
that "Local functions follow ..." section header comment, which
already covers that, so it seems pointless mentioning NaNs and
infinities again for this function (none of the other local functions
in that section of the file do).

5). It seems a bit odd that numeric_to_double_no_overflow() handles
infinite inputs, but not NaN inputs, while its only caller
numeric_float8_no_overflow() handles NaNs, but not infinities. ISTM
that it would be neater to have all the special-handling in one place
(in the caller). That would also stop numeric_to_double_no_overflow()
being an exception to the preceding section header comment about local
functions not handling Nan/Inf. In fact, I wonder why keep
numeric_to_double_no_overflow() at all? It could just be rolled into
its caller, making it more like numeric_float8().

6). The next function, numericvar_to_double_no_overflow(), has a
comment that just says "As above, but work from a NumericVar", but it
isn't really "as above" anymore, since it doesn't handle infinite
inputs. Depending on what happens to numeric_to_double_no_overflow(),
this function's comment might need some tweaking.

7). The new function numeric_is_integral() feels out of place where it
is, amongst arithmetic functions operating on NumericVar's, because it
operates on a Numeric, and also because it handles NaNs, making it
another exception to the preceding comment about local functions that
don't handle NaNs. Perhaps it would fit in better after
numeric_is_nan() and numeric_is_inf(). Even though it's a local
function, it feels more akin to those functions.

Finally, not really in the scope of this patch, but something I
noticed anyway while looking at edge cases -- float and numeric handle
NaN/0 differently:

SELECT 'nan'::float8 / 0::float8;
ERROR:  division by zero

SELECT 'nan'::numeric / 0::numeric;
 ?column?
----------
      NaN

I'm not sure if this is worth worrying about, or which behaviour is
preferable though.

Regards,
Dean