Обсуждение: pgsql: Ensure that numeric.c compiles with other NBASE values.
Ensure that numeric.c compiles with other NBASE values. As noted in the comments, support for different NBASE values is really only of historical interest, but as long as we're keeping it, we might as well make sure that it compiles. Joel Jacobson Discussion: https://postgr.es/m/06712c29-98e9-43b3-98da-f234d81c6e49%40app.fastmail.com Branch ------ master Details ------- https://git.postgresql.org/pg/commitdiff/9a84f2947bf9345ad6b93ba37da63633649eaea8 Modified Files -------------- src/backend/utils/adt/numeric.c | 8 ++++++++ 1 file changed, 8 insertions(+)
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