Re: Since '2001-09-09 01:46:40'::timestamp microseconds are lost when extracting epoch

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
20 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: Since '2001-09-09 01:46:40'::timestamp microseconds are lost when extracting epoch

Peter Eisentraut-6
On 2019-12-02 23:52, Thomas Munro wrote:
>> I'm not an expert in floating point math but hopefully it means that no
>> type change is required - double precision can handle it.
> Me neither, but the SQL standard requires us to use an exact numeric
> type, so it's wrong on that level by definition.

I looked into this (changing the return types of date_part()/extract()
from float8 to numeric).

One problem (other than perhaps performance, tbd.) is that this would no
longer allow processing infinite timestamps, since numeric does not
support infinity.  It could be argued that running extract() on infinite
timestamps isn't very useful, but it's something to consider explicitly.

--
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


Reply | Threaded
Open this post in threaded view
|

Re: Since '2001-09-09 01:46:40'::timestamp microseconds are lost when extracting epoch

Tom Lane-2
Peter Eisentraut <[hidden email]> writes:
> One problem (other than perhaps performance, tbd.) is that this would no
> longer allow processing infinite timestamps, since numeric does not
> support infinity.  It could be argued that running extract() on infinite
> timestamps isn't very useful, but it's something to consider explicitly.

I wonder if it's time to fix that, ie introduce +-Infinity into numeric.c.
This isn't the first time we've seen issues with numeric not being a
superset of float, and it won't be the last.

At first glance there's no free bits in the on-disk format for numeric,
but we could do something by defining the low-order bits of the header
word for a NaN to distinguish between real NaN and +/- infinity.
It looks like those bits should reliably be zero right now.

                        regards, tom lane


Reply | Threaded
Open this post in threaded view
|

Re: Since '2001-09-09 01:46:40'::timestamp microseconds are lost when extracting epoch

Vik Fearing-6
In reply to this post by Peter Eisentraut-6
On 5/25/20 3:28 PM, Peter Eisentraut wrote:
> On 2019-12-02 23:52, Thomas Munro wrote:
>>> I'm not an expert in floating point math but hopefully it means that no
>>> type change is required - double precision can handle it.
>> Me neither, but the SQL standard requires us to use an exact numeric
>> type, so it's wrong on that level by definition.
>
> I looked into this (changing the return types of date_part()/extract()
> from float8 to numeric).

I think what would be better is to have a specific date_part function
for each part and have extract translate to the appropriate one.  This
is particularly interesting for epoch but it would also allow us to
return the correct type mandated by the spec.

(I would also accept a specific date_part per return type instead of per
part, that would probably even be better.)
--
Vik Fearing


Reply | Threaded
Open this post in threaded view
|

Re: Since '2001-09-09 01:46:40'::timestamp microseconds are lost when extracting epoch

Tom Lane-2
Vik Fearing <[hidden email]> writes:
> On 5/25/20 3:28 PM, Peter Eisentraut wrote:
>> I looked into this (changing the return types of date_part()/extract()
>> from float8 to numeric).

> I think what would be better is to have a specific date_part function
> for each part and have extract translate to the appropriate one.

Doesn't really work for upwards compatibility with existing views,
which will have calls to date_part(text, ...) embedded in them.

Actually, now that I think about it, changing the result type of
date_part() is likely to be problematic anyway for such cases.
It's not going to be good if pg_upgrade's dump/restore of a view
results in a new output column type; especially if it's a
materialized view.

So maybe what we'd have to do is leave date_part() alone for
legacy compatibility, and invent new functions that the extract()
syntax would now be translated to.  While at it, maybe we could
fix things so that the syntax reverse-lists the same way instead
of injecting Postgres-isms...

                        regards, tom lane


Reply | Threaded
Open this post in threaded view
|

Re: Since '2001-09-09 01:46:40'::timestamp microseconds are lost when extracting epoch

Vik Fearing-6
On 5/25/20 6:40 PM, Tom Lane wrote:

> Vik Fearing <[hidden email]> writes:
>> On 5/25/20 3:28 PM, Peter Eisentraut wrote:
>>> I looked into this (changing the return types of date_part()/extract()
>>> from float8 to numeric).
>
>> I think what would be better is to have a specific date_part function
>> for each part and have extract translate to the appropriate one.
>
> Doesn't really work for upwards compatibility with existing views,
> which will have calls to date_part(text, ...) embedded in them.
>
> Actually, now that I think about it, changing the result type of
> date_part() is likely to be problematic anyway for such cases.
> It's not going to be good if pg_upgrade's dump/restore of a view
> results in a new output column type; especially if it's a
> materialized view.
>
> So maybe what we'd have to do is leave date_part() alone for
> legacy compatibility, and invent new functions that the extract()
> syntax would now be translated to.


I'm sorry, I wasn't clear.  I was suggesting adding new functions while
also keeping the current generic function.  So exactly what you say in
that last paragraph.

Although <extract expression> has a fixed list of constant parts,
date_part() allows the part to be variable.  So we need to keep it
anyway for cases like this contrived example:

    SELECT date_part(p, now())
    FROM UNNEST(ARRAY['epoch', 'year', 'second']) AS u (p)


> While at it, maybe we could
> fix things so that the syntax reverse-lists the same way instead
> of injecting Postgres-isms...


I'm not sure what this means.
--
Vik Fearing


Reply | Threaded
Open this post in threaded view
|

Re: Since '2001-09-09 01:46:40'::timestamp microseconds are lost when extracting epoch

Tom Lane-2
Vik Fearing <[hidden email]> writes:
> On 5/25/20 6:40 PM, Tom Lane wrote:
>> While at it, maybe we could
>> fix things so that the syntax reverse-lists the same way instead
>> of injecting Postgres-isms...

