On 19.03.21 21:06, Tom Lane wrote:
> Yeah, I was wondering if we could do something like that, but I hadn't
> got as far as figuring a way to deal with divisors not a multiple of
> NBASE.
>
> Looking at the proposed code, I wonder if it wouldn't be better to
> define the function as taking the base-10-log of the divisor, so that
> you'd have the number of digits to shift (and the dscale) immediately
> instead of needing repeated integer divisions to get that.
done that way, much simpler now
> Also, the
> risk of intermediate overflow here seems annoying:
>
> + if (unlikely(pg_mul_s64_overflow(val1, NBASE/x, &val1)))
> + elog(ERROR, "overflow");
>
> Maybe that's unreachable for the ranges of inputs the current patch could
> create, but it seems like it makes the function distinctly less
> general-purpose than one would think from its comment. Maybe, if that
> overflows, we could handle the failure by making that adjustment after
> we've converted to numeric?
also done
I also figured out a way to combine the float8 and numeric
implementations so that there is not so much duplication. Added tests
to cover all the edge and overflow cases.
I think this is solid now.
The extract(julian from timestamp) is still a bit in the slow mode, but
as I previously stated, it's not documented and gives the wrong result,
so it's not clear whether it should be fixed and what it should do. I
think I'll register that part as an open item in any case, to see what
we should do about that.