'infinity'::Interval should be added

Started by Simon Riggsabout 7 years ago22 messages
#1Simon Riggs
simon@2ndquadrant.com

At present we have a timestamp of 'infinity' and infinite ranges, but no
infinite interval

SELECT 'infinity'::timestamp;
works

SELECT 'infinity'::interval;
ERROR: invalid input syntax for type interval: "infinity"

Seems a strange anomaly that we should fix.

--
Simon Riggs http://www.2ndQuadrant.com/
<http://www.2ndquadrant.com/&gt;
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

#2Chapman Flack
chap@anastigmatix.net
In reply to: Simon Riggs (#1)
Re: 'infinity'::Interval should be added

On 12/13/18 04:41, Simon Riggs wrote:

SELECT 'infinity'::timestamp;
works

SELECT 'infinity'::interval;
ERROR: invalid input syntax for type interval: "infinity"

... and if that is made to work, perhaps then arithmetic should be
updated as well, with this producing an infinite interval result:

SELECT timestamptz 'infinity' - current_timestamp;
ERROR: cannot subtract infinite timestamps

ISO (2011 draft) seems to have nothing to say about infinity, for
datetimes or intervals.

-Chap

#3Tom Lane
tgl@sss.pgh.pa.us
In reply to: Chapman Flack (#2)
Re: 'infinity'::Interval should be added

Chapman Flack <chap@anastigmatix.net> writes:

On 12/13/18 04:41, Simon Riggs wrote:

[ we should allow this: ]
SELECT 'infinity'::interval;
ERROR: invalid input syntax for type interval: "infinity"

... and if that is made to work, perhaps then arithmetic should be
updated as well,

That wouldn't be optional. All built-in functions on interval
would *have* to be updated to deal sanely with the new values.

(Fortunately, there aren't that many of them, IIRC.)

regards, tom lane

#4Robert Haas
robertmhaas@gmail.com
In reply to: Simon Riggs (#1)
Re: 'infinity'::Interval should be added

On Thu, Dec 13, 2018 at 9:42 AM Simon Riggs <simon@2ndquadrant.com> wrote:

At present we have a timestamp of 'infinity' and infinite ranges, but no infinite interval

SELECT 'infinity'::timestamp;
works

SELECT 'infinity'::interval;
ERROR: invalid input syntax for type interval: "infinity"

Seems a strange anomaly that we should fix.

Why? I consider it somewhat of a wart that timestamps allow infinity
- it adds special case coding all over the place. Allowing intervals
to be infinite as well seems like it'll just create more special cases
without really solving anything.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

#5Isaac Morland
isaac.morland@gmail.com
In reply to: Robert Haas (#4)
Re: 'infinity'::Interval should be added

On Fri, 14 Dec 2018 at 15:16, Robert Haas <robertmhaas@gmail.com> wrote:

Why? I consider it somewhat of a wart that timestamps allow infinity
- it adds special case coding all over the place. Allowing intervals
to be infinite as well seems like it'll just create more special cases
without really solving anything.

Au contraire, it eliminates special case coding all over the place. For
example, if I have authorizations that don't expire, I can just give them
an expiration of 'infinity' and operations like "today's date less than
expiration" just work; I don't have to say "expiration null or greater than
today's date".

I would be interested if you have an example where the ability of
date/timestamp values to be infinite adds special case coding.

If we allow intervals to be infinity, then we will eliminate more special
cases. Right now, whenever timestamp values are subtracted, the programmer
has to consider what happens if an operand is infinity and handle that case
specially. With infinite intervals allowed, this problem is reduced. For
example, how long until expiry? Right now:

CASE WHEN expiry = 'infinity' THEN NULL WHEN expiry = '-infinity' THEN '0'
ELSE expiry - current_timestamp END

With the proposal:

expiry - current_timestamp

And another improvement: infinity is represented by 'infinity' rather than
the somewhat ambiguous NULL.

Just for definiteness, I believe the following are the correct semantics
for subtraction involving infinity:

∞ - t = ∞ (t ≠ ∞)
-∞ - t = -∞ (t ≠ -∞)
∞ - ∞ = err
-∞ - -∞ = err

I'm not sure whether "err" should be an error, as it is now ("cannot
subtract infinite dates") and like division by 0, or NULL.

The wart I'm worried about is subtraction of infinite dates. Right now
dates subtract to give integers; and there are no infinite integers. All
the clever solutions to this I have right now involve making highly
backward-incompatible changes.

#6Tom Lane
tgl@sss.pgh.pa.us
In reply to: Isaac Morland (#5)
Re: 'infinity'::Interval should be added

Isaac Morland <isaac.morland@gmail.com> writes:

I would be interested if you have an example where the ability of
date/timestamp values to be infinite adds special case coding.

I think Robert is talking about the implementation functions for
timestamp-related operations, which typically do have to special-case
infinite inputs. I take your point that there might be fewer special
cases in the calling SQL code, though.

The wart I'm worried about is subtraction of infinite dates. Right now
dates subtract to give integers; and there are no infinite integers. All
the clever solutions to this I have right now involve making highly
backward-incompatible changes.

As far as that goes, I'm content to say that infinity is outside the
domain of type date. If you need infinities, use timestamp.

regards, tom lane

#7Robert Haas
robertmhaas@gmail.com
In reply to: Tom Lane (#6)
Re: 'infinity'::Interval should be added

On Fri, Dec 14, 2018 at 9:11 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Isaac Morland <isaac.morland@gmail.com> writes:

I would be interested if you have an example where the ability of
date/timestamp values to be infinite adds special case coding.

I think Robert is talking about the implementation functions for
timestamp-related operations, which typically do have to special-case
infinite inputs. I take your point that there might be fewer special
cases in the calling SQL code, though.

Exactly. And now that we have JIT compilation, the overhead of such
cases in more worth considering than previously. To take a related
example, int4pl() can be reduced to a single instruction if you ignore
overflow -- but you can't ignore overflow, which means you needs some
more instructions to handle the overflow case, which means that the
number of instructions is actually increasing quite a bit. Without
JIT, maybe that's not too noticeable because you've got to pay the
cost of setting up a stack frame for the function and tearing it down,
but JIT can optimize those costs away.

So essentially I think supporting special values like infinity boils
down to trading away some small amount of performance -- more likely
to be noticeable with JIT -- for some amount of possible programmer
convenience. Some people may think that's a good trade, and other
people may not like it. It just depends on whether the 'infinity'
value is useful to you. If it is, you'll probably like the trade,
because aside from the notational cost which Isaac mentioned, it's
probably vastly faster to handle the infinity values in C code than to
stick CASE..WHEN in to an SQL query. If it's not, you may dislike it.
If your application code now has to know about the possibility
'infinity' value that it otherwise wouldn't have to worry about, you
may dislike it for that reason also.

I'm not sure there's one right answer here - my personal feeling is
that infinite values are a wart, but I grant Isaac's point that they
can sometimes simplify SQL coding.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

#8Simon Riggs
simon@2ndquadrant.com
In reply to: Robert Haas (#7)
Re: 'infinity'::Interval should be added

On Fri, 14 Dec 2018 at 22:24, Robert Haas <robertmhaas@gmail.com> wrote:

So essentially I think supporting special values like infinity boils
down to trading away some small amount of performance -- more likely
to be noticeable with JIT -- for some amount of possible programmer
convenience. Some people may think that's a good trade, and other
people may not like it. It just depends on whether the 'infinity'
value is useful to you. If it is, you'll probably like the trade,
because aside from the notational cost which Isaac mentioned, it's
probably vastly faster to handle the infinity values in C code than to
stick CASE..WHEN in to an SQL query. If it's not, you may dislike it.
If your application code now has to know about the possibility
'infinity' value that it otherwise wouldn't have to worry about, you
may dislike it for that reason also.

I'm not sure there's one right answer here - my personal feeling is
that infinite values are a wart, but I grant Isaac's point that they
can sometimes simplify SQL coding.

I have no objection to the introduction of another datatype that is
stripped down for performance. I understand and agree with that need.

But the current datatypes do handle much complexity already. Blocking this
proposal would not change that, IMHO. All that is being proposed is a small
change to rationalize the existing code.

--
Simon Riggs http://www.2ndQuadrant.com/
<http://www.2ndquadrant.com/&gt;
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

#9Tom Lane
tgl@sss.pgh.pa.us
In reply to: Simon Riggs (#8)
Re: 'infinity'::Interval should be added

Simon Riggs <simon@2ndquadrant.com> writes:

On Fri, 14 Dec 2018 at 22:24, Robert Haas <robertmhaas@gmail.com> wrote:

So essentially I think supporting special values like infinity boils
down to trading away some small amount of performance -- more likely
to be noticeable with JIT -- for some amount of possible programmer
convenience.

But the current datatypes do handle much complexity already. Blocking this
proposal would not change that, IMHO. All that is being proposed is a small
change to rationalize the existing code.

Yes. The performance argument has some merit for cases like int4 and
float8, where the "useful work" might be as small as one machine
instruction. But timestamp and interval operations are, for the most
part, pretty darn expensive. I doubt that adding special cases to
them for infinity is going to move the needle noticeably. (And as
for JIT, I sincerely hope that the compiler is not dumb enough to try
to in-line those functions.)

regards, tom lane

#10Andres Freund
andres@anarazel.de
In reply to: Tom Lane (#9)
Re: 'infinity'::Interval should be added

Hi,

On 2018-12-15 09:44:50 -0500, Tom Lane wrote:

Simon Riggs <simon@2ndquadrant.com> writes:

On Fri, 14 Dec 2018 at 22:24, Robert Haas <robertmhaas@gmail.com> wrote:

So essentially I think supporting special values like infinity boils
down to trading away some small amount of performance -- more likely
to be noticeable with JIT -- for some amount of possible programmer
convenience.

I don't think the overflow checks are inherently that expensive. Look
how the int[48]{pl,mi,...} checks are done these days, that's fairly
fast (just adds a single branch, using a flag test). We still generate
quite sub-standard code for sequence of int operations, but that's less
the fault of the overflow check itself, and more of a) parts of the
function call interface that don't get optimized away properly b) the
ereport() calls, which all reference custom arguments, like the line
number. The latter is bad because it's a fair bit of code, and because
it prevents the compiler from optimizing away redundant checks - I'd
experimented with moving the overflow checks into a noinline, noreturn
function, and that resolved the issue to a significant degree.

I've previously harped on about the overflow checks for floats being
expensive, but that's because checking whether there was an overflow is
quite expensive for floats...

Right now the overflow checks end up with emitting heaps of

%41 = tail call zeroext i1 @errstart(i32 20, i8* getelementptr inbounds ([62 x i8], [62 x i8]* @.str, i64 0, i64 0), i32 3101, i8* getelementptr inbounds ([12 x i8], [12 x i8]* @__func__.interval_pl, i64 0, i64 0), i8* null) #11
%42 = tail call i32 @errcode(i32 134217858) #11
%43 = tail call i32 (i8*, ...) @errmsg(i8* getelementptr inbounds ([22 x i8], [22 x i8]* @.str.15, i64 0, i64 0)) #11
tail call void (i32, ...) @errfinish(i32 %42, i32 %43) #11
unreachable

sometimes 3 times per function, repeated nearly identically dozens of
times. If we were to move that to a
interval_overflows()/int4_overflows/ ... operation declared noreturn,
noinline, we'd have signficantly less code. At the price of slightly
less informative error messages, obviously.

But the current datatypes do handle much complexity already. Blocking this
proposal would not change that, IMHO. All that is being proposed is a small
change to rationalize the existing code.

Yes. The performance argument has some merit for cases like int4 and
float8, where the "useful work" might be as small as one machine
instruction. But timestamp and interval operations are, for the most
part, pretty darn expensive. I doubt that adding special cases to
them for infinity is going to move the needle noticeably. (And as
for JIT, I sincerely hope that the compiler is not dumb enough to try
to in-line those functions.)

Something like interval_pl is cheap enough to inline (without inlinining
palloc, and elog.c infrastructure), even though there's repeated
ereports() checks with different parameters / line numbers [1]We should optimize the computation using pg_add_s{32,64}_overflow, the generated code after that change is *substantially* better (as well as the original code much shorter, and correct). And consider, as mentioned further up, moving throwing the error out of line.. Boils
down to ~70 instructions, which allows inlining. But I suspect more
important than those, are operations like timestamp_lt etc, which
currently are ~8 instructions. It's pretty common to have a
timestamp_lt or such in sequential scans, so that's good too.

I'm not quite sure why it'd be that dumb to inline those functions? Did
you think they'd be much longer?

[1]: We should optimize the computation using pg_add_s{32,64}_overflow, the generated code after that change is *substantially* better (as well as the original code much shorter, and correct). And consider, as mentioned further up, moving throwing the error out of line.
the generated code after that change is *substantially* better (as
well as the original code much shorter, and correct). And consider,
as mentioned further up, moving throwing the error out of line.

Greetings,

Andres Freund

#11Tom Lane
tgl@sss.pgh.pa.us
In reply to: Andres Freund (#10)
Re: 'infinity'::Interval should be added

Andres Freund <andres@anarazel.de> writes:

On 2018-12-15 09:44:50 -0500, Tom Lane wrote:

Yes. The performance argument has some merit for cases like int4 and
float8, where the "useful work" might be as small as one machine
instruction. But timestamp and interval operations are, for the most
part, pretty darn expensive. I doubt that adding special cases to
them for infinity is going to move the needle noticeably. (And as
for JIT, I sincerely hope that the compiler is not dumb enough to try
to in-line those functions.)

Something like interval_pl is cheap enough to inline (without inlinining
palloc, and elog.c infrastructure), even though there's repeated
ereports() checks with different parameters / line numbers [1]. Boils
down to ~70 instructions, which allows inlining. But I suspect more
important than those, are operations like timestamp_lt etc, which
currently are ~8 instructions. It's pretty common to have a
timestamp_lt or such in sequential scans, so that's good too.

I said "for the most part", I did not say that every single function
on those types is expensive.

Note that timestamp_lt etc don't actually need any special case for
infinity, and we could hope that the infinity representation for interval
makes it possible to likewise not special-case it in interval comparisons.
But I think it's silly to argue that infinity handling is a significant
fraction of something like timestamp_pl_interval or timestamp_part.

regards, tom lane

#12Andres Freund
andres@anarazel.de
In reply to: Tom Lane (#11)
Re: 'infinity'::Interval should be added

Hi,

On 2018-12-15 14:43:49 -0500, Tom Lane wrote:

Note that timestamp_lt etc don't actually need any special case for
infinity, and we could hope that the infinity representation for interval
makes it possible to likewise not special-case it in interval comparisons.
But I think it's silly to argue that infinity handling is a significant
fraction of something like timestamp_pl_interval or timestamp_part.

I'm inclined to agree that if done carefully the overhead here is
probably acceptable.

Greetings,

Andres Freund

#13Tom Lane
tgl@sss.pgh.pa.us
In reply to: Andres Freund (#12)
Re: 'infinity'::Interval should be added

Andres Freund <andres@anarazel.de> writes:

On 2018-12-15 14:43:49 -0500, Tom Lane wrote:

Note that timestamp_lt etc don't actually need any special case for
infinity, and we could hope that the infinity representation for interval
makes it possible to likewise not special-case it in interval comparisons.
But I think it's silly to argue that infinity handling is a significant
fraction of something like timestamp_pl_interval or timestamp_part.

I'm inclined to agree that if done carefully the overhead here is
probably acceptable.

Backing up to look at the actual code ... if the infinity representation
is the max or min value in each of the three fields, then the conversion
done by interval_cmp_value would yield an int128 value that's certainly
greater than or less than any other interval value, and I don't think it
can overflow, so there's no need to add any code to the comparison cases.

regards, tom lane

#14Robert Haas
robertmhaas@gmail.com
In reply to: Tom Lane (#13)
Re: 'infinity'::Interval should be added

On Sat, Dec 15, 2018 at 3:02 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Andres Freund <andres@anarazel.de> writes:

On 2018-12-15 14:43:49 -0500, Tom Lane wrote:

Note that timestamp_lt etc don't actually need any special case for
infinity, and we could hope that the infinity representation for interval
makes it possible to likewise not special-case it in interval comparisons.
But I think it's silly to argue that infinity handling is a significant
fraction of something like timestamp_pl_interval or timestamp_part.

I'm inclined to agree that if done carefully the overhead here is
probably acceptable.

Backing up to look at the actual code ... if the infinity representation
is the max or min value in each of the three fields, then the conversion
done by interval_cmp_value would yield an int128 value that's certainly
greater than or less than any other interval value, and I don't think it
can overflow, so there's no need to add any code to the comparison cases.

OK, well, I stand corrected. Not sure that answers the question of
whether we want this, but it's nice to know that if we do, it can be
done without causing too much slowdown.

Simon's argument for adding this is that we support 'infinity' for
timestamp, but is that a good argument for making 'interval' do it,
given that there are many other types like date for which we don't
support it?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

#15Tom Lane
tgl@sss.pgh.pa.us
In reply to: Robert Haas (#14)
Re: 'infinity'::Interval should be added

Robert Haas <robertmhaas@gmail.com> writes:

Simon's argument for adding this is that we support 'infinity' for
timestamp, but is that a good argument for making 'interval' do it,
given that there are many other types like date for which we don't
support it?

My feeling is that date is to timestamp as integer is to float.
We have infinities in the latter types but not the former, and
that seems just fine: infinity is one of the additional values
that you get to have with the bigger/more expensive type.
So I don't really feel that the lack of infinity in date is an
argument against whether to provide it for interval.

The positive argument for adding infinity to interval is that
we define operations such as timestamp minus timestamp as
yielding interval. That's why this has to fail right now:

regression=# select timestamp 'infinity' - timestamp 'now';
ERROR: cannot subtract infinite timestamps

But if we had infinite intervals then that would have a well
defined result, just as this works:

regression=# select extract(epoch from timestamp 'infinity');
date_part
-----------
Infinity
(1 row)

Of course, there are still cases like timestamp 'infinity' -
timestamp 'infinity' that would need to fail, but that has a
semantic basis rather than "the output type can't represent it".
(No, I don't want to invent an interval equivalent of NaN
to make that not fail.)

[ wanders away wondering why type numeric has NaN but not infinity ]

regards, tom lane

#16Isaac Morland
isaac.morland@gmail.com
In reply to: Robert Haas (#14)
Re: 'infinity'::Interval should be added

On Sun, 16 Dec 2018 at 22:27, Robert Haas <robertmhaas@gmail.com> wrote:

Simon's argument for adding this is that we support 'infinity' for
timestamp, but is that a good argument for making 'interval' do it,
given that there are many other types like date for which we don't
support it?

postgres=> select 'infinity'::date, '-infinity'::date;
date | date
----------+-----------
infinity | -infinity
(1 row)

postgres=>

Works since 8.4:

https://www.postgresql.org/docs/8.4/datatype-datetime.html

I think that in principle an argument can be made for having infinity and
-infinity in every data type, sort of like how every data type has null,
but based on the nature of the system as it exists I'm not going to advance
that argument.

I do think that decimal/numeric probably ought to have infinity, assuming
there is a reasonable way to fit them into the representation,
but integer types are currently exactly 2/4/8-byte 2's complement values so
I don't see how to squeeze in infinite values in a way that wouldn't mess
up existing code (or cause handling of integer values to be much slower).

#17Simon Riggs
simon@2ndquadrant.com
In reply to: Tom Lane (#15)
Re: 'infinity'::Interval should be added

On Mon, 17 Dec 2018 at 03:48, Tom Lane <tgl@sss.pgh.pa.us> wrote:

The positive argument for adding infinity to interval is that
we define operations such as timestamp minus timestamp as
yielding interval. That's why this has to fail right now:

regression=# select timestamp 'infinity' - timestamp 'now';
ERROR: cannot subtract infinite timestamps

I would like to represent 'infinity' as interval->months = INT_MAX

The documented maximum for an Interval datatype is 178000000 years, which
is 2136000000 months.
but it is possible to have a higher value (up to 2147483647), since we
don't check inputs.
As a result, it is possible that someone is already storing values above
the stated limits, so this would change behavior for them. But if they were
the net effect of it would be the same, it is still a very, very long
interval. It's not long enough to store useful time intervals for geology
or astrophysics, so I doubt it is used at all for that purpose.

Would there be objection to using the INT_MAX value? If so, what else can
be used?

Of course, there are still cases like timestamp 'infinity' -
timestamp 'infinity' that would need to fail, but that has a
semantic basis rather than "the output type can't represent it".
(No, I don't want to invent an interval equivalent of NaN
to make that not fail.)

Currently

postgres=# select 'infinity'::timestamp = 'infinity'::timestamp;
?column?
----------
t

so I was thinking that

postgres=# select 'infinity'::timestamp - 'infinity'::timestamp;

would be zero rather than an error, for least surprise.

--
Simon Riggs http://www.2ndQuadrant.com/
<http://www.2ndquadrant.com/&gt;
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

#18Chapman Flack
chap@anastigmatix.net
In reply to: Simon Riggs (#17)
Re: 'infinity'::Interval should be added

On 12/17/18 5:37 PM, Simon Riggs wrote:

postgres=# select 'infinity'::timestamp = 'infinity'::timestamp;
?column?
----------
t

I'm not persuaded that's a good idea, and would prefer to see an
is_infinite() predicate, and an = operator that complains. But if
the above is current behavior, I wouldn't say I feel strongly enough
to want to change it.

postgres=# select 'infinity'::timestamp - 'infinity'::timestamp;

would be zero rather than an error, for least surprise.

Here, though, I really think an error is appropriate. Getting a
definite finite result from two operands that really mean "I am
huge but otherwise unknown" seems very surprising to me.

-Chap

#19Tom Lane
tgl@sss.pgh.pa.us
In reply to: Simon Riggs (#17)
Re: 'infinity'::Interval should be added

Simon Riggs <simon@2ndquadrant.com> writes:

I would like to represent 'infinity' as interval->months = INT_MAX

Per the discussion upthread, what we need when creating an
infinite value is to set
month = INT32_MAX, day = INT32_MAX, time = INT64_MAX (for +Infinity)
month = INT32_MIN, day = INT32_MIN, time = INT64_MIN (for -Infinity)
else we'll need to put special-case logic into comparisons.

However, I think that we only need to restrict ordinary values from
not having month equal to INT32_MIN/MAX, that is the full range
of the other two fields can still be available for normal use.
So when *testing* for an infinity, you would only need to look at the
month field.

Currently
postgres=# select 'infinity'::timestamp = 'infinity'::timestamp;
?column?
----------
t

Correct, we'd need that here too, if only because btree indexes and
sorting require the reflexive axiom to hold.

so I was thinking that
postgres=# select 'infinity'::timestamp - 'infinity'::timestamp;
would be zero rather than an error, for least surprise.

Wrong. This case needs to be undefined, because "infinity"
isn't a specific value. That's what makes it okay to define, say,
infinity plus any finite value as infinity. There are very
well-defined rules about how to calculate with infinity, and
not following them is not the way to proceed here.

tl;dr: we should model it after the behavior of IEEE float infinities,
except we'll want to throw errors where those produce NaNs.

regards, tom lane

#20Andres Freund
andres@anarazel.de
In reply to: Andres Freund (#10)
1 attachment(s)
Re: 'infinity'::Interval should be added

Hi,

On 2018-12-15 11:34:10 -0800, Andres Freund wrote:

[1] We should optimize the computation using pg_add_s{32,64}_overflow,
the generated code after that change is *substantially* better (as
well as the original code much shorter, and correct). And consider,
as mentioned further up, moving throwing the error out of line.

I don't have the focus to pursue this right now, so here's the changes
I'd made (for either somebody else to pick up, or as an archive for
myself).

Greetings,

Andres Freund

Attachments:

interval-overflow-checks.difftext/x-diff; charset=us-asciiDownload
diff --git a/src/backend/utils/adt/timestamp.c b/src/backend/utils/adt/timestamp.c
index b377c38c8af..db4481bb5d1 100644
--- a/src/backend/utils/adt/timestamp.c
+++ b/src/backend/utils/adt/timestamp.c
@@ -23,6 +23,7 @@
 #include "access/hash.h"
 #include "access/xact.h"
 #include "catalog/pg_type.h"
+#include "common/int.h"
 #include "common/int128.h"
 #include "funcapi.h"
 #include "libpq/pqformat.h"
@@ -3028,19 +3029,9 @@ interval_um(PG_FUNCTION_ARGS)
 
 	result = (Interval *) palloc(sizeof(Interval));
 
-	result->time = -interval->time;
-	/* overflow check copied from int4um */
-	if (interval->time != 0 && SAMESIGN(result->time, interval->time))
-		ereport(ERROR,
-				(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
-				 errmsg("interval out of range")));
-	result->day = -interval->day;
-	if (interval->day != 0 && SAMESIGN(result->day, interval->day))
-		ereport(ERROR,
-				(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
-				 errmsg("interval out of range")));
-	result->month = -interval->month;
-	if (interval->month != 0 && SAMESIGN(result->month, interval->month))
+	if (pg_sub_s32_overflow(0, interval->month, &result->month) ||
+		pg_sub_s32_overflow(0, interval->day, &result->day) ||
+		pg_sub_s64_overflow(0, interval->time, &result->time))
 		ereport(ERROR,
 				(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
 				 errmsg("interval out of range")));
@@ -3088,23 +3079,13 @@ interval_pl(PG_FUNCTION_ARGS)
 	result = (Interval *) palloc(sizeof(Interval));
 
 	result->month = span1->month + span2->month;
-	/* overflow check copied from int4pl */
-	if (SAMESIGN(span1->month, span2->month) &&
-		!SAMESIGN(result->month, span1->month))
-		ereport(ERROR,
-				(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
-				 errmsg("interval out of range")));
 
-	result->day = span1->day + span2->day;
-	if (SAMESIGN(span1->day, span2->day) &&
-		!SAMESIGN(result->day, span1->day))
-		ereport(ERROR,
-				(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
-				 errmsg("interval out of range")));
+	StaticAssertExpr(sizeof(result->time) == sizeof(int64),
+					 "adjust me");
 
-	result->time = span1->time + span2->time;
-	if (SAMESIGN(span1->time, span2->time) &&
-		!SAMESIGN(result->time, span1->time))
+	if (pg_add_s32_overflow(span1->month, span2->month, &result->month) ||
+		pg_add_s32_overflow(span1->day, span2->day, &result->day) ||
+		pg_add_s64_overflow(span1->time, span2->time, &result->time))
 		ereport(ERROR,
 				(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
 				 errmsg("interval out of range")));
@@ -3121,24 +3102,10 @@ interval_mi(PG_FUNCTION_ARGS)
 
 	result = (Interval *) palloc(sizeof(Interval));
 
-	result->month = span1->month - span2->month;
-	/* overflow check copied from int4mi */
-	if (!SAMESIGN(span1->month, span2->month) &&
-		!SAMESIGN(result->month, span1->month))
-		ereport(ERROR,
-				(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
-				 errmsg("interval out of range")));
 
-	result->day = span1->day - span2->day;
-	if (!SAMESIGN(span1->day, span2->day) &&
-		!SAMESIGN(result->day, span1->day))
-		ereport(ERROR,
-				(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
-				 errmsg("interval out of range")));
-
-	result->time = span1->time - span2->time;
-	if (!SAMESIGN(span1->time, span2->time) &&
-		!SAMESIGN(result->time, span1->time))
+	if (pg_sub_s32_overflow(span1->month, span2->month, &result->month) ||
+		pg_sub_s32_overflow(span1->day, span2->day, &result->day) ||
+		pg_sub_s64_overflow(span1->time, span2->time, &result->time))
 		ereport(ERROR,
 				(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
 				 errmsg("interval out of range")));
#21Isaac Morland
isaac.morland@gmail.com
In reply to: Tom Lane (#19)
Re: 'infinity'::Interval should be added

On Mon, 17 Dec 2018 at 18:00, Tom Lane <tgl@sss.pgh.pa.us> wrote:

so I was thinking that
postgres=# select 'infinity'::timestamp - 'infinity'::timestamp;
would be zero rather than an error, for least surprise.

Wrong. This case needs to be undefined, because "infinity"
isn't a specific value. That's what makes it okay to define, say,
infinity plus any finite value as infinity. There are very
well-defined rules about how to calculate with infinity, and
not following them is not the way to proceed here.

tl;dr: we should model it after the behavior of IEEE float infinities,
except we'll want to throw errors where those produce NaNs.

Would it be OK to return NULL for ∞ - ∞? Then anybody who wanted 0 could
get it with coalesce (although I think this is a worse idea than anybody
who wants it probably realizes), and anybody who wanted the calculation to
continue on would just get a NULL propagating.

Also am I right to assume that -infinity would use -INT_MAX, etc.? Or
possibly -INT_MAX - 1?

#22Tom Lane
tgl@sss.pgh.pa.us
In reply to: Isaac Morland (#21)
Re: 'infinity'::Interval should be added

Isaac Morland <isaac.morland@gmail.com> writes:

On Mon, 17 Dec 2018 at 18:00, Tom Lane <tgl@sss.pgh.pa.us> wrote:

tl;dr: we should model it after the behavior of IEEE float infinities,
except we'll want to throw errors where those produce NaNs.

Would it be OK to return NULL for ∞ - ∞?

IMO, no. The only thing worse than inventing NaN for intervals would be
trying to use NULL as a substitute for it. That'd amount to taking all
the semantic problems IEEE-arithmetic NaNs already have, and mixing
them irretrievably with SQL NULL's semantic problems (which are related
but different).

Also am I right to assume that -infinity would use -INT_MAX, etc.? Or
possibly -INT_MAX - 1?

The latter, which is why I mentioned INT_MIN. There must not be any
finite value that would be less than -infinity, so if you don't use
INT_MIN for -infinity then you're just throwing away one useful
bitpattern.

(Conceivably, if we decided we did want NaN for intervals, we'd define
that as being INT_MIN and -infinity as being -INT_MAX. But I really think
we don't want to go there, especially since we have not got NaN for
timestamps. All these related datatypes need to be making similar
decisions, or we're just creating messy new edge cases.)

regards, tom lane