> I'm not sure what this means.

This:

regression=# create view myview as select extract(year from current_timestamp) as y;
CREATE VIEW
regression=# \d+ myview
...
View definition:
 SELECT date_part('year'::text, CURRENT_TIMESTAMP) AS y;

What had been a 100% spec-compliant view definition is now quite
Postgres-specific.  I fixed some similar problems in 0bb51aa96 (before
that, the CURRENT_TIMESTAMP part would've reverse-listed differently
too); but I didn't tackle EXTRACT(), SUBSTRING(), and other cases.

I'm not claiming that we really need to fix all of those.  But if we are
going to pick nits about which data type EXTRACT() returns then I think
it's legit to worry about its reverse-list representation at the same
time ... especially if we must touch the grammar's translation anyway.

                        regards, tom lane


Reply | Threaded
Open this post in threaded view
|

Re: Since '2001-09-09 01:46:40'::timestamp microseconds are lost when extracting epoch

Tom Lane-2
I wrote:
> What had been a 100% spec-compliant view definition is now quite
> Postgres-specific.  I fixed some similar problems in 0bb51aa96 (before
> that, the CURRENT_TIMESTAMP part would've reverse-listed differently
> too); but I didn't tackle EXTRACT(), SUBSTRING(), and other cases.
> I'm not claiming that we really need to fix all of those.  But if we are
> going to pick nits about which data type EXTRACT() returns then I think
> it's legit to worry about its reverse-list representation at the same
> time ... especially if we must touch the grammar's translation anyway.

