Re: Have I found an interval arithmetic bug?

From: Bryn Llewellyn <bryn(at)yugabyte(dot)com>
To: Tom Lane PostgreSQL <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com>, Bruce Momjian <bruce(at)momjian(dot)us>, Zhihong Yu <zyu(at)yugabyte(dot)com>, Daniel Gustafsson <daniel(at)yesql(dot)se>, Justin Pryzby <pryzby(at)telsasoft(dot)com>, Isaac Morland <isaac(dot)morland(at)gmail(dot)com>, John W Higgins <wishdev(at)gmail(dot)com>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Have I found an interval arithmetic bug?
Date: 2021-07-21 17:18:34
Message-ID: CEA45CB6-7C0B-4960-9913-BD009DD04533@yugabyte.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-general pgsql-hackers

> On 21-Jul-2021, at 02:58, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> writes:
>> Hmm, looking at this whole thread, I have to say that I prefer the old
>> behaviour of spilling down to lower units.
>
>> For example, with this patch:
>
>> SELECT '0.5 weeks'::interval;
>> interval
>> ----------
>> 4 days
>
>> which I don't think is really an improvement. My expectation is that
>> half a week is 3.5 days, and I prefer what it used to return, namely
>> '3 days 12:00:00'.
>
> Yeah, that is clearly a significant dis-improvement.
>
> In general, considering that (most of?) the existing behavior has stood
> for decades, I think we need to tread VERY carefully about changing it.
> I don't want to see this patch changing any case that is not indisputably
> broken.

It was me that started the enormous thread with the title “Have I found an interval arithmetic bug?” on 01-Apr-2021. I presented this testcase:

> select interval '-1.7 years'; -- -1 years -8 mons
>
> select interval '29.4 months'; -- 2 years 5 mons 12 days
>
> select interval '-1.7 years 29.4 months'; -- 8 mons 12 days << wrong
> select interval '29.4 months -1.7 years'; -- 9 mons 12 days
>
> select interval '-1.7 years' + interval '29.4 months'; -- 9 mons 12 days
> select interval '29.4 months' + interval '-1.7 years'; -- 9 mons 12 days

The consensus was that the outcome that I flagged with “wrong” does indeed have that status. After all, it’s hard to see how anybody could intend this rule (that anyway holds in only some cases):

-a + b <> b - a

It seems odd that there’s been no recent reference to my testcase and how it behaves in the environment of Bruce’s patch.

I don’t recall the history of the thread. But Bruce took on the task of fixing this narrow issue. Anyway, somehow, the whole question of “spill down” came up for discussion. The rules aren’t documented and I’ve been unable to find any reference even to the phenomenon. I have managed to implement a model, in PL/pgSQL, that gets the same results as the native implementation in every one of many tests that I’ve done. I appreciate that this doesn’t prove that my model is correct. But it would seem that it must be on the right track. The rules that my PL/pgSQL uses are self-evidently whimsical—but they were needed precisely to get the same outcomes as the native implementation. There was some discussion of all this somewhere in this thread.

If memory serves, it was Tom who suggested changing the spill-down rules. This was possibly meant entirely rhetorically. But it seems that Bruce did set about implementing a change here. (I was unable to find a clear prose functional spec for the new behavior. Probably I didn’t know where to look.

There’s no doubt that a change in these rules would change the behavior of extant code. But then, in a purist sense, this is the case with any bug fix.

I’m simply waiting on a final ruling and final outcome.

Meanwhile, I’ve worked out a way to tame all this business (by using domain types and associated functionality) so that application code can deal confidently with only pure months, pure days, and pure seconds interval values (thinking of the internal [mm, dd, ss] representation). The scheme ensures that spill-down never occurs by rounding the years or the months field to integral values. If you want to add a “mixed” interval to a timestamp, then you simply add the different kinds of interval in the one expression. And you use parentheses to assert, visibly, the priority rule that you intend.

Because this is ordinary application code, there are no compatibility issues for me. My approach won’t see a change in behavior no matter what is decided about the present patch.

In response to

Responses

Browse pgsql-general by date

  From Date Subject
Next Message Nicolas Seinlet 2021-07-21 17:25:43 Re: More records after sort
Previous Message Ninad Shah 2021-07-21 14:10:42 Re: Obsolete or dead serverconnections after reboot

Browse pgsql-hackers by date

  From Date Subject
Next Message vignesh C 2021-07-21 17:26:04 Re: Printing backtrace of postgres processes
Previous Message Tom Lane 2021-07-21 16:49:16 Re: Case expression pushdown