Thread: Infinite Interval

Hi all,

There have been multiple threads in the past discussing infinite
intervals:
https://www.postgresql.org/message-id/flat/4EB095C8.1050703%40agliodbs.com
https://www.postgresql.org/message-id/flat/200101241913.f0OJDUu45423%40hub.org
https://www.postgresql.org/message-id/flat/CANP8%2BjKTxQh4Mj%2BU3mWO3JHYb11SeQX9FW8SENrGbTdVxu6NNA%40mail.gmail.com

As well as an entry in the TODO list:
https://wiki.postgresql.org/wiki/Todo#Dates_and_Times

However, it doesn't seem like this was ever implemented. Is there still
any interest in this feature? If so, I'd like to try and implement it.

The proposed design from the most recent thread was to reserve
INT32_MAX months for infinity and INT32_MIN months for negative
infinity. As pointed out in the thread, these are currently valid
non-infinite intervals, but they are out of the documented range.

Thanks,
Joe Koshakow

Hi Joseph,
I stumbled upon this requirement a few times. So I started working on
this support in my spare time as a hobby project to understand
horology code in PostgreSQL. This was sitting in my repositories for
more than an year. Now that I have someone else showing an interest,
it's time for it to face the world. Rebased it, fixed conflicts.

PFA patch implementing infinite interval. It's still WIP, there are
TODOs in the code and also the commit message lists things that are
known to be incomplete. You might want to assess expected output
carefully

On Sun, Dec 11, 2022 at 12:51 AM Joseph Koshakow <koshy44@gmail.com> wrote:>
> The proposed design from the most recent thread was to reserve
> INT32_MAX months for infinity and INT32_MIN months for negative
> infinity. As pointed out in the thread, these are currently valid
> non-infinite intervals, but they are out of the documented range.

The patch uses both months and days together to avoid this problem.

Please feel free to complete the patch, work on review comments etc. I
will help as and when I find time.

-- 
Best Wishes,
Ashutosh Bapat

On Mon, Dec 12, 2022 at 8:05 AM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:
>
> Hi Joseph,
> I stumbled upon this requirement a few times. So I started working on
> this support in my spare time as a hobby project to understand
> horology code in PostgreSQL. This was sitting in my repositories for
> more than an year. Now that I have someone else showing an interest,
> it's time for it to face the world. Rebased it, fixed conflicts.
>
> PFA patch implementing infinite interval. It's still WIP, there are
> TODOs in the code and also the commit message lists things that are
> known to be incomplete. You might want to assess expected output
> carefully

That's great! I was also planning to just work on it as a hobby
project, so I'll try and review and add updates as I find free
time as well.

> > The proposed design from the most recent thread was to reserve
> > INT32_MAX months for infinity and INT32_MIN months for negative
> > infinity. As pointed out in the thread, these are currently valid
> > non-infinite intervals, but they are out of the documented range.
>
> The patch uses both months and days together to avoid this problem.

Can you expand on this part? I believe the full range of representable
intervals are considered valid as of v15.

- Joe Koshakow

Hi Ashutosh,

I've added tests for all the operators and functions involving
intervals and what I think the expected behaviors to be. The
formatting might be slightly off and I've left the contents of the
error messages as TODOs. Hopefully it's a good reference for the
implementation.

> Adding infinite interval to an infinite timestamp with opposite
> direction is not going to yield 0 but some infinity. Since we are adding
> interval to the timestamp the resultant timestamp is an infinity
> preserving the direction.

I think I disagree with this. Tom Lane in one of the previous threads
said:
> tl;dr: we should model it after the behavior of IEEE float infinities,
> except we'll want to throw errors where those produce NaNs.
and I agree with this opinion. I believe that means that adding an
infinite interval to an infinite timestamp with opposite directions
should yield an error instead of some infinity. Since with floats this
would yield a NaN.

> Dividing infinite interval by finite number keeps it infinite.
> TODO: Do we change the sign of infinity if factor is negative?
Again if we model this after the IEEE float behavior, then the answer
is yes, we do change the sign of infinity.

- Joe Koshakow

On Sat, Dec 17, 2022 at 2:34 PM Joseph Koshakow <koshy44@gmail.com> wrote:
>
> Hi Ashutosh,
>
> I've added tests for all the operators and functions involving
> intervals and what I think the expected behaviors to be. The
> formatting might be slightly off and I've left the contents of the
> error messages as TODOs. Hopefully it's a good reference for the
> implementation.
>
> > Adding infinite interval to an infinite timestamp with opposite
> > direction is not going to yield 0 but some infinity. Since we are adding
> > interval to the timestamp the resultant timestamp is an infinity
> > preserving the direction.
>
> I think I disagree with this. Tom Lane in one of the previous threads
> said:
> > tl;dr: we should model it after the behavior of IEEE float infinities,
> > except we'll want to throw errors where those produce NaNs.
> and I agree with this opinion. I believe that means that adding an
> infinite interval to an infinite timestamp with opposite directions
> should yield an error instead of some infinity. Since with floats this
> would yield a NaN.
>
> > Dividing infinite interval by finite number keeps it infinite.
> > TODO: Do we change the sign of infinity if factor is negative?
> Again if we model this after the IEEE float behavior, then the answer
> is yes, we do change the sign of infinity.
>
> - Joe Koshakow
I ended up doing some more work in the attached patch. Here are some
updates:

- I modified the arithmetic operators to more closely match IEEE
floats. Error messages are still all TODO, and they may have the wrong
error code.
- I implemented some more operators and functions.
- I moved the helper functions you created into macros in timestamp.h
to more closely match the implementation of infinite timestamps and
dates. Also so dates.c could access them.
- There seems to be an existing overflow error with interval
subtraction. Many of the arithmetic operators of the form
`X - Interval` are converted to `X + (-Interval)`. This will overflow
in the case that some interval field is INT32_MIN or INT64_MIN.
Additionally, negating a positive infinity interval won't result in a
negative infinity interval and vice versa. We'll have to come up with
an efficient solution for this.

- Joe Koshakow

Hi Ashutosh,

I ended up doing some more work on this today. All of the major
features should be implemented now. Below are what I think are the
outstanding TODOs:
- Clean up error messages and error codes
- Figure out how to correctly implement interval_part for infinite
intervals. For now I pretty much copied the implementation of
timestamp_part, but I'm not convinced that's correct.
- Fix horology tests.
- Test consolidation. After looking through the interval tests, I
realized that I may have duplicated some test cases. It would probably
be best to remove those duplicate tests.
- General cleanup, remove TODOs.

Attached is my most recent patch.

- Joe Koshakow

I have another update, I cleaned up some of the error messages, fixed
the horology tests, and ran pgindent.

- Joe

On 12/31/22 06:09, jian he wrote:
> 
> Since in float8 you can use '+inf', '+infinity', So should we also make
> interval '+infinity' valid?

Yes.

> Also in timestamp. '+infinity'::timestamp is invalid, should we make it
> valid.

Yes, we should.  I wrote a trivial patch for this a while ago but it 
appears I never posted it.  I will post that in a new thread so as not 
to confuse the bots.
-- 
Vik Fearing

On Sat, Dec 31, 2022 at 12:09 AM jian he <jian.universality@gmail.com> wrote:
> In float8, select float8 'inf' / float8 'inf' return NaN. Now in your patch  select interval 'infinity' / float8
'infinity';returns infinity.
 