BTW, shortly after sending that I had an idea about how to do it without
adding a boatload of new parsetree infrastructure, which has been the
main reason why nobody has wanted to tackle it.  The obvious way to do
this is to make a new kind of expression node, but that cascades into
lots and lots of places (see 0bb51aa96, plus the later commits that
fixed oversights in it :-().  It's a lot of work for a mostly-cosmetic
issue.

However: suppose that we continue to translate these things into FuncExpr
nodes, the same as always, but we add a new CoercionForm variant, say
COERCE_SQL_SYNTAX.  99% of the system ignores FuncExpr.funcformat,
and would continue to do so, but ruleutils.c would take it to mean
that (1) the call should be reverse-listed as some special SQL syntax
and (2) the funcid is one of a small set of built-in functions for
which ruleutils.c knows what to emit.  (If it doesn't recognize the
funcid, it could either throw an error, or fall back to normal display
of the node.)  For cases such as EXTRACT, this would also represent
a promise that specific arguments are Const nodes from which the
desired keyword can be extracted.

This is kind of an abuse of "CoercionForm", since that typedef name
implies that it only talks about how to handle cast cases, but
semantically it's always been a how-to-display-function-calls thing.
We could either hold our noses about that or rename the typedef.

If we went this way then we could easily clean up most of the other
weird-SQL-syntax function call cases, incrementally over time,
without a lot of additional work.

                        regards, tom lane


Reply | Threaded
Open this post in threaded view
|

Re: Since '2001-09-09 01:46:40'::timestamp microseconds are lost when extracting epoch

David Fetter
In reply to this post by Tom Lane-2
On Mon, May 25, 2020 at 09:43:32AM -0400, Tom Lane wrote:

> Peter Eisentraut <[hidden email]> writes:
> > One problem (other than perhaps performance, tbd.) is that this would no
> > longer allow processing infinite timestamps, since numeric does not
> > support infinity.  It could be argued that running extract() on infinite
> > timestamps isn't very useful, but it's something to consider explicitly.
>
> I wonder if it's time to fix that, ie introduce +-Infinity into numeric.c.
> This isn't the first time we've seen issues with numeric not being a
> superset of float, and it won't be the last.
>
> At first glance there's no free bits in the on-disk format for numeric,
> but we could do something by defining the low-order bits of the header
> word for a NaN to distinguish between real NaN and +/- infinity.
> It looks like those bits should reliably be zero right now.

+1 for adding +/- infinity to NUMERIC.

Best,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate


Reply | Threaded
Open this post in threaded view
|

Re: Since '2001-09-09 01:46:40'::timestamp microseconds are lost when extracting epoch

Peter Eisentraut-6
In reply to this post by Peter Eisentraut-6
On 2020-05-25 15:28, Peter Eisentraut wrote:

> On 2019-12-02 23:52, Thomas Munro wrote:
>>> I'm not an expert in floating point math but hopefully it means that no
>>> type change is required - double precision can handle it.
>> Me neither, but the SQL standard requires us to use an exact numeric
>> type, so it's wrong on that level by definition.
>
> I looked into this (changing the return types of date_part()/extract()
> from float8 to numeric).
>
> One problem (other than perhaps performance, tbd.) is that this would no
> longer allow processing infinite timestamps, since numeric does not
> support infinity.  It could be argued that running extract() on infinite
> timestamps isn't very useful, but it's something to consider explicitly.
Now that numeric supports infinity, here is a patch that changes the
return types of date_part() to numeric.  It's not meant to be a final
version, but it is useful for discussing a few things.

The internal implementation could be made a bit more elegant if we had
variants of int4_numeric() and int8_numeric() that don't have to go
through fmgr.  This would also help in other areas of the code.  There
are probably also other ways in which the internals could be made more
compact; I just converted them fairly directly.

When extracting seconds or microseconds, I made it always produce 6 or 3
decimal places, even if they are zero.  I don't know if we want that or
what behavior we want.  That's what all the changes in the regression
tests are about.  Everything else passes unchanged.

The 'julian' field is a bit of a mystery.  First of all it's not
documented.  The regression tests only test the rounded output, perhaps
to avoid floating point differences.  When you do date_part('julian',
date), then you get a correct Julian Day.  But date_part('julian',
timestamp[tz]) gives incorrect Julian Date values that are off by 12
hours.  My patch doesn't change that, I just noticed when I took away
the round() call in the regression tests.  Those calls now produce a
different number of decimal places.

It might make sense to make date_part(..., date) a separate C function
instead of an SQL wrapper around date_part(..., timestamp).  That could
return integer and could reject nonsensical fields such as "minute".
Then we could also make a less contorted implementation of
date_part('julian', date) that matches to_char(date, 'J') and remove the
incorrect implementation of date_part('julian', timestamp).

--
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

v1-0001-Change-return-type-of-EXTRACT-to-numeric.patch (112K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Since '2001-09-09 01:46:40'::timestamp microseconds are lost when extracting epoch

Pavel Stehule


út 4. 8. 2020 v 16:08 odesílatel Peter Eisentraut <[hidden email]> napsal:
On 2020-05-25 15:28, Peter Eisentraut wrote:
> On 2019-12-02 23:52, Thomas Munro wrote:
>>> I'm not an expert in floating point math but hopefully it means that no
>>> type change is required - double precision can handle it.
>> Me neither, but the SQL standard requires us to use an exact numeric
>> type, so it's wrong on that level by definition.
>
> I looked into this (changing the return types of date_part()/extract()
> from float8 to numeric).
>
> One problem (other than perhaps performance, tbd.) is that this would no
> longer allow processing infinite timestamps, since numeric does not
> support infinity.  It could be argued that running extract() on infinite
> timestamps isn't very useful, but it's something to consider explicitly.

Now that numeric supports infinity, here is a patch that changes the
return types of date_part() to numeric.  It's not meant to be a final
version, but it is useful for discussing a few things.

The internal implementation could be made a bit more elegant if we had
variants of int4_numeric() and int8_numeric() that don't have to go
through fmgr.  This would also help in other areas of the code.  There
are probably also other ways in which the internals could be made more
compact; I just converted them fairly directly.

When extracting seconds or microseconds, I made it always produce 6 or 3
decimal places, even if they are zero.  I don't know if we want that or
what behavior we want.  That's what all the changes in the regression
tests are about.  Everything else passes unchanged.

The 'julian' field is a bit of a mystery.  First of all it's not
documented.  The regression tests only test the rounded output, perhaps
to avoid floating point differences.  When you do date_part('julian',
date), then you get a correct Julian Day.  But date_part('julian',
timestamp[tz]) gives incorrect Julian Date values that are off by 12
hours.  My patch doesn't change that, I just noticed when I took away
the round() call in the regression tests.  Those calls now produce a
different number of decimal places.

It might make sense to make date_part(..., date) a separate C function
instead of an SQL wrapper around date_part(..., timestamp).  That could
return integer and could reject nonsensical fields such as "minute".
Then we could also make a less contorted implementation of
date_part('julian', date) that matches to_char(date, 'J') and remove the
incorrect implementation of date_part('julian', timestamp).

I like a idea to have d date variant of date_part

Pavel


--
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Reply | Threaded
Open this post in threaded view
|

Re: Since '2001-09-09 01:46:40'::timestamp microseconds are lost when extracting epoch

Peter Eisentraut-6
Here is a new patch series version.

I have created a new internal function for converting integers to
numeric, to make the implementation a bit more elegant and compact.

I have also created a new date_part(..., date) in C, and added more test
coverage for that.

Other than some of the semantic issues mentioned in the previous
message, this version looks pretty good to me in principle.

I have done some performance tests to assess the impact of changing from
float to numeric.  I did tests like this:

create table t1 (a int, b timestamp with time zone);
insert into t1 select generate_series(1, 10000000), current_timestamp +
random() * interval '1000 days';

select extract(dow from b) from t1 \g /dev/null
select extract(epoch from b) from t1 \g /dev/null

There appears to be about a 20% increase in run time for these tests.
These are obviously extreme tests, so I think that would be okay.  More
tests and testing ideas are welcome.

--
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

v2-0001-Add-more-tests-for-extract-of-date-type.patch (15K) Download Attachment
v2-0002-Expose-internal-function-for-converting-int64-to-.patch (17K) Download Attachment
v2-0003-Change-return-type-of-EXTRACT-to-numeric.patch (116K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Since '2001-09-09 01:46:40'::timestamp microseconds are lost when extracting epoch

Tom Lane-2
Peter Eisentraut <[hidden email]> writes:
> Here is a new patch series version.
> I have created a new internal function for converting integers to
> numeric, to make the implementation a bit more elegant and compact.

I reviewed the 0002 patch, finding one bug (in int8_sum) and a few
more calls of int8_numeric that could be converted.  I think the
attached updated version is committable, and I'd recommend going
ahead with that regardless of the rest of this.  I hadn't realized
how many random calls of int8_numeric and int4_numeric we'd grown,
but there are a lot, so this is nice cleanup.

I continue to think that we can't commit 0003 in this form, because
of the breakage that will ensure in stored views.  As I said upthread,
we should leave the existing SQL-exposed functions alone, invent
new ones that return numeric, and alter the parser to translate
EXTRACT constructs to the new functions.  This approach would also
provide an "out" for anyone who does complain about the performance
cost --- they can just continue to use the old functions.

                        regards, tom lane


diff --git a/contrib/btree_gist/btree_numeric.c b/contrib/btree_gist/btree_numeric.c
index d66901680e..35e466cdd9 100644
--- a/contrib/btree_gist/btree_numeric.c
+++ b/contrib/btree_gist/btree_numeric.c
@@ -195,7 +195,7 @@ gbt_numeric_penalty(PG_FUNCTION_ARGS)
  }
  else
  {
- Numeric nul = DatumGetNumeric(DirectFunctionCall1(int4_numeric, Int32GetDatum(0)));
+ Numeric nul = int64_to_numeric(0);
 
  *result = 0.0;
 
diff --git a/contrib/jsonb_plperl/jsonb_plperl.c b/contrib/jsonb_plperl/jsonb_plperl.c
index b81ba54b80..22e90afe1b 100644
--- a/contrib/jsonb_plperl/jsonb_plperl.c
+++ b/contrib/jsonb_plperl/jsonb_plperl.c
@@ -216,9 +216,7 @@ SV_to_JsonbValue(SV *in, JsonbParseState **jsonb_state, bool is_elem)
  IV ival = SvIV(in);
 
  out.type = jbvNumeric;
- out.val.numeric =
- DatumGetNumeric(DirectFunctionCall1(int8_numeric,
- Int64GetDatum((int64) ival)));
+ out.val.numeric = int64_to_numeric(ival);
  }
  else if (SvNOK(in))
  {
diff --git a/src/backend/utils/adt/cash.c b/src/backend/utils/adt/cash.c
index 6515fc8ec6..d093ce8038 100644
--- a/src/backend/utils/adt/cash.c
+++ b/src/backend/utils/adt/cash.c
@@ -1042,7 +1042,7 @@ cash_numeric(PG_FUNCTION_ARGS)
  fpoint = 2;
 
  /* convert the integral money value to numeric */
- result = DirectFunctionCall1(int8_numeric, Int64GetDatum(money));
+ result = NumericGetDatum(int64_to_numeric(money));
 
  /* scale appropriately, if needed */
  if (fpoint > 0)
@@ -1056,8 +1056,7 @@ cash_numeric(PG_FUNCTION_ARGS)
  scale = 1;
  for (i = 0; i < fpoint; i++)
  scale *= 10;
- numeric_scale = DirectFunctionCall1(int8_numeric,
- Int64GetDatum(scale));
+ numeric_scale = NumericGetDatum(int64_to_numeric(scale));
 
  /*
  * Given integral inputs approaching INT64_MAX, select_div_scale()
@@ -1107,7 +1106,7 @@ numeric_cash(PG_FUNCTION_ARGS)
  scale *= 10;
 
  /* multiply the input amount by scale factor */
- numeric_scale = DirectFunctionCall1(int8_numeric, Int64GetDatum(scale));
+ numeric_scale = NumericGetDatum(int64_to_numeric(scale));
  amount = DirectFunctionCall2(numeric_mul, amount, numeric_scale);
 
  /* note that numeric_int8 will round to nearest integer for us */
diff --git a/src/backend/utils/adt/dbsize.c b/src/backend/utils/adt/dbsize.c
index 2320c06a9b..7def7392b9 100644
--- a/src/backend/utils/adt/dbsize.c
+++ b/src/backend/utils/adt/dbsize.c
@@ -579,14 +579,6 @@ numeric_to_cstring(Numeric n)
  return DatumGetCString(DirectFunctionCall1(numeric_out, d));
 }
 
-static Numeric
-int64_to_numeric(int64 v)
-{
- Datum d = Int64GetDatum(v);
-
- return DatumGetNumeric(DirectFunctionCall1(int8_numeric, d));
-}
-
 static bool
 numeric_is_less(Numeric a, Numeric b)
 {
@@ -615,9 +607,9 @@ numeric_half_rounded(Numeric n)
  Datum two;
  Datum result;
 
- zero = DirectFunctionCall1(int8_numeric, Int64GetDatum(0));
- one = DirectFunctionCall1(int8_numeric, Int64GetDatum(1));
- two = DirectFunctionCall1(int8_numeric, Int64GetDatum(2));
+ zero = NumericGetDatum(int64_to_numeric(0));
+ one = NumericGetDatum(int64_to_numeric(1));
+ two = NumericGetDatum(int64_to_numeric(2));
 
  if (DatumGetBool(DirectFunctionCall2(numeric_ge, d, zero)))
  d = DirectFunctionCall2(numeric_add, d, one);
@@ -632,12 +624,10 @@ static Numeric
 numeric_shift_right(Numeric n, unsigned count)
 {
  Datum d = NumericGetDatum(n);
- Datum divisor_int64;
  Datum divisor_numeric;
  Datum result;
 
- divisor_int64 = Int64GetDatum((int64) (1 << count));
- divisor_numeric = DirectFunctionCall1(int8_numeric, divisor_int64);
+ divisor_numeric = NumericGetDatum(int64_to_numeric(1 << count));
  result = DirectFunctionCall2(numeric_div_trunc, d, divisor_numeric);
  return DatumGetNumeric(result);
 }
@@ -832,8 +822,7 @@ pg_size_bytes(PG_FUNCTION_ARGS)
  {
  Numeric mul_num;
 
- mul_num = DatumGetNumeric(DirectFunctionCall1(int8_numeric,
-  Int64GetDatum(multiplier)));
+ mul_num = int64_to_numeric(multiplier);
 
  num = DatumGetNumeric(DirectFunctionCall2(numeric_mul,
   NumericGetDatum(mul_num),
diff --git a/src/backend/utils/adt/formatting.c b/src/backend/utils/adt/formatting.c
index 7d09537d82..f9aa968f09 100644
--- a/src/backend/utils/adt/formatting.c
+++ b/src/backend/utils/adt/formatting.c
@@ -6070,10 +6070,8 @@ numeric_to_number(PG_FUNCTION_ARGS)
  if (IS_MULTI(&Num))
  {
  Numeric x;
- Numeric a = DatumGetNumeric(DirectFunctionCall1(int4_numeric,
- Int32GetDatum(10)));
- Numeric b = DatumGetNumeric(DirectFunctionCall1(int4_numeric,
- Int32GetDatum(-Num.multi)));
+ Numeric a = int64_to_numeric(10);
+ Numeric b = int64_to_numeric(-Num.multi);
 
  x = DatumGetNumeric(DirectFunctionCall2(numeric_power,
  NumericGetDatum(a),
@@ -6162,10 +6160,8 @@ numeric_to_char(PG_FUNCTION_ARGS)
 
  if (IS_MULTI(&Num))
  {
- Numeric a = DatumGetNumeric(DirectFunctionCall1(int4_numeric,
- Int32GetDatum(10)));
- Numeric b = DatumGetNumeric(DirectFunctionCall1(int4_numeric,
- Int32GetDatum(Num.multi)));
+ Numeric a = int64_to_numeric(10);
+ Numeric b = int64_to_numeric(Num.multi);
 
  x = DatumGetNumeric(DirectFunctionCall2(numeric_power,
  NumericGetDatum(a),
@@ -6339,11 +6335,8 @@ int8_to_char(PG_FUNCTION_ARGS)
  else if (IS_EEEE(&Num))
  {
  /* to avoid loss of precision, must go via numeric not float8 */
- Numeric val;
-
- val = DatumGetNumeric(DirectFunctionCall1(int8_numeric,
-  Int64GetDatum(value)));
- orgnum = numeric_out_sci(val, Num.post);
+ orgnum = numeric_out_sci(int64_to_numeric(value),
+ Num.post);
 
  /*
  * numeric_out_sci() does not emit a sign for positive numbers.  We
diff --git a/src/backend/utils/adt/jsonpath_exec.c b/src/backend/utils/adt/jsonpath_exec.c
index f146767bfc..7403c760b4 100644
--- a/src/backend/utils/adt/jsonpath_exec.c
+++ b/src/backend/utils/adt/jsonpath_exec.c
@@ -842,9 +842,7 @@ executeItemOptUnwrapTarget(JsonPathExecContext *cxt, JsonPathItem *jsp,
  lastjbv = hasNext ? &tmpjbv : palloc(sizeof(*lastjbv));
 
  lastjbv->type = jbvNumeric;
- lastjbv->val.numeric =
- DatumGetNumeric(DirectFunctionCall1(int4_numeric,
- Int32GetDatum(last)));
+ lastjbv->val.numeric = int64_to_numeric(last);
 
  res = executeNextItem(cxt, jsp, &elem,
   lastjbv, found, hasNext);
@@ -1012,9 +1010,7 @@ executeItemOptUnwrapTarget(JsonPathExecContext *cxt, JsonPathItem *jsp,
  jb = palloc(sizeof(*jb));
 
  jb->type = jbvNumeric;
- jb->val.numeric =
- DatumGetNumeric(DirectFunctionCall1(int4_numeric,
- Int32GetDatum(size)));
+ jb->val.numeric = int64_to_numeric(size);
 
  res = executeNextItem(cxt, jsp, NULL, jb, found, false);
  }
@@ -1979,8 +1975,7 @@ executeKeyValueMethod(JsonPathExecContext *cxt, JsonPathItem *jsp,
  id += (int64) cxt->baseObject.id * INT64CONST(10000000000);
 
  idval.type = jbvNumeric;
- idval.val.numeric = DatumGetNumeric(DirectFunctionCall1(int8_numeric,
- Int64GetDatum(id)));
+ idval.val.numeric = int64_to_numeric(id);
 
  it = JsonbIteratorInit(jbc);
 
diff --git a/src/backend/utils/adt/numeric.c b/src/backend/utils/adt/numeric.c
index ed825a1fdd..d2cc74b284 100644
--- a/src/backend/utils/adt/numeric.c
+++ b/src/backend/utils/adt/numeric.c
@@ -4073,23 +4073,29 @@ numeric_trim_scale(PG_FUNCTION_ARGS)
  * ----------------------------------------------------------------------
  */
 
-
-Datum
-int4_numeric(PG_FUNCTION_ARGS)
+Numeric
+int64_to_numeric(int64 val)
 {
- int32 val = PG_GETARG_INT32(0);
  Numeric res;
  NumericVar result;
 
  init_var(&result);
 
- int64_to_numericvar((int64) val, &result);
+ int64_to_numericvar(val, &result);
 
  res = make_result(&result);
 
  free_var(&result);
 
- PG_RETURN_NUMERIC(res);
+ return res;
+}
+
+Datum
+int4_numeric(PG_FUNCTION_ARGS)
+{
+ int32 val = PG_GETARG_INT32(0);
+
+ PG_RETURN_NUMERIC(int64_to_numeric(val));
 }
 
 int32
@@ -4174,18 +4180,8 @@ Datum
 int8_numeric(PG_FUNCTION_ARGS)
 {
  int64 val = PG_GETARG_INT64(0);
- Numeric res;
- NumericVar result;
 
- init_var(&result);
-
- int64_to_numericvar(val, &result);
-
- res = make_result(&result);
-
- free_var(&result);
-
- PG_RETURN_NUMERIC(res);
+ PG_RETURN_NUMERIC(int64_to_numeric(val));
 }
 
 
@@ -4224,18 +4220,8 @@ Datum
 int2_numeric(PG_FUNCTION_ARGS)
 {
  int16 val = PG_GETARG_INT16(0);
- Numeric res;
- NumericVar result;
-
- init_var(&result);
-
- int64_to_numericvar((int64) val, &result);
-
- res = make_result(&result);
 
- free_var(&result);
-
- PG_RETURN_NUMERIC(res);
+ PG_RETURN_NUMERIC(int64_to_numeric(val));
 }
 
 
@@ -5290,11 +5276,7 @@ int2_accum(PG_FUNCTION_ARGS)
 #ifdef HAVE_INT128
  do_int128_accum(state, (int128) PG_GETARG_INT16(1));
 #else
- Numeric newval;
-
- newval = DatumGetNumeric(DirectFunctionCall1(int2_numeric,
- PG_GETARG_DATUM(1)));
- do_numeric_accum(state, newval);
+ do_numeric_accum(state, int64_to_numeric(PG_GETARG_INT16(1)));
 #endif
  }
 
@@ -5317,11 +5299,7 @@ int4_accum(PG_FUNCTION_ARGS)
 #ifdef HAVE_INT128
  do_int128_accum(state, (int128) PG_GETARG_INT32(1));
 #else
- Numeric newval;
-
- newval = DatumGetNumeric(DirectFunctionCall1(int4_numeric,
- PG_GETARG_DATUM(1)));
- do_numeric_accum(state, newval);
+ do_numeric_accum(state, int64_to_numeric(PG_GETARG_INT32(1)));
 #endif
  }
 
@@ -5340,13 +5318,7 @@ int8_accum(PG_FUNCTION_ARGS)
  state = makeNumericAggState(fcinfo, true);
 
  if (!PG_ARGISNULL(1))
- {
- Numeric newval;
-
- newval = DatumGetNumeric(DirectFunctionCall1(int8_numeric,
- PG_GETARG_DATUM(1)));
- do_numeric_accum(state, newval);
- }
+ do_numeric_accum(state, int64_to_numeric(PG_GETARG_INT64(1)));
 
  PG_RETURN_POINTER(state);
 }
@@ -5570,11 +5542,7 @@ int8_avg_accum(PG_FUNCTION_ARGS)
 #ifdef HAVE_INT128
  do_int128_accum(state, (int128) PG_GETARG_INT64(1));
 #else
- Numeric newval;
-
- newval = DatumGetNumeric(DirectFunctionCall1(int8_numeric,
- PG_GETARG_DATUM(1)));
- do_numeric_accum(state, newval);
+ do_numeric_accum(state, int64_to_numeric(PG_GETARG_INT64(1)));
 #endif
  }
 
@@ -5767,13 +5735,8 @@ int2_accum_inv(PG_FUNCTION_ARGS)
 #ifdef HAVE_INT128
  do_int128_discard(state, (int128) PG_GETARG_INT16(1));
 #else
- Numeric newval;
-
- newval = DatumGetNumeric(DirectFunctionCall1(int2_numeric,
- PG_GETARG_DATUM(1)));
-
  /* Should never fail, all inputs have dscale 0 */
- if (!do_numeric_discard(state, newval))
+ if (!do_numeric_discard(state, int64_to_numeric(PG_GETARG_INT16(1))))
  elog(ERROR, "do_numeric_discard failed unexpectedly");
 #endif
  }
@@ -5797,13 +5760,8 @@ int4_accum_inv(PG_FUNCTION_ARGS)
 #ifdef HAVE_INT128
  do_int128_discard(state, (int128) PG_GETARG_INT32(1));
 #else
- Numeric newval;
-
- newval = DatumGetNumeric(DirectFunctionCall1(int4_numeric,
- PG_GETARG_DATUM(1)));
-
  /* Should never fail, all inputs have dscale 0 */
- if (!do_numeric_discard(state, newval))
+ if (!do_numeric_discard(state, int64_to_numeric(PG_GETARG_INT32(1))))
  elog(ERROR, "do_numeric_discard failed unexpectedly");
 #endif
  }
@@ -5824,13 +5782,8 @@ int8_accum_inv(PG_FUNCTION_ARGS)
 
  if (!PG_ARGISNULL(1))
  {
- Numeric newval;
-
- newval = DatumGetNumeric(DirectFunctionCall1(int8_numeric,
- PG_GETARG_DATUM(1)));
-
  /* Should never fail, all inputs have dscale 0 */
- if (!do_numeric_discard(state, newval))
+ if (!do_numeric_discard(state, int64_to_numeric(PG_GETARG_INT64(1))))
  elog(ERROR, "do_numeric_discard failed unexpectedly");
  }
 
@@ -5853,13 +5806,8 @@ int8_avg_accum_inv(PG_FUNCTION_ARGS)
 #ifdef HAVE_INT128
  do_int128_discard(state, (int128) PG_GETARG_INT64(1));
 #else
- Numeric newval;
-
- newval = DatumGetNumeric(DirectFunctionCall1(int8_numeric,
- PG_GETARG_DATUM(1)));
-
  /* Should never fail, all inputs have dscale 0 */
- if (!do_numeric_discard(state, newval))
+ if (!do_numeric_discard(state, int64_to_numeric(PG_GETARG_INT64(1))))
  elog(ERROR, "do_numeric_discard failed unexpectedly");
 #endif
  }
@@ -5914,8 +5862,7 @@ numeric_poly_avg(PG_FUNCTION_ARGS)
 
  int128_to_numericvar(state->sumX, &result);
 
- countd = DirectFunctionCall1(int8_numeric,
- Int64GetDatumFast(state->N));
+ countd = NumericGetDatum(int64_to_numeric(state->N));
  sumd = NumericGetDatum(make_result(&result));
 
  free_var(&result);
@@ -5951,7 +5898,7 @@ numeric_avg(PG_FUNCTION_ARGS)
  if (state->nInfcount > 0)
  PG_RETURN_NUMERIC(make_result(&const_ninf));
 
- N_datum = DirectFunctionCall1(int8_numeric, Int64GetDatum(state->N));
+ N_datum = NumericGetDatum(int64_to_numeric(state->N));
 
  init_var(&sumX_var);
  accum_sum_final(&state->sumX, &sumX_var);
@@ -6411,7 +6358,6 @@ Datum
 int8_sum(PG_FUNCTION_ARGS)
 {
  Numeric oldsum;
- Datum newval;
 
  if (PG_ARGISNULL(0))
  {
@@ -6419,8 +6365,7 @@ int8_sum(PG_FUNCTION_ARGS)
  if (PG_ARGISNULL(1))
  PG_RETURN_NULL(); /* still no non-null */
  /* This is the first non-null input. */
- newval = DirectFunctionCall1(int8_numeric, PG_GETARG_DATUM(1));
- PG_RETURN_DATUM(newval);
+ PG_RETURN_NUMERIC(int64_to_numeric(PG_GETARG_INT64(1)));
  }
 
  /*
@@ -6436,10 +6381,9 @@ int8_sum(PG_FUNCTION_ARGS)
  PG_RETURN_NUMERIC(oldsum);
 
  /* OK to do the addition. */
- newval = DirectFunctionCall1(int8_numeric, PG_GETARG_DATUM(1));
-
  PG_RETURN_DATUM(DirectFunctionCall2(numeric_add,
- NumericGetDatum(oldsum), newval));
+ NumericGetDatum(oldsum),
+ NumericGetDatum(int64_to_numeric(PG_GETARG_INT64(1)))));
 }
 
 
@@ -6618,10 +6562,8 @@ int8_avg(PG_FUNCTION_ARGS)
  if (transdata->count == 0)
  PG_RETURN_NULL();
 
- countd = DirectFunctionCall1(int8_numeric,
- Int64GetDatumFast(transdata->count));
- sumd = DirectFunctionCall1(int8_numeric,
-   Int64GetDatumFast(transdata->sum));
+ countd = NumericGetDatum(int64_to_numeric(transdata->count));
+ sumd = NumericGetDatum(int64_to_numeric(transdata->sum));
 
  PG_RETURN_DATUM(DirectFunctionCall2(numeric_div, sumd, countd));
 }
diff --git a/src/include/utils/numeric.h b/src/include/utils/numeric.h
index 0b7d4ba3c4..2a768b9a04 100644
--- a/src/include/utils/numeric.h
+++ b/src/include/utils/numeric.h
@@ -62,6 +62,8 @@ int32 numeric_maximum_size(int32 typmod);
 extern char *numeric_out_sci(Numeric num, int scale);
 extern char *numeric_normalize(Numeric num);
 
+extern Numeric int64_to_numeric(int64 val);
+
 extern Numeric numeric_add_opt_error(Numeric num1, Numeric num2,
  bool *have_error);
 extern Numeric numeric_sub_opt_error(Numeric num1, Numeric num2,
Reply | Threaded
Open this post in threaded view
|

Re: Since '2001-09-09 01:46:40'::timestamp microseconds are lost when extracting epoch

Pavel Stehule


po 7. 9. 2020 v 1:46 odesílatel Tom Lane <[hidden email]> napsal:
Peter Eisentraut <[hidden email]> writes:
> Here is a new patch series version.
> I have created a new internal function for converting integers to
> numeric, to make the implementation a bit more elegant and compact.

I reviewed the 0002 patch, finding one bug (in int8_sum) and a few
more calls of int8_numeric that could be converted.  I think the
attached updated version is committable, and I'd recommend going
ahead with that regardless of the rest of this.  I hadn't realized
how many random calls of int8_numeric and int4_numeric we'd grown,
but there are a lot, so this is nice cleanup.

This patch is a clean win.

+1


I continue to think that we can't commit 0003 in this form, because
of the breakage that will ensure in stored views.  As I said upthread,
we should leave the existing SQL-exposed functions alone, invent
new ones that return numeric, and alter the parser to translate
EXTRACT constructs to the new functions.  This approach would also
provide an "out" for anyone who does complain about the performance
cost --- they can just continue to use the old functions.

+1

Regards

Pavel


                        regards, tom lane

Reply | Threaded
Open this post in threaded view
|

Re: Since '2001-09-09 01:46:40'::timestamp microseconds are lost when extracting epoch

Peter Eisentraut-6
In reply to this post by Tom Lane-2
On 2020-09-07 01:46, Tom Lane wrote:
> Peter Eisentraut <[hidden email]> writes:
>> Here is a new patch series version.
>> I have created a new internal function for converting integers to
>> numeric, to make the implementation a bit more elegant and compact.
>
> I reviewed the 0002 patch, finding one bug (in int8_sum)

Ouch, no test coverage.  Should we perhaps remove this function, since
it's obsolete and unused?

> and a few
> more calls of int8_numeric that could be converted.  I think the
> attached updated version is committable, and I'd recommend going
> ahead with that regardless of the rest of this.  I hadn't realized
> how many random calls of int8_numeric and int4_numeric we'd grown,
> but there are a lot, so this is nice cleanup.

Yes, please go ahead with it.

> I continue to think that we can't commit 0003 in this form, because
> of the breakage that will ensure in stored views.  As I said upthread,
> we should leave the existing SQL-exposed functions alone, invent
> new ones that return numeric, and alter the parser to translate
> EXTRACT constructs to the new functions.  This approach would also
> provide an "out" for anyone who does complain about the performance
> cost --- they can just continue to use the old functions.

Okay, I will continue looking into this.

--
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


Reply | Threaded
Open this post in threaded view
|

Re: Since '2001-09-09 01:46:40'::timestamp microseconds are lost when extracting epoch

Tom Lane-2
Peter Eisentraut <[hidden email]> writes:
> On 2020-09-07 01:46, Tom Lane wrote:
>> I reviewed the 0002 patch, finding one bug (in int8_sum)

> Ouch, no test coverage.  Should we perhaps remove this function, since
> it's obsolete and unused?

I don't feel a need to.

>> and a few
>> more calls of int8_numeric that could be converted.  I think the
>> attached updated version is committable, and I'd recommend going
>> ahead with that regardless of the rest of this.  I hadn't realized
>> how many random calls of int8_numeric and int4_numeric we'd grown,
>> but there are a lot, so this is nice cleanup.

> Yes, please go ahead with it.

It's your patch, I figured you'd want to commit it.

                        regards, tom lane


Reply | Threaded
Open this post in threaded view
|

Re: Since '2001-09-09 01:46:40'::timestamp microseconds are lost when extracting epoch

Peter Eisentraut-6
On 2020-09-09 15:38, Tom Lane wrote:

>>> and a few
>>> more calls of int8_numeric that could be converted.  I think the
>>> attached updated version is committable, and I'd recommend going
>>> ahead with that regardless of the rest of this.  I hadn't realized
>>> how many random calls of int8_numeric and int4_numeric we'd grown,
>>> but there are a lot, so this is nice cleanup.
>
>> Yes, please go ahead with it.
>
> It's your patch, I figured you'd want to commit it.

ok done

--
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


Reply | Threaded
Open this post in threaded view
|

Re: Since '2001-09-09 01:46:40'::timestamp microseconds are lost when extracting epoch

Ranier Vilela-2
In reply to this post by Peter Eisentraut-6
Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com> writes:
> ok done
msvc 2019 (64 bits), is worried about it:

"C:\dll\postgres\pgsql.sln" (default target) (1) ->
"C:\dll\postgres\cyrillic_and_mic.vcxproj" (default target) (37) ->
"C:\dll\postgres\postgres.vcxproj" (default target) (38) ->
(ClCompile target) ->
  C:\dll\postgres\src\backend\utils\adt\dbsize.c(630,20): warning C4334: '<<': resultado de 32-bit shift convertido implicitamente para 64 bits (64-bit shift era pretendid
o?) [C:\dll\postgres\postgres.vcxproj]

    1 Warning(s)
    0 Error(s)

"result of 32-bit shift implicitly converted to 64 bits (was 64-bit shift intended?)"

regards,
Ranier Vilela
Reply | Threaded
Open this post in threaded view
|

Re: Since '2001-09-09 01:46:40'::timestamp microseconds are lost when extracting epoch

Tom Lane-2
Ranier Vilela <[hidden email]> writes:
> msvc 2019 (64 bits), is worried about it:
>   C:\dll\postgres\src\backend\utils\adt\dbsize.c(630,20): warning C4334:
> '<<': resultado de 32-bit shift convertido implicitamente para 64 bits
> (64-bit shift era pretendid
> o?) [C:\dll\postgres\postgres.vcxproj]

Yeah, most/all of the MSVC buildfarm members are showing this warning too.
The previous coding was

        Int64GetDatum((int64) (1 << count))

which apparently is enough to silence MSVC, though it makes no difference
as to the actual overflow risk involved.

I'm a bit inclined to make the new code be

        NumericGetDatum(int64_to_numeric(INT64CONST(1) << count));

ie do the shift in 64 bits.  That's either free or next door to free, and
it allows larger count values to be accepted.  The existing callers don't
care of course, but someday somebody might try to expose this function
more widely.

                        regards, tom lane


Reply | Threaded
Open this post in threaded view
|

Re: Since '2001-09-09 01:46:40'::timestamp microseconds are lost when extracting epoch

Ranier Vilela-2
Em seg., 14 de set. de 2020 às 15:33, Tom Lane <[hidden email]> escreveu:
Ranier Vilela <[hidden email]> writes:
> msvc 2019 (64 bits), is worried about it:
>   C:\dll\postgres\src\backend\utils\adt\dbsize.c(630,20): warning C4334:
> '<<': resultado de 32-bit shift convertido implicitamente para 64 bits
> (64-bit shift era pretendid
> o?) [C:\dll\postgres\postgres.vcxproj]

Yeah, most/all of the MSVC buildfarm members are showing this warning too.
The previous coding was

        Int64GetDatum((int64) (1 << count))

which apparently is enough to silence MSVC, though it makes no difference
as to the actual overflow risk involved.

I'm a bit inclined to make the new code be

        NumericGetDatum(int64_to_numeric(INT64CONST(1) << count));
+1
msvc 2019 (64 bits), is happy with INT64CONST(1)

regards,
Ranier Vilela
Reply | Threaded
Open this post in threaded view
|

Re: Since '2001-09-09 01:46:40'::timestamp microseconds are lost when extracting epoch

Michael Paquier-2
In reply to this post by Peter Eisentraut-6
On Wed, Sep 09, 2020 at 08:47:36PM +0200, Peter Eisentraut wrote:
> ok done

As far as I can see, patches 0001 and 0002 have been already applied,
but not 0003.  Could you send a rebase to allow the CF bot to run, at
least?
--
Michael

signature.asc (849 bytes) Download Attachment