Horton

Thread

Commits

  1. Fix inconsistent equalfuncs.c behavior for FuncCall.funcformat.

  2. Doc: fix discussion of how to get real Julian Dates.

  3. Doc: document EXTRACT(JULIAN ...), improve Julian Date explanation.

  4. Change return type of EXTRACT to numeric

  5. Improve our ability to regurgitate SQL-syntax function calls.

  6. Add more tests for EXTRACT of date type

  7. Expose internal function for converting int64 to numeric

  8. Change floating-point output format for improved performance.

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

    Petr Fedorov <petr.fedorov@phystech.edu> — 2019-11-30T09:28:18Z

    Hello,

Steps to reproduce:

select extract(epoch from '2001-09-09 01:46:39.999999'::timestamp)

returns 999999999.999999 as expected

while

select extract(epoch from '2001-09-09 01:46:40.000021'::timestamp)

returns 1000000000.00002 - 1 microsecond is truncated.

Obviously, it is due to the fact that extract epoch returns double
precision which in turn has 15 decimal digits precision.

While there is a pretty simple workaround in C, that returns
microseconds since Unix epoch:

Datum
to_microseconds(PG_FUNCTION_ARGS) {
 Timestamp arg = PG_GETARG_TIMESTAMP(0)+946684800000000;
  PG_RETURN_INT64(arg);
}

I was not able to find the other way of doing that (i.e. without C
function).

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-11-30T15:21:01Z

    Petr Fedorov <petr.fedorov@phystech.edu> writes:
> select extract(epoch from '2001-09-09 01:46:40.000021'::timestamp)
> returns 1000000000.00002 - 1 microsecond is truncated.
> Obviously, it is due to the fact that extract epoch returns double
> precision which in turn has 15 decimal digits precision.

I can't get very excited about this.  However, it might be worth
noting that v12 and HEAD print "1000000000.000021" as expected,
thanks to the Ryu float output code.  You can get that from older
branches as well if you set extra_float_digits = 1.

By my arithmetic, IEEE float8 ought to be able to represent
microseconds accurately out to about 285 years either way from the
1970 epoch, so for practical purposes it'll be fine for a long time.

			regards, tom lane

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

    Thomas Munro <thomas.munro@gmail.com> — 2019-12-01T22:50:00Z

    On Sat, Nov 30, 2019 at 10:28 PM Petr Fedorov <petr.fedorov@phystech.edu> wrote:
> Obviously, it is due to the fact that extract epoch returns double
> precision which in turn has 15 decimal digits precision.

I guess this deviation from the SQL standard ("exact numeric") made
sense when PostgreSQL used double for timestamps, but would break a
lot of queries if we changed it.

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-12-01T22:59:11Z

    Thomas Munro <thomas.munro@gmail.com> writes:
> On Sat, Nov 30, 2019 at 10:28 PM Petr Fedorov <petr.fedorov@phystech.edu> wrote:
>> Obviously, it is due to the fact that extract epoch returns double
>> precision which in turn has 15 decimal digits precision.

> I guess this deviation from the SQL standard ("exact numeric") made
> sense when PostgreSQL used double for timestamps, but would break a
> lot of queries if we changed it.

Hmmm ... well, now that you mention it, would it really break things
if we made it return numeric?  There's an implicit cast to float8,
so it seems like queries requiring that type would still work.

There might be a performance-related argument against switching,
perhaps.

			regards, tom lane

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

    Petr Fedorov <petr.fedorov@phystech.edu> — 2019-12-02T11:08:43Z

    It appears that extract epoch returns double precision, not float8.  And
the program below seems to be demonstrating that there are enough
'floating-point numbers' as defined by  IEEE-754 to represent
1000000000.000021 precisely enough:

#include <cmath>
#include <iostream>
#include <iomanip>
#include <limits>

int main() {
 double from = 1000000000.000020;
 std::cout << std::setprecision(56) << from  << " (" << std::hexfloat <<
from << ") " << std::endl;
 for(auto i = 0; i < 15; ++i) {
   double to = std::nextafter( from, std::numeric_limits<double>::max());
   std::cout << std::defaultfloat << to << std::hexfloat << " (" << to
<< ") " << std::endl;
   from = to;
 }
}

Outputs:

1000000000.00002002716064453125 (0x1.dcd65000000a8p+29)
1000000000.00002014636993408203125 (0x1.dcd65000000a9p+29)
1000000000.0000202655792236328125 (0x1.dcd65000000aap+29)
1000000000.00002038478851318359375 (0x1.dcd65000000abp+29)
1000000000.000020503997802734375 (0x1.dcd65000000acp+29)
1000000000.00002062320709228515625 (0x1.dcd65000000adp+29)
1000000000.0000207424163818359375 (0x1.dcd65000000aep+29)
1000000000.00002086162567138671875 (0x1.dcd65000000afp+29)
1000000000.0000209808349609375 (0x1.dcd65000000bp+29)
1000000000.00002110004425048828125 (0x1.dcd65000000b1p+29)
1000000000.0000212192535400390625 (0x1.dcd65000000b2p+29)
1000000000.00002133846282958984375 (0x1.dcd65000000b3p+29)
1000000000.000021457672119140625 (0x1.dcd65000000b4p+29)
1000000000.00002157688140869140625 (0x1.dcd65000000b5p+29)
1000000000.0000216960906982421875 (0x1.dcd65000000b6p+29)
1000000000.00002181529998779296875 (0x1.dcd65000000b7p+29)

I'm not an expert in floating point math but hopefully it means that no
type change is required - double precision can handle it. 

And since it works correctly on v12 for this particular date may be all
what is needed it to verify that it works for the other dates too! For
example what was changed in v12 (comparing to 11.6 I use) so extract
epoch works correctly?


02.12.2019 01:59, Tom Lane пишет:
> Thomas Munro <thomas.munro@gmail.com> writes:
>> On Sat, Nov 30, 2019 at 10:28 PM Petr Fedorov <petr.fedorov@phystech.edu> wrote:
>>> Obviously, it is due to the fact that extract epoch returns double
>>> precision which in turn has 15 decimal digits precision.
>> I guess this deviation from the SQL standard ("exact numeric") made
>> sense when PostgreSQL used double for timestamps, but would break a
>> lot of queries if we changed it.
> Hmmm ... well, now that you mention it, would it really break things
> if we made it return numeric?  There's an implicit cast to float8,
> so it seems like queries requiring that type would still work.
>
> There might be a performance-related argument against switching,
> perhaps.
>
> 			regards, tom lane

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

    Thomas Munro <thomas.munro@gmail.com> — 2019-12-02T22:52:31Z

    On Tue, Dec 3, 2019 at 12:08 AM Petr Fedorov <petr.fedorov@phystech.edu> wrote:
> It appears that extract epoch returns double precision, not float8.  And
> the program below seems to be demonstrating that there are enough
> 'floating-point numbers' as defined by  IEEE-754 to represent
> 1000000000.000021 precisely enough:

Double precision and float8 are different names for the same type in PostgreSQL.

> 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.

It's also wrong because binary floating point numbers can't represent
0.000001 (one microsecond represented as seconds) exactly, and that's
our unit of counting for timestamps.  You can get pretty far by
thinking of the decimal number you see on the screen as the true
number and the double as a fuzzy internal storage or transport that
does the job just fine due to the round trip conversion guarantee
provided by DBL_DIG, but the double is still going to have the wrong
value in some cases.  As soon as you start doing any arithmetic or
comparisons with the double directly, interesting things can start to
happen to make the error visible and break things; for example
0.1::float8 + 0.2::float8 = 0.3::float8 is false.

> And since it works correctly on v12 for this particular date may be all
> what is needed it to verify that it works for the other dates too! For
> example what was changed in v12 (comparing to 11.6 I use) so extract
> epoch works correctly?

PostgreSQL 12 adopted a different algorithm[1] for converting float8
to text that can affect how many digits are shown, as Tom explained.
The manual has some notes about it[2].

[1] https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=02ddd499322ab6f2f0d58692955dc9633c2150fc
[2] https://www.postgresql.org/docs/12/datatype-numeric.html#DATATYPE-FLOAT

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

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2020-05-25T13:28:54Z

    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

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-05-25T13:43:32Z

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> 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

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

    Vik Fearing <vik@postgresfriends.org> — 2020-05-25T16:01:07Z

    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

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-05-25T16:40:27Z

    Vik Fearing <vik@postgresfriends.org> 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

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

    Vik Fearing <vik@postgresfriends.org> — 2020-05-25T16:52:01Z

    On 5/25/20 6:40 PM, Tom Lane wrote:
> Vik Fearing <vik@postgresfriends.org> 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

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-05-25T17:07:30Z

    Vik Fearing <vik@postgresfriends.org> 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

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-05-25T17:35:04Z

    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

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

    David Fetter <david@fetter.org> — 2020-05-25T19:29:21Z

    On Mon, May 25, 2020 at 09:43:32AM -0400, Tom Lane wrote:
> Peter Eisentraut <peter.eisentraut@2ndquadrant.com> 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

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

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2020-08-04T14:08:07Z

    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

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

    Pavel Stehule <pavel.stehule@gmail.com> — 2020-08-04T14:21:56Z

    út 4. 8. 2020 v 16:08 odesílatel Peter Eisentraut <