> I am not sure it's right. I found this related post
(https://math.stackexchange.com/questions/181304/what-is-infinity-divided-by-infinity).

Good point, I agree this should return an error. We also need to
properly handle multiplication and division of infinite intervals by
float8 'nan'. My patch is returning an infinite interval, but it should
be returning an error. I'll upload a new patch shortly.

- Joe

On Mon, Jan 2, 2023 at 1:21 PM Joseph Koshakow <koshy44@gmail.com> wrote:
>
> On Sat, Dec 31, 2022 at 12:09 AM jian he <jian.universality@gmail.com> wrote:
> > In float8, select float8 'inf' / float8 'inf' return NaN. Now in your patch  select interval 'infinity' / float8
'infinity';returns infinity.
 
> > I am not sure it's right. I found this related post
(https://math.stackexchange.com/questions/181304/what-is-infinity-divided-by-infinity).
>
> Good point, I agree this should return an error. We also need to
> properly handle multiplication and division of infinite intervals by
> float8 'nan'. My patch is returning an infinite interval, but it should
> be returning an error. I'll upload a new patch shortly.
>
> - Joe

Attached is the patch to handle these scenarios. Apparently dividing by
NaN is currently broken:
    postgres=# SELECT INTERVAL '1 day' / float8 'nan';
                         ?column?
    ---------------------------------------------------
     -178956970 years -8 mons -2562047788:00:54.775808
    (1 row)

This patch will fix the issue, but we may want a separate patch that
handles this specific, existing issue. Any thoughts?

- Joe

I have another patch, this one adds validations to operations that
return intervals and updated error messages. I tried to give all of the
error messages meaningful text, but I'm starting to think that almost all
of them should just say "interval out of range". The current approach
may reveal some implementation details and lead to confusion. For
example, some subtractions are converted to additions which would lead
to an error message about addition.

    SELECT date 'infinity' - interval 'infinity';
    ERROR:  cannot add infinite values with opposite signs

I've also updated the commit message to include the remaining TODOs,
which I've copied below

  1. Various TODOs in code.
  2. Correctly implement interval_part for infinite intervals.
  3. Test consolidation.
  4. Should we just use the months field to test for infinity?

On Thu, Jan 5, 2023 at 5:20 AM jian he <jian.universality@gmail.com> wrote:
>
>
>
> On Wed, Jan 4, 2023 at 10:13 PM jian he <jian.universality@gmail.com> wrote:
>>
>>
>>
>> I don't know how to generate an interval.out file.

Personally I just write the .out files manually. I think it especially
helps as a way to double-check that the results are what you expected.
After running make check a regressions.diff file will be generated with
all the differences between your .out file and the results of the test.


> logic combine and clean up for functions in backend/utils/adt/timestamp.c
(timestamp_pl_interval,timestamptz_pl_interval,interval_pl, interval_mi).
 

One thing I was hoping to achieve was to avoid redundant checks if
possible. For example, in the following code:
> +    if ((INTERVAL_IS_NOBEGIN(span1) && INTERVAL_IS_NOEND(span2))
> +      ||(INTERVAL_IS_NOBEGIN(span1) && !INTERVAL_NOT_FINITE(span2))
> +      ||(!INTERVAL_NOT_FINITE(span1) && INTERVAL_IS_NOEND(span2)))
> +           INTERVAL_NOBEGIN(result);
If `(INTERVAL_IS_NOBEGIN(span1) && INTERVAL_IS_NOEND(span2))` is false,
then we end up checking `INTERVAL_IS_NOBEGIN(span1)` twice

> For 1. I don't know how to format the code. I have a problem installing pg_indent. If the format is wrong, please
reformat.

I'll run pg_indent and send an updated patch if anything changes.

Thanks for your help on this patch!

- Joe Koshakow

Jian,

I incorporated your changes and updated interval.out and ran
pgindent. Looks like some of the error messages have changed and we
have some issues with parsing "+infinity" after rebasing.

- Joe

On Thu, Jan 5, 2023 at 11:30 PM jian he <jian.universality@gmail.com> wrote:
>
>
>
> On Fri, Jan 6, 2023 at 6:54 AM Joseph Koshakow <koshy44@gmail.com> wrote:
>>
>> Looks like some of the error messages have changed and we
>> have some issues with parsing "+infinity" after rebasing.
>
>
> There is a commit 2ceea5adb02603ef52579b568ca2c5aebed87358
> https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=2ceea5adb02603ef52579b568ca2c5aebed87358
> if you pull this commit then you can do select interval '+infinity', even though I don't know why.

It turns out that I was just misreading the error. The test was
expecting us to fail on "+infinity" but we succeeded. I just removed
that test case.

>> pgindent. Looks like some of the error messages have changed

The conditions for checking valid addition/subtraction between infinite
values were missing some cases which explains the change in error
messages. I've updated the logic and removed duplicate checks.

I removed the extract/date_part tests since they were duplicated in a
test above. I also converted the DO command tests to using SQL with
joins so it more closely matches the existing tests.

I've updated the extract/date_part logic for infinite intervals. Fields
that are monotonically increasing should return +/-infinity and all
others should return NULL. For Intervals, the fields are the same as
timestamps plus the hour and day fields since those don't overflow into
the next highest field.

I think this patch is just about ready for review, except for the
following two questions:
  1. Should finite checks on intervals only look at months or all three
  fields?
  2. Should we make the error messages for adding/subtracting infinite
  values more generic or leave them as is?

My opinions are
  1. We should only look at months.
  2. We should make the errors more generic.

Anyone else have any thoughts?

- Joe

On Sat, Jan 7, 2023 at 3:04 PM Joseph Koshakow <koshy44@gmail.com> wrote:
>
> On Thu, Jan 5, 2023 at 11:30 PM jian he <jian.universality@gmail.com> wrote:
> >
> >
> >
> > On Fri, Jan 6, 2023 at 6:54 AM Joseph Koshakow <koshy44@gmail.com> wrote:
> >>
> >> Looks like some of the error messages have changed and we
> >> have some issues with parsing "+infinity" after rebasing.
> >
> >
> > There is a commit 2ceea5adb02603ef52579b568ca2c5aebed87358
> > https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=2ceea5adb02603ef52579b568ca2c5aebed87358
> > if you pull this commit then you can do select interval '+infinity', even though I don't know why.
>
> It turns out that I was just misreading the error. The test was
> expecting us to fail on "+infinity" but we succeeded. I just removed
> that test case.
>
> >> pgindent. Looks like some of the error messages have changed
>
> The conditions for checking valid addition/subtraction between infinite
> values were missing some cases which explains the change in error
> messages. I've updated the logic and removed duplicate checks.
>
> I removed the extract/date_part tests since they were duplicated in a
> test above. I also converted the DO command tests to using SQL with
> joins so it more closely matches the existing tests.
>
> I've updated the extract/date_part logic for infinite intervals. Fields
> that are monotonically increasing should return +/-infinity and all
> others should return NULL. For Intervals, the fields are the same as
> timestamps plus the hour and day fields since those don't overflow into
> the next highest field.
>
> I think this patch is just about ready for review, except for the
> following two questions:
>   1. Should finite checks on intervals only look at months or all three
>   fields?
>   2. Should we make the error messages for adding/subtracting infinite
>   values more generic or leave them as is?
>
> My opinions are
>   1. We should only look at months.
>   2. We should make the errors more generic.
>
> Anyone else have any thoughts?
>
> - Joe

Oops I forgot the actual patch. Please see attached.

On Sat, Jan 7, 2023 at 3:05 PM Joseph Koshakow <koshy44@gmail.com> wrote:
>
> On Sat, Jan 7, 2023 at 3:04 PM Joseph Koshakow <koshy44@gmail.com> wrote:
> >
> > I think this patch is just about ready for review, except for the
> > following two questions:
> >   1. Should finite checks on intervals only look at months or all three
> >   fields?
> >   2. Should we make the error messages for adding/subtracting infinite
> >   values more generic or leave them as is?
> >
> > My opinions are
> >   1. We should only look at months.
> >   2. We should make the errors more generic.
> >
> > Anyone else have any thoughts?

Here's a patch with the more generic error messages.

- Joe

On Sun, Jan 8, 2023 at 11:17 PM jian he <jian.universality@gmail.com> wrote:
>
>
>
> On Sun, Jan 8, 2023 at 4:22 AM Joseph Koshakow <koshy44@gmail.com> wrote:
>>
>> On Sat, Jan 7, 2023 at 3:05 PM Joseph Koshakow <koshy44@gmail.com> wrote:
>> >
>> > On Sat, Jan 7, 2023 at 3:04 PM Joseph Koshakow <koshy44@gmail.com> wrote:
>> > >
>> > > I think this patch is just about ready for review, except for the
>> > > following two questions:
>> > >   1. Should finite checks on intervals only look at months or all three
>> > >   fields?
>> > >   2. Should we make the error messages for adding/subtracting infinite
>> > >   values more generic or leave them as is?
>> > >
>> > > My opinions are
>> > >   1. We should only look at months.
>> > >   2. We should make the errors more generic.
>> > >
>> > > Anyone else have any thoughts?
>>
>> Here's a patch with the more generic error messages.
>>
>> - Joe
>
>
> HI.
>
> I just found out another problem.
>
> select * from  generate_series(timestamp'-infinity', timestamp 'infinity', interval 'infinity');
> ERROR:  timestamp out of range
>
> select * from  generate_series(timestamp'-infinity',timestamp 'infinity', interval '-infinity'); --return following
>
>  generate_series
> -----------------
> (0 rows)
>
>
> select * from generate_series(timestamp 'infinity',timestamp 'infinity', interval 'infinity');
> --will run all the time.
>
> select * from  generate_series(timestamp 'infinity',timestamp 'infinity', interval '-infinity');
> ERROR:  timestamp out of range
>
>  select * from  generate_series(timestamp'-infinity',timestamp'-infinity', interval 'infinity');
> ERROR:  timestamp out of range
>
> select * from  generate_series(timestamp'-infinity',timestamp'-infinity', interval '-infinity');
> --will run all the time.

Good catch, I didn't think to check non date/time functions.
Unfortunately, I think you may have opened Pandoras box. I went through
pg_proc.dat and found the following functions that all involve
intervals. We should probably investigate all of them and make sure
that they handle infinite intervals properly.

{ oid => '1026', descr => 'adjust timestamp to new time zone',
proname => 'timezone', prorettype => 'timestamp',
proargtypes => 'interval timestamptz', prosrc => 'timestamptz_izone' },

{ oid => '4133', descr => 'window RANGE support',
proname => 'in_range', prorettype => 'bool',
proargtypes => 'date date interval bool bool',
prosrc => 'in_range_date_interval' },

{ oid => '1305', descr => 'intervals overlap?',
proname => 'overlaps', prolang => 'sql', proisstrict => 'f',
provolatile => 's', prorettype => 'bool',
proargtypes => 'timestamptz interval timestamptz interval',
prosrc => 'see system_functions.sql' },

{ oid => '1305', descr => 'intervals overlap?',
proname => 'overlaps', prolang => 'sql', proisstrict => 'f',
provolatile => 's', prorettype => 'bool',
proargtypes => 'timestamptz interval timestamptz interval',
prosrc => 'see system_functions.sql' },
{ oid => '1306', descr => 'intervals overlap?',
proname => 'overlaps', prolang => 'sql', proisstrict => 'f',
provolatile => 's', prorettype => 'bool',
proargtypes => 'timestamptz timestamptz timestamptz interval',
prosrc => 'see system_functions.sql' },
{ oid => '1307', descr => 'intervals overlap?',
proname => 'overlaps', prolang => 'sql', proisstrict => 'f',
provolatile => 's', prorettype => 'bool',
proargtypes => 'timestamptz interval timestamptz timestamptz',
prosrc => 'see system_functions.sql' },

{ oid => '1308', descr => 'intervals overlap?',
proname => 'overlaps', proisstrict => 'f', prorettype => 'bool',
proargtypes => 'time time time time', prosrc => 'overlaps_time' },
{ oid => '1309', descr => 'intervals overlap?',
proname => 'overlaps', prolang => 'sql', proisstrict => 'f',
prorettype => 'bool', proargtypes => 'time interval time interval',
prosrc => 'see system_functions.sql' },
{ oid => '1310', descr => 'intervals overlap?',
proname => 'overlaps', prolang => 'sql', proisstrict => 'f',
prorettype => 'bool', proargtypes => 'time time time interval',
prosrc => 'see system_functions.sql' },
{ oid => '1311', descr => 'intervals overlap?',
proname => 'overlaps', prolang => 'sql', proisstrict => 'f',
prorettype => 'bool', proargtypes => 'time interval time time',
prosrc => 'see system_functions.sql' },

{ oid => '1386',
descr => 'date difference from today preserving months and years',
proname => 'age', prolang => 'sql', provolatile => 's',
prorettype => 'interval', proargtypes => 'timestamptz',
prosrc => 'see system_functions.sql' },

{ oid => '2042', descr => 'intervals overlap?',
proname => 'overlaps', prolang => 'sql', proisstrict => 'f',
prorettype => 'bool', proargtypes => 'timestamp interval timestamp interval',
prosrc => 'see system_functions.sql' },
{ oid => '2043', descr => 'intervals overlap?',
proname => 'overlaps', prolang => 'sql', proisstrict => 'f',
prorettype => 'bool', proargtypes => 'timestamp timestamp timestamp interval',
prosrc => 'see system_functions.sql' },
{ oid => '2044', descr => 'intervals overlap?',
proname => 'overlaps', prolang => 'sql', proisstrict => 'f',
prorettype => 'bool', proargtypes => 'timestamp interval timestamp timestamp',
prosrc => 'see system_functions.sql' },

{ oid => '4134', descr => 'window RANGE support',
proname => 'in_range', prorettype => 'bool',
proargtypes => 'timestamp timestamp interval bool bool',
prosrc => 'in_range_timestamp_interval' },
{ oid => '4135', descr => 'window RANGE support',
proname => 'in_range', provolatile => 's', prorettype => 'bool',
proargtypes => 'timestamptz timestamptz interval bool bool',
prosrc => 'in_range_timestamptz_interval' },
{ oid => '4136', descr => 'window RANGE support',
proname => 'in_range', prorettype => 'bool',
proargtypes => 'interval interval interval bool bool',
prosrc => 'in_range_interval_interval' },
{ oid => '4137', descr => 'window RANGE support',
proname => 'in_range', prorettype => 'bool',
proargtypes => 'time time interval bool bool',
prosrc => 'in_range_time_interval' },
{ oid => '4138', descr => 'window RANGE support',
proname => 'in_range', prorettype => 'bool',
proargtypes => 'timetz timetz interval bool bool',
prosrc => 'in_range_timetz_interval' },

{ oid => '2058', descr => 'date difference preserving months and years',
proname => 'age', prorettype => 'interval',
proargtypes => 'timestamp timestamp', prosrc => 'timestamp_age' },
{ oid => '2059',
descr => 'date difference from today preserving months and years',
proname => 'age', prolang => 'sql', provolatile => 's',
prorettype => 'interval', proargtypes => 'timestamp',
prosrc => 'see system_functions.sql' },

{ oid => '2070', descr => 'adjust timestamp to new time zone',
proname => 'timezone', prorettype => 'timestamptz',
proargtypes => 'interval timestamp', prosrc => 'timestamp_izone' },

{ oid => '3935', descr => 'sleep for the specified interval',
proname => 'pg_sleep_for', prolang => 'sql', provolatile => 'v',
prorettype => 'void', proargtypes => 'interval',
prosrc => 'see system_functions.sql' },

{ oid => '2599', descr => 'get the available time zone abbreviations',
proname => 'pg_timezone_abbrevs', prorows => '1000', proretset => 't',
provolatile => 's', prorettype => 'record', proargtypes => '',
proallargtypes => '{text,interval,bool}', proargmodes => '{o,o,o}',
proargnames => '{abbrev,utc_offset,is_dst}',
prosrc => 'pg_timezone_abbrevs' },
{ oid => '2856', descr => 'get the available time zone names',
proname => 'pg_timezone_names', prorows => '1000', proretset => 't',
provolatile => 's', prorettype => 'record', proargtypes => '',
proallargtypes => '{text,text,interval,bool}', proargmodes => '{o,o,o,o}',
proargnames => '{name,abbrev,utc_offset,is_dst}',
prosrc => 'pg_timezone_names' },

{ oid => '939', descr => 'non-persistent series generator',
proname => 'generate_series', prorows => '1000', proretset => 't',
provolatile => 's', prorettype => 'timestamptz',
proargtypes => 'timestamptz timestamptz interval',
prosrc => 'generate_series_timestamptz' },

{ oid => '3976', descr => 'continuous distribution percentile',
proname => 'percentile_cont', prokind => 'a', proisstrict => 'f',
prorettype => 'interval', proargtypes => 'float8 interval',
prosrc => 'aggregate_dummy' },
{ oid => '3977', descr => 'aggregate final function',
proname => 'percentile_cont_interval_final', proisstrict => 'f',
prorettype => 'interval', proargtypes => 'internal float8',
prosrc => 'percentile_cont_interval_final' },

{ oid => '3982', descr => 'multiple continuous percentiles',
proname => 'percentile_cont', prokind => 'a', proisstrict => 'f',
prorettype => '_interval', proargtypes => '_float8 interval',
prosrc => 'aggregate_dummy' },
{ oid => '3983', descr => 'aggregate final function',
proname => 'percentile_cont_interval_multi_final', proisstrict => 'f',
prorettype => '_interval', proargtypes => 'internal _float8',
prosrc => 'percentile_cont_interval_multi_final' },

- Joe

Ok, I've updated the patch to handle every function that inputs or
outputs intervals, as well as added some tests. In the process I
noticed that some of the existing date/timestamp/timestamptz don't
handle infinite values properly. For example,
postgres=# SELECT age('infinity'::timestamp);
age
--------------------------------------------------
-292253 years -11 mons -26 days -04:00:54.775807
(1 row)

It might be worth going through all those functions separately
and making sure they are correct.

I also added some overflow handling to make_interval.

I also added handling of infinite timestamp subtraction.

At this point the patch is ready for review again except for the one
outstanding question of: Should finite checks on intervals only look at
months or all three fields?

- Joe

On Sat, Jan 14, 2023 at 4:22 PM Joseph Koshakow <koshy44@gmail.com> wrote:
>
> At this point the patch is ready for review again except for the one
> outstanding question of: Should finite checks on intervals only look at
> months or all three fields?
>
> - Joe

I've gone ahead and updated the patch to only look at the months field.
I'll submit this email and patch to the Feb commitfest.

- Joe

On Sun, 15 Jan 2023 at 11:44, Joseph Koshakow <koshy44@gmail.com> wrote:
>
> On Sat, Jan 14, 2023 at 4:22 PM Joseph Koshakow <koshy44@gmail.com> wrote:

> I've gone ahead and updated the patch to only look at the months field.
> I'll submit this email and patch to the Feb commitfest.


It looks like this patch needs a (perhaps trivial) rebase.

It sounds like all the design questions are resolved so perhaps this
can be set to Ready for Committer once it's rebased?

-- 
Gregory Stark
As Commitfest Manager

Hi Joseph,

Thanks for working on the patch. Sorry for taking so long to review
this patch. But here's it finally, review of code changes

 static pg_tz *FetchDynamicTimeZone(TimeZoneAbbrevTable *tbl, const datetkn *tp,
-                                   DateTimeErrorExtra *extra);
+                                   DateTimeErrorExtra * extra);

There are a lot of these diffs. PG code doesn't leave an extra space between
variable name and *.


     /* Handle the integer part */
-    if (!int64_multiply_add(val, scale, &itm_in->tm_usec))
+    if (pg_mul_add_s64_overflow(val, scale, &itm_in->tm_usec))

I think this is a good change, since we are moving the function to int.h where
it belongs. We could separate these kind of changes into another patch for easy
review.

+
+    result->day = days;
+    if (pg_mul_add_s32_overflow(weeks, 7, &result->day))
+        ereport(ERROR,
+                (errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+                 errmsg("interval out of range")));

I think such changes are also good, but probably a separate patch for ease of
review.

     secs = rint(secs * USECS_PER_SEC);
-    result->time = hours mj* ((int64) SECS_PER_HOUR * USECS_PER_SEC) +
-        mins * ((int64) SECS_PER_MINUTE * USECS_PER_SEC) +
-        (int64) secs;
+
+    result->time = secs;
+    if (pg_mul_add_s64_overflow(mins, ((int64) SECS_PER_MINUTE *
USECS_PER_SEC), &result->time))
+        ereport(ERROR,
+                (errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+                 errmsg("interval out of range")));
+    if (pg_mul_add_s64_overflow(hours, ((int64) SECS_PER_HOUR *
USECS_PER_SEC), &result->time))
+        ereport(ERROR,
+                (errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+                 errmsg("interval out of range")));

    shouldn't this be
    secs = rint(secs);
    result->time = 0;
    pg_mul_add_s64_overflow(secs, USECS_PER_SEC, &result->time) to catch
    overflow error early?

+        if TIMESTAMP_IS_NOBEGIN
+            (dt2)

Better be written as if (TIMESTAMP_IS_NOBEGIN(dt2))? There are more corrections
like this.

+    if (INTERVAL_NOT_FINITE(result))
+        ereport(ERROR,
+                (errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+                 errmsg("interval out of range")));

Probably, I added these kind of checks. But I don't remember if those are
defensive checks or whether it's really possible that the arithmetic above
these lines can yield an non-finite interval.


+    else
+    {
+        result->time = -interval->time;
+        result->day = -interval->day;
+        result->month = -interval->month;
+
+        if (INTERVAL_NOT_FINITE(result))
+            ereport(ERROR,
+                    (errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+                     errmsg("interval out of range")));

If this error ever gets to the user, it could be confusing. Can we elaborate by
adding context e.g. errcontext("while negating an interval") or some such?

-
-    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))
-        ereport(ERROR,
-                (errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
-                 errmsg("interval out of range")));
+    interval_um_internal(interval, result);

Shouldn't we incorporate these checks in interval_um_internal()? I don't think
INTERVAL_NOT_FINITE() covers all of those.

+    /*
+     * Subtracting two infinite intervals with different signs results in an
+     * infinite interval with the same sign as the left operand. Subtracting
+     * two infinte intervals with the same sign results in an error.
+     */

I think we need someone to validate these assumptions and similar assumptions
in interval_pl(). Googling gives confusing results in some cases. I have not
looked for IEEE standard around this specificallly.

