pgsql: Ensure that numeric.c compiles with other NBASE values.

Dean Rasheed <dean.a.rasheed@gmail.com> writes:

Ensure that numeric.c compiles with other NBASE values.

Looking at this diff made me wonder why the static pow10[] array
isn't marked "const"?

regards, tom lane

On Thu, 2 Feb 2023 at 15:15, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Looking at this diff made me wonder why the static pow10[] array
isn't marked "const"?

Good point.

However, looking more closely, I think this function is more or less
completely broken:

1). It doesn't work if log10val2 < 0, because then m < 0, and it
doesn't multiply by the remainder. And it then throws an overflow
error, because result.dscale comes out wrong when m < 0.

2). The result.dscale calculation is wrong if log10val2 is a multiple
of DEC_DIGITS, causing it to drop the last 4 digits.

3). If the scaled-up dividend doesn't fit in an int64, the numeric
computation breaks if log10val2 >= 10 due to integer overflow.

So for example:

int64_div_fast_to_numeric(123456, -1) -> ERROR: value overflows numeric format
int64_div_fast_to_numeric(123456, 0) -> ERROR: value overflows numeric format
int64_div_fast_to_numeric(123456, 4) -> 12
int64_div_fast_to_numeric(123456, 8) -> 0.0012
int64_div_fast_to_numeric(1000000000000000000, 10) -> 709186959.9285992800

As it happens, none of the above represents a live bug, because we
currently only call it with log10val2 = 3 or 6, but it's definitely a
bug waiting to happen.

This was added by a2da77cdb4 ("Change return type of EXTRACT to
numeric"), so it only goes back as far as v14.

After hacking on it for a while, I ended up with the attached.

Regards,
Dean

Dean Rasheed <dean.a.rasheed@gmail.com> writes:

However, looking more closely, I think this function is more or less
completely broken:

1). It doesn't work if log10val2 < 0, because then m < 0, and it
doesn't multiply by the remainder. And it then throws an overflow
error, because result.dscale comes out wrong when m < 0.

2). The result.dscale calculation is wrong if log10val2 is a multiple
of DEC_DIGITS, causing it to drop the last 4 digits.

3). If the scaled-up dividend doesn't fit in an int64, the numeric
computation breaks if log10val2 >= 10 due to integer overflow.

Ugh.

I'm not quite sure that it's worth expending code space on the
log10val2 < 0 case (compared to just "Assert(log10val2 >= 0").
On the other hand, it's not much extra code, and committing it now
might save somebody reinventing that logic in future.

I've not actually tested the patch, but it looks reasonable
by eyeball.

regards, tom lane

On Fri, 3 Feb 2023 at 01:18, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Dean Rasheed <dean.a.rasheed@gmail.com> writes:

1). It doesn't work if log10val2 < 0, because then m < 0, and it
doesn't multiply by the remainder. And it then throws an overflow
error, because result.dscale comes out wrong when m < 0.

I'm not quite sure that it's worth expending code space on the
log10val2 < 0 case (compared to just "Assert(log10val2 >= 0").
On the other hand, it's not much extra code, and committing it now
might save somebody reinventing that logic in future.

Yeah, I thought about that, but it's hardly any code to support that
case. Also, this function is out there now (I found an example on
Stack Overflow of someone using it), so we have no control over how
people will use it in their own C code, and so I think it's worth
making it robust across the range of possible inputs.

Regards,
Dean