peter.eisentraut@2ndquadrant.com> 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
>

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

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2020-08-12T07:04:07Z

    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

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-09-06T23:46:32Z

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> 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

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

    Pavel Stehule <pavel.stehule@gmail.com> — 2020-09-07T03:46:57Z

    po 7. 9. 2020 v 1:46 odesílatel Tom Lane <tgl@sss.pgh.pa.us> napsal:

> Peter Eisentraut <peter.eisentraut@2ndquadrant.com> 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
>
>

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

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2020-09-09T08:08:06Z

    On 2020-09-07 01:46, Tom Lane wrote:
> Peter Eisentraut <peter.eisentraut@2ndquadrant.com> 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

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-09-09T13:38:31Z

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> 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

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

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2020-09-09T18:47:36Z

    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

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

    Michael Paquier <michael@paquier.xyz> — 2020-09-17T05:53:27Z

    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

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

    Michael Paquier <michael@paquier.xyz> — 2020-09-30T07:15:43Z

    On Thu, Sep 17, 2020 at 02:53:27PM +0900, Michael Paquier wrote:
> 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?

This was two weeks ago.  Looking at 0003, the thing is not really
complicated, but as this thread has stalled I have marked the entry as
RwF.
--
Michael

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-10-31T23:57:23Z

    I wrote:
> 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.

Attached is a draft patch that does this.  I'm fairly pleased with it,
but there are some loose ends as described below.  As the patch stands,
it reverse-lists all our special-format function call syntaxes
*except* EXTRACT.  I left that out since I think we want to apply the
reverse-listing change when we add the numeric-output extraction
functions, as I said upthread.

The main thing that's incomplete here is that the switch on function
OID fails to cover some cases that ought to be covered, as a result
of limitations of Gen_fmgrtab.pl:

* Some C functions such as text_substr have multiple pg_proc entries,
and Gen_fmgrtab.pl chooses the wrong one for our purpose.  We could
either invent new Gen_fmgrtab.pl behavior to allow having macros for
all the pg_proc entries, or we could add duplicate C functions so that
the pg_proc entries can point to different C symbols.

* Some of the functions we need to reference aren't C functions at
all, but SQL functions, for instance OID 1305 is defined as
	select ($1, ($1 + $2)) overlaps ($3, ($3 + $4))
I think our best bet here is to replace these SQL definitions with
C equivalents, because really this implementation is pretty sucky.
Even if we manage to inline the SQL definition, that's expensive
to do; and evaluating some of the arguments twice is not nice either.

> 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.

I did nothing about that here, since it'd bloat the patch without
making anything but cosmetic changes.  I'm tempted to propose though
that we rename "CoercionForm" to "DisplayForm" and rename its
COERCE_XXX values to DISPLAY_XXX, to make this less confusing.

Another bit of follow-up work we could contemplate is to get rid of
the SQLValueFunction node type, since there's nothing it does that
we couldn't do with regular FuncExpr nodes and COERCE_SQL_SYNTAX.
But that's just cleanup, and I don't think it would save a very
large amount of code.

Thoughts?

			regards, tom lane

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-11-02T22:50:08Z

    I wrote:
> Attached is a draft patch that does this.  I'm fairly pleased with it,
> but there are some loose ends as described below.  As the patch stands,
> it reverse-lists all our special-format function call syntaxes
> *except* EXTRACT.  I left that out since I think we want to apply the
> reverse-listing change when we add the numeric-output extraction
> functions, as I said upthread.

> The main thing that's incomplete here is that the switch on function
> OID fails to cover some cases that ought to be covered, as a result
> of limitations of Gen_fmgrtab.pl:

Now that 8e1f37c07 fixed that, here's a complete version, with better
test coverage.  (I still think we might want to rewrite those SQL
functions as C, but that can be an independent project now.)

Remaining open issues:

* I notice that this will sometimes transform non-SQL-spec syntax
into SQL-spec, for example

# explain verbose select substring(now()::text, 'foo');
                     QUERY PLAN                      
-----------------------------------------------------
 Result  (cost=0.00..0.02 rows=1 width=32)
   Output: SUBSTRING((now())::text FROM 'foo'::text)
(2 rows)

I'm not sure that that satisfies the POLA.  This particular case is
especially not great, because this is really textregexsubstr() which
is *not* SQL compatible, so the display is more than a bit misleading.
The reason this happens is that we've included expr_list as a variant of
substr_list, so that the func_expr_common_subexpr production has no idea
whether the argument list was really special syntax or not.  What I'm
inclined to do, but have not done yet, is to split that apart into
separate variants so that when the SQL-spec decoration is not used we
just generate a perfectly vanilla FuncCall.  In fact, I'd sort of argue
that we should not force the function to be sought in pg_catalog in such
a case either.  The comments in substr_list claim that we're trying to
allow extension functions named substring(), but using SystemFuncName is
100% hostile to that.

