Обсуждение: [PATCH] Supporting +-Infinity values by to_timestamp(float8)

Hello, Hackers!

I worked on a patch[1] allows "EXTRACT(epoch FROM
+-Inf::timestamp[tz])" to return "+-Inf::float8".
There is an opposite function "to_timestamp(float8)" which now defined as:
SELECT ('epoch'::timestamptz + $1 * '1 second'::interval)

Since intervals do not support infinity values, it is impossible to do
something like:
SELECT to_timestamp('infinity'::float8);

... which is not good.

Supporting of such converting is in the TODO list[2] (by "converting
between infinity timestamp and float8").

Proposed patch implements it.

There is an other patch in the CF[3] 2016-03 implements checking of
timestamp[tz] for being in allowed range. Since it is wise to set
(fix) the upper boundary of timestamp[tz]s, I've included the file
"src/include/datatype/timestamp.h" from there to check that an input
value and a result are in the allowed range.

There is no changes in a documentation because allowed range is the
same as officially supported[4] (i.e. until 294277 AD).


[1]http://git.postgresql.org/pg/commitdiff/647d87c56ab6da70adb753c08d7cdf7ee905ea8a
[2]https://wiki.postgresql.org/wiki/Todo#Dates_and_Times
[3]https://commitfest.postgresql.org/9/540/
[4]http://www.postgresql.org/docs/devel/static/datatype-datetime.html

--
Best regards,
Vitaly Burovoy

Added to the CF 2016-03:
https://commitfest.postgresql.org/9/546/

-- 
Best regards,
Vitaly Burovoy

On 2/26/16, Vitaly Burovoy <vitaly.burovoy@gmail.com> wrote:
> Proposed patch implements it.

I'm sorry, I forgot to leave a note for reviewers and committers:

This patch requires CATALOG_VERSION_NO be bumped.

Since pg_proc.h entry has changed, it is important to check and run
regress tests on a new cluster (as if CATALOG_VERSION_NO was bumped).

-- 
Best regards,
Vitaly Burovoy

27.02.2016 09:57, Vitaly Burovoy:<br /><blockquote
cite="mid:CAKOSWN=qbbnF_oRue4rfsMmWb+7wkBN6fq5XNHgt5vR0TiAA5g@mail.gmail.com"type="cite"><pre wrap="">Hello, Hackers!
 

I worked on a patch[1] allows "EXTRACT(epoch FROM
+-Inf::timestamp[tz])" to return "+-Inf::float8".
There is an opposite function "to_timestamp(float8)" which now defined as:
SELECT ('epoch'::timestamptz + $1 * '1 second'::interval)
</pre></blockquote><br /> Hi, <br /> thank you for the patches.<br /> Could you explain, whether they depend on each
other?<br/><br /><blockquote cite="mid:CAKOSWN=qbbnF_oRue4rfsMmWb+7wkBN6fq5XNHgt5vR0TiAA5g@mail.gmail.com"
type="cite"><prewrap="">Since intervals do not support infinity values, it is impossible to do
 
something like:

SELECT to_timestamp('infinity'::float8);

... which is not good.

