Обсуждение: '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

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

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

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

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

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

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),
i323101, 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]. 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.

Greetings,

Andres Freund

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

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

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

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

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

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

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

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

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