Proposal for internal Numeric to Uint64 conversion function.

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

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