Supporting of such converting is in the TODO list[2] (by "converting
between infinity timestamp and float8").
</pre></blockquote><br /> You mention intervals here, and TODO item definitely says about 'infinity' interval,<br />
whilepatch and all the following discussion concerns to timestamps.<br /> Is it a typo or I misunderstood something
important?<br /> I assumed that following query will work, but it isn't. Could you clarify that?<br /> select
to_timestamp('infinity'::interval);<br/><br /><blockquote
cite="mid:CAKOSWN=qbbnF_oRue4rfsMmWb+7wkBN6fq5XNHgt5vR0TiAA5g@mail.gmail.com"type="cite"><pre wrap="">Proposed patch
implementsit.
 

There is an other patch in the CF[3] 2016-03 implements checking of
timestamp[tz] for being in allowed range. Since it is wise to set
(fix) the upper boundary of timestamp[tz]s, I've included the file
"src/include/datatype/timestamp.h" from there to check that an input
value and a result are in the allowed range.

There is no changes in a documentation because allowed range is the
same as officially supported[4] (i.e. until 294277 AD).
</pre></blockquote><br /> I think that you should update documentation. At least description of epoch on this page:<br
/><ahref="http://www.postgresql.org/docs/devel/static/functions-datetime.html"><a class="moz-txt-link-freetext"
href="http://www.postgresql.org/docs/devel/static/functions-datetime.html">http://www.postgresql.org/docs/devel/static/functions-datetime.html</a></a><br
/><p>Hereis how you can convert an epoch value back to a time stamp:<pre class="SCREEN">SELECT TIMESTAMP WITH TIME ZONE
'epoch'+ 982384720.12 * INTERVAL '1 second';
 
</pre><p>(The <code class="FUNCTION">to_timestamp</code> function encapsulates the above conversion.)<br /> More
thoughtsabout the patch:<br /><br /> 1. When I copy value from hints for min and max values (see examples below), it
worksfine for min, while max still leads to error.<br /> It comes from the check   "if (seconds >= epoch_ubound)". I
wonder,whether you should change hint message? <br /><br /> select to_timestamp(-210866803200.000000);<br />          
to_timestamp          <br /> ---------------------------------<br />  4714-11-24 02:30:17+02:30:17 BC<br /> (1 row)<br
/><br/><br /> select to_timestamp(9224318016000.000000);<br /> ERROR:  UNIX epoch out of range:
"9224318016000.000000"<br/> HINT:  Maximal UNIX epoch value is "9224318016000.000000"<br /><br /> 2. There is a comment
aboutJULIAN_MAXYEAR inaccuracy in timestamp.h:<br /><br />  * IS_VALID_JULIAN checks the minimum date exactly, but is a
bitsloppy<br />  * about the maximum, since it's far enough out to not be especially<br />  * interesting.<br /><br />
Maybeyou can expand it?<br /> - Is JULIAN_MAXYEAR4STAMPS helps to avoid overflow in all possible cases?<br /> - Why do
weneed to hold both definitions? I suppose, it's a matter of backward compatibility, isn't it?<br /><br /> 3.
(nitpicking)I don't sure about "4STAMPS" suffix. "4" is nice abbreviation, but it seems slightly confusing to me.<br
/><preclass="moz-signature" cols="72">-- 
 
Anastasia Lubennikova
Postgres Professional: <a class="moz-txt-link-freetext"
href="http://www.postgrespro.com">http://www.postgrespro.com</a>
The Russian Postgres Company</pre>

On 3/4/16, Anastasia Lubennikova <a.lubennikova@postgrespro.ru> wrote:
> 27.02.2016 09:57, Vitaly Burovoy:
>> Hello, Hackers!
>>
>> I worked on a patch[1] allows "EXTRACT(epoch FROM
>> +-Inf::timestamp[tz])" to return "+-Inf::float8".
>> There is an opposite function "to_timestamp(float8)" which now defined
>> as:
>> SELECT ('epoch'::timestamptz + $1 * '1 second'::interval)
>
> Hi,
> thank you for the patches.

Thank you for the review.

> Could you explain, whether they depend on each other?

Only logically. They reverse each other:
postgres=# SELECT v, to_timestamp(v), extract(epoch FROM to_timestamp(v)) FROM
postgres-#   unnest(ARRAY['+inf', '-inf', 0, 65536, 982384720.12]::float8[]) v;     v       |       to_timestamp
| date_part
 
--------------+---------------------------+--------------    Infinity | infinity                  |     Infinity
-Infinity| -infinity                 |    -Infinity           0 | 1970-01-01 00:00:00+00    |            0       65536
|1970-01-01 18:12:16+00    |        65536982384720.12 | 2001-02-17 04:38:40.12+00 | 982384720.12
 
(5 rows)


