Re: 'infinity'::Interval should be added - Mailing list pgsql-hackers

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