* Still waiting for comments on whether to rename CoercionForm.

			regards, tom lane

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-11-04T00:22:14Z

    I wrote:
> * I notice that this will sometimes transform non-SQL-spec syntax
> into SQL-spec, for example ...
> I'm not sure that that satisfies the POLA.  This particular case is
> especially not great, because this is really textregexsubstr() which
> is *not* SQL compatible, so the display is more than a bit misleading.

Actually, the problem there is that I made ruleutils.c willing to
reverse-list textregexsubstr() in SQL syntax, which it really shouldn't
since there is no such function per SQL.  So deleting that "case" value
is enough to fix most of the problem.  Still:

> ... In fact, I'd sort of argue
> that we should not force the function to be sought in pg_catalog in such
> a case either.  The comments in substr_list claim that we're trying to
> allow extension functions named substring(), but using SystemFuncName is
> 100% hostile to that.

... this seems like a reasonable argument.  However, in the attached
I only did that for SUBSTRING and OVERLAY.  I had thought of doing
it for POSITION and TRIM, but both of those are weird enough that
allowing a "normal function call" seems error-prone.  For example,
the fact that TRIM(expr_list) works out as a call to btrim() is a mess,
but I don't think we can change it.  (But of course you can still call a
user-defined trim() function if you double-quote the function name.)

I did get rid of the empty variant for position_list, which AFAICS
has no value except adding confusion: there are no zero-argument
functions named "position" in pg_catalog.

I feel like this is committable at this point --- any objections?

			regards, tom lane

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

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2020-12-15T14:03:31Z

    Here is a new patch for this.  This now follows the implementation that 
Tom has suggested:  Leave date_part() alone, add a new set of extract() 
functions, and map the SQL EXTRACT construct to those.  I have basically 
just copied over the implementations from my previous patch and placed 
them next to the existing date_part() implementations.  So all the 
behavior is still the same as in the previous patches.

One thing I still need to look into is how to not lose all the test 
coverage for date_part().  But that should be fairly mechanical, so I'm 
leaving it off in this version.

-- 
Peter Eisentraut
2ndQuadrant, an EDB company
https://www.2ndquadrant.com/

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

    David Steele <david@pgmasters.net> — 2021-03-15T12:18:29Z

    On 12/15/20 9:03 AM, Peter Eisentraut wrote:
> Here is a new patch for this.  This now follows the implementation that 
> Tom has suggested:  Leave date_part() alone, add a new set of extract() 
> functions, and map the SQL EXTRACT construct to those.  I have basically 
> just copied over the implementations from my previous patch and placed 
> them next to the existing date_part() implementations.  So all the 
> behavior is still the same as in the previous patches.
> 
> One thing I still need to look into is how to not lose all the test 
> coverage for date_part().  But that should be fairly mechanical, so I'm 
> leaving it off in this version.

Tom, what do you think of the updated patch?

Regards,
-- 
-David
david@pgmasters.net

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-03-15T17:35:12Z

    David Steele <david@pgmasters.net> writes:
> On 12/15/20 9:03 AM, Peter Eisentraut wrote:
>> Here is a new patch for this.  This now follows the implementation that 
>> Tom has suggested:  Leave date_part() alone, add a new set of extract() 
>> functions, and map the SQL EXTRACT construct to those.  I have basically 
>> just copied over the implementations from my previous patch and placed 
>> them next to the existing date_part() implementations.  So all the 
>> behavior is still the same as in the previous patches.
>> 
>> One thing I still need to look into is how to not lose all the test 
>> coverage for date_part().  But that should be fairly mechanical, so I'm 
>> leaving it off in this version.

> Tom, what do you think of the updated patch?

Oh, I didn't think I was on the hook to review this ;-)

Anyway, taking a quick look at the v4 patch, the only complaint
I have is that it seems a bit bulky and brute-force to duplicate
so much code.  Is it feasible to share most of the implementation
between old and new functions, returning (say) an int64 that can
then be converted to either numeric or float8 by a wrapper?  That
would also reduce the pressure to duplicate all the test cases.

(I don't intend this complaint as a deal-breaker; Peter may well
have considered this alternative already and rejected it for good
reasons.)

			regards, tom lane

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

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2021-03-18T08:28:38Z

    On 15.03.21 18:35, Tom Lane wrote:
> Anyway, taking a quick look at the v4 patch, the only complaint
> I have is that it seems a bit bulky and brute-force to duplicate
> so much code.  Is it feasible to share most of the implementation
> between old and new functions, returning (say) an int64 that can
> then be converted to either numeric or float8 by a wrapper?  That
> would also reduce the pressure to duplicate all the test cases.

Yeah, it's not straightforward to do this, because you'd also need to 
carry around scale and infinity information, so you might end up 
creating a mini-numeric implementation just for this.

An easy way to reduce duplication would be to convert the existing 
date_part() into a wrapper around the new extract(), with a cast.  But 
then you'd pay the performance penalty of the numeric version.

Which leads me to:  After retesting this now, with a new machine, the 
performance of the numeric implementation is brutal compared to the 
float implementation, for cases where we need numeric division, which is 
milliseconds, seconds, and epoch.  In the first two cases, I imagine we 
could rewrite this a bit to avoid a lot of the numeric work, but for the 
epoch case (which is what started this thread), there isn't enough space 
in int64 to make this work.  Perhaps int128 could be pressed into 
service, optionally.  I think it would also help if we cracked open the 
numeric APIs a bit to avoid all the repeated unpacking and packing for 
each step.

So I think we need to do a bit more thinking and work here, meaning it 
will have to be postponed.


Here are the kinds of tests I ran:

=> select date_part('epoch', localtime + generate_series(0, 10000000) * 
interval '1 second') \g /dev/null
Time: 2537.482 ms (00:02.537)

=> select extract(epoch from localtime + generate_series(0, 10000000) * 
interval '1 second') \g /dev/null
Time: 6106.586 ms (00:06.107)

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

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2021-03-19T19:37:04Z

    On 18.03.21 09:28, Peter Eisentraut wrote:
> Which leads me to:  After retesting this now, with a new machine, the 
> performance of the numeric implementation is brutal compared to the 
> float implementation, for cases where we need numeric division, which is 
> milliseconds, seconds, and epoch.  In the first two cases, I imagine we 
> could rewrite this a bit to avoid a lot of the numeric work, but for the 
> epoch case (which is what started this thread), there isn't enough space 
> in int64 to make this work.  Perhaps int128 could be pressed into 
> service, optionally.  I think it would also help if we cracked open the 
> numeric APIs a bit to avoid all the repeated unpacking and packing for 
> each step.
> 
> So I think we need to do a bit more thinking and work here, meaning it 
> will have to be postponed.

Well, I had an idea that I put to work.  In most of these cases where we 
need division, we divide an integer by a power of 10.  That can be done 
with numeric very quickly by just shifting the weight and scale around. 
So I wrote a function that does that specifically (look for 
int64_div_fast_to_numeric()).  With that, the slow cases I mentioned now 
have the same performance as the other cases that didn't have any 
numeric division.  You just get the overhead for constructing and 
passing around a numeric instead of a double, which can't be avoided.

So here is an intermediate patch that does this.  I haven't gotten rid 
of all numeric_div_opt_error() calls yet, but if this seems acceptable, 
I can work on the remaining ones.

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-03-19T20:06:57Z

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> Well, I had an idea that I put to work.  In most of these cases where we 
> need division, we divide an integer by a power of 10.  That can be done 
> with numeric very quickly by just shifting the weight and scale around. 
> So I wrote a function that does that specifically (look for 
> int64_div_fast_to_numeric()).  With that, the slow cases I mentioned now 
> have the same performance as the other cases that didn't have any 
> numeric division.  You just get the overhead for constructing and 
> passing around a numeric instead of a double, which can't be avoided.

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.  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?

> So here is an intermediate patch that does this.  I haven't gotten rid 
> of all numeric_div_opt_error() calls yet, but if this seems acceptable, 
> I can work on the remaining ones.

I guess the immediate question is how much of a performance gap there
is now between the float and numeric implementations.

			regards, tom lane

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

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2021-03-22T20:58:20Z

    On 19.03.21 21:06, Tom Lane wrote:
> I guess the immediate question is how much of a performance gap there
> is now between the float and numeric implementations.

Attached are my test script and the full output.

To summarize, for cases that don't do any interesting computation and 
where the overhead is only the data type passing, the difference is like 
this:

-- old
select date_part('microseconds', current_timestamp + generate_series(0, 
10000000) * interval '1 second') \g /dev/null
Time: 2760.966 ms (00:02.761)

-- new
select extract(microseconds from current_timestamp + generate_series(0, 
10000000) * interval '1 second') \g /dev/null
Time: 3178.477 ms (00:03.178)

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-03-23T20:52:01Z

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> On 19.03.21 21:06, Tom Lane wrote:
>> I guess the immediate question is how much of a performance gap there
>> is now between the float and numeric implementations.

> Attached are my test script and the full output.

OK ... I prefer to do this sort of timing in a way that's not so
dependent on client I/O speeds, along the lines of

select count(date_part('day', current_date + g * interval '1 day')) from generate_series(0, :N) g;

I applied the v5 patch and ran your test suite that way, producing
the attached results.  It looks pretty promising for me, too.
Most of the cases show about 10%-15% degradation:

# select extract, date_part, extract/date_part as ratio, unit from (select sum(msec) filter (where fn = 'extract') as extract, sum(msec) filter (where fn = 'date_part') as date_part, unit from timings group by unit) ss order by ratio;
  extract  | date_part |         ratio          |      unit       
-----------+-----------+------------------------+-----------------
 22690.100 | 20705.402 | 1.09585411575201486066 | decade
 22810.005 | 20754.296 | 1.09904980636298142804 | century
 11238.122 | 10190.385 | 1.10281623314526389337 | timezone_minute
 20201.992 | 18303.982 |     1.1036938301184955 | doy
 20121.073 | 18206.290 |     1.1051715094069138 | dow
 23209.090 | 20915.715 | 1.10964841507928368693 | millennium
 18839.455 | 16943.063 | 1.11192734159106886399 | week
 20130.843 | 18010.011 |     1.1177585066438882 | isoyear
 19755.296 | 17668.497 | 1.11810846163089027890 | isodow
 22500.373 | 20112.264 | 1.11873894455641592612 | day
 22631.485 | 20200.266 | 1.12035579135443067928 | month
 22883.344 | 20407.733 | 1.12130749652594925659 | quarter
 22628.524 | 20172.361 | 1.12175882634660365239 | year
 26503.545 | 23493.288 | 1.12813263941598979249 | minute
 26381.817 | 23329.924 | 1.13081452815705700542 | hour
 27236.886 | 24070.860 | 1.13152940941869131390 | microseconds
 11563.820 |  9948.148 |     1.1624093248311143 | timezone_hour
 27728.212 | 23567.973 | 1.17652086583771968849 | second
 28348.328 | 23984.219 | 1.18195751965073367617 | milliseconds
 49902.129 | 30798.034 |     1.6203024193037776 | epoch
 31544.035 | 18250.745 |     1.7283697186060076 | julian
(21 rows)

The outliers are epoch and julian, which unsurprisingly are the
ones you didn't fix yet.

I think a ten-percent-ish slowdown is acceptable for this purpose,
so I think if you can address the points already raised then we're
pretty much good to go with this.

			regards, tom lane

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

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2021-04-01T18:49:45Z

    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.

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

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2021-04-06T05:48:21Z

    On 01.04.21 20:49, Peter Eisentraut wrote:
> 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.

committed and done

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-04-19T15:57:25Z

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> 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.

I looked into this issue.  It's not quite true that the behavior is
entirely undocumented: Appendix B (datetime.sgml) says

   In the Julian Date system, each day has a sequential number, starting
   from JD 0 (which is sometimes called <emphasis>the</emphasis> Julian Date).
   JD 0 corresponds to 1 January 4713 BC in the Julian calendar, or
   24 November 4714 BC in the Gregorian calendar.  Julian Date counting
   is most often used by astronomers for labeling their nightly observations,
   and therefore a date runs from noon UTC to the next noon UTC, rather than
   from midnight to midnight: JD 0 designates the 24 hours from noon UTC on
   24 November 4714 BC to noon UTC on 25 November 4714 BC.
  </para>

  <para>
   Although <productname>PostgreSQL</productname> supports Julian Date notation for
   input and output of dates (and also uses Julian dates for some internal
   datetime calculations), it does not observe the nicety of having dates
   run from noon to noon.  <productname>PostgreSQL</productname> treats a Julian Date
   as running from midnight to midnight.
  </para>

That last bit requires clarification: we treat a Julian date as running
from *local* midnight to local midnight (ie in the active timezone, not
UTC midnight).  So far as I can see, the behavior of extract(julian) is
consistent with that definition:

regression=# show timezone;
     TimeZone     
------------------
 America/New_York
(1 row)

regression=# select date_part('julian', '2021-04-19 00:00:01-04'::timestamptz);
     date_part     
-------------------
 2459324.000011574
(1 row)

regression=# select date_part('julian', '2021-04-19 23:59:00-04'::timestamptz);
     date_part      
--------------------
 2459324.9993055556
(1 row)

regression=# select date_part('julian', '2021-04-19'::date);
 date_part 
-----------
   2459324
(1 row)

I don't see that to_char's J mode differs from this, either.

So I don't think there's any code change required (unless you are still
worried about speed).  What we do need is documentation fixes:

* clarify the above bit about local vs UTC midnight

* document the existence of the julian field for date_part/extract

* fix this bit in the to_char docs to agree with reality,
ie s/UTC/local time/:

       <row>
        <entry><literal>J</literal></entry>
        <entry>Julian Day (integer days since November 24, 4714 BC at midnight UTC)</entry>
       </row>

Perhaps it'd be worth documenting that you can get the standard
astronomical definition of Julian date by transposing to time zone UTC-12
before converting.  But I think trying to change PG's behavior at this
point would be a bad idea.

(We could also consider back-patching these doc fixes.)

			regards, tom lane

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-04-22T20:26:53Z

    I wrote:
> So I don't think there's any code change required (unless you are still
> worried about speed).  What we do need is documentation fixes:
> * clarify the above bit about local vs UTC midnight
> * document the existence of the julian field for date_part/extract
> * fix this bit in the to_char docs to agree with reality,
> ie s/UTC/local time/:
>         <entry>Julian Day (integer days since November 24, 4714 BC at midnight UTC)</entry>
> Perhaps it'd be worth documenting that you can get the standard
> astronomical definition of Julian date by transposing to time zone UTC-12
> before converting.  But I think trying to change PG's behavior at this
> point would be a bad idea.

Here's a concrete documentation proposal covering this.

			regards, tom lane

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-04-27T15:56:22Z

    I wrote:
>> Perhaps it'd be worth documenting that you can get the standard
>> astronomical definition of Julian date by transposing to time zone UTC-12
>> before converting.

BTW ... I'd first thought that the way to do this was to rotate to
time zone UTC+12.  I convinced myself on two separate days that UTC-12
was correct instead, but now I'm thinking I was right the first time.
In particular, the results I'm getting with UTC-12 don't square with
the example on Wikipedia [1], which says "the Julian Date for
00:30:00.0 UT January 1, 2013, is 2 456 293.520 833":

regression=# select extract(julian from '2013-01-01 00:30+00'::timestamptz at time zone 'utc-12');
           extract            
------------------------------
 2456294.52083333333333333333
(1 row)

But using UTC+12 does match:

regression=# select extract(julian from '2013-01-01 00:30+00'::timestamptz at time zone 'utc+12');
           extract            
------------------------------
 2456293.52083333333333333333
(1 row)

Of course Wikipedia has been known to contain errors, but now
I'm inclined to think I blew this.  Anyone want to check my work?

			regards, tom lane

[1] https://en.wikipedia.org/wiki/Julian_day

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

    Thomas Munro <thomas.munro@gmail.com> — 2021-04-28T05:43:27Z

    On Wed, Apr 28, 2021 at 3:56 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Of course Wikipedia has been known to contain errors, but now
> I'm inclined to think I blew this.  Anyone want to check my work?

I tried a couple of examples not from Wikipedia.  First, from the
definition of Julian days as used by astronomers[1], counting from
noon on 4713-01-01 BC Julian AKA 4714-11-24 BC Gregorian, days 0 and 1
look right with 'utc+12':

postgres=# select extract(julian from '4714-11-24 11:00:00+00
BC'::timestamptz at time zone 'utc+12');
ERROR:  timestamp out of range
postgres=# select extract(julian from '4714-11-24 12:00:00+00
BC'::timestamptz at time zone 'utc+12');
            extract
--------------------------------
 0.0000000000000000000000000000
(1 row)

postgres=# select extract(julian from '4714-11-25 11:00:00+00
BC'::timestamptz at time zone 'utc+12');
        extract
------------------------
 0.95833333333333333333
(1 row)

postgres=# select extract(julian from '4714-11-25 12:00:00+00
BC'::timestamptz at time zone 'utc+12');
            extract
--------------------------------
 1.0000000000000000000000000000
(1 row)

Next I found a worked example in an aerospace textbook[1] and it agrees, too:

postgres=# select extract(julian from '2004-05-12
14:45:30+00'::timestamptz at time zone 'utc+12');
           extract
------------------------------
 2453138.11493055555555555556
(1 row)

[1] http://curious.astro.cornell.edu/people-and-astronomy/125-observational-astronomy/timekeeping/calendars/763-how-was-the-starting-point-for-the-julian-date-system-chosen-advanced
[2] https://www.sciencedirect.com/topics/engineering/julian-day-number

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-04-28T13:43:58Z

    Thomas Munro <thomas.munro@gmail.com> writes:
> On Wed, Apr 28, 2021 at 3:56 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Of course Wikipedia has been known to contain errors, but now
>> I'm inclined to think I blew this.  Anyone want to check my work?

> I tried a couple of examples not from Wikipedia.  ...

Thanks for checking!  I'll go adjust the documentation.

			regards, tom lane

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

    Noah Misch <noah@leadboat.com> — 2021-06-06T06:33:31Z

    On Tue, Nov 03, 2020 at 07:22:14PM -0500, Tom Lane wrote:
> I feel like this is committable at this point --- any objections?

(This became commit 40c24bf, "Improve our ability to regurgitate SQL-syntax
function calls.")

> --- a/src/backend/nodes/equalfuncs.c
> +++ b/src/backend/nodes/equalfuncs.c
> @@ -2369,11 +2369,12 @@ _equalFuncCall(const FuncCall *a, const FuncCall *b)

> +	COMPARE_SCALAR_FIELD(funcformat);

equalfuncs.c has been using COMPARE_COERCIONFORM_FIELD() to ignore differences
in fields of this type.  Does this spot have cause to depart from the pattern?

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-06-06T14:37:58Z

    Noah Misch <noah@leadboat.com> writes:
> equalfuncs.c has been using COMPARE_COERCIONFORM_FIELD() to ignore differences
> in fields of this type.  Does this spot have cause to depart from the pattern?

Oversight, I think.  Will fix.

			regards, tom lane

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-06-06T16:38:26Z

    I wrote:
> Noah Misch <noah@leadboat.com> writes:
>> equalfuncs.c has been using COMPARE_COERCIONFORM_FIELD() to ignore differences
>> in fields of this type.  Does this spot have cause to depart from the pattern?

> Oversight, I think.  Will fix.

After looking closer, I see that there are a couple of very very minor
ways in which parse analysis changes behavior based on the value of
FuncCall.funcformat:

* transformRangeFunction won't apply the appropriate transformation to
a multiple-argument unnest() unless the format is COERCE_EXPLICIT_CALL.
(This is likely a no-op, though, as no grammar production that emits
COERCE_SQL_SYNTAX could apply to the function name "unnest".)

* ParseFuncOrColumn will not believe that a FuncCall could_be_projection
unless the format is COERCE_EXPLICIT_CALL.  This is next door to a no-op,
since other restrictions such as nargs == 1 would usually suffice to
reject COERCE_SQL_SYNTAX calls, but maybe there are corner cases where
it'd matter.

So if you wanted to be picky you could claim that within FuncCall,
funcformat is semantically significant and thus that equalfuncs.c is
coded correctly.  Nonetheless I'm inclined to think that it'd be better
to use COMPARE_COERCIONFORM_FIELD here.  I'm quite sure I didn't make
the above analysis when I wrote the code; using COMPARE_SCALAR_FIELD
was just reflex.

We could make use of COMPARE_COERCIONFORM_FIELD 100% correct by removing
these two tests of the funcformat value, but on the whole I doubt that
would be better.

BTW, I'm not sure any of this matters anyway; do we ever use equal()
on raw parse trees, except for debug purposes?

Thoughts?

			regards, tom lane

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-06-06T19:10:07Z

    I wrote:
> We could make use of COMPARE_COERCIONFORM_FIELD 100% correct by removing
> these two tests of the funcformat value, but on the whole I doubt that
> would be better.

On still closer inspection, that seems like it'd be fine.  All of
the gram.y productions that emit COERCE_SQL_SYNTAX also produce
schema-qualified function names (via SystemFuncName); and it seems
hard to see a use-case where we'd not do that.  This makes the two
checks I cited 100% redundant, because the conditions they are in
also insist on an unqualified function name.  So let's just take them
out again, making it strictly OK to use COMPARE_COERCIONFORM_FIELD.

			regards, tom lane

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

    Noah Misch <noah@leadboat.com> — 2021-06-08T05:31:55Z

    On Sun, Jun 06, 2021 at 03:10:07PM -0400, Tom Lane wrote:
> I wrote:
> > We could make use of COMPARE_COERCIONFORM_FIELD 100% correct by removing
> > these two tests of the funcformat value, but on the whole I doubt that
> > would be better.
> 
> On still closer inspection, that seems like it'd be fine.  All of
> the gram.y productions that emit COERCE_SQL_SYNTAX also produce
> schema-qualified function names (via SystemFuncName); and it seems
> hard to see a use-case where we'd not do that.  This makes the two
> checks I cited 100% redundant, because the conditions they are in
> also insist on an unqualified function name.  So let's just take them
> out again, making it strictly OK to use COMPARE_COERCIONFORM_FIELD.

I have little intuition on this exact topic, but I have no particular concerns
about the change you pushed.