+    if (INTERVAL_NOT_FINITE(interval))
+    {
+        double        r = NonFiniteIntervalPart(type, val, lowunits,
+                                              INTERVAL_IS_NOBEGIN(interval),
+                                              false);
+
+        if (r)

I see that this code is very similar to the corresponding code in timestamp and
timestamptz, so it's bound to be correct. But I always thought float equality
is unreliable. if (r) is equivalent to if (r == 0.0) so it will not work as
intended. But may be (float) 0.0 is a special value for which equality holds
true.

+static inline bool
+pg_mul_add_s64_overflow(int64 val, int64 multiplier, int64 *sum)

I think this needs a prologue similar to int64_multiply_add(), that the patch
removes. Similarly for pg_mul_add_s32_overflow().

On Thu, Mar 2, 2023 at 3:51 AM Joseph Koshakow <koshy44@gmail.com> wrote:
>
> On Wed, Mar 1, 2023 at 3:03 PM Gregory Stark (as CFM) <stark.cfm@gmail.com> wrote:
> >
> >    It looks like this patch needs a (perhaps trivial) rebase.
>
> Attached is a rebased patch.
>
> >    It sounds like all the design questions are resolved so perhaps this
> >    can be set to Ready for Committer once it's rebased?
>
> There hasn't really been a review of this patch yet. It's just been
> mostly me talking to myself in this thread, and a couple of
> contributions from jian.
>
> - Joe Koshakow



--
Best Wishes,
Ashutosh Bapat

Joseph Koshakow <koshy44@gmail.com> writes:
> On Thu, Mar 9, 2023 at 12:42 PM Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
> wrote:
>> There are a lot of these diffs. PG code doesn't leave an extra space
>> between variable name and *.

> Those appeared from running pg_indent. I've removed them all.

More specifically, those are from running pg_indent with an obsolete
typedefs list.  Good practice is to fetch an up-to-date list from
the buildfarm:

curl https://buildfarm.postgresql.org/cgi-bin/typedefs.pl -o .../typedefs.list

and use that.  (If your patch adds any typedefs, you can then add them
to that list.)  There's been talk of trying harder to keep
src/tools/pgindent/typedefs.list up to date, but not much has happened
yet.

> I've separated this out into another patch attached to this email.
> Should I start a new email thread or is it ok to include it in this
> one?

Having separate threads with interdependent patches is generally a
bad idea :-( ... the cfbot certainly won't cope.

>> I see that this code is very similar to the corresponding code in
>> timestamp and
>> timestamptz, so it's bound to be correct. But I always thought float
>> equality
>> is unreliable. if (r) is equivalent to if (r == 0.0) so it will not
>> work as
>> intended. But may be (float) 0.0 is a special value for which equality
>> holds
>> true.

> I'm not familiar with float equality being unreliable, but I'm by no
> means a C or float expert. Can you link me to some docs/explanation?

The specific issue with float zero is that plus zero and minus zero
are distinct concepts with distinct bit patterns, but the IEEE spec
says that they compare as equal.  The C standard says about "if":

       [#1] The controlling expression of  an  if  statement  shall
       have scalar type.
       [#2]  In  both  forms, the first substatement is executed if
       the expression compares unequal to 0.  In the else form, the
       second  substatement  is executed if the expression compares
       equal to 0.

so it sure looks to me like a float control expression is valid and
minus zero should be treated as "false".  Nonetheless, personally
I'd consider this to be poor style and would write "r != 0" or
"r != 0.0" rather than depending on that.

BTW, this may already need a rebase over 75bd846b6.

            regards, tom lane

Joseph Koshakow <koshy44@gmail.com> writes:
> On Sat, Mar 18, 2023 at 3:08 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> More specifically, those are from running pg_indent with an obsolete
>> typedefs list.

> I must be doing something wrong because even after doing that I get the
> same strange formatting. Specifically from the root directory I ran

Hmm, I dunno what's going on there.  When I do this:

>   curl https://buildfarm.postgresql.org/cgi-bin/typedefs.pl -o
> src/tools/pgindent/typedefs.list

I end up with a plausible set of updates, notably

$ git diff
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index 097f42e1b3..667f8e13ed 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
...
@@ -545,10 +548,12 @@ DataDumperPtr
 DataPageDeleteStack
 DatabaseInfo
 DateADT
+DateTimeErrorExtra
 Datum
 DatumTupleFields
 DbInfo
 DbInfoArr
+DbLocaleInfo
 DeClonePtrType
 DeadLockState
 DeallocateStmt

so it sure ought to know DateTimeErrorExtra is a typedef.
I then tried pgindent'ing datetime.c and timestamp.c,
and it did not want to change either file.  I do get
diffs like

 DecodeDateTime(char **field, int *ftype, int nf,
               int *dtype, struct pg_tm *tm, fsec_t *fsec, int *tzp,
-              DateTimeErrorExtra *extra)
+              DateTimeErrorExtra * extra)
 {
    int         fmask = 0,

if I try to pgindent datetime.c with typedefs.list as it
stands in HEAD.  That's pretty much pgindent's normal
behavior when it doesn't recognize a name as a typedef.

            regards, tom lane

Joseph Koshakow <koshy44@gmail.com> writes:
> I must have been doing something wrong because I tried again today and
> it worked fine. However, I go get a lot of changes like the following:

>   -               if TIMESTAMP_IS_NOBEGIN(dt2)
>   -                       ereport(ERROR,
>   -
> (errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
>   -                                        errmsg("timestamp out of
> range")));
>   +               if TIMESTAMP_IS_NOBEGIN
>   +                       (dt2)
>   +                               ereport(ERROR,
>   +
> (errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
>   +                                                errmsg("timestamp out of
> range")));

> Should I keep these pgindent changes or keep it the way I have it?

Did you actually write "if TIMESTAMP_IS_NOBEGIN(dt2)" and not
"if (TIMESTAMP_IS_NOBEGIN(dt2))"?  If the former, I'm not surprised
that pgindent gets confused.  The parentheses are required by the
C standard.  Your code might accidentally work because the macro
has parentheses internally, but call sites have no business
knowing that.  For example, it would be completely legit to change
TIMESTAMP_IS_NOBEGIN to be a plain function, and then this would be
syntactically incorrect.

            regards, tom lane

On Sun, Mar 19, 2023 at 1:04 AM Joseph Koshakow <koshy44@gmail.com> wrote:
>
> The patches in this email should be rebased over master.
>

Reviewed 0001 -
Looks good to me. The new function is properly placed along with other
signed 64 bit functions. All existing calls to int64_multiply_add()
have been replaced with the new function and negated the result.

Reviewed 0002
+   result->day = days;
+   if (pg_mul_add_s32_overflow(weeks, 7, &result->day))

You don't need to do this, but looks like we can add DAYS_PER_WEEK macro and
use it here.

The first two patches look good to me; ready for a committer. Can be
committed independent of the third patch.

Will look at the third patch soon.

--
Best Wishes,
Ashutosh Bapat

Joseph Koshakow <koshy44@gmail.com> writes:
> In terms of adding/subtracting infinities, the IEEE standard is pay
> walled and I don't have a copy. I tried finding information online but
> I also wasn't able to find anything useful. I additionally checked to see
> the results of C++, C, and Java, and they all match which increases my
> confidence that we're doing the right thing. Does anyone happen to have
> a copy of the standard and can confirm?

I think you can take it as read that simple C test programs on modern
platforms will exhibit IEEE-compliant handling of float infinities.

            regards, tom lane

On Sat, Mar 25, 2023 at 9:13 PM Joseph Koshakow <koshy44@gmail.com> wrote:
>
> On Fri, Mar 24, 2023 at 9:43 AM Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> wrote:
> >
> >    You don't need to do this, but looks like we can add DAYS_PER_WEEK macro and
> >    use it here.
>
> I've attached a patch with this new macro. There's probably tons of
> places it can be used instead of hardcoding the number 7, but I'll save
> that for a future patch.

Thanks. Yes, changing other existing usages is out of scope for this patch.

Looks good to me.

--
Best Wishes,
Ashutosh Bapat

On Sun, Mar 19, 2023 at 12:18 AM Joseph Koshakow <koshy44@gmail.com> wrote:
>
> The problem is that `secs = rint(secs)` rounds the seconds too early
> and loses any fractional seconds. Do we have an overflow detecting
> multiplication function for floats?

We have float8_mul() which checks for overflow. typedef double float8;

>
> >    +    if (INTERVAL_NOT_FINITE(result))
> >    +        ereport(ERROR,
> >    +                (errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
> >    +                 errmsg("interval out of range")));
> >
> >    Probably, I added these kind of checks. But I don't remember if those are
> >    defensive checks or whether it's really possible that the arithmetic above
> >    these lines can yield an non-finite interval.
>
> These checks appear in `make_interval`, `justify_X`,
> `interval_um_internal`, `interval_pl`, `interval_mi`, `interval_mul`,
> `interval_div`. For all of these it's possible that the interval
> overflows/underflows the non-finite ranges, but does not
> overflow/underflow the data type. For example
> `SELECT INTERVAL '2147483646 months' + INTERVAL '1 month'` would error
> on this check.

Without this patch
postgres@64807=#SELECT INTERVAL '2147483646 months' + INTERVAL '1 month';
        ?column?
------------------------
 178956970 years 7 mons
(1 row)

That result looks correct

postgres@64807=#select 178956970 * 12 + 7;
  ?column?
------------
 2147483647
(1 row)

So some backward compatibility break. I don't think we can avoid the
backward compatibility break without expanding interval structure and
thus causing on-disk breakage. But we can reduce the chances of
breaking, if we change INTERVAL_NOT_FINITE to check all the three
fields, instead of just month.

>
>
> >    +    else
> >    +    {
> >    +        result->time = -interval->time;
> >    +        result->day = -interval->day;
> >    +        result->month = -interval->month;
> >    +
> >    +        if (INTERVAL_NOT_FINITE(result))
> >    +            ereport(ERROR,
> >    +                    (errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
> >    +                     errmsg("interval out of range")));
> >
> >    If this error ever gets to the user, it could be confusing. Can we elaborate by
> >    adding context e.g. errcontext("while negating an interval") or some such?
>
> Done.

Thanks. Can we add relevant contexts at similar other places?

Also if we use all the three fields, we will need to add such checks
in interval_justify_hours()

>
> I replaced these checks with the following:
>
> + else if (interval->time == PG_INT64_MIN || interval->day == PG_INT32_MIN || interval->month == PG_INT32_MIN)
> + ereport(ERROR,
> + (errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
> + errmsg("interval out of range")));
>
> I think this covers the same overflow check but is maybe a bit more
> obvious. Unless, there's something I'm missing?

Thanks. Your current version is closer to int4um().

Some more review comments in the following email.

--
Best Wishes,
Ashutosh Bapat

On Sun, Mar 26, 2023 at 1:28 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> I think you can take it as read that simple C test programs on modern
> platforms will exhibit IEEE-compliant handling of float infinities.
>

For the record, I tried the attached. It gives a warning at compilation time.

$gcc float_inf.c
float_inf.c: In function ‘main’:
float_inf.c:10:17: warning: division by zero [-Wdiv-by-zero]
   10 |  float inf = 1.0/0;
      |                 ^
float_inf.c:11:20: warning: division by zero [-Wdiv-by-zero]
   11 |  float n_inf = -1.0/0;
      |                    ^
$ ./a.out
inf = inf
-inf = -inf
inf + inf = inf
inf + -inf = -nan
-inf + inf = -nan
-inf + -inf = -inf
inf - inf = -nan
inf - -inf = inf
-inf - inf = -inf
-inf - -inf = -nan
float 0.0 equals 0.0
float 1.0 equals 1.0
 5.0 * inf = inf
 5.0 * - inf = -inf
 5.0 / inf = 0.000000
 5.0 / - inf = -0.000000
 inf / 5.0 = inf
 - inf / 5.0 = -inf

The changes in the patch are compliant with the observations above.

--
Best Wishes,
Ashutosh Bapat

On Mon, Mar 20, 2023 at 3:16 AM Joseph Koshakow <koshy44@gmail.com> wrote:
>
>
>
> On Sun, Mar 19, 2023 at 5:13 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >
> >    Did you actually write "if TIMESTAMP_IS_NOBEGIN(dt2)" and not
> >    "if (TIMESTAMP_IS_NOBEGIN(dt2))"?  If the former, I'm not surprised
> >    that pgindent gets confused.  The parentheses are required by the
> >    C standard.  Your code might accidentally work because the macro
> >    has parentheses internally, but call sites have no business
> >    knowing that.  For example, it would be completely legit to change
> >    TIMESTAMP_IS_NOBEGIN to be a plain function, and then this would be
> >    syntactically incorrect.
>
> Oh duh. I've been doing too much Rust development and did this without
> thinking. I've attached a patch with a fix.
>

Thanks for fixing this.

On this latest patch, I have one code comment

@@ -3047,7 +3180,30 @@ timestamptz_pl_interval_internal(TimestampTz timestamp,
    TimestampTz result;
    int         tz;

-   if (TIMESTAMP_NOT_FINITE(timestamp))
+   /*
+    * Adding two infinites with the same sign results in an infinite
+    * timestamp with the same sign. Adding two infintes with different signs
+    * results in an error.
+    */
+   if (INTERVAL_IS_NOBEGIN(span))
+   {
+       if TIMESTAMP_IS_NOEND(timestamp)
+           ereport(ERROR,
+                   (errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+                    errmsg("interval out of range")));
+       else
+           TIMESTAMP_NOBEGIN(result);
+   }
+   else if (INTERVAL_IS_NOEND(span))
+   {
+       if TIMESTAMP_IS_NOBEGIN(timestamp)
+           ereport(ERROR,
+                   (errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+                    errmsg("interval out of range")));
+       else
+           TIMESTAMP_NOEND(result);
+   }
+   else if (TIMESTAMP_NOT_FINITE(timestamp))

This code is duplicated in timestamp_pl_interval(). We could create a function
to encode the infinity handling rules and then call it in these two places. The
argument types are different, Timestamp and TimestampTz viz. which map to in64,
so shouldn't be a problem. But it will be slightly unreadable. Or use macros
but then it will be difficult to debug.

What do you think?

Next I will review the test changes and also make sure that every
operator that interval as one of its operands or the result has been
covered in the code. This is the following list

#select oprname, oprcode from pg_operator where oprleft =
'interval'::regtype or oprright = 'interval'::regtype or oprresult =
'interval'::regtype;
 oprname |         oprcode
---------+-------------------------
 +       | date_pl_interval
 -       | date_mi_interval
 +       | timestamptz_pl_interval
 -       | timestamptz_mi
 -       | timestamptz_mi_interval
 =       | interval_eq
 <>      | interval_ne
 <       | interval_lt
 <=      | interval_le
 >       | interval_gt
 >=      | interval_ge
 -       | interval_um
 +       | interval_pl
 -       | interval_mi
 -       | time_mi_time
 *       | interval_mul
 *       | mul_d_interval
 /       | interval_div
 +       | time_pl_interval
 -       | time_mi_interval
 +       | timetz_pl_interval
 -       | timetz_mi_interval
 +       | interval_pl_time
 +       | timestamp_pl_interval
 -       | timestamp_mi
 -       | timestamp_mi_interval
 +       | interval_pl_date
 +       | interval_pl_timetz
 +       | interval_pl_timestamp
 +       | interval_pl_timestamptz
(30 rows)

--
Best Wishes,
Ashutosh Bapat

On Tue, Mar 28, 2023 at 7:17 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:
 > make sure that every
> operator that interval as one of its operands or the result has been
> covered in the code.

time_mi_time - do we want to add an Assert to make sure that this
function does not produce an Interval structure which looks like
non-finite interval?

multiplying an interval by infinity throws an error
#select '5 days'::interval * 'infinity'::float8;
2023-03-29 19:40:15.797 IST [136240] ERROR:  interval out of range
2023-03-29 19:40:15.797 IST [136240] STATEMENT:  select '5
days'::interval * 'infinity'::float8;
ERROR:  interval out of range

I think this should produce an infinite interval now. Attached patch
to fix this, to be applied on top of your patch. With the patch
#select '5 days'::interval * 'infinity'::float8;
 ?column?
----------
 infinity
(1 row)

Going through the tests now.

--
Best Wishes,
Ashutosh Bapat

I hurried too much on the previous patch. It introduced other
problems. Attached is a better patch and also fixes problem below
#select 'infinity'::interval * 0;
 ?column?
----------
 infinity
(1 row)

with the patch we see
#select 'infinity'::interval * 0;
2023-03-31 18:00:43.131 IST [240892] ERROR:  interval out of range
2023-03-31 18:00:43.131 IST [240892] STATEMENT:  select
'infinity'::interval * 0;
ERROR:  interval out of range

which looks more appropriate given 0 * inf = Nan for float.

There's some way to avoid separate checks for infinite-ness of
interval and factor and use a single block using some integer
arithmetic. But I think this is more readable. So I avoided doing
that. Let me know if this works for you.

Also added some test cases.

--
Best Wishes,
Ashutosh Bapat

On Fri, Mar 31, 2023 at 3:46 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:
>
> On Tue, Mar 28, 2023 at 7:17 PM Ashutosh Bapat
> <ashutosh.bapat.oss@gmail.com> wrote:
>  > make sure that every
> > operator that interval as one of its operands or the result has been
> > covered in the code.
>
> time_mi_time - do we want to add an Assert to make sure that this
> function does not produce an Interval structure which looks like
> non-finite interval?
>
> multiplying an interval by infinity throws an error
> #select '5 days'::interval * 'infinity'::float8;
> 2023-03-29 19:40:15.797 IST [136240] ERROR:  interval out of range
> 2023-03-29 19:40:15.797 IST [136240] STATEMENT:  select '5
> days'::interval * 'infinity'::float8;
> ERROR:  interval out of range
>
> I think this should produce an infinite interval now. Attached patch
> to fix this, to be applied on top of your patch. With the patch
> #select '5 days'::interval * 'infinity'::float8;
>  ?column?
> ----------
>  infinity
> (1 row)
>
> Going through the tests now.
>
> --
> Best Wishes,
> Ashutosh Bapat

Joseph Koshakow <koshy44@gmail.com> writes:
> I've attached a patch with these changes that is meant to be applied
> over the previous three patches. Let me know what you think.

Does not really seem like an improvement to me --- I think it's
adding more complexity than it removes.  The changes in CONTEXT
messages are definitely not an improvement; you might as well
not have the context messages at all as give misleading ones.
(Those context messages are added by the previous patches, no?
They do not really seem per project style, and I'm not sure
that they are helpful.)

            regards, tom lane

Joseph Koshakow <koshy44@gmail.com> writes:
>> I've added an errcontext to all the errors of the form "X out of
>> range".

Please note the style guidelines [1]:

    errcontext(const char *msg, ...) is not normally called directly from
    an ereport message site; rather it is used in error_context_stack
    callback functions to provide information about the context in which
    an error occurred, such as the current location in a PL function.

If we should have this at all, which I doubt, it's probably
errdetail not errcontext.

> How do you feel about redefining interval_mi in terms of interval_um
> and interval_pl? That one felt like an improvement to me even outside
> of the context of this change.

I did not think so.  For one thing, it introduces integer-overflow
hazards that you would not have otherwise; ie, interval_um might have
to throw an error for INT_MIN input, even though the end result of
the calculation would have been in range.

            regards, tom lane

[1] https://www.postgresql.org/docs/devel/error-message-reporting.html

Hi Joseph,
thanks for addressing comments.

On Sat, Apr 1, 2023 at 10:53 PM Joseph Koshakow <koshy44@gmail.com> wrote:
> So I added a check for FLOAT8_FITS_IN_INT64() and a test with this
> scenario.

I like that. Thanks.

>
> For what it's worth I think that 2147483647 months only became a valid
> interval in v15 as part of this commit [0]. It's also outside of the
> documented valid range [1], which is
> [-178000000 years, 178000000 years] or
> [-14833333 months, 14833333 months].

you mean +/-2136000000 months :). In that sense the current code
actually fixes a bug introduced in v15. So I am fine with it.

>
> The rationale for only checking the month's field is that it's faster
> than checking all three fields, though I'm not entirely sure if it's
> the right trade-off. Any thoughts on this?

Hmm, comparing one integer is certainly faster than comparing three.
We do that check at least once per interval operation. So the thrice
CPU cycles might show some impact when millions of rows are processed.

Given that we have clear documentation of bounds, just using months
field is fine. If needed we can always expand it later.

>
> > There's some way to avoid separate checks for infinite-ness of
> > interval and factor and use a single block using some integer
> > arithmetic. But I think this is more readable. So I avoided doing
> > that. Let me know if this works for you.
>
> I think the patch looks good, I've combined it with the existing patch.
>
> > Also added some test cases.
>
> I didn't see any tests in the patch, did you forget to include it?

Sorry I forgot to include those. Attached.

Please see my reply to your latest email as well.


--
Best Wishes,
Ashutosh Bapat

Hi Joseph,


On Mon, Apr 3, 2023 at 6:02 AM Joseph Koshakow <koshy44@gmail.com> wrote:

>
> I've attached a patch with all of the errcontext calls removed. None of
> the existing out of range errors have an errdetail call so I think this
> is more consistent. If we do want to add errdetail, then we should
> probably do it in a later patch and add it to all out of range errors,
> not just the ones related to infinity.

Hmm, I realize my errcontext suggestion was in wrong direction. We can
use errdetail if required in future. But not for this patch.

Here are comments on the test and output.

+ infinity                      |             |             |
  |        |  Infinity |  Infinity |       |         |  Infinity |
Infinity |  Infinity |   Infinity |          Infinity
+ -infinity                     |             |             |
  |        | -Infinity | -Infinity |       |         | -Infinity |
-Infinity | -Infinity |  -Infinity |         -Infinity

This is more for my education. It looks like for oscillating units we report
NULL here but for monotonically increasing units we report infinity. I came
across those terms in the code. But I didn't find definitions of those terms.
Can you please point me to the document/resources defining those terms.

diff --git a/src/test/regress/sql/horology.sql
b/src/test/regress/sql/horology.sql
index f7f8c8d2dd..1d0ab322c0 100644
--- a/src/test/regress/sql/horology.sql
+++ b/src/test/regress/sql/horology.sql
@@ -207,14 +207,17 @@ SELECT t.d1 AS t, i.f1 AS i, t.d1 + i.f1 AS
"add", t.d1 - i.f1 AS "subtract"
   FROM TIMESTAMP_TBL t, INTERVAL_TBL i
   WHERE t.d1 BETWEEN '1990-01-01' AND '2001-01-01'
     AND i.f1 BETWEEN '00:00' AND '23:00'
+    AND isfinite(i.f1)

I removed this and it did not have any effect on results. I think the
isfinite(i.f1) is already covered by the two existing conditions.

 SELECT t.f1 AS t, i.f1 AS i, t.f1 + i.f1 AS "add", t.f1 - i.f1 AS "subtract"
   FROM TIME_TBL t, INTERVAL_TBL i
+  WHERE isfinite(i.f1)
   ORDER BY 1,2;

 SELECT t.f1 AS t, i.f1 AS i, t.f1 + i.f1 AS "add", t.f1 - i.f1 AS "subtract"
   FROM TIMETZ_TBL t, INTERVAL_TBL i
+  WHERE isfinite(i.f1)
   ORDER BY 1,2;

 -- SQL9x OVERLAPS operator
@@ -287,11 +290,12 @@ SELECT f1 AS "timestamp"

 SELECT d.f1 AS "timestamp", t.f1 AS "interval", d.f1 + t.f1 AS plus
   FROM TEMP_TIMESTAMP d, INTERVAL_TBL t
+  WHERE isfinite(t.f1)
   ORDER BY plus, "timestamp", "interval";

 SELECT d.f1 AS "timestamp", t.f1 AS "interval", d.f1 - t.f1 AS minus
   FROM TEMP_TIMESTAMP d, INTERVAL_TBL t
-  WHERE isfinite(d.f1)
+  WHERE isfinite(t.f1)
   ORDER BY minus, "timestamp", "interval";

IIUC, the isfinite() conditions are added to avoid any changes to the
output due to new
values added to INTERVAL_TBL. Instead, it might be a good idea to not add these
conditions and avoid extra queries testing infinity arithmetic in interval.sql,
timestamptz.sql and timestamp.sql like below

+
+-- infinite intervals

... some lines folded

+
+SELECT date '1995-08-06' + interval 'infinity';
+SELECT date '1995-08-06' + interval '-infinity';
+SELECT date '1995-08-06' - interval 'infinity';
+SELECT date '1995-08-06' - interval '-infinity';

... block truncated

With that I have reviewed the entire patch-set. Once you address these
comments, we can mark it as ready for committer. I already see Tom
looking at the patch. So that might be just a formality.

--
Best Wishes,
Ashutosh Bapat

On Sat, Apr 8, 2023 at 8:54 PM Joseph Koshakow <koshy44@gmail.com> wrote:
>
> I was also unable to find a definition of oscillating or monotonically
> increasing in this context. I used the existing timestamps and dates
> code to form my own definition:
>
> If there exists an two intervals with the same sign, such that adding
> them together results in an interval with a unit that is less than the
> unit of at least one of the original intervals, then that unit is
> oscillating. Otherwise it is monotonically increasing.
>
> So for example `INTERVAL '30 seconds' + INTERVAL '30 seconds'` results
> in an interval with 0 seconds, so seconds are oscillating. You couldn't
> find a similar example for days or hours, so they're monotonically
> increasing.

Thanks for the explanation with an example. Makes sense considering
that the hours and days are not convertible to their wider units
without temporal context.

>
> >     SELECT t.f1 AS t, i.f1 AS i, t.f1 + i.f1 AS "add", t.f1 - i.f1 AS "subtract"
> >       FROM TIME_TBL t, INTERVAL_TBL i
> >    +  WHERE isfinite(i.f1)
> >       ORDER BY 1,2;
> >
> >     SELECT t.f1 AS t, i.f1 AS i, t.f1 + i.f1 AS "add", t.f1 - i.f1 AS "subtract"
> >       FROM TIMETZ_TBL t, INTERVAL_TBL i
> >    +  WHERE isfinite(i.f1)
> >       ORDER BY 1,2;

Those two are operations with Time which does not allow infinity. So I
think this is fine.

> >
> >     -- SQL9x OVERLAPS operator
> >    @@ -287,11 +290,12 @@ SELECT f1 AS "timestamp"
> >
> >     SELECT d.f1 AS "timestamp", t.f1 AS "interval", d.f1 + t.f1 AS plus
> >       FROM TEMP_TIMESTAMP d, INTERVAL_TBL t
> >    +  WHERE isfinite(t.f1)
> >       ORDER BY plus, "timestamp", "interval";
> >
> >     SELECT d.f1 AS "timestamp", t.f1 AS "interval", d.f1 - t.f1 AS minus
> >       FROM TEMP_TIMESTAMP d, INTERVAL_TBL t
> >    -  WHERE isfinite(d.f1)
> >    +  WHERE isfinite(t.f1)
> >       ORDER BY minus, "timestamp", "interval";
> I originally tried that, but the issue here is that errors propagate
> through the whole query. So if one row produces an error then no rows
> are produced and instead a single error is returned. So the rows that
> would execute, for example,
> SELECT date 'infinity' + interval '-infinity' would cause the entire
> query to error out. If you have any suggestions to get around this
> please let me know.

I modified this to WHERE isfinite(t.f1) or isfinite(d.f1). The output
contains a lot of additions with infinity::interval but that might be
ok. No errors. We could further improve it to allow operations between
infinity which do not result in error e.g, both operands being same
signed for plus and opposite signed for minus. But I think we can
leave this to the committer's judgement. Which route to choose.

>
> >    With that I have reviewed the entire patch-set. Once you address these
> >    comments, we can mark it as ready for committer. I already see Tom
> >    looking at the patch. So that might be just a formality.
>
> Thanks so much for taking the time to review this!

My pleasure. I am very much interested to see this being part of code.
Given that the last commit fest for v16 has ended, let's target this
for v17. I will mark this as ready for committer now.

--
Best Wishes,
Ashutosh Bapat

Looks like cfbot didn't like the names of these patches. It tried to
apply v20-0003 first and that failed. Attached patches with names in
sequential order. Let's see if that makes cfbot happy.

On Thu, Apr 13, 2023 at 12:05 AM Joseph Koshakow <koshy44@gmail.com> wrote:
>
>
>
> On Wed, Apr 12, 2023 at 9:11 AM Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> wrote:
>
> >    I modified this to WHERE isfinite(t.f1) or isfinite(d.f1). The output
> >    contains a lot of additions with infinity::interval but that might be
> >    ok. No errors.
>
> Attached is a patch with this testing change.
>
> Thanks,
> Joe Koshakow



--
Best Wishes,
Ashutosh Bapat

Resending with .patch at the end in case cfbot needs that too.

On Fri, Jun 23, 2023 at 12:57 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:
>
> Looks like cfbot didn't like the names of these patches. It tried to
> apply v20-0003 first and that failed. Attached patches with names in
> sequential order. Let's see if that makes cfbot happy.
>
> On Thu, Apr 13, 2023 at 12:05 AM Joseph Koshakow <koshy44@gmail.com> wrote:
> >
> >
> >
> > On Wed, Apr 12, 2023 at 9:11 AM Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> wrote:
> >
> > >    I modified this to WHERE isfinite(t.f1) or isfinite(d.f1). The output
> > >    contains a lot of additions with infinity::interval but that might be
> > >    ok. No errors.
> >
> > Attached is a patch with this testing change.
> >
> > Thanks,
> > Joe Koshakow
>
>
>
> --
> Best Wishes,
> Ashutosh Bapat



--
Best Wishes,
Ashutosh Bapat

Fixed assertion in time_mi_time(). It needed to assert that the result
is FINITE but it was doing the other way round and that triggered some
failures in cfbot.

On Fri, Jun 23, 2023 at 1:13 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:
>
> Resending with .patch at the end in case cfbot needs that too.
>
> On Fri, Jun 23, 2023 at 12:57 PM Ashutosh Bapat
> <ashutosh.bapat.oss@gmail.com> wrote:
> >
> > Looks like cfbot didn't like the names of these patches. It tried to
> > apply v20-0003 first and that failed. Attached patches with names in
> > sequential order. Let's see if that makes cfbot happy.
> >
> > On Thu, Apr 13, 2023 at 12:05 AM Joseph Koshakow <koshy44@gmail.com> wrote:
> > >
> > >
> > >
> > > On Wed, Apr 12, 2023 at 9:11 AM Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> wrote:
> > >
> > > >    I modified this to WHERE isfinite(t.f1) or isfinite(d.f1). The output
> > > >    contains a lot of additions with infinity::interval but that might be
> > > >    ok. No errors.
> > >
> > > Attached is a patch with this testing change.
> > >
> > > Thanks,
> > > Joe Koshakow
> >
> >
> >
> > --
> > Best Wishes,
> > Ashutosh Bapat
>
>
>
> --
> Best Wishes,
> Ashutosh Bapat



--
Best Wishes,
Ashutosh Bapat

Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> writes:
> Fixed assertion in time_mi_time(). It needed to assert that the result
> is FINITE but it was doing the other way round and that triggered some
> failures in cfbot.

It's still not passing in the cfbot, at least not on any non-Linux
platforms.  I believe the reason is that the patch thinks isinf()
delivers a three-way result, but per POSIX you can only expect
zero or nonzero (ie, finite or not).

            regards, tom lane

The patches still apply. But here's a rebased version with one white
space error fixed. Also ran pgindent.

On Sun, Jul 9, 2023 at 12:58 AM Joseph Koshakow <koshy44@gmail.com> wrote:
>
> On Sat, Jul 8, 2023 at 2:38 PM Joseph Koshakow <koshy44@gmail.com> wrote:
>
> > I've updated the patch to fix this issue.
>
> That seems to have fixed the cfbot failures. Though I left in an
> unused variable. Here's another set of patches with the compiler
> warnings fixed.
>
> Thanks,
> Joe Koshakow



--
Best Wishes,
Ashutosh Bapat

On Thu, 24 Aug 2023 at 14:51, Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:
>
> The patches still apply. But here's a rebased version with one white
> space error fixed. Also ran pgindent.
>

This needs another rebase, and it looks like the infinite interval
input code is broken.

I took a quick look, and had a couple of other review comments:

1). In interval_mul(), I think "result_sign" would be a more accurate
name than "result_is_inf" for the local variable.

2). interval_accum() and interval_accum_inv() don't work correctly
with infinite intervals. To make them work, they need to count the
number of infinities seen, to allow them to be subtracted off by the
inverse function (similar to the code in numeric.c, except for the
NaN-handling, which will need to be different). Consider, for example:

SELECT x, avg(x) OVER(ROWS BETWEEN CURRENT ROW AND 1 FOLLOWING)
  FROM (VALUES ('1 day'::interval),
               ('3 days'::interval),
               ('infinity'::timestamptz - now()),
               ('4 days'::interval),
               ('6 days'::interval)) v(x);
ERROR:  interval out of range

as compared to:

SELECT x, avg(x) OVER(ROWS BETWEEN CURRENT ROW AND 1 FOLLOWING)
  FROM (VALUES (1::numeric),
               (3::numeric),
               ('infinity'::numeric),
               (4::numeric),
               (6::numeric)) v(x);

    x     |        avg
----------+--------------------
        1 | 2.0000000000000000
        3 |           Infinity
 Infinity |           Infinity
        4 | 5.0000000000000000
        6 | 6.0000000000000000
(5 rows)

Regards,
Dean

On Tue, Sep 12, 2023 at 2:39 PM Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
>
> On Thu, 24 Aug 2023 at 14:51, Ashutosh Bapat
> <ashutosh.bapat.oss@gmail.com> wrote:
> >
> > The patches still apply. But here's a rebased version with one white
> > space error fixed. Also ran pgindent.
> >
>
> This needs another rebase,

Fixed.

> and it looks like the infinite interval
> input code is broken.
>

The code required to handle 'infinity' as an input value was removed by
d6d1430f404386162831bc32906ad174b2007776. I have added a separate
commit which reverts that commit as 0004, which should be merged into
0003.

> I took a quick look, and had a couple of other review comments:
>
> 1). In interval_mul(), I think "result_sign" would be a more accurate
> name than "result_is_inf" for the local variable.

Fixed as part of 0003.

>
> 2). interval_accum() and interval_accum_inv() don't work correctly
> with infinite intervals. To make them work, they need to count the
> number of infinities seen, to allow them to be subtracted off by the
> inverse function (similar to the code in numeric.c, except for the
> NaN-handling, which will need to be different). Consider, for example:
>
> SELECT x, avg(x) OVER(ROWS BETWEEN CURRENT ROW AND 1 FOLLOWING)
>   FROM (VALUES ('1 day'::interval),
>                ('3 days'::interval),
>                ('infinity'::timestamptz - now()),
>                ('4 days'::interval),
>                ('6 days'::interval)) v(x);
> ERROR:  interval out of range
>
> as compared to:
>
> SELECT x, avg(x) OVER(ROWS BETWEEN CURRENT ROW AND 1 FOLLOWING)
>   FROM (VALUES (1::numeric),
>                (3::numeric),
>                ('infinity'::numeric),
>                (4::numeric),
>                (6::numeric)) v(x);
>
>     x     |        avg
> ----------+--------------------
>         1 | 2.0000000000000000
>         3 |           Infinity
>  Infinity |           Infinity
>         4 | 5.0000000000000000
>         6 | 6.0000000000000000
> (5 rows)

Nice catch. I agree that we need to do something similar to
numeric_accum and numeric_accum_inv. As part of that also add test for
window aggregates on interval data type. We might also need some fix
to sum(). I am planning to work on this next week but in case somebody
else wants to pick this up here are patches with other things fixed.

--
Best Wishes,
Ashutosh Bapat

On Wed, Sep 13, 2023 at 6:13 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:
>
> to sum(). I am planning to work on this next week but in case somebody
> else wants to pick this up here are patches with other things fixed.
>
> --
> Best Wishes,
> Ashutosh Bapat


hi. some doc issues.

-     <literal>decade</literal>, <literal>century</literal>, and
<literal>millennium</literal>).
+     <literal>decade</literal>, <literal>century</literal>, and
<literal>millennium</literal>
+     for all types and <literal>hour</literal> and
<literal>day</literal> just for <type>interval</type>).

The above part seems not right. some fields do not apply to interval data types.
test case:
SELECT EXTRACT(epoch FROM interval 'infinity')  as epoch
        ,EXTRACT(YEAR FROM interval 'infinity') as year
        ,EXTRACT(decade FROM interval 'infinity') as decade
        ,EXTRACT(century FROM interval 'infinity') as century
        ,EXTRACT(millennium FROM interval 'infinity') as millennium
        ,EXTRACT(month FROM interval 'infinity') as mon
        ,EXTRACT(day FROM interval 'infinity')  as day
        ,EXTRACT(hour FROM interval 'infinity') as hour
        ,EXTRACT(min FROM interval 'infinity')  as min
        ,EXTRACT(second FROM interval 'infinity') as sec;

--------------------

-          <entry><type>date</type>, <type>timestamp</type></entry>
+          <entry><type>date</type>, <type>timestamp</type>,
<type>interval</type></entry>
           <entry>later than all other time stamps</entry>

it seems we have forgotten to mention the -infinity case, we can fix
the doc together, since <type>timestamptz</type>  also applies to
+/-infinity.

hi.

fixed the doc special value inf/-inf reference. didn't fix the EXTRACT
function doc issue.

I refactor the avg(interval), sum(interval), so moving aggregate,
plain aggregate both work with +inf/-inf.
no performance degradation, in fact, some performance gains.

--setup for test performance.
create unlogged table interval_aggtest AS
select  g::int as a
        ,make_interval(years => g % 100, days => g % 100, hours => g %
200 , secs => random()::numeric(3,2) *100 ) as b
from    generate_series(1, 100_000) g;
--use foreign data wrapper to copy exact content to interval_aggtest_no_patch
create unlogged table interval_aggtest_no_patch AS
select * from interval_aggtest;

--queryA
explain (analyze, costs off, buffers)
SELECT a, avg(b) OVER(ROWS BETWEEN 1 preceding AND 2 FOLLOWING)
from    interval_aggtest \watch i=0.1 c=10

--queryB
explain (analyze, costs off, buffers)
SELECT a, avg(b) OVER(ROWS BETWEEN 1 preceding  AND 2 FOLLOWING)
from    interval_aggtest_no_patch \watch i=0.1 c=10

--queryC
explain (analyze, costs off, buffers)
SELECT a, sum(b) OVER(ROWS BETWEEN 1 preceding AND 2 FOLLOWING)
from    interval_aggtest \watch i=0.1 c=10

--queryD
explain (analyze, costs off, buffers)
SELECT a, sum(b) OVER(ROWS BETWEEN 1 preceding AND 2 FOLLOWING)
from    interval_aggtest_no_patch \watch i=0.1 c=10

--queryE
explain (analyze, costs off, buffers)
SELECT sum(b), avg(b)
from    interval_aggtest \watch i=0.1 c=10

--queryF
explain (analyze, costs off, buffers)
SELECT sum(b), avg(b)
from    interval_aggtest_no_patch \watch i=0.1 c=10

queryA  execute 10 time, last executed time(ms) 748.258
queryB  execute 10 time, last executed time(ms) 1059.750

queryC  execute 10 time, last executed time(ms) 697.887
queryD  execute 10 time, last executed time(ms) 708.462

queryE  execute 10 time, last executed time(ms) 156.237
queryF  execute 10 time, last executed time(ms) 405.451
---------------------------------------------------------------------
The result seems right, I am not %100 sure the code it's correct.
That's the best I can think of. You can work based on that.

On Thu, Sep 14, 2023 at 11:58 AM jian he <jian.universality@gmail.com> wrote:
>
> -     <literal>decade</literal>, <literal>century</literal>, and
> <literal>millennium</literal>).
> +     <literal>decade</literal>, <literal>century</literal>, and
> <literal>millennium</literal>
> +     for all types and <literal>hour</literal> and
> <literal>day</literal> just for <type>interval</type>).

It seems you have changed a paragraph from
https://www.postgresql.org/docs/current/datatype-datetime.html#DATATYPE-INTERVAL-INPUT.
But that section is only for interval "8.5.4. Interval Input ". So
mentioning " ... for all types ..." wouldn't fit the section's title.
I don't see why it needs to be changed.

>
> The above part seems not right. some fields do not apply to interval data types.
> test case:
> SELECT EXTRACT(epoch FROM interval 'infinity')  as epoch
>         ,EXTRACT(YEAR FROM interval 'infinity') as year
>         ,EXTRACT(decade FROM interval 'infinity') as decade
>         ,EXTRACT(century FROM interval 'infinity') as century
>         ,EXTRACT(millennium FROM interval 'infinity') as millennium
>         ,EXTRACT(month FROM interval 'infinity') as mon
>         ,EXTRACT(day FROM interval 'infinity')  as day
>         ,EXTRACT(hour FROM interval 'infinity') as hour
>         ,EXTRACT(min FROM interval 'infinity')  as min
>         ,EXTRACT(second FROM interval 'infinity') as sec;

For this query, I get output
#SELECT EXTRACT(epoch FROM interval 'infinity')  as epoch
        ,EXTRACT(YEAR FROM interval 'infinity') as year
        ,EXTRACT(decade FROM interval 'infinity') as decade
        ,EXTRACT(century FROM interval 'infinity') as century
        ,EXTRACT(millennium FROM interval 'infinity') as millennium
        ,EXTRACT(month FROM interval 'infinity') as mon
        ,EXTRACT(day FROM timestamp 'infinity')  as day
        ,EXTRACT(hour FROM interval 'infinity') as hour
        ,EXTRACT(min FROM interval 'infinity')  as min
        ,EXTRACT(second FROM interval 'infinity') as sec;
  epoch   |   year   |  decade  | century  | millennium | mon | day |
 hour   | min | sec
----------+----------+----------+----------+------------+-----+-----+----------+-----+-----
 Infinity | Infinity | Infinity | Infinity |   Infinity |     |     |
Infinity |     |

EXTRACT( .... FROM interval '[-]infinity')  is implemented similar to
EXTRACT (... FROM timestamp '[-]infinity). Hence this is the output.
This has been discussed earlier [1].

>
> --------------------
>
> -          <entry><type>date</type>, <type>timestamp</type></entry>
> +          <entry><type>date</type>, <type>timestamp</type>,
> <type>interval</type></entry>
>            <entry>later than all other time stamps</entry>
>
> it seems we have forgotten to mention the -infinity case, we can fix
> the doc together, since <type>timestamptz</type>  also applies to
> +/-infinity.

Your point about -infinity is right. But timestamp corresponds to both
timestamp with and without timezone as per table 8.9 on the same page
. https://www.postgresql.org/docs/current/datatype-datetime.html#DATATYPE-DATETIME-TABLE.
So I don't see a need to specify timestamptz separately.

[1] https://www.postgresql.org/message-id/CAExHW5ut4bR4KSNWAhXb_EZ8PyY=J100guA6ZumNhvoia1ZRjw@mail.gmail.com

--
Best Wishes,
Ashutosh Bapat

On Wed, 13 Sept 2023 at 11:13, Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:
>
> On Tue, Sep 12, 2023 at 2:39 PM Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
> >
> > and it looks like the infinite interval
> > input code is broken.
>
> The code required to handle 'infinity' as an input value was removed by
> d6d1430f404386162831bc32906ad174b2007776. I have added a separate
> commit which reverts that commit as 0004, which should be merged into
> 0003.
>

I think that simply reverting d6d1430f404386162831bc32906ad174b2007776
is not sufficient. This does not make it clear what the point is of
the code in the "case RESERV" block. That code really should check the
value returned by DecodeSpecial(), otherwise invalid inputs are not
caught until later, and the error reported is not ideal. For example:

select interval 'now';
ERROR:  unexpected dtype 12 while parsing interval "now"

So DecodeInterval() should return DTERR_BAD_FORMAT in such cases (see
similar code in DecodeTimeOnly(), for example).

I'd also suggest a comment to indicate why itm_in isn't updated in
this case (see similar case in DecodeDateTime(), for example).


Another point to consider is what should happen if "ago" is specified
with infinite inputs. As it stands, it is accepted, but does nothing:

select interval 'infinity ago';
 interval
----------
 infinity
(1 row)

select interval '-infinity ago';
 interval
-----------
 -infinity
(1 row)

This could be made to invert the sign, as it does for finite inputs,
but I think perhaps it would be better to simply reject such inputs.

Regards,
Dean

On Sat, 16 Sept 2023 at 01:00, jian he <jian.universality@gmail.com> wrote:
>
> I refactor the avg(interval), sum(interval), so moving aggregate,
> plain aggregate both work with +inf/-inf.
> no performance degradation, in fact, some performance gains.
>

I haven't reviewed this part in any detail yet, but I can confirm that
there are some impressive performance improvements for avg(). However,
for me, sum() seems to be consistently a few percent slower with this
patch.

The introduction of an internal transition state struct seems like a
promising approach, but I think there is more to be gained by
eliminating per-row pallocs, and IntervalAggState's MemoryContext
(interval addition, unlike numeric addition, doesn't require memory
allocation, right?).

Also, this needs to include serialization and deserialization
functions, otherwise these aggregates will no longer be able to use
parallel workers. That makes a big difference to queryE, if the size
of the test data is scaled up.

This comment:

+   int64       N;              /* count of processed numbers */

should be "count of processed intervals".

Regards,
Dean

On Tue, Sep 19, 2023 at 7:14 PM Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
>
>
> I haven't reviewed this part in any detail yet, but I can confirm that
> there are some impressive performance improvements for avg(). However,
> for me, sum() seems to be consistently a few percent slower with this
> patch.

Now there should be no degradation.

> The introduction of an internal transition state struct seems like a
> promising approach, but I think there is more to be gained by
> eliminating per-row pallocs, and IntervalAggState's MemoryContext
> (interval addition, unlike numeric addition, doesn't require memory
> allocation, right?).

"eliminating per-row pallocs"
I guess I understand. If not , please point it out.

> IntervalAggState's MemoryContext
> (interval addition, unlike numeric addition, doesn't require memory
> allocation, right?).

if I remove IntervalAggState's element: MemoryContext, it will not work.
so I don't understand what the above sentence means...... Sorry. (it's
my problem)

> Also, this needs to include serialization and deserialization
> functions, otherwise these aggregates will no longer be able to use
> parallel workers. That makes a big difference to queryE, if the size
> of the test data is scaled up.
>
I tried, but failed. sum(interval) result is correct, but
avg(interval) result is wrong.

Datum
interval_avg_serialize(PG_FUNCTION_ARGS)
{
IntervalAggState *state;
StringInfoData buf;
bytea    *result;
/* Ensure we disallow calling when not in aggregate context */
if (!AggCheckCallContext(fcinfo, NULL))
elog(ERROR, "aggregate function called in non-aggregate context");
state = (IntervalAggState *) PG_GETARG_POINTER(0);
pq_begintypsend(&buf);
/* N */
pq_sendint64(&buf, state->N);
/* Interval struct elements, one by one. */
pq_sendint64(&buf, state->sumX.time);
pq_sendint32(&buf, state->sumX.day);
pq_sendint32(&buf, state->sumX.month);
/* pInfcount */
pq_sendint64(&buf, state->pInfcount);
/* nInfcount */
pq_sendint64(&buf, state->nInfcount);
result = pq_endtypsend(&buf);
PG_RETURN_BYTEA_P(result);
}

SELECT sum(b) ,avg(b)
        ,avg(b) = sum(b)/count(*) as should_be_true
        ,avg(b) * count(*) = sum(b) as should_be_true_too
from interval_aggtest_1m; --1million row.
The above query expects two bool columns to return true, but actually
both returns false.(spend some time found out parallel mode will make
the number of rows to 1_000_002, should be 1_000_0000).


> This comment:
>
> +   int64       N;              /* count of processed numbers */
>
> should be "count of processed intervals".

fixed.

On Wed, 20 Sept 2023 at 11:27, jian he <jian.universality@gmail.com> wrote:
>
> if I remove IntervalAggState's element: MemoryContext, it will not work.
> so I don't understand what the above sentence means...... Sorry. (it's
> my problem)
>

I don't see why it won't work. The point is to try to simplify
do_interval_accum() as much as possible. Looking at the current code,
I see a few places that could be simpler:

+    X.day = newval->day;
+    X.month = newval->month;
+    X.time = newval->time;
+
+    temp.day = state->sumX.day;
+    temp.month = state->sumX.month;
+    temp.time = state->sumX.time;

Why do we need these local variables X and temp? It could just add the
values from newval directly to those in state->sumX.

+    /* The rest of this needs to work in the aggregate context */
+    old_context = MemoryContextSwitchTo(state->agg_context);

Why? It's not allocating any memory here, so I don't see a need to
switch context.

So basically, do_interval_accum() could be simplified to:

static void
do_interval_accum(IntervalAggState *state, Interval *newval)
{
    /* Count infinite intervals separately from all else */
    if (INTERVAL_IS_NOBEGIN (newval))
    {
        state->nInfcount++;
        return;
    }
    if (INTERVAL_IS_NOEND(newval))
    {
        state->pInfcount++;
        return;
    }

    /* Update count of finite intervals */
    state->N++;

    /* Update sum of finite intervals */
    if (unlikely(pg_add_s32_overflow(state->sumX.month, newval->month,
                                     &state->sumX.month)))
        ereport(ERROR,
                errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
                errmsg("interval out of range"));

    if (unlikely(pg_add_s32_overflow(state->sumX.day, newval->day,
                                     &state->sumX.day)))
        ereport(ERROR,
                errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
                errmsg("interval out of range"));

    if (unlikely(pg_add_s64_overflow(state->sumX.time, newval->time,
                                     &state->sumX.time)))
        ereport(ERROR,
                errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
                errmsg("interval out of range"));

    return;
}

and that can be further refactored, as described below, and similarly
for do_interval_discard(), except using pg_sub_s32/64_overflow().


> > Also, this needs to include serialization and deserialization
> > functions, otherwise these aggregates will no longer be able to use
> > parallel workers. That makes a big difference to queryE, if the size
> > of the test data is scaled up.
> >
> I tried, but failed. sum(interval) result is correct, but
> avg(interval) result is wrong.
>
> SELECT sum(b) ,avg(b)
>         ,avg(b) = sum(b)/count(*) as should_be_true
>         ,avg(b) * count(*) = sum(b) as should_be_true_too
> from interval_aggtest_1m; --1million row.
> The above query expects two bool columns to return true, but actually
> both returns false.(spend some time found out parallel mode will make
> the number of rows to 1_000_002, should be 1_000_0000).
>

I think the reason for your wrong results is this code in
interval_avg_combine():

+    if (state2->N > 0)
+    {
+        /* The rest of this needs to work in the aggregate context */
+        old_context = MemoryContextSwitchTo(agg_context);
+
+        /* Accumulate interval values */
+        do_interval_accum(state1, &state2->sumX);
+
+        MemoryContextSwitchTo(old_context);
+    }

The problem is that using do_interval_accum() to add the 2 sums
together also adds 1 to the count N, making it incorrect. This code
should only be adding state2->sumX to state1->sumX, not touching
state1->N. And, as in do_interval_accum(), there is no need to switch
memory context.

Given that there are multiple places in this file that need to add
intervals, I think it makes sense to further refactor, and add a local
function to add 2 finite intervals, along the lines of the code above.
This can then be called from do_interval_accum(),
interval_avg_combine(), and interval_pl(). And similarly for
subtracting 2 finite intervals.

Regards,
Dean

On Wed, 20 Sept 2023 at 13:09, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
>
> So basically, do_interval_accum() could be simplified to:
>

Oh, and I guess it also needs an INTERVAL_NOT_FINITE() check, to make
sure that finite values don't sum to our representation of infinity,
as in interval_pl().

Regards,
Dean

On Mon, Sep 18, 2023 at 5:09 PM Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
>
> On Wed, 13 Sept 2023 at 11:13, Ashutosh Bapat
> <ashutosh.bapat.oss@gmail.com> wrote:
> >
> > On Tue, Sep 12, 2023 at 2:39 PM Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
> > >
> > > and it looks like the infinite interval
> > > input code is broken.
> >
> > The code required to handle 'infinity' as an input value was removed by
> > d6d1430f404386162831bc32906ad174b2007776. I have added a separate
> > commit which reverts that commit as 0004, which should be merged into
> > 0003.
> >
>
> I think that simply reverting d6d1430f404386162831bc32906ad174b2007776
> is not sufficient. This does not make it clear what the point is of
> the code in the "case RESERV" block. That code really should check the
> value returned by DecodeSpecial(), otherwise invalid inputs are not
> caught until later, and the error reported is not ideal. For example:
>
> select interval 'now';
> ERROR:  unexpected dtype 12 while parsing interval "now"
>
> So DecodeInterval() should return DTERR_BAD_FORMAT in such cases (see
> similar code in DecodeTimeOnly(), for example).

The since the code was there earlier, I missed that part. Sorry.

>
> I'd also suggest a comment to indicate why itm_in isn't updated in
> this case (see similar case in DecodeDateTime(), for example).
>

Added but in the function prologue since it's part of the API.

>
> Another point to consider is what should happen if "ago" is specified
> with infinite inputs. As it stands, it is accepted, but does nothing:
>
> select interval 'infinity ago';
>  interval
> ----------
>  infinity
> (1 row)
>
> select interval '-infinity ago';
>  interval
> -----------
>  -infinity
> (1 row)
>
> This could be made to invert the sign, as it does for finite inputs,
> but I think perhaps it would be better to simply reject such inputs.

Fixed this. After studying what DecodeInterval is doing, I think it's
better to treat all infinity specifications similar to "ago". They
need to be the last part of the input string. Rest of the code makes
sure that nothing preceds infinity specification as other case blocks
do not handle RESERVE or DTK_LATE and DTK_EARLY. This means that
"+infinity", "-infinity" or "infinity" can be the only allowed word as
a valid interval input when either of them is specified.

I will post these changes in another email along with other patches.

--
Best Wishes,
Ashutosh Bapat

On Wed, Sep 20, 2023 at 5:39 PM Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
>
> On Wed, 20 Sept 2023 at 11:27, jian he <jian.universality@gmail.com> wrote:
> >
> > if I remove IntervalAggState's element: MemoryContext, it will not work.
> > so I don't understand what the above sentence means...... Sorry. (it's
> > my problem)
> >
>
> I don't see why it won't work. The point is to try to simplify
> do_interval_accum() as much as possible. Looking at the current code,
> I see a few places that could be simpler:
>
> +    X.day = newval->day;
> +    X.month = newval->month;
> +    X.time = newval->time;
> +
> +    temp.day = state->sumX.day;
> +    temp.month = state->sumX.month;
> +    temp.time = state->sumX.time;
>
> Why do we need these local variables X and temp? It could just add the
> values from newval directly to those in state->sumX.
>
> +    /* The rest of this needs to work in the aggregate context */
> +    old_context = MemoryContextSwitchTo(state->agg_context);
>
> Why? It's not allocating any memory here, so I don't see a need to
> switch context.
>
> So basically, do_interval_accum() could be simplified to:
>
> static void
> do_interval_accum(IntervalAggState *state, Interval *newval)
> {
>     /* Count infinite intervals separately from all else */
>     if (INTERVAL_IS_NOBEGIN (newval))
>     {
>         state->nInfcount++;
>         return;
>     }
>     if (INTERVAL_IS_NOEND(newval))
>     {
>         state->pInfcount++;
>         return;
>     }
>
>     /* Update count of finite intervals */
>     state->N++;
>
>     /* Update sum of finite intervals */
>     if (unlikely(pg_add_s32_overflow(state->sumX.month, newval->month,
>                                      &state->sumX.month)))
>         ereport(ERROR,
>                 errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
>                 errmsg("interval out of range"));
>
>     if (unlikely(pg_add_s32_overflow(state->sumX.day, newval->day,
>                                      &state->sumX.day)))
>         ereport(ERROR,
>                 errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
>                 errmsg("interval out of range"));
>
>     if (unlikely(pg_add_s64_overflow(state->sumX.time, newval->time,
>                                      &state->sumX.time)))
>         ereport(ERROR,
>                 errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
>                 errmsg("interval out of range"));
>
>     return;
> }
>
> and that can be further refactored, as described below, and similarly
> for do_interval_discard(), except using pg_sub_s32/64_overflow().
>
>
> > > Also, this needs to include serialization and deserialization
> > > functions, otherwise these aggregates will no longer be able to use
> > > parallel workers. That makes a big difference to queryE, if the size
> > > of the test data is scaled up.
> > >
> > I tried, but failed. sum(interval) result is correct, but
> > avg(interval) result is wrong.
> >
> > SELECT sum(b) ,avg(b)
> >         ,avg(b) = sum(b)/count(*) as should_be_true
> >         ,avg(b) * count(*) = sum(b) as should_be_true_too
> > from interval_aggtest_1m; --1million row.
> > The above query expects two bool columns to return true, but actually
> > both returns false.(spend some time found out parallel mode will make
> > the number of rows to 1_000_002, should be 1_000_0000).
> >
>
> I think the reason for your wrong results is this code in
> interval_avg_combine():
>
> +    if (state2->N > 0)
> +    {
> +        /* The rest of this needs to work in the aggregate context */
> +        old_context = MemoryContextSwitchTo(agg_context);
> +
> +        /* Accumulate interval values */
> +        do_interval_accum(state1, &state2->sumX);
> +
> +        MemoryContextSwitchTo(old_context);
> +    }
>
> The problem is that using do_interval_accum() to add the 2 sums
> together also adds 1 to the count N, making it incorrect. This code
> should only be adding state2->sumX to state1->sumX, not touching
> state1->N. And, as in do_interval_accum(), there is no need to switch
> memory context.
>
> Given that there are multiple places in this file that need to add
> intervals, I think it makes sense to further refactor, and add a local
> function to add 2 finite intervals, along the lines of the code above.
> This can then be called from do_interval_accum(),
> interval_avg_combine(), and interval_pl(). And similarly for
> subtracting 2 finite intervals.

I was working on refactoring Jian's patches but forgot to mention it
there. I think the patchset attached has addressed all your comments.
But they do not implement serialization and deserialization yet. I
will take a look at Jian's patch for the same and incorporate/refactor
those changes.

Jian,
I don't understand why there's two sets of test queries, one with
ORDER BY and one without? Does ORDER BY specification make any
difference in the testing?

Patches are thus
0001, 0002 are same
0003 - earlier 0003 + incorporated doc changes suggested by Jian
0004 - fixes DecodeInterval()
0005 - Refactored Jian's code fixing window functions. Does not
contain the changes for serialization and deserialization. Jian,
please let me know if I have missed anything else.

In my testing, I saw the timings for queries as below
Query A: 145.164 ms
Query B: 222.419 ms

Query C: 136.995 ms
Query D: 146.893 ms

Query E: 38.053 ms
Query F: 80.112 ms

I didn't see degradation in case of sum().

--
Best Wishes,
Ashutosh Bapat

On Wed, Sep 20, 2023 at 5:44 PM Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
>
> On Wed, 20 Sept 2023 at 13:09, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
> >
> > So basically, do_interval_accum() could be simplified to:
> >
>
> Oh, and I guess it also needs an INTERVAL_NOT_FINITE() check, to make
> sure that finite values don't sum to our representation of infinity,
> as in interval_pl().

Fixed in the latest patch set.

--
Best Wishes,
Ashutosh Bapat

On Wed, 20 Sept 2023 at 15:00, Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:
>
> 0005 - Refactored Jian's code fixing window functions. Does not
> contain the changes for serialization and deserialization. Jian,
> please let me know if I have missed anything else.
>

That looks a lot neater. One thing I don't care for is this code
pattern in finite_interval_pl():

+    result->month = span1->month + span2->month;
+    /* overflow check copied from int4pl */
+    if (SAMESIGN(span1->month, span2->month) &&
+        !SAMESIGN(result->month, span1->month))
+        ereport(ERROR,

The problem is that this is a bug waiting to happen for anyone who
uses this function with "result" pointing to the same Interval struct
as "span1" or "span2". I understand that the current code avoids this
by careful use of temporary Interval structs, but it's still a pretty
ugly pattern. This can be avoided by using pg_add_s32/64_overflow(),
which then allows the callers to be simplified, getting rid of the
temporary Interval structs and memcpy()'s.

Also, in do_interval_discard(), this seems a bit risky:

+        neg_val.day = -newval->day;
+        neg_val.month = -newval->month;
+        neg_val.time = -newval->time;

because it could in theory turn a finite large negative interval into
an infinite one (-INT_MAX -> INT_MAX), leading to an assertion failure
in finite_interval_pl(). Now maybe that's not possible for some other
reasons, but I think we may as well do the same refactoring for
interval_mi() as we're doing for interval_pl() -- i.e., introduce a
finite_interval_mi() function, making the addition and subtraction
code match, and removing the need for neg_val in
do_interval_discard().

Regards,
Dean

> On Wed, 20 Sept 2023 at 15:00, Ashutosh Bapat
> <ashutosh.bapat.oss@gmail.com> wrote:
> >
> > 0005 - Refactored Jian's code fixing window functions. Does not
> > contain the changes for serialization and deserialization. Jian,
> > please let me know if I have missed anything else.
> >

attached serialization and deserialization function.


>
> Also, in do_interval_discard(), this seems a bit risky:
>
> +        neg_val.day = -newval->day;
> +        neg_val.month = -newval->month;
> +        neg_val.time = -newval->time;
>

we already have interval negate function, So I changed to interval_um_internal.
based on 20230920 patches. I have made the attached changes.

The serialization do make big difference when configure to parallel mode.

On Wed, Sep 20, 2023 at 8:23 PM Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
>
> On Wed, 20 Sept 2023 at 15:00, Ashutosh Bapat
> <ashutosh.bapat.oss@gmail.com> wrote:
> >
> > 0005 - Refactored Jian's code fixing window functions. Does not
> > contain the changes for serialization and deserialization. Jian,
> > please let me know if I have missed anything else.
> >
>
> That looks a lot neater. One thing I don't care for is this code
> pattern in finite_interval_pl():
>
> +    result->month = span1->month + span2->month;
> +    /* overflow check copied from int4pl */
> +    if (SAMESIGN(span1->month, span2->month) &&
> +        !SAMESIGN(result->month, span1->month))
> +        ereport(ERROR,
>
> The problem is that this is a bug waiting to happen for anyone who
> uses this function with "result" pointing to the same Interval struct
> as "span1" or "span2". I understand that the current code avoids this
> by careful use of temporary Interval structs, but it's still a pretty
> ugly pattern. This can be avoided by using pg_add_s32/64_overflow(),
> which then allows the callers to be simplified, getting rid of the
> temporary Interval structs and memcpy()'s.

That's a good idea. Done.

>
> Also, in do_interval_discard(), this seems a bit risky:
>
> +        neg_val.day = -newval->day;
> +        neg_val.month = -newval->month;
> +        neg_val.time = -newval->time;
>
> because it could in theory turn a finite large negative interval into
> an infinite one (-INT_MAX -> INT_MAX), leading to an assertion failure
> in finite_interval_pl(). Now maybe that's not possible for some other
> reasons, but I think we may as well do the same refactoring for
> interval_mi() as we're doing for interval_pl() -- i.e., introduce a
> finite_interval_mi() function, making the addition and subtraction
> code match, and removing the need for neg_val in
> do_interval_discard().

Your suspicion is correct. It did throw an error. Added tests for the
same. Introduced finite_interval_mi() which uses
pg_sub_s32/s64_overflow() functions.

I will send updated patches with my next reply.

--
Best Wishes,
Ashutosh Bapat

On Thu, Sep 21, 2023 at 8:35 AM jian he <jian.universality@gmail.com> wrote:
>
> > On Wed, 20 Sept 2023 at 15:00, Ashutosh Bapat
> > <ashutosh.bapat.oss@gmail.com> wrote:
> > >
> > > 0005 - Refactored Jian's code fixing window functions. Does not
> > > contain the changes for serialization and deserialization. Jian,
> > > please let me know if I have missed anything else.
> > >
>
> attached serialization and deserialization function.
>

Thanks. They look good. I have incorporated those in the attached patch set.

One thing I didn't understand though is the use of
makeIntervalAggState() in interval_avg_deserialize(). In all other
deserialization functions like numeric_avg_deserialize() we create the
Agg State in CurrentMemoryContext but makeIntervalAggState() creates
it in aggcontext. And it works. We could change the code to allocate
agg state in aggcontext. Not a big change. But I did not find any
explanation as to why we use CurrentMemoryContext in other places.
Dean, do you have any idea?

>
> >
> > Also, in do_interval_discard(), this seems a bit risky:
> >
> > +        neg_val.day = -newval->day;
> > +        neg_val.month = -newval->month;
> > +        neg_val.time = -newval->time;
> >
>
> we already have interval negate function, So I changed to interval_um_internal.
> based on 20230920 patches. I have made the attached changes.

I didn't use this since it still requires neg_val variable and the
implementation for finite interval subtraction would still be differ
in interval_mi and do_interval_discard().

>
> The serialization do make big difference when configure to parallel mode.

Yes. On my machine queryE shows following timings, that's huge change
because of parallel query.
with the ser/deser functions: 112.193 ms
without those functions: 272.759 ms.

Before the introduction of Internal IntervalAggState, there were no
serialize, deserialize functions. I wonder how did parallel query
work. Did it just use serialize/deserialize functions of _interval?

The attached patches are thus
0001 - 0005 - same as the last patch set.
Dean, if you are fine with the changes in 0004, I would like to merge
that into 0003.

0006 - uses pg_add/sub_s32/64_overflow functions in finite_interval_pl
and also introduces finite_interval_mi as suggested by Dean.
0007 - implements serialization and deserialization functions, but
uses aggcontext for deser.

Once we are fine with the last three patches, they need to be merged into 0003.

--
Best Wishes,
Ashutosh Bapat

On Thu, Sep 21, 2023 at 7:21 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:
>
> One thing I didn't understand though is the use of
> makeIntervalAggState() in interval_avg_deserialize(). In all other
> deserialization functions like numeric_avg_deserialize() we create the
> Agg State in CurrentMemoryContext but makeIntervalAggState() creates
> it in aggcontext. And it works. We could change the code to allocate
> agg state in aggcontext. Not a big change. But I did not find any
> explanation as to why we use CurrentMemoryContext in other places.
> Dean, do you have any idea?

Following code in ExecInterpExpr makes it clear that the
deserialization function is be executed in per tuple memory context.
Whereas the aggregate's context is different from this context and may
lives longer that the context in which deserialization is expected to
happen.

/* evaluate aggregate deserialization function (non-strict portion) */
EEO_CASE(EEOP_AGG_DESERIALIZE)
{
FunctionCallInfo fcinfo = op->d.agg_deserialize.fcinfo_data;
AggState *aggstate = castNode(AggState, state->parent);
MemoryContext oldContext;

/*
* We run the deserialization functions in per-input-tuple memory
* context.
*/
oldContext = MemoryContextSwitchTo(aggstate->tmpcontext->ecxt_per_tuple_memory);
fcinfo->isnull = false;
*op->resvalue = FunctionCallInvoke(fcinfo);
*op->resnull = fcinfo->isnull;
MemoryContextSwitchTo(oldContext);

Hence I have changed interval_avg_deserialize() in 0007 to use
CurrentMemoryContext instead of aggcontext. Rest of the patches are
same as previous set.

--
Best Wishes,
Ashutosh Bapat

On Fri, 22 Sept 2023 at 08:49, Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:
>
> Following code in ExecInterpExpr makes it clear that the
> deserialization function is be executed in per tuple memory context.
> Whereas the aggregate's context is different from this context and may
> lives longer that the context in which deserialization is expected to
> happen.
>

Right. I was about to reply, saying much the same thing, but it's
always better when you see it for yourself.

> Hence I have changed interval_avg_deserialize() in 0007 to use
> CurrentMemoryContext instead of aggcontext.

+1. And consistency with other deserialisation functions is good.

> Rest of the patches are
> same as previous set.
>

OK, I'll take a look.

Regards,
Dean

On Fri, Sep 22, 2023 at 3:49 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:
>
> On Thu, Sep 21, 2023 at 7:21 PM Ashutosh Bapat
> <ashutosh.bapat.oss@gmail.com> wrote:
> >
> Hence I have changed interval_avg_deserialize() in 0007 to use
> CurrentMemoryContext instead of aggcontext. Rest of the patches are
> same as previous set.
>
> --
> Best Wishes,
> Ashutosh Bapat

/* TODO: Handle NULL inputs? */
since interval_avg_serialize is strict, so handle null would be like:
if (PG_ARGISNULL(0)) then PG_RETURN_NULL();

On Fri, Sep 22, 2023 at 2:35 PM jian he <jian.universality@gmail.com> wrote:
>
> /* TODO: Handle NULL inputs? */
> since interval_avg_serialize is strict, so handle null would be like:
> if (PG_ARGISNULL(0)) then PG_RETURN_NULL();

That's automatically taken care of by the executor. Functions need to
handle NULL inputs if they are *not* strict.

#select proisstrict from pg_proc where proname = 'interval_avg_serialize';
 proisstrict
-------------
 t
(1 row)

#select proisstrict from pg_proc where proname = 'interval_avg_deserialize';
 proisstrict
-------------
 t
(1 row)


--
Best Wishes,
Ashutosh Bapat

On Fri, 22 Sept 2023 at 09:09, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
>
> On Fri, 22 Sept 2023 at 08:49, Ashutosh Bapat
> <ashutosh.bapat.oss@gmail.com> wrote:
> >
> > Rest of the patches are
> > same as previous set.
>
> OK, I'll take a look.
>

I've been going over this in more detail, and I'm attaching my review
comments as an incremental patch to make it easier to see the changes.

Aside from some cosmetic stuff, I've made the following more
substantive changes:

1. I don't think it's really worth adding the
pg_mul_add_sNN_overflow() functions to int.h.

I thought about this for a while, and it looks to me as though they're
really only of very limited general use, and even this patch only used
them in a couple of places.

Looking around more widely, the majority of places that multiply then
add actually require a 4-argument function that computes result = a *
b + c. But even then, such functions would offer no performance
benefit, and wouldn't really do much to improve code readability at
call sites.

And there's another issue: someone using these functions might
reasonably expect them to return true if the result overflows, but
actually, as written, they also return true if the intermediate
product overflows, which isn't necessarily what's expected / wanted.

So I think it's best to just drop these functions, getting rid of
0001, and rewriting 0002 to just use the existing int.h functions.
After a little more copy-editing, I think that actually makes the code
in make_interval() more readable.

I think that part is now ready to commit, and I plan to push this fix
to make_interval() separately, since it's really a bug-fix, not
related to support for infinite intervals. In line with recent
precedent, I don't think it's worth back-patching though, since such
inputs are pretty unlikely in production.


2. The various in_range() functions needed adjusting to handle
infinite interval offsets.

For timestamp values, I followed the precedent set by the equivalent
float/numeric code. I.e., all (finite and non-finite) timestamps are
regarded as infinitely following -infinity and infinitely preceding
+infinity.

For time values, it's a bit different because no time values precede
or follow any other by more than 24 hours, so a window frame between
+inf following and +inf following is empty (whereas in the timestamp
case it contains +inf). Put another way, such a window frame is empty
because a time value can't be infinity.


3. I got rid of interval2timestamp_no_overflow() because I don't think
it really makes much sense to convert an interval to a timestamp, and
it's a bit of a hack anyway (as selfuncs.c itself admits). Actually, I
think it's OK to just leave selfuncs.c as it is. The existing code
will cope just fine with infinite intervals, since they aren't really
infinite, just larger than any others.


4. I tested pg_upgrade on a table with an interval with INT_MAX
months, and it was silently converted to infinity. I think that's
probably the best outcome (better than failing). However, this means
that we really should require all 3 fields of an interval to be
INT_MIN/MAX for it to be considered infinite, otherwise it would be
possible to have multiple internal representations of infinity that do
not compare as equal.

Similarly, interval_in() needs to accept such inputs, otherwise things
like pg_dump/restore from pre-17 databases could fail. But since it
now requires all 3 fields of the interval to be INT_MIN/MAX for it to
be infinite, the odds of that happening by accident are vanishingly
small in practice.

This approach also means that the range of allowed finite intervals is
only reduced by 1 microsecond at each end of the range, rather than a
whole month.

Also, it means that it is no longer necessary to change a number of
the regression tests (such as the justify_interval() tests) for values
near INT_MIN/MAX.


Overall, I think this is now pretty close to being ready for commit.

Regards,
Dean

On Fri, Sep 29, 2023 at 12:43 PM Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
>
> I think that part is now ready to commit, and I plan to push this fix
> to make_interval() separately, since it's really a bug-fix, not
> related to support for infinite intervals. In line with recent
> precedent, I don't think it's worth back-patching though, since such
> inputs are pretty unlikely in production.

The changes look good to me. I am not a fan of goto construct. But
this looks nicer.

I think we should introduce interval_out_of_range_error() function on
the lines of float_overflow_error(). Later patches introduce more
places where we raise that error. We can introduce the function as
part of those patches.

>
>
> 2. The various in_range() functions needed adjusting to handle
> infinite interval offsets.
>
> For timestamp values, I followed the precedent set by the equivalent
> float/numeric code. I.e., all (finite and non-finite) timestamps are
> regarded as infinitely following -infinity and infinitely preceding
> +infinity.
>
> For time values, it's a bit different because no time values precede
> or follow any other by more than 24 hours, so a window frame between
> +inf following and +inf following is empty (whereas in the timestamp
> case it contains +inf). Put another way, such a window frame is empty
> because a time value can't be infinity.
>

I will review and test this. I will also take a look at what else we
might be missing in the patch. [5] did mention that in_range()
functions need to be assessed but I don't see corresponding changes in
the subsequent patches. I will go over that list again.

>
> 3. I got rid of interval2timestamp_no_overflow() because I don't think
> it really makes much sense to convert an interval to a timestamp, and
> it's a bit of a hack anyway (as selfuncs.c itself admits). Actually, I
> think it's OK to just leave selfuncs.c as it is. The existing code
> will cope just fine with infinite intervals, since they aren't really
> infinite, just larger than any others.
>

This looks odd next to date2timestamp_no_overflow() which returns
-DBL_MIN/DBL_MAX for infinite value. But it's in agreement with what
we do with timestamp i.e. we don't convert infinities to DBL_MIN/MAX.
So I am fine with just adding a comment, the way you have done it.
Don't have much preference here.

>
> 4. I tested pg_upgrade on a table with an interval with INT_MAX
> months, and it was silently converted to infinity. I think that's
> probably the best outcome (better than failing).

[1] mentions that Interval with month = INT_MAX is a valid finite
value but out of documented range of interval [2]. The highest value
of Interval = 178000000 (years) * 12 = 2136000000 months which is less
than (2^32 - 1). But we do not prohibit such a value from entering the
database, albeit very less probable.

> However, this means
> that we really should require all 3 fields of an interval to be
> INT_MIN/MAX for it to be considered infinite, otherwise it would be
> possible to have multiple internal representations of infinity that do
> not compare as equal.
>
> Similarly, interval_in() needs to accept such inputs, otherwise things
> like pg_dump/restore from pre-17 databases could fail. But since it
> now requires all 3 fields of the interval to be INT_MIN/MAX for it to
> be infinite, the odds of that happening by accident are vanishingly
> small in practice.
>
> This approach also means that the range of allowed finite intervals is
> only reduced by 1 microsecond at each end of the range, rather than a
> whole month.
>
> Also, it means that it is no longer necessary to change a number of
> the regression tests (such as the justify_interval() tests) for values
> near INT_MIN/MAX.

My first patch was comparing all the three fields to determine whether
a given Interval value represents infinity. [3] changed that to use
only the month field. I guess that was based on the discussion at [4].
You may want to review that discussion if not already done. I am fine
either way. We should be able to change the comparison code later if
we see performance getting impacted.

>
>
> Overall, I think this is now pretty close to being ready for commit.

Thanks.

[1] https://www.postgresql.org/message-id/CAAvxfHea4%2BsPybKK7agDYOMo9N-Z3J6ZXf3BOM79pFsFNcRjwA%40mail.gmail.com
[2] Table 8.9 at
https://www.postgresql.org/docs/current/datatype-datetime.html#DATATYPE-DATETIME
[3] https://www.postgresql.org/message-id/CAAvxfHf0-T99i%3DOrve_xfonVCvsCuPy7C4avVm%3D%2Byu128ujSGg%40mail.gmail.com
[4] https://www.postgresql.org/message-id/26022.1545087636%40sss.pgh.pa.us
[5] https://www.postgresql.org/message-id/CAAvxfHdzd5JLRBXDAW7OPhsNNACvhsCP3f5R4LNhRVaDuQG0gg%40mail.gmail.com

--
Best Wishes,
Ashutosh Bapat

Hi Dean,

On Wed, Oct 4, 2023 at 6:59 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:
>
> I think we should introduce interval_out_of_range_error() function on
> the lines of float_overflow_error(). Later patches introduce more
> places where we raise that error. We can introduce the function as
> part of those patches.
>

Not done yet. The error is raised from multiple places and we might
find improving error message at some of those places. This refactoring
will make that work difficult. Let me know if you think otherwise.

> >
> >
> > 2. The various in_range() functions needed adjusting to handle
> > infinite interval offsets.
> >
> > For timestamp values, I followed the precedent set by the equivalent
> > float/numeric code. I.e., all (finite and non-finite) timestamps are
> > regarded as infinitely following -infinity and infinitely preceding
> > +infinity.
> >
> > For time values, it's a bit different because no time values precede
> > or follow any other by more than 24 hours, so a window frame between
> > +inf following and +inf following is empty (whereas in the timestamp
> > case it contains +inf). Put another way, such a window frame is empty
> > because a time value can't be infinity.
> >

I think this code is reasonable. But users may find it inconsistent
with the other ways to achieve the same outcome. For example, We don't
allow non-finite intervals to be added or subtracted from time or
timetz. So if someone tries to compare the rows using val >/<= base
+/- offset, those queries will fail whereas similar implied conditions
in window specification will not throw an error. If you have
considered this already, I am fine with the code as is.

This code doesn't handle non-finite intervals explicitly. But that's
inline with the interval comparison functions (interval_le/ge etc.)
which rely on infinities being represented by extreme values.

>
> I will review and test this. I will also take a look at what else we
> might be missing in the patch. [5] did mention that in_range()
> functions need to be assessed but I don't see corresponding changes in
> the subsequent patches. I will go over that list again.

Added a separate patch (0009) to fix
brin_minmax_multi_distance_interval(). The fix is inconsistent with
the way infinte timestamp and date is handled in that file. But I
think infinite timestamp and date handling itself is inconsistent with
the way infinite values of float are handled. I have tried to be
consistent with float. May be we should fix date and timestamp
functions as well.

I also changed brin_multi.sql to test infinte interval values in BRIN
index. This required some further changes to existing queries.
I thought about combining these two INSERTs but decided against that
since we would loose NULL interval values.
-- throw in some NULL's and different values
INSERT INTO brintest_multi (inetcol, cidrcol) SELECT
inet 'fe80::6e40:8ff:fea9:8c46' + tenthous,
cidr 'fe80::6e40:8ff:fea9:8c46' + tenthous
FROM tenk1 ORDER BY thousand, tenthous LIMIT 25;

INSERT INTO brintest_multi(intervalcol) VALUES ('-infinity'), ('+infinity');

I took some time understand BRIN before making any changes. That took time.

On your patches.
I like the eval() function you have used and its usage. It's a bit
harder to understand it initially but makes the query and output
crisp. Saves some SQL and output lines too. I tried to use the same
trick for time and timetz
select t as time, i as interval,
                eval(format('time %L + interval %L', t, i)) AS time_pl,
                eval(format('time %L - interval %L', t, i)) AS time_mi,
                eval(format('timetz %L + interval %L', t, i)) AS timetz_pl,
                eval(format('timetz %L - interval %L', t, i)) AS timetz_mi
        from (values ('11:27:42')) t1(t),
                (values ('infinity'),
                        ('-infinity')) as t2(i);
The query and output take the same space. So I decided against using it.

I have added a separate patch (0008) to test negative interval values,
including -infinity, in preceding and following specification.

Patches from 0001 to 0007 are same as what you attached but rebased on
the latest HEAD.

I think we should squash 0002 to 0007.

--
Best Wishes,
Ashutosh Bapat

On Wed, 4 Oct 2023 at 14:29, Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:
>
> I think we should introduce interval_out_of_range_error() function on
> the lines of float_overflow_error(). Later patches introduce more
> places where we raise that error. We can introduce the function as
> part of those patches.
>

I'm not convinced that it is really worth it. Note also that even with
this patch, there are still more places that throw "timestamp out of
range" errors than "interval out of range" errors.

> > 4. I tested pg_upgrade on a table with an interval with INT_MAX
> > months, and it was silently converted to infinity. I think that's
> > probably the best outcome (better than failing). However, this means
> > that we really should require all 3 fields of an interval to be
> > INT_MIN/MAX for it to be considered infinite, otherwise it would be
> > possible to have multiple internal representations of infinity that do
> > not compare as equal.
> >
> My first patch was comparing all the three fields to determine whether
> a given Interval value represents infinity. [3] changed that to use
> only the month field. I guess that was based on the discussion at [4].
> You may want to review that discussion if not already done. I am fine
> either way. We should be able to change the comparison code later if
> we see performance getting impacted.
>

Before looking at the details more closely, I might have agreed with
that earlier discussion. However, given that things like pg_upgrade
have the possibility of turning formerly allowed, finite intervals
into infinity, we really need to ensure that there is only one value
equal to infinity, otherwise the results are likely to be very
confusing and counter-intuitive. That means that we have to continue
to regard intervals like INT32_MAX months + 10 days as finite.

While I haven't done any performance testing, I wouldn't expect this
to have much impact. In a 64-bit build, this actually generates 2
comparisons rather than 3 -- one comparing the combined month and day
fields against a 64-bit value containing 2 copies of INT32_MAX, and
one testing the time field. In practice, only the first test will be
executed in the vast majority of cases.


Something that perhaps does need discussing is the fact that
'2147483647 months 2147483647 days 9223372036854775807 usecs' is now
accepted by interval_in() and gives infinity. That's a bit ugly, but I
think it's defensible as a measure to prevent dump/restore errors from
older databases, and in any case, such an interval is outside the
documented range of supported intervals, and is a highly contrived
example, vanishingly improbable in practice.

Alternatively, we could have interval_in() reject this, which would
open up the possibility of dump/restore errors. It could be argued
that that's OK, for similar reasons -- the failing value is highly
unlikely/contrived, and out of the documented range. I don't like that
though. I don't think dump/restore should fail under any
circumstances, however unlikely.

Another alternative is to accept this input, but emit a WARNING. I
don't particularly like that either, since it's forcing a check on
every input value, just to cater for this one specific highly unlikely
input. In fact, both these alternative approaches (rejecting the
value, or emitting a warning), would impose a small performance
penalty on every interval input, which I don't think is really worth
it.

So overall, my preference is to just accept it. Anything else is more
work, for no practical benefit.

Regards,
Dean

On Tue, 10 Oct 2023 at 12:36, Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:
>
> > > 2. The various in_range() functions needed adjusting to handle
> > > infinite interval offsets.
> > >
> I think this code is reasonable. But users may find it inconsistent
> with the other ways to achieve the same outcome. For example, We don't
> allow non-finite intervals to be added or subtracted from time or
> timetz. So if someone tries to compare the rows using val >/<= base
> +/- offset, those queries will fail whereas similar implied conditions
> in window specification will not throw an error. If you have
> considered this already, I am fine with the code as is.
>

It's consistent with the documented contract of the in_range support
functions. See https://www.postgresql.org/docs/current/btree-support-funcs.html

In particular, this part:

   An additional expectation is that in_range functions should, if
   practical, avoid throwing an error if base + offset or base - offset
   would overflow. The correct comparison result can be determined even
   if that value would be out of the data type's range. Note that if the
   data type includes concepts such as "infinity" or "NaN", extra
   care may be needed to ensure that in_range's results agree with the
   normal sort order of the operator family.

> Added a separate patch (0009) to fix
> brin_minmax_multi_distance_interval().
>

I think we can drop this from this thread now, given the discussion
over on the other thread
(https://www.postgresql.org/message-id/eef0ea8c-4aaa-8d0d-027f-58b1f35dd170%40enterprisedb.com)

> I have added a separate patch (0008) to test negative interval values,
> including -infinity, in preceding and following specification.
>
> Patches from 0001 to 0007 are same as what you attached but rebased on
> the latest HEAD.
>

I'm attaching another update, with a minor change to the aggregate
deserialization function, in line with the recent change to how these
now work elsewhere (see 0c882a298881056176a27ccc44c5c3bb7c8f308c).

0008 seems reasonable. I have added some comments to indicate that
those tests are expected to fail, and why.

> I think we should squash 0002 to 0007.
>

Yes, let's do that with the next update. In fact, we may as well
squash 0002 to 0008.

Regards,
Dean

On Fri, Oct 27, 2023 at 2:07 PM Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
>
> On Wed, 4 Oct 2023 at 14:29, Ashutosh Bapat
> <ashutosh.bapat.oss@gmail.com> wrote:
> >
> > I think we should introduce interval_out_of_range_error() function on
> > the lines of float_overflow_error(). Later patches introduce more
> > places where we raise that error. We can introduce the function as
> > part of those patches.
> >
>
> I'm not convinced that it is really worth it. Note also that even with
> this patch, there are still more places that throw "timestamp out of
> range" errors than "interval out of range" errors.

Fine with me.

>
> > > 4. I tested pg_upgrade on a table with an interval with INT_MAX
> > > months, and it was silently converted to infinity. I think that's
> > > probably the best outcome (better than failing). However, this means
> > > that we really should require all 3 fields of an interval to be
> > > INT_MIN/MAX for it to be considered infinite, otherwise it would be
> > > possible to have multiple internal representations of infinity that do
> > > not compare as equal.
> > >
> > My first patch was comparing all the three fields to determine whether
> > a given Interval value represents infinity. [3] changed that to use
> > only the month field. I guess that was based on the discussion at [4].
> > You may want to review that discussion if not already done. I am fine
> > either way. We should be able to change the comparison code later if
> > we see performance getting impacted.
> >
>
> Before looking at the details more closely, I might have agreed with
> that earlier discussion. However, given that things like pg_upgrade
> have the possibility of turning formerly allowed, finite intervals
> into infinity, we really need to ensure that there is only one value
> equal to infinity, otherwise the results are likely to be very
> confusing and counter-intuitive. That means that we have to continue
> to regard intervals like INT32_MAX months + 10 days as finite.
>
> While I haven't done any performance testing, I wouldn't expect this
> to have much impact. In a 64-bit build, this actually generates 2
> comparisons rather than 3 -- one comparing the combined month and day
> fields against a 64-bit value containing 2 copies of INT32_MAX, and
> one testing the time field. In practice, only the first test will be
> executed in the vast majority of cases.
>

Thanks for the analysis.

>
> Something that perhaps does need discussing is the fact that
> '2147483647 months 2147483647 days 9223372036854775807 usecs' is now
> accepted by interval_in() and gives infinity. That's a bit ugly, but I
> think it's defensible as a measure to prevent dump/restore errors from
> older databases, and in any case, such an interval is outside the
> documented range of supported intervals, and is a highly contrived
> example, vanishingly improbable in practice.

Agreed.

>
> Alternatively, we could have interval_in() reject this, which would
> open up the possibility of dump/restore errors. It could be argued
> that that's OK, for similar reasons -- the failing value is highly
> unlikely/contrived, and out of the documented range. I don't like that
> though. I don't think dump/restore should fail under any
> circumstances, however unlikely.

I agree that dump/restore shouldn't fail, especially when restore on
one major version succeeds and fails on another.

>
> Another alternative is to accept this input, but emit a WARNING. I
> don't particularly like that either, since it's forcing a check on
> every input value, just to cater for this one specific highly unlikely
> input. In fact, both these alternative approaches (rejecting the
> value, or emitting a warning), would impose a small performance
> penalty on every interval input, which I don't think is really worth
> it.

Agreed.

>
> So overall, my preference is to just accept it. Anything else is more
> work, for no practical benefit.
>

Ok.

--
Best Wishes,
Ashutosh Bapat

On Fri, 27 Oct 2023 at 09:38, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
>
> On Tue, 10 Oct 2023 at 12:36, Ashutosh Bapat
>
> > I think we should squash 0002 to 0007.
>
> Yes, let's do that with the next update. In fact, we may as well
> squash 0002 to 0008.
>

I have pushed 0001. Here is 0002-0008, squashed down to one commit,
plus the change discussed to use INTERVAL_NOBEGIN() in the btree_gin
code.

It could use another read-through, and then I think it will be ready for commit.

Regards,
Dean

On Sun, Oct 29, 2023 at 10:09 PM Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
>
> On Fri, 27 Oct 2023 at 09:38, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
> >
> > On Tue, 10 Oct 2023 at 12:36, Ashutosh Bapat
> >
> > > I think we should squash 0002 to 0007.
> >
> > Yes, let's do that with the next update. In fact, we may as well
> > squash 0002 to 0008.
> >
>
> I have pushed 0001. Here is 0002-0008, squashed down to one commit,
> plus the change discussed to use INTERVAL_NOBEGIN() in the btree_gin
> code.

Thanks. I had to leave this halfway on Friday because of severe tooth ache.

I was actually working on the test part of 0009. Though the code
changes are not required, I think it's better to have a test case
along the same lines as the tests added by Tomas (now committed in
8da86d62a11269e926765c0d6ef6f532b2b8b749).  I have attached 0002 for
the same along with your v28.

>
> It could use another read-through, and then I think it will be ready for commit.
>
Thanks. I went through the whole patch again and am quite fine with it.

Here's my version of commit message

```
Support Infinite interval values

Interval datatype uses the same input and output representation for
infinite intervals as other datatypes representing time that support
infinity. An interval larger than any other interval is represented by
string literal 'infinity' or '+infinity'. An interval which is smaller
than any other interval is represented as '-infinity'. Internally
positive infinity is represented as maximum values supported by all
the member types of Interval datastructure and negative infinity is
represented as minimum values set to all the members. INTERVAL_NOBEGIN
and INTERVAL_NOEND macros can be used to set an Interval structure to
negative and positive infinity respectively. INTERVAL_IS_NOBEGIN and
INTERVAL_IS_NOEND macros are used to test respective values.
INTERVAL_NOT_FINITE macro is used to test whether a given Interval
value is infinite.

Implementation of all known operators now handles infinite interval
values along with operations related to BRIN index, windowing and
selectivity. Regression tests are added to test these implementation.

If a user has stored interval values '-2147483648 months -2147483648
days -9223372036854775807 us' and '2147483647 months 2147483647 days
9223372036854775806 us' in PostgreSQL versions 16 or earlier. Those
values will turn into '-infinity' and 'infinity' respectively after
upgrading to v17. These values are outside the documented range
supported by interval datatype and thus there's almost no possibility
of this occurrence. But it will be good to watch for these values
during upgrade.
```

--
Best Wishes,
Ashutosh Bapat

On Mon, Oct 30, 2023 at 6:01 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:
>
>
> Here's my version of commit message
>
> ```
> Support Infinite interval values
>
> Interval datatype uses the same input and output representation for
> infinite intervals as other datatypes representing time that support
> infinity. An interval larger than any other interval is represented by
> string literal 'infinity' or '+infinity'. An interval which is smaller
> than any other interval is represented as '-infinity'. Internally
> positive infinity is represented as maximum values supported by all
> the member types of Interval datastructure and negative infinity is
> represented as minimum values set to all the members. INTERVAL_NOBEGIN
> and INTERVAL_NOEND macros can be used to set an Interval structure to
> negative and positive infinity respectively. INTERVAL_IS_NOBEGIN and
> INTERVAL_IS_NOEND macros are used to test respective values.
> INTERVAL_NOT_FINITE macro is used to test whether a given Interval
> value is infinite.
>
> Implementation of all known operators now handles infinite interval
> values along with operations related to BRIN index, windowing and
> selectivity. Regression tests are added to test these implementation.
>
> If a user has stored interval values '-2147483648 months -2147483648
> days -9223372036854775807 us' and '2147483647 months 2147483647 days
> 9223372036854775806 us' in PostgreSQL versions 16 or earlier. Those
> values will turn into '-infinity' and 'infinity' respectively after
> upgrading to v17. These values are outside the documented range
> supported by interval datatype and thus there's almost no possibility
> of this occurrence. But it will be good to watch for these values
> during upgrade.
> ```
>
the message is plain enough. I can understand it. thanks!

On Mon, 30 Oct 2023 at 10:01, Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:
>
> Thanks. I went through the whole patch again and am quite fine with it.
>
> Here's my version of commit message
>

Going over this again, I noticed a pre-existing integer overflow
problem in interval_time(), which patch 0001 attached fixes.

I squashed the other 2 patches (main patch + new BRIN tests) together
into 0002, and did another copy-editing pass over it. I found one
other issue, which is that overflow checking in interval_um() had gone
missing, and there didn't seem to be any regression test coverage for
that, so I added some.

I also changed the error message in interval_time to "cannot convert
infinite interval to time", which is slightly more informative, and
more consistent with the nearby error messages in time_pl_interval()
and time_mi_interval().

Finally, I rewrote the commit message in slightly higher-level terms,
but that's really up to the committer to decide on.

I'm marking this as ready-for-committer. I'll probably pick it up
myself in a few days, unless another committer claims it first.

Regards,
Dean

On Mon, 6 Nov 2023 at 17:08, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
>
> I'm marking this as ready-for-committer. I'll probably pick it up
> myself in a few days, unless another committer claims it first.
>

Ah, it seems that one of this patch's new OIDs conflicts with a recent
commit. The best way to avoid that (or at least make it much less
likely) is by using the suggestion at the end of the unused_oids
script output, which is a random value in the 8000-9999 range.

New version attached doing that, to run it past the cfbot again.

Regards,
Dean

On Tue, 7 Nov 2023 at 14:33, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
>
> New version attached doing that, to run it past the cfbot again.
>

Ah, Windows Server didn't like that. Trying again with "INT64CONST(0)"
instead of just "0" in interval_um().

Regards,
Dean

On Wed, Nov 8, 2023 at 7:42 AM Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
>
> On Tue, 7 Nov 2023 at 14:33, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
> >
> > New version attached doing that, to run it past the cfbot again.
> >
>
> Ah, Windows Server didn't like that. Trying again with "INT64CONST(0)"
> instead of just "0" in interval_um().
>
> Regards,
> Dean

I found this:
https://developercommunity.visualstudio.com/t/please-implement-integer-overflow-detection/409051
maybe related.

On Wed, 8 Nov 2023 at 06:56, jian he <jian.universality@gmail.com> wrote:
>
> > On Tue, 7 Nov 2023 at 14:33, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
> >
> > Ah, Windows Server didn't like that. Trying again with "INT64CONST(0)"
> > instead of just "0" in interval_um().
> >
> I found this:
> https://developercommunity.visualstudio.com/t/please-implement-integer-overflow-detection/409051
> maybe related.

Hmm, actually, this has revealed a bug in our 64-bit integer
subtraction code, on platforms that don't have builtins or 128-bit
integer support. I have created a new thread for that, since it's
nothing to do with this patch.

Regards,
Dean

On Wed, Nov 8, 2023 at 5:32 PM Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
>
> On Wed, 8 Nov 2023 at 06:56, jian he <jian.universality@gmail.com> wrote:
> >
> > > On Tue, 7 Nov 2023 at 14:33, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
> > >
> > > Ah, Windows Server didn't like that. Trying again with "INT64CONST(0)"
> > > instead of just "0" in interval_um().
> > >
> > I found this:
> > https://developercommunity.visualstudio.com/t/please-implement-integer-overflow-detection/409051
> > maybe related.
>
> Hmm, actually, this has revealed a bug in our 64-bit integer
> subtraction code, on platforms that don't have builtins or 128-bit
> integer support. I have created a new thread for that, since it's
> nothing to do with this patch.

Just to test whether that bug fix also fixes the failure seen with
this patchset, I am attaching the patchset including the patch with
the fix.

0001 - fix in other thread
0002 and 0003 are 0001 and 0002 in the previous patch set.

--
Best Wishes,
Ashutosh Bapat

On Thu, 9 Nov 2023 at 07:15, Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:
>
> Just to test whether that bug fix also fixes the failure seen with
> this patchset, I am attaching the patchset including the patch with
> the fix.
>
> 0001 - fix in other thread
> 0002 and 0003 are 0001 and 0002 in the previous patch set.
>

Thanks. That's confirmed, it has indeed turned green!

Regards,
Dean

On Thu, 9 Nov 2023 at 08:37, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
>
> On Thu, 9 Nov 2023 at 07:15, Ashutosh Bapat
> <ashutosh.bapat.oss@gmail.com> wrote:
> >
> > Just to test whether that bug fix also fixes the failure seen with
> > this patchset, I am attaching the patchset including the patch with
> > the fix.
> >
> > 0001 - fix in other thread
> > 0002 and 0003 are 0001 and 0002 in the previous patch set.
>
> Thanks. That's confirmed, it has indeed turned green!
>

OK, I have pushed 0001 and 0002. Here's the remaining (main) patch.

I couldn't resist making one more cosmetic change -- I moved
finite_interval_pl() and finite_interval_mi() up to where they're
first used, which is where that code was originally, making it
slightly easier to compare old-vs-new code side-by-side, and because I
think that's the more natural place for them.

Regards,
Dean

On Thu, 9 Nov 2023 at 12:49, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
>
> OK, I have pushed 0001 and 0002. Here's the remaining (main) patch.
>

OK, I have now pushed the main patch.

Regards,
Dean

On Tue, Nov 14, 2023 at 4:39 PM Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
>
> On Thu, 9 Nov 2023 at 12:49, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
> >
> > OK, I have pushed 0001 and 0002. Here's the remaining (main) patch.
> >
>
> OK, I have now pushed the main patch.

Thanks a lot Dean.


--
Best Wishes,
Ashutosh Bapat