Thread: Proposal for internal Numeric to Uint64 conversion function.
Hi, Currently, numeric_pg_lsn is the only one that accepts the Numeric value and converts to uint64 and that is the reason all the type conversion code is embedded into it. I think it would be helpful if that numeric-to-uint64 conversion code is extracted as a separate function so that any other SQL function like numeric_pg_lsn which wants to accept values up to the uint64 range can use that function. Thoughts? Comments? -- Regards, Amul Sul EDB: http://www.enterprisedb.com
Attachment
On 16.02.22 06:00, Amul Sul wrote: > Currently, numeric_pg_lsn is the only one that accepts the Numeric > value and converts to uint64 and that is the reason all the type > conversion code is embedded into it. There are other functions such as numeric_int8() that work similarly. If you are going to refactor, then they should all be treated similarly. I'm not sure if it's going to end up being beneficial.
On Wed, Feb 16, 2022 at 4:50 PM Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote: > > On 16.02.22 06:00, Amul Sul wrote: > > Currently, numeric_pg_lsn is the only one that accepts the Numeric > > value and converts to uint64 and that is the reason all the type > > conversion code is embedded into it. > > There are other functions such as numeric_int8() that work similarly. > If you are going to refactor, then they should all be treated similarly. > I'm not sure if it's going to end up being beneficial. Yeah, that's true, I am too not sure if we really need to refactor all those; If we want, I can give it a try. The intention here is to add a function that will convert numeric to uint64 -- we don't have any as of now, if I am not wrong. Regards, Amul
Amul Sul <sulamul@gmail.com> writes:
> On Wed, Feb 16, 2022 at 4:50 PM Peter Eisentraut
> <peter.eisentraut@enterprisedb.com> wrote:
>> On 16.02.22 06:00, Amul Sul wrote:
>>> Currently, numeric_pg_lsn is the only one that accepts the Numeric
>>> value and converts to uint64 and that is the reason all the type
>>> conversion code is embedded into it.
>> There are other functions such as numeric_int8() that work similarly.
>> If you are going to refactor, then they should all be treated similarly.
>> I'm not sure if it's going to end up being beneficial.
> Yeah, that's true, I am too not sure if we really need to refactor
> all those; If we want, I can give it a try.
There are several places that call numeric_int8, and right now they
have to go through DirectFunctionCall1, which is ugly and inefficient.
I think a refactoring that exposes some more-convenient API for that
could be useful. The patch as-presented isn't very compelling for
lack of callers of the new function; but if it were handling the
int64 and uint64 cases alike, and maybe the float8 case too, that
would seem more convincing. We already expose APIs like int64_to_numeric,
so the lack of similar APIs for the other direction seems odd.
It also feels to me that numeric_pg_lsn(), per se, doesn't belong in
numeric.c. A pretty direct comparison is numeric_cash(), which is
not in numeric.c but cash.c.
regards, tom lane
On Fri, 11 Mar 2022 at 15:17, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Amul Sul <sulamul@gmail.com> writes: > > Yeah, that's true, I am too not sure if we really need to refactor > > all those; If we want, I can give it a try. > > The patch as-presented isn't very compelling for > lack of callers of the new function Tom, are you saying you think we're not interested in just adding this function unless it's part of this refactoring? Amul, do you think if we did numeric_to_int64/numeric_to_uint64 as a refactored API and a second patch that made numeric_pg_lsn and other consumers use it it would clean up the code significantly? -- greg
Greg Stark <stark@mit.edu> writes:
> On Fri, 11 Mar 2022 at 15:17, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> The patch as-presented isn't very compelling for
>> lack of callers of the new function
> Tom, are you saying you think we're not interested in just adding this
> function unless it's part of this refactoring?
Pretty much, yeah. I'm way more interested in cleaning up the code
we have than in making things prettier for hypothetical future
call sites. In particular, the problem with writing an API in a
vacuum is that you have little evidence that it's actually useful
as given (e.g., did you handle error cases in a useful way). If we
create a numeric_to_int64 that is actually used right away by some
existing callers, then we've got some evidence that we did it right;
and then introducing a parallel numeric_to_uint64 is less of a leap
of faith.
regards, tom lane
On Thu, Mar 17, 2022 at 4:02 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > Pretty much, yeah. I'm way more interested in cleaning up the code > we have than in making things prettier for hypothetical future > call sites. In particular, the problem with writing an API in a > vacuum is that you have little evidence that it's actually useful > as given (e.g., did you handle error cases in a useful way). If we > create a numeric_to_int64 that is actually used right away by some > existing callers, then we've got some evidence that we did it right; > and then introducing a parallel numeric_to_uint64 is less of a leap > of faith. Based on this review and the fact that there's been no new patch since the original version, I've marked this Returned with Feedback in the CommitFest. If Amul decides to update the patch as Tom is describing, he can reactivate the CommitFest entry at that time. -- Robert Haas EDB: http://www.enterprisedb.com
On Fri, Mar 18, 2022 at 1:17 AM Greg Stark <stark@mit.edu> wrote: > > On Fri, 11 Mar 2022 at 15:17, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > > > Amul Sul <sulamul@gmail.com> writes: > > > > Yeah, that's true, I am too not sure if we really need to refactor > > > all those; If we want, I can give it a try. > > > > The patch as-presented isn't very compelling for > > lack of callers of the new function > > Tom, are you saying you think we're not interested in just adding this > function unless it's part of this refactoring? > > Amul, do you think if we did numeric_to_int64/numeric_to_uint64 as a > refactored API and a second patch that made numeric_pg_lsn and other > consumers use it it would clean up the code significantly? Sorry for the long absence. Yes, I think we can do cleanup to some extent. Attaching the following patches that first intend to remove DirectFunctionCall as much as possible: 0001: Refactors and moves numeric_pg_lsn to pg_lsn.c file. Function gut is in numeric.c as numeric_to_uint64_type() generalised function that can be used elsewhere too. 0002: Does little more cleanup to pg_lsn.c file -- replace few DirectFunctionCall1() by the numeric_to_uint64_type(). 0003: Refactors numeric_int8() function and replace few DirectFunctionCall1() to numeric_int8 by the newly added numeric_to_int64() and numeric_to_int64_type(). numeric_to_int64_type() version can be called only when you want to refer to the specific type name in the error message like numeric_to_uint64_type, e.g.MONEY type. 0004: Adding float8_to_numeric and numeric_to_float8 by refactoring float8_numeric and numeric_float8 respectively. I am a bit confused about whether the type should be referred to as float8 or double. Replaces a few DirectFunctionCall() calls by these c functions. 0005: This one replaces all DirectFunctionCall needed for the numeric arithmetic operations. Regards, Amul
Attachment
On 22.04.22 14:26, Amul Sul wrote: > Yes, I think we can do cleanup to some extent. Attaching the > following patches that first intend to remove DirectFunctionCall as > much as possible: Do you have any data that supports removing DirectionFunctionCall() invocations? I suppose some performance benefit could be expected, or what do you have in mind?
On Mon, May 2, 2022 at 8:23 PM Peter Eisentraut
<peter.eisentraut@enterprisedb.com> wrote:
>
> On 22.04.22 14:26, Amul Sul wrote:
> > Yes, I think we can do cleanup to some extent. Attaching the
> > following patches that first intend to remove DirectFunctionCall as
> > much as possible:
>
> Do you have any data that supports removing DirectionFunctionCall()
> invocations? I suppose some performance benefit could be expected, or
> what do you have in mind?
>
Not really, the suggestion to avoid DirectionFunctionCall() is from Tom.
For a trial, I have called the money(numeric) function 10M times to
see the difference with and without patch but there is not much.
(I don't have any knowledge of micro profiling/benchmarking).
Next I have looked in the profile stack for numeric_mul_opt_error()
execution which is called by the numeric_cash() via
DirectionFunctionCall() to numeric_mul() see this:
0.99% postgres postgres [.] numeric_mul_opt_error
|
--- numeric_mul_opt_error
|
|--94.86%-- numeric_mul
| DirectFunctionCall2Coll
| numeric_cash
| ExecInterpExpr
| ExecScan
| ExecSeqScan
| standard_ExecutorRun
| ExecutorRun
| PortalRunSelect
| PortalRun
| exec_simple_query
| PostgresMain
| ServerLoop
| PostmasterMain
| main
| __libc_start_main
|
--5.14%-- DirectFunctionCall2Coll
numeric_cash
ExecInterpExpr
ExecScan
ExecSeqScan
standard_ExecutorRun
ExecutorRun
PortalRunSelect
PortalRun
exec_simple_query
PostgresMain
ServerLoop
PostmasterMain
main
__libc_start_main
AFAIC, DirectionFunctionCall() involves some cost and the question is
why not to call numeric_mul_opt_error() directly, if that is possible.
Regards,
Amul
On 03.05.22 08:50, Amul Sul wrote: >> Do you have any data that supports removing DirectionFunctionCall() >> invocations? I suppose some performance benefit could be expected, or >> what do you have in mind? >> > Not really, the suggestion to avoid DirectionFunctionCall() is from Tom. > > For a trial, I have called the money(numeric) function 10M times to > see the difference with and without patch but there is not much. > (I don't have any knowledge of micro profiling/benchmarking). Ok. I have lost track of what you are trying to achieve with this patch set. It's apparently not for performance, and in terms of refactoring you end up with more lines of code than before, so that doesn't seem too appealing either. So I'm not sure what the end goal is.
On Tue, May 3, 2022 at 8:04 PM Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote: > > On 03.05.22 08:50, Amul Sul wrote: > >> Do you have any data that supports removing DirectionFunctionCall() > >> invocations? I suppose some performance benefit could be expected, or > >> what do you have in mind? > >> > > Not really, the suggestion to avoid DirectionFunctionCall() is from Tom. > > > > For a trial, I have called the money(numeric) function 10M times to > > see the difference with and without patch but there is not much. > > (I don't have any knowledge of micro profiling/benchmarking). > > Ok. I have lost track of what you are trying to achieve with this patch > set. It's apparently not for performance, and in terms of refactoring > you end up with more lines of code than before, so that doesn't seem too > appealing either. So I'm not sure what the end goal is. > Well, that is still worth it, IMHO. Refactoring allows us to place numeric_pg_lsn to the correct file where it should be. This refactoring, giving a function that converts numeric to uint64. The same function is used in other routines of pg_lsn.c which is otherwise using DirectFunctionCall(). Refactoring of numeric_int8() giving a function to convert numeric to int64 which can be used at the many places without using DirectFuctionCall(), and that is not the sole reason of it -- we have a function that converts int64 to numeric but for the opposite conversion needs to use DirectFuctionCall() which doesn't make sense. Along with this refactoring there are different routing calling functions for the numeric arithmetic operation through DirectFunctionCall() which isn't necessary since the required arithmetic functions are already accessible. The last benefit that is not completely worthless is avoiding DirectFunctionCall() one less function call stack and one less work that the system needs to do the same task. Regards, Amul
Hi, The patch for this CF entry doesn't apply currently, and it looks like that's been the case for quite a while... Greetings, Andres Freund
2022年10月3日(月) 1:55 Andres Freund <andres@anarazel.de>: > > Hi, > > The patch for this CF entry doesn't apply currently, and it looks like that's > been the case for quite a while... As that's remained the case and the last update from the author was in May, we'll close this as "returned with feedback". A new entry can always be created in future commitfest if work on the patch is resumed. Regards Ian Barwick