>> Since intervals do not support infinity values, it is impossible to do
>> something like:
>>
>> SELECT to_timestamp('infinity'::float8);
>>
>> ... which is not good.
>>
>> Supporting of such converting is in the TODO list[2] (by "converting
>> between infinity timestamp and float8").
>
> You mention intervals here, and TODO item definitely says about
> 'infinity' interval,

Yes, it is in the same block. But I wanted to point to the link
"converting between infinity timestamp and float8". There are two-way
conversion examples.

> while patch and all the following discussion concerns to timestamps.
> Is it a typo or I misunderstood something important?

It is just a reason why I rewrote it as an internal function.
I asked whether to just rewrite the function
"pg_catalog.to_timestamp(float8)" as an internal one or to add support
of infinite intervals. Tom Lane answered[5] "you should stay away from
infinite intervals".
So I left intervals as is.

> I assumed that following query will work, but it isn't. Could you
> clarify that?
> select to_timestamp('infinity'::interval);

It is not hard. There is no logical way to convert interval (e.g.
"5minutes") to a timestamp (or date).
There never was a function "to_timestamp(interval)" and never will be.
postgres=# select to_timestamp('5min'::interval);
ERROR:  function to_timestamp(interval) does not exist
LINE 1: select to_timestamp('1min'::interval);              ^
HINT:  No function matches the given name and argument types. You
might need to add explicit type casts.

>> Proposed patch implements it.
>>
>> There is an other patch in the CF[3] 2016-03 implements checking of
>> timestamp[tz] for being in allowed range. Since it is wise to set
>> (fix) the upper boundary of timestamp[tz]s, I've included the file
>> "src/include/datatype/timestamp.h" from there to check that an input
>> value and a result are in the allowed range.
>>
>> There is no changes in a documentation because allowed range is the
>> same as officially supported[4] (i.e. until 294277 AD).
>
> I think that you should update documentation. At least description of
> epoch on this page:
> http://www.postgresql.org/docs/devel/static/functions-datetime.html

Thank you very much for pointing where it is located (I saw only
"to_timestamp(TEXT, TEXT)").
I'll think how to update it.

> More thoughts about the patch:
>
> 1. When I copy value from hints for min and max values (see examples
> below), it works fine for min, while max still leads to error.
> It comes from the check   "if (seconds >= epoch_ubound)". I wonder,
> whether you should change hint message?
>
> select to_timestamp(-210866803200.000000);
>            to_timestamp
> ---------------------------------
>   4714-11-24 02:30:17+02:30:17 BC
> (1 row)
>
>
> select to_timestamp(9224318016000.000000);
> ERROR:  UNIX epoch out of range: "9224318016000.000000"
> HINT:  Maximal UNIX epoch value is "9224318016000.000000"

I agree, it is a little confusing. Do you (or anyone) know how to
construct a condensed phrase that it is an exclusive upper bound of an
allowed UNIX epoch range?

> 2. There is a comment about JULIAN_MAXYEAR inaccuracy in timestamp.h:
>
>   * IS_VALID_JULIAN checks the minimum date exactly, but is a bit sloppy
>   * about the maximum, since it's far enough out to not be especially
>   * interesting.

It is just about the accuracy to the day for a lower bound, and to the
year (not to a day) for an upper bound.

> Maybe you can expand it?
> - Is JULIAN_MAXYEAR4STAMPS helps to avoid overflow in all possible cases?
> - Why do we need to hold both definitions? I suppose, it's a matter of
> backward compatibility, isn't it?

Yes. I tried to be less invasive from the point of view of endusers.
They can be sure if they follow the documentation they won't get into
trouble.

> 3. (nitpicking) I don't sure about "4STAMPS" suffix. "4" is nice
> abbreviation, but it seems slightly confusing to me.

It doesn't matter for me what it is called, it is short enough and
reflects a type on which it is applied.
What would the best name be for it?

[5]http://www.postgresql.org/message-id/21367.1447046745@sss.pgh.pa.us
-- 
Best regards,
Vitaly Burovoy

On 3/4/16 2:56 PM, Vitaly Burovoy wrote:

> On 3/4/16, Anastasia Lubennikova <a.lubennikova@postgrespro.ru> wrote:
>
>> I think that you should update documentation. At least description of
>> epoch on this page:
>> http://www.postgresql.org/docs/devel/static/functions-datetime.html
> 
> Thank you very much for pointing where it is located (I saw only
> "to_timestamp(TEXT, TEXT)").
> I'll think how to update it.

Vitaly, have you decided how to update this yet?

>> 3. (nitpicking) I don't sure about "4STAMPS" suffix. "4" is nice
>> abbreviation, but it seems slightly confusing to me.
> 
> It doesn't matter for me what it is called, it is short enough and
> reflects a type on which it is applied.
> What would the best name be for it?

Anastasia, any suggestions for a better name, or just leave it as is?

I'm not in favor of the "4", either.  I think I would prefer
JULIAN_MAXYEAR_STAMP.

-- 
-David
david@pgmasters.net

15.03.2016 22:28, David Steele:
> On 3/4/16 2:56 PM, Vitaly Burovoy wrote:
>
>> On 3/4/16, Anastasia Lubennikova <a.lubennikova@postgrespro.ru> wrote:
>>
>>> I think that you should update documentation. At least description of
>>> epoch on this page:
>>> http://www.postgresql.org/docs/devel/static/functions-datetime.html
>> Thank you very much for pointing where it is located (I saw only
>> "to_timestamp(TEXT, TEXT)").
>> I'll think how to update it.
> Vitaly, have you decided how to update this yet?
>
>>> 3. (nitpicking) I don't sure about "4STAMPS" suffix. "4" is nice
>>> abbreviation, but it seems slightly confusing to me.
>> It doesn't matter for me what it is called, it is short enough and
>> reflects a type on which it is applied.
>> What would the best name be for it?
> Anastasia, any suggestions for a better name, or just leave it as is?
>
> I'm not in favor of the "4", either.  I think I would prefer
> JULIAN_MAXYEAR_STAMP.
>

This point is related to another patch 
https://commitfest.postgresql.org/9/540/.
And added to this patch just for compatibility.
If Tom wouldn't change the name of the macros there, I don't see any 
reasons why should we do it in this patch.

-- 
Anastasia Lubennikova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On 2016-03-15, David Steele <david@pgmasters.net> wrote:
> On 3/4/16 2:56 PM, Vitaly Burovoy wrote:
>> On 3/4/16, Anastasia Lubennikova <a.lubennikova@postgrespro.ru> wrote:
>>
>>> I think that you should update documentation. At least description of
>>> epoch on this page:
>>> http://www.postgresql.org/docs/devel/static/functions-datetime.html
>>
>> Thank you very much for pointing where it is located (I saw only
>> "to_timestamp(TEXT, TEXT)").
>> I'll think how to update it.
>
> Vitaly, have you decided how to update this yet?

Yes, there are three versions:
* remove mentioning how to get timestamptz from UNIX stamp;
* leave a note how to get timestamptz and add a note that such
encapsulation existed prior to 9.6;
* replace to the proposed current behavior (without interval).

I decided to implement the third case (but a result there has a time
zone which can be different, so the result can be not exactly same for
a concrete user). If a committer decides to do somehow else, he is
free to choose one from the list above or to do something else.

>>> 3. (nitpicking) I don't sure about "4STAMPS" suffix. "4" is nice
>>> abbreviation, but it seems slightly confusing to me.
>>
>> It doesn't matter for me what it is called, it is short enough and
>> reflects a type on which it is applied.
>> What would the best name be for it?
>
> Anastasia, any suggestions for a better name, or just leave it as is?
>
> I'm not in favor of the "4", either.  I think I would prefer
> JULIAN_MAXYEAR_STAMP.

It turns out that Tom has changed almost one third of timestamp.h and
now the constant has a name "TIMESTAMP_END_JULIAN".

I've rebased the patch to the current master (5db5146) and changed it
according to the new timestamp.h.

Now it passes both versions (integer and float timestamps).
I deleted test for the upper boundary for integer timestamps, changed
a little comments.

I decided to delete hints about minimum and maximum allowed values
because no one type has such hint.

Please find attached a new version of the patch.

--
Best regards,
Vitaly Burovoy

Anastasia Lubennikova <a.lubennikova@postgrespro.ru> writes:
> 15.03.2016 22:28, David Steele:
>> I'm not in favor of the "4", either.  I think I would prefer
>> JULIAN_MAXYEAR_STAMP.

> This point is related to another patch 
> https://commitfest.postgresql.org/9/540/.
> And added to this patch just for compatibility.
> If Tom wouldn't change the name of the macros there, I don't see any 
> reasons why should we do it in this patch.

Yeah, I didn't like the "4STAMPS" terminology at all.  It ended up being
moot for that patch, because the answer eventually turned out to be that
we needed to decouple the Julian-date boundaries from the datatype
boundaries altogether.  But I would've renamed those macros to something
else if they'd stayed.
        regards, tom lane

17.03.2016 06:27, Vitaly Burovoy:
> On 2016-03-15, David Steele <david@pgmasters.net> wrote:
>> On 3/4/16 2:56 PM, Vitaly Burovoy wrote:
>>> On 3/4/16, Anastasia Lubennikova <a.lubennikova@postgrespro.ru> wrote:
>>>
>>>> I think that you should update documentation. At least description of
>>>> epoch on this page:
>>>> http://www.postgresql.org/docs/devel/static/functions-datetime.html
>>> Thank you very much for pointing where it is located (I saw only
>>> "to_timestamp(TEXT, TEXT)").
>>> I'll think how to update it.
>> Vitaly, have you decided how to update this yet?
> Yes, there are three versions:
> * remove mentioning how to get timestamptz from UNIX stamp;
> * leave a note how to get timestamptz and add a note that such
> encapsulation existed prior to 9.6;
> * replace to the proposed current behavior (without interval).
>
> I decided to implement the third case (but a result there has a time
> zone which can be different, so the result can be not exactly same for
> a concrete user). If a committer decides to do somehow else, he is
> free to choose one from the list above or to do something else.
>
>>>> 3. (nitpicking) I don't sure about "4STAMPS" suffix. "4" is nice
>>>> abbreviation, but it seems slightly confusing to me.
>>> It doesn't matter for me what it is called, it is short enough and
>>> reflects a type on which it is applied.
>>> What would the best name be for it?
>> Anastasia, any suggestions for a better name, or just leave it as is?
>>
>> I'm not in favor of the "4", either.  I think I would prefer
>> JULIAN_MAXYEAR_STAMP.
> It turns out that Tom has changed almost one third of timestamp.h and
> now the constant has a name "TIMESTAMP_END_JULIAN".
>
> I've rebased the patch to the current master (5db5146) and changed it
> according to the new timestamp.h.
>
> Now it passes both versions (integer and float timestamps).
> I deleted test for the upper boundary for integer timestamps, changed
> a little comments.
>
> I decided to delete hints about minimum and maximum allowed values
> because no one type has such hint.
>
> Please find attached a new version of the patch.
>

I think, I should write something as a reviewer.
I read the patch again and I don't see any issues with it.
It applies to the master and works as expected. So I'll mark it as 
"Ready for committer"

-- 
Anastasia Lubennikova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Anastasia Lubennikova <a.lubennikova@postgrespro.ru> writes:
> 17.03.2016 06:27, Vitaly Burovoy:
>> Please find attached a new version of the patch.

> I think, I should write something as a reviewer.
> I read the patch again and I don't see any issues with it.
> It applies to the master and works as expected. So I'll mark it as 
> "Ready for committer"

Pushed with minor adjustments.
        regards, tom lane

On 3/29/16, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Pushed with minor adjustments.
>
>             regards, tom lane
>

Thank you very much!

-- 
Best regards,
Vitaly Burovoy