PL/pgSQL — "commit" illegal in the executable section of a block statement that has an exception section

[Bryn Llewellyn]

I work for YugaByte, Inc (www.yugabyte.com ). 
YugabyteDB re-uses the source code that implements the “upper half” of 
PostgreSQL Version 11.2. See here:

https://blog.yugabyte.com/distributed-postgresql-on-a-google-spanner-architecture-query-layer/

This means that the problem that the PostgreSQL issue I describe tracks affects 
users of YugabyteDB too.

I wrote up the problem here:

https://github.com/yugabyte/yugabyte-db/issues/2464 


Please read my account and comment.

Peter Eisentraut, I hope that you’ll read this. I’m told that you're the 
authority for txn control in PL/pgSQL.

Cannot commit or rollback in “security definer” PL/pgSQL proc

[Bryn Llewellyn]

Here’s a cut-down version of Umair Shahid’s blog post here:

https://www.2ndquadrant.com/en/blog/postgresql-11-server-side-procedures-part-1/
 

__

create table t(k int primary key, v int not null);

create or replace procedure p()
  language plpgsql
  security invoker
as $$
begin
  insert into t(k, v) values(1, 17);
  rollback;
  insert into t(k, v) values(1, 42);
  commit;
end
$$;

call p();
select * from t order by k;
__

It runs without error and shows that the effect of “rollback” and “commit” is 
what the names of those statements tells you to expect.

The post starts with “Thanks to the work done by 2ndQuadrant contributors, we 
now have the ability to write Stored Procedures in PostgreSQL… [with] 
transaction control – allowing us to COMMIT and ROLLBACK inside procedures.”. I 
believe that Umair is referring to work done by Peter Eisentraut.

But simply change “security invoker” to “security definer” and rerun the test. 
You get the notorious error “2D000: invalid transaction termination”.

Please tell me that this is a plain bug—and not the intended semantics.

Re: jsonb_object() seems to be buggy. jsonb_build_object() is good.

[Bryn Llewellyn]

Bryn Llewellyn wrote:

> ...I wrote my own wrapper for jsonb_build_array()
> and jsonb_build_object():
> 
> create function my_jsonb_build(
>   kind in varchar,
>   variadic_elements in varchar)
>   returns jsonb
>   immutable
>   language plpgsql
> as $body$
> declare
>   stmt varchar :=
> case kind
>  when 'array' then
>'select jsonb_build_array('||variadic_elements||')'
>  when 'object' then
>'select jsonb_build_object('||variadic_elements||')'
> end;
>   j jsonb;
> begin
>   execute stmt into j;
>   return j;
> end;
> $body$;
> 

Andrew replied

Please don't top-post on PostgreSQL lists.  See
<http://idallen.com/topposting.html>

The function above has many deficiencies, including lack of error
checking and use of 'execute' which will significantly affect
performance. Still, if it works for you, that's your affair.

These functions were written to accommodate PostgreSQL limitations. We
don't have a heterogenous array type.  So json_object() will return an
object where all the values are strings, even if they look like numbers,
booleans etc. And indeed, this is shown in the documented examples.
jsonb_build_object and jsonb_build_array overcome that issue, but there
the PostgreSQL limitation is that you can't pass in an actual array as
the variadic element, again because we don't have heterogenous arrays.

Bryn replies:

Ah… I didn’t know about the bottom-posting rule.

Of course I didn’t show error handling. Doing so would have increased the 
source text size and made it harder to appreciate the point.

I used dynamic SQL because I was modeling the use case where on-the-fly 
analysis determines what JSON object or array must be built—i.e. the number of 
components and the datatype of each. It’s nice that jsonb_build_object() and 
jsonb_build_array() accommodate this dynamic need by being variadic. But I 
can’t see a way to wrote the invocation using only static code.

What am I missing?

Re: jsonb_object() seems to be buggy. jsonb_build_object() is good.

[Bryn Llewellyn]

Andrew Dunstan  wrote:

Bryn Llewellyn wrote:
> 
> Andrew replied
> 
> The function above has many deficiencies, including lack of error
> checking and use of 'execute' which will significantly affect
> performance. Still, if it works for you, that's your affair.
> 
> These functions were written to accommodate PostgreSQL limitations. We
> don't have a heterogenous array type.  So json_object() will return an
> object where all the values are strings, even if they look like numbers,
> booleans etc. And indeed, this is shown in the documented examples.
> jsonb_build_object and jsonb_build_array overcome that issue, but there
> the PostgreSQL limitation is that you can't pass in an actual array as
> the variadic element, again because we don't have heterogenous arrays.
> 
> Bryn replies:
> 
> 
> Of course I didn’t show error handling. Doing so would have increased the 
> source text size and made it harder to appreciate the point.
> 
> I used dynamic SQL because I was modeling the use case where on-the-fly 
> analysis determines what JSON object or array must be built—i.e. the number 
> of components and the datatype of each. It’s nice that jsonb_build_object() 
> and jsonb_build_array() accommodate this dynamic need by being variadic. But 
> I can’t see a way to wrote the invocation using only static code.
> 
> What am I missing?



Probably not much, These functions work best from application code which
builds up the query. But if you do that and then call a function which
in turn calls execute you get a double whammy of interpreter overhead.
I'm also not a fan of functions that in effect take bits of SQL text and
interpolate them into a query in plpgsql, like your query does.


json_object() is meant to be an analog of the hstore() function that
takes one or two text arrays and return an hstore. Of course, it doesn't
have the issue you complained about, since all values in an hstore are
strings.

Bryn replied:

We don’t yet support the hstore() function in YugabyteDB. So, meanwhile, I see 
no alternative to the approach that I illustrated—whatever that implies for 
doing things of which you’re not a fan. That’s why I asked “ What am I 
missing?”. But your “ Probably not much” seems, then, to force my hand.

B.t.w., you earlier said “The double quotes  [around “dog”] serve a specific 
purpose, to allow values containing commas to be treated as a single value (see 
syntax details for the exact rules) in the resulting array of text values.” But 
this test shows that they are not needed for that purpose:

select jsonb_pretty(jsonb_object(
  '{a, 17, b, dog house, c, true}'::varchar[]
  ))

This is the result:

 {+
 "a": "17",   +
 "b": "dog house",+
 "c": "true"  +
 }

The commas are sufficient separators.

It seems to me, therefore, that writing the double quotes gives the wrong 
message: they make it look like you are indeed specifying a text value rather 
than a numeric or integer value. But we know that the double quotes do *not* 
achieve this.

Re: jsonb_object() seems to be buggy. jsonb_build_object() is good.

[Bryn Llewellyn]

On 16-Feb-2020, at 16:40, Andrew Dunstan  wrote:

On 2/16/20 7:25 PM, Bryn Llewellyn wrote:
> 
> B.t.w., you earlier said “The double quotes  [around “dog”] serve a specific 
> purpose, to allow values containing commas to be treated as a single value 
> (see syntax details for the exact rules) in the resulting array of text 
> values.” But this test shows that they are not needed for that purpose:


I didn't say that. Someone else did.


> 
> select jsonb_pretty(jsonb_object(
>  '{a, 17, b, dog house, c, true}'::varchar[]
>  ))
> 
> This is the result:
> 
> {+
> "a": "17",   +
> "b": "dog house",+
> "c": "true"  +
> }
> 
> The commas are sufficient separators.
> 
> It seems to me, therefore, that writing the double quotes gives the wrong 
> message: they make it look like you are indeed specifying a text value rather 
> than a numeric or integer value. But we know that the double quotes do *not* 
> achieve this.
> 


No, you haven't understood what they said. If the field value contains a
comma it needs to be quoted. But none of the fields in your example do.
If your field were "dog,house" instead of "dog house" it would need to
be quoted. This had nothing to do with json, BTW, it's simply from the
rules for array literals.

Bryn replied:

Got it! Thanks for helping me out, Andrew.

jsonb_object() seems to be buggy. jsonb_build_object() is good.

[Bryn Llewellyn]

Execute this:

select jsonb_pretty(jsonb_build_object(
  'a'::varchar, 1.7::numeric,
  'b'::varchar, 'dog'::varchar,
  'c'::varchar, true::boolean
  ))

It produces the result that I expect:

 {  +
 "a": 1.7,  +
 "b": "dog",+
 "c": true  +
 }

Notice that the numeric, text, and boolean primitive values are properly 
rendered with the text value double-quoted and the numeric and boolean values 
unquoted.

Now execute this supposed functional equivalent:

select jsonb_pretty(jsonb_object(
  '{a, 17, b, "dog", c, true}'::varchar[]
  ))

It is meant to be a nice alternative when you want to build an object (rather 
than an array) because the syntax is less verbose.

However, it gets the wrong answer, thus:

 {  +
 "a": "17", +
 "b": "dog",+
 "c": "true"+
 }

Now, the numeric value and the boolean value are double-quoted—in other words, 
they have been implicitly converted to JSON primitive text values.

Do you agree that this is a bug?

Notice that I see this behavior in vanilla PostgreSQL 11.2 and in YugabyteDB 
Version 2.0.11.0. See this blogpost:

“Distributed PostgreSQL on a Google Spanner Architecture—Query Layer”
https://blog.yugabyte.com/distributed-postgresql-on-a-google-spanner-architecture-query-layer/

YugabyteDB uses the PostgreSQL source code for its SQL upper half.

Regards, Bryn Llewellyn, Yugabyte

Re: jsonb_object() seems to be buggy. jsonb_build_object() is good.

[Bryn Llewellyn]

Thank you both, Vik, and David, for bing so quick to respond. All is clear now. 
It seems to me that the price (giving up the ability to say explicitly what 
primitive JSON values you want) is too great to pay for the benefit (being able 
to build the semantic equivalent of a variadic list of actual arguments as text.

So I wrote my own wrapper for jsonb_build_array() and jsonb_build_object():

create function my_jsonb_build(
  kind in varchar,
  variadic_elements in varchar)
  returns jsonb
  immutable
  language plpgsql
as $body$
declare
  stmt varchar :=
case kind
 when 'array' then
   'select jsonb_build_array('||variadic_elements||')'
 when 'object' then
   'select jsonb_build_object('||variadic_elements||')'
end;
  j jsonb;
begin
  execute stmt into j;
  return j;
end;
$body$;

create type t1 as(a int, b varchar);

———
— Test it.

select jsonb_pretty(my_jsonb_build(
  'array',
  $$
17::integer, 'dog'::varchar, true::boolean
  $$));

select jsonb_pretty(my_jsonb_build(
  'array',
  $$
17::integer,
'dog'::varchar,
true::boolean,
(17::int, 'dog'::varchar)::t1
  $$));

select jsonb_pretty(my_jsonb_build(
  'object',
  $$
'a'::varchar,  17::integer,
'b'::varchar,  'dog'::varchar,
'c'::varchar,  true::boolean
  $$));

It produces the result that I want. And I’m prepared to pay the price of using 
$$ to avoid doubling up interior single quotes..

On 14-Feb-2020, at 19:24, David G. Johnston  wrote:

On Friday, February 14, 2020, Bryn Llewellyn mailto:b...@yugabyte.com>> wrote:

select jsonb_pretty(jsonb_object(
 '{a, 17, b, "dog", c, true}'::varchar[]
 ))

In other words, do the double quotes around "dog" have no effect? That would be 
a bad thing—and it would limit the usefulness of the jsonb_object() function.

The double quotes serve a specific purpose, to allow values containing commas 
to be treated as a single value (see syntax details for the exact rules) in the 
resulting array of text values.  The fact you don’t have to quote the other 
strings is a convenience behavior of the feature.

David J.

Re: jsonb_object() seems to be buggy. jsonb_build_object() is good.

[Bryn Llewellyn]

This:

select jsonb_pretty(jsonb_build_object(
 'a'::varchar, 1.7::numeric,
 'b'::varchar, 'dog'::varchar,
 'c'::varchar, true::boolean
 ))

allows me to express what I want. That’s a good thing. Are you saying that this:

select jsonb_pretty(jsonb_object(
 '{a, 17, b, "dog", c, true}'::varchar[]
 ))

simply lacks that power of expression and that every item in the array is 
assumed to be intended to end up as a JSON text primitive value? In other 
words, do the double quotes around "dog" have no effect? That would be a bad 
thing—and it would limit the usefulness of the jsonb_object() function.

The doc (“Builds a JSON object out of a text array.”) is simply too terse to 
inform an answer to this question.

On 14-Feb-2020, at 18:28, Vik Fearing  wrote:

On 15/02/2020 03:21, Bryn Llewellyn wrote:
> Now execute this supposed functional equivalent:
> 
> select jsonb_pretty(jsonb_object(
>  '{a, 17, b, "dog", c, true}'::varchar[]
>  ))
> 
> It is meant to be a nice alternative when you want to build an object (rather 
> than an array) because the syntax is less verbose.
> 
> However, it gets the wrong answer, thus:
> 
> {  +
> "a": "17", +
> "b": "dog",+
> "c": "true"+
> }
> 
> Now, the numeric value and the boolean value are double-quoted—in other 
> words, they have been implicitly converted to JSON primitive text values.

They haven't been implicitly converted, you gave an array of varchars.
How should it know that you don't want texts?

> Do you agree that this is a bug?
No.
-- 
Vik Fearing

Syntax rules for a text value inside the literal for a user-defined type—doc section “8.16.2. Constructing Composite Values”

[Bryn Llewellyn]

I’m using PostgreSQL Version 11.2. Try this:

create type rt as (a text, b text);
create table t(k serial primary key, r rt);
insert into t(r) values
  ('("a","b")'),
  ('( "e" , "f" )'),
  ('( "g (h)" , "i, j" )');
select
  k,
  '>'||(r).a||'<' as a,
  '>'||(r).b||'<' as b
from t order by k;

This is the result.

 k | a |b 
---+---+--
 1 | >a<   | >b<
 2 | > e < | > f <
 3 | > g (h) < | > i, j <

The documentation in section “8.16.2. Constructing Composite Values” here:

https://www.postgresql.org/docs/11/rowtypes.html#ROWTYPES-IO-SYNTAX 


shows examples of using double quotes to surround a text value inside the 
literal for a user-defined record type—and it states that this is optional 
unless the text value contains a comma or a parenthesis. But in every example 
there is no case where the syntax elements (the surrounding parentheses and the 
comma separators) outside of the values themselves have space(s) on either 
side. So it gives no basis to explain the result that I show for “k=2” and 
“k=3”.

Intuition tells me that if a text value is surrounded by double quotes, then 
these delimit the string and that anything outside of this (baring the special 
case of a text value that is *not* surrounded by double quotes), then 
whitespace can be used—as it is in every other case that I can think of in 
PostgreSQL’s SQL and PL/pgSQL, at the programers discretion to improve 
readability.

This, by the way, is the rule for a JSON string value.

In fact, the rule seems to be this:

“When a text value is written inside the literal for a user-defined type (which 
data type is given by its declaration), the entire run of characters between 
the syntax elements—the opening left parenthesis, an interior comma, or the 
closing right parenthesis— is taken to be the text value, including spaces 
outside of the quotes.”

Have I stumbled on a bug? If not, please explain the rationale for what seems 
to me to be a counter-intuitive syntax design choice.


.

Re: Syntax rules for a text value inside the literal for a user-defined type—doc section “8.16.2. Constructing Composite Values”

[Bryn Llewellyn]

On 02-Apr-2020, at 19:25, Tom Lane  wrote:

Bryn Llewellyn  writes:
> The documentation in section “8.16.2. Constructing Composite Values” here:
> https://www.postgresql.org/docs/11/rowtypes.html#ROWTYPES-IO-SYNTAX 
> <https://www.postgresql.org/docs/11/rowtypes.html#ROWTYPES-IO-SYNTAX>

The authoritative documentation for that is at 8.16.6 "Composite Type
Input and Output Syntax", and it says quite clearly that whitespace is
not ignored (except for before and after the outer parentheses).

regards, tom lane


Thanks for the super-fast reply, Tom. Yes, I had read that part. I should have 
said this:

“The documentation in section “8.16.2. Constructing Composite Values” et seq 
shows examples…”

This is the text to which you refer me:

“Whitespace outside the parentheses is ignored, but within the parentheses it 
is considered part of the field value, and might or might not be significant 
depending on the input conversion rules for the field data type. For example, 
in:

'(  42)'

the whitespace will be ignored if the field type is integer, but not if it is 
text. As shown previously, when writing a composite value you can write double 
quotes around any individual field value.”

Notice the wording “double quotes around any individual field value.” The word 
“around” was the source of my confusion. For the docs to communicate what, it 
seems, they ought to, then the word should be “within”. This demonstrates my 
point:

create type rt as (a text, b text);
with v as (select '(a "b c" d, e "f,g" h)'::rt as r)
select
  '>'||(r).a||'<' as a,
  '>'||(r).b||'<' as b
from v;

It shows this:

 a | b  
---+
 >a b c d< | > e f,g h<

So these are the resulting parsed-out text values:

a b c d

and

 e f,g h

This demonstrates that, in my input, the double quotes are *within* each of the 
two text values—and definitely *do not surround* them.

I really would appreciate a reply to the second part of my earlier question:

“please explain the rationale for what seems to me to be a counter-intuitive 
syntax design choice.”

I say “counter-intuitive” because JSON had to solve the same high-level goal—to 
distinguish between a string value on the one hand and, for example, a number 
or boolean value on the other hand. They chose the rule that a string value 
*must* be surrounded by double quotes and that other values must *not* be so 
surrounded. The JSON model resonates with my intuition. It also has mechanisms 
to escape interior double quotes and other special characters. I am very 
interested to know why the PostgreSQL designers preferred their model.

Re: Syntax rules for a text value inside the literal for a user-defined type—doc section “8.16.2. Constructing Composite Values”

[Bryn Llewellyn]

On 03-Apr-2020, at 00:05, David G. Johnston  wrote:

On Thu, Apr 2, 2020 at 8:46 PM Bryn Llewellyn mailto:b...@yugabyte.com>> wrote:
On 02-Apr-2020, at 19:25, Tom Lane mailto:t...@sss.pgh.pa.us>> wrote:

Bryn Llewellyn mailto:b...@yugabyte.com>> writes:
> The documentation in section “8.16.2. Constructing Composite Values” here:
> https://www.postgresql.org/docs/11/rowtypes.html#ROWTYPES-IO-SYNTAX 
> <https://www.postgresql.org/docs/11/rowtypes.html#ROWTYPES-IO-SYNTAX> 
> <https://www.postgresql.org/docs/11/rowtypes.html#ROWTYPES-IO-SYNTAX 
> <https://www.postgresql.org/docs/11/rowtypes.html#ROWTYPES-IO-SYNTAX>>

The authoritative documentation for that is at 8.16.6 "Composite Type
Input and Output Syntax", and it says quite clearly that whitespace is
not ignored (except for before and after the outer parentheses). 
[...] 

“Whitespace outside the parentheses is ignored, but within the parentheses it 
is considered part of the field value, and might or might not be significant 
depending on the input conversion rules for the field data type. For example, 
in:

'(  42)'

the whitespace will be ignored if the field type is integer, but not if it is 
text. As shown previously, when writing a composite value you can write double 
quotes around any individual field value.”

Notice the wording “double quotes around any individual field value.” The word 
“around” was the source of my confusion. For the docs to communicate what, it 
seems, they ought to, then the word should be “within”. This demonstrates my 
point:

Actually, they do mean around (as in, the double-quotes must be adjacent to the 
commas that delimit the fields, or the parens).

The next two sentences clear things up a bit:

"You must do so if the field value would otherwise confuse the composite-value 
parser.  In particular, fields containing parentheses, commas, double quotes, 
or backslashes must be double-quoted."

That said the documentation doesn't match the behavior (which is considerably 
more forgiving and also willing to simply discard double-quotes instead of 
error-ing out when the documented rules are not adhered to)

Specifically:  '(a \"b c\" d, e \"f,g\" h)'::rt leaves the double-quote 
while '(a ""b c"" d, e ""f,g"" h)'::rt does not.  Neither have the field 
surround with double-quotes so should be invalid per the documentation.  When 
you follow the documentation they then both retain the double-quote.

So if you follow the guidelines set forth in the documentation you get the 
result the documentation promises.  If you fail to follow the guidelines you 
may still get a result but there is no promise made as to its contents.  Not 
ideal but also not likely to be changed after all this time.

create type rt as (a text, b text);
with v as (select '(a "b c" d, e "f,g" h)'::rt as r)
select
  '>'||(r).a||'<' as a,
  '>'||(r).b||'<' as b
from v;

This demonstrates that, in my input, the double quotes are *within* each of the 
two text values—and definitely *do not surround* them.

Yep, which is why you have an issue.   The "surround them" is indeed what the 
text meant to say.


I really would appreciate a reply to the second part of my earlier question:

“please explain the rationale for what seems to me to be a counter-intuitive 
syntax design choice.”
[...]
They chose the rule that a string value *must* be surrounded by double quotes 
and that other values must *not* be so surrounded. The JSON model resonates 
with my intuition.

This point wasn't answered because there is no good answer to be given.  The 
above is how someone in the mid-90s or so decided PostgreSQL should handle 
this.  I'll personally agree that more verbose but explicit, and less 
forgiving, syntax and parsing, would have been a better choice.  But the choice 
has been made and isn't subject to change.

But regardless of what drove the original design choice if you really care 
about it in a "want to learn" mode then it is very easy to observe the defined 
behavior and critique it independently of how it came to be.  If all you want 
to do is make a left-handed jab at PostgreSQL for having a non-intuitive to you 
design choice do act surprised when people don't choose to respond - especially 
when none of us made the original choice.

The only thing we can do today is describe the system more clearly if we have 
failed to do so adequately.  You are probably in the best position, then, to 
learn what it does and propose new wording that someone with inexperienced (or 
biased toward a different system) eyes would understand quickly and clearly.

David J.


Thanks for the explanation, David. It helped me a lot. You said “If all you 
want to do is make a left-handed jab at PostgreSQL”. That was not at all my 
intention. Rather, my question was driven by my experi

Re: Have I found an interval arithmetic bug?

[Bryn Llewellyn]

> On 05-Apr-2021, at 13:35, Bruce Momjian  wrote:
> 
> On Mon, Apr 5, 2021 at 01:06:36PM -0700, Bryn Llewellyn wrote:
>>> On 05-Apr-2021, at 11:37, Bruce Momjian  wrote On:
>>> Mon, Apr 5, 2021 at 01:15:22PM -0500, Justin Pryzby wrote  :
>> 
>> It seems to me that this whole business is an irrevocable mess. The
>> original design could have brought three overload-distinguishable
>> types, "interval month", "interval day", and "interval second"—each
>> represented internally as a scalar. There could have been built-ins
>> to convert between them using conventionally specified rules. Then
>> interval arithmetic would have been clear. For example, an attempt to
>> assign the difference between two timestamps to anything but "interval
>> second" would cause an error (as it does in Oracle database, even
>> though there there are only two interval kinds). But we can only deal
>> with what we have and accept the fact that the doc will inevitably be
>> tortuous.
> 
> The problem with making three data types is that someone is going to
> want to use a mixture of those, so I am not sure it is a win.
> 
>> Givea this, I agree that fractional years should simply convert to
>> fractional months (to be then added to verbetim-given fractional
>> months) just before representing the months as the trunc() of the
>> value and cascading the remainder down to days. Units like century
>> would fall out naturally in the same way.
> 
> I am confused --- are you saying we should do the interval addition,
> then truncate, because we don't do that now, and it would be hard to do.
> 
>> ABOUT LEAP SECONDS
>> 
>> Look at this (from Feb 2005):
>> 
>> « PostgreSQL does not support leap seconds
>> https://www.google.com/url?q=https://www.postgresql.org/message-id/1162319515.20050202141132@mail.r=gmail-imap=161825973900=AOvVaw0lT0Zz_HDsCrF5HrWCjplE
>> u »
>> 
>> I don't know if the title reports a state of affairs in the hope that
>> this be changed to bring such support—or whether it simply states
>> what obtains and always will. Anyway, a simple test (below) shows that
>> PG Version 13.2 doesn't honor leap seconds.
> 
> Postgres is documented as not supporting leap seconds:
> 
>   
> https://www.google.com/url?q=https://www.postgresql.org/docs/13/functions-datetime.html%23FUNCTIONS-DATETIME-EXTRACT=gmail-imap=161825973900=AOvVaw35xJBdHRIsAYVV4pTzs0wR
>   
>   timezone
>   
>   The time zone offset from UTC, measured in seconds. Positive values
>   correspond to time zones east of UTC, negative values to zones west of
>   UTC. (Technically, PostgreSQL does not use UTC because leap seconds are
>   not handled.)

Thanks for the “leap seconds not supported” link. Google’s search within site 
refused to find that for me. (Talk about well hidden).

About “ three data [interval] types” it’s too late anyway. So I’ll say no more.

Re “are you saying we should do the interval addition, then truncate, because 
we don't do that now, and it would be hard to do.” I wan’t thinking of interval 
addition at all. Simply how the three values that that make up the internal 
representation are computed from a specified interval value. Like the PL/pgSQL 
simulation I showed you in an earlier reply. I can't find that in the archive 
now. So here it is again. Sorry for the repetition.

p.yy, p.mo, p.dd, p.hh, p.mi, and p.ss are th input

m, d, and s are the internal representation

  m1   int not null := trunc(p.mo);
  m_remainder  numeric not null := p.mo - m1::numeric;
  mint not null := trunc(p.yy*12) + m1;

  d_real   numeric not null := p.dd + m_remainder*30.0;
  dint not null := floor(d_real);
  d_remainder  numeric not null := d_real - d::numeric;

  snumeric not null := d_remainder*24.0*60.0*60.0 +
   p.hh*60.0*60.0 +
   p.mi*60.0 +
   p.ss;

I have a harness to supply years, months, days, hours, minutes, and seconds 
values (like the lteral does the,) and to get them back (as "extract" gets 
them) using the actual implementation and my simulation. The two approaches 
have never disagreed using a wide range of inputs.

The algorithm that my code shows (esp with both trunc() and float() in play) is 
too hard to describe in words.

Re: Have I found an interval arithmetic bug?

[Bryn Llewellyn]

br...@momjian.us wrote:

> I have just posted a new version of the patch which I think covers all the 
> right areas.

I found the relevant email from you to pgsql-hackers here:

https://www.postgresql.org/message-id/20210402234732.GA29125%40momjian.us

You said:

> I have modified the patch to prevent partial months from creating partial 
> hours/minutes/seconds… Partial years keeps it in months, partial months takes 
> it to days, and partial days take it to hours/minutes/seconds. This seems 
> like an improvement.

I have written some PL/pgSQL code that faithfully emulates the behavior that I 
see in my present vanilla PostgreSQL Version 13.2 system in a wide range of 
tests. This is the key part:

  m1   int not null := trunc(p.mo);
  m_remainder  numeric not null := p.mo - m1::numeric;
  mint not null := trunc(p.yy*12) + m1;

  d_real   numeric not null := p.dd + m_remainder*30.0;
  dint not null := floor(d_real);
  d_remainder  numeric not null := d_real - d::numeric;

  snumeric not null := d_remainder*24.0*60.0*60.0 +
   p.hh*60.0*60.0 +
   p.mi*60.0 +
   p.ss;
begin
  return (m, d, s)::modeled_interval_t;
end;

These quantities:

p.yy, p.mo, p.dd, p.hh, p.mi, and p.ss

are the user’s parameterization. All are real numbers. Because non-integral 
values for years, months, days, hours, and minutes are allowed when you specify 
a value using the ::interval typecast, my reference doc must state the rules. I 
would have struggled to express these rules in prose—especially given the use 
both of trunc() and floor(). I would have struggled more to explain what 
requirements these rules meet.

I gather that by the time YugabyteDB has adopted your patch, my PL/pgSQL will 
no longer be a correct emulation. So I’ll re-write it then.

I intend to advise users always to constrain the values, when they specify an 
interval value explicitly, so the the years, months, days, hours, and minutes 
are integers. This is, after all, the discipline that the make_interval() 
built-in imposes. So I might recommend using only that.

Re: Have I found an interval arithmetic bug?

[Bryn Llewellyn]

> On 08-Apr-2021, at 10:24, Bruce Momjian  wrote:
> 
> On Mon, Apr  5, 2021 at 02:01:58PM -0400, Bruce Momjian wrote:
>> On Mon, Apr  5, 2021 at 11:33:10AM -0500, Justin Pryzby wrote:
>> Well, bug or not, we are not going to change back branches for this, and
>> if you want a larger discussion, it will have to wait for PG 15.
>> 
 https://www.google.com/url?q=https://www.postgresql.org/docs/current/datatype-datetime.html%23DATATYPE-INTERVAL-INPUT=gmail-imap=161850748900=AOvVaw2h2TNbK7O41zsDn8HfD88C
 « …field values can have fractional parts; for example '1.5 week' or 
 '01:02:03.45'. Such input is converted to the appropriate number of 
 months, days, and seconds for storage. When this would result in a 
 fractional number of months or days, the fraction is added to the 
 lower-order fields using the conversion factors 1 month = 30 days and 1 
 day = 24 hours. For example, '1.5 month' becomes 1 month and 15 days. Only 
 seconds will ever be shown as fractional on output. »
>> 
>> I see that.  What is not clear here is how far we flow down.  I was
>> looking at adding documentation or regression tests for that, but was
>> unsure.  I adjusted the docs slightly in the attached patch.
> 
> Here is an updated patch, which will be for PG 15.  It updates the
> documentation to state:
> 
>   The fractional parts are used to compute appropriate values for the next
>   lower-order internal fields (months, days, seconds).
> 
> It removes the flow from fractional months/weeks to
> hours-minutes-seconds, and adds missing rounding for fractional
> computations.

Thank you Bruce. I look forward to documenting this new algorithm for 
YugabyteDB. The algorithm implements the transformation from this:

[
  yy_in numeric,
  mo_in numeric,
  dd_in numeric,
  hh_in numeric,
  mi_in numeric,
  ss_in numeric
]

to this:

[
  mo_internal_representation int,
  dd_internal_representation int,
  ss_internal_representation numeric(1000,6)
]

I am convinced that a prose account of the algorithm, by itself, is not the 
best way to tell the reader the rules that the algorithm implements. Rather, 
psuedocode is needed. I mentioned before that, better still, is actual 
executable PL/pgSQL code. (I can expect readers to be fluent in PL/pgSQL.) 
Given this executable simulation, an informal prose sketch of what it does will 
definitely add value.

May I ask you to fill in the body of this stub by translating the C that you 
have in hand?

create type internal_representation_t as(
  mo_internal_representation int,
  dd_internal_representation int,
  ss_internal_representation numeric(1000,6));

create function internal_representation(
  yy_in numeric default 0,
  mo_in numeric default 0,
  dd_in numeric default 0,
  hh_in numeric default 0,
  mi_in numeric default 0,
  ss_in numeric default 0)
  returns internal_representation_t
  language plpgsql
as $body$
declare
  mo_internal_representation  int not null := 0;
  dd_internal_representation  int not null := 0;
  ss_internal_representation  numeric not null := 0.0;

  ok constant boolean :=
(yy_in is not null) and
(mo_in is not null) and
(dd_in is not null) and
(hh_in is not null) and
(mi_in is not null) and
(ss_in is not null);
begin
  assert ok, 'No actual argument, when provided, may be null';

  -- The algorithm.

  return (mo_internal_representation, dd_internal_representation, 
ss_internal_representation)::internal_representation_t;
end;
$body$;

By the way, I believe that a user might well decide always to supply all the 
fields in a "from text to interval" typecast, except for the seconds, as 
integral values. This, after all, is what the "make_interval()" function 
enforces. But, because the typecast approach allows non-integral values, the 
reference documentation must explain the rules unambiguously so that the reader 
can predict the outcome of any ad hoc test that they might try.

It's a huge pity that the three values of the internal representation cannot be 
observed directly using SQL because each behaves with different semantics when 
an interval value is added to a timestamptz value. However, as a second best 
(and knowing the algorithm above), a user can create interval values where only 
one of the three fields is populated and test their understanding of the 
semantic rules that way.

Re: Have I found an interval arithmetic bug?

[Bryn Llewellyn]

br...@momjian.us wrote:
> 
> z...@yugabyte.com wrote:
>> Among previous examples given by Bryn, the following produces correct result 
>> based on Bruce's patch.
>> 
>> # select interval '-1.7 years 29.4 months';
>> interval
>> 
>>  9 mons 12 days
> 
> Yes, that changed is caused by the rounding fixes, and not by the unit 
> pushdown adjustments.

I showed you all this example a long time ago:

select (
'
  3.853467 years
'::interval
  )::text as i;

This behavior is the same in the env. of Bruce’s patch as in unpatched PG 13.2. 
This is the result.

3 years 10 mons

Notice that "3.853467 years" is "3 years" plus "10.241604 months". This 
explains the "10 mons" in the result. But the 0.241604 months remainder did not 
spill down into days.

Can anybody defend this quirk? An extension of this example with a real number 
of month in the user input is correspondingly yet more quirky. The rules can be 
written down. But they’re too tortuos to allow an ordinary mortal confidently 
to design code that relies on them.

(I was unable to find any rule statement that lets the user predict this in the 
doc. But maybe that’s because of my feeble searching skills.)

If there is no defense (and I cannot imagine one) might Bruce’s patch normalize 
this too to follow this rule:

— convert 'y years m months' to the real number y*12 + m.

— record truc( y*12 + m) in the "months" field of the internal representation

— flow the remainder down to days (but no further)

After all, you've bitten the bullet now and changed the behavior. This means 
that the semantics of some extant applications will change. So... in for a 
penny, in for a pound?

Re: Have I found an interval arithmetic bug?

[Bryn Llewellyn]

> On 12-Apr-2021, at 17:25, Bruce Momjian  wrote:
> 
> On Mon, Apr 12, 2021 at 05:20:43PM -0700, Bryn Llewellyn wrote:
>> I’d argue that the fact that this:
>> 
>> ('0.3 months'::interval) + ('0.7 months'::interval)
>> 
>> Is reported as '30 days' and not '1 month' is yet another
>> bug—precisely because of what I said in my previous email (sorry
>> that I forked the thread) where I referred to the fact that, in the
>> right test, adding 1 month gets a different answer than adding 30
>> days. 
> 
> Flowing _up_ is what these functions do:
> 
>   \df *justify*
>  List of functions
>  Schema   |   Name   | Result data type | Argument data types 
> | Type
>   
> +--+--+-+--
>pg_catalog | justify_days | interval | interval
> | func
>pg_catalog | justify_hours| interval | interval
> | func
>pg_catalog | justify_interval | interval | interval
> | func
> 
>> Yet another convincing reason to get rid of this flow down
>> business altogether.
> 
> We can certainly get rid of all downflow, which in the current patch is
> only when fractional internal units are specified.
> 
>> If some application wants to model flow-down, then it can do so with
>> trivial programming and full control over its own definition of the
>> rules.

“Yes please!” re Bruce’s “We can certainly get rid of all downflow, which in 
the current patch is only when fractional internal units are specified.”

Notice that a user who creates interval values explicitly only by using 
“make_interval()” will see no behavior change.

There’s another case of up-flow. When you subtract one timestamp value from 
another, and when they’re far enough apart, then the (internal representation 
of the) resulting interval value has both a seconds component and a days 
component. (But never, in my tests, a months component.) I assume that the 
implementation first gets the difference between the two timestamp values in 
seconds using (the moral equivalent of) “extract epoch”. And then, if this is 
greater than 24*60*60, it implements up-flow using the “rule-of-24”—never mind 
that this means that if you add the answer back to the timestamp value that you 
subtracted, then you might not get the timestamp value from which you 
subtracted. (This happens around a DST change and has been discussed earlier in 
the thread.)

The purpose of these three “justify” functions is dubious. I think that it’s 
best to think of the 3-component interval vector like an [x, y, z] vector in 3d 
geometric space, where the three coordinates are not mutually convertible 
because each has unique semantics. This point has been rehearsed many times in 
this long thread.

Having said this, it isn’t hard to understand the rules that the functions 
implement. And, most importantly, their use is voluntary. They are, though, no 
more than shipped and documented wrappers for a few special cases. A user could 
so easily write their own function like this:

1. Compute the values of the three components of the internal representation of 
the passed-in interval value using the “extract” feature and some simple 
arithmetic.

2. Derive the [minutes, days, seconds] values of a new representation using 
whatever rules you feel for.

3. Use these new values to create the return interval value.

For example, I might follow a discipline to use interval values that have only 
one of the three components of the internal representation non-zero. It’s easy 
to use the “domain” feature for this. (I can use these in any context where I 
can use the shipped interval.) I could then write a function to convert a “pure 
seconds” value of my_interval to a “pure years” value. And I could implement my 
own rules:

— Makes sense only for a large number of seconds that comes out to at least 
five years (else assert failure).

— Converts seconds to years using the rule that 1 year is, on average, 
365.25*24*60*60 seconds, and then truncates it.

There’s no shipped function that does this, and this makes me suspect that I’d 
prefer to roll my own for any serious purpose.

The existence of the three “justify” functions is, therefore, harmless.

Re: Have I found an interval arithmetic bug?

[Bryn Llewellyn]

> On 12-Apr-2021, at 17:00, Bruce Momjian  wrote:
> 
> On Mon, Apr 12, 2021 at 07:38:21PM -0400, Tom Lane wrote:
>> Bruce Momjian  writes:
>>> On Mon, Apr 12, 2021 at 03:09:48PM -0700, Bryn Llewellyn wrote:
>>>> After all, you've bitten the bullet now and changed the behavior. This 
>>>> means that the semantics of some extant applications will change. So... in 
>>>> for a penny, in for a pound?
>> 
>>> The docs now say:
>> 
>>> Field values can have fractional parts;  for example, '1.5
>>> weeks' or '01:02:03.45'.  The fractional
>>> -->  parts are used to compute appropriate values for the next lower-order
>>> internal fields (months, days, seconds).
>> 
>>> meaning fractional years flows to the next lower internal unit, months,
>>> and no further.  Fractional months would flow to days.  The idea of not
>>> flowing past the next lower-order internal field is that the
>>> approximations between units are not precise enough to flow accurately.
>> 
>> Um, what's the argument for down-converting AT ALL?  The problem is
>> precisely that any such conversion is mostly fictional.
> 
> True.
> 
>>> With my patch, the output is now:
>> 
>>> SELECT INTERVAL '3 years 10.241604 months';
>>> interval
>>> 
>>>  3 years 10 mons 7 days
>> 
>>> It used to flow to seconds.
>> 
>> Yeah, that's better than before, but I don't see any principled argument
>> for it not to be "3 years 10 months", full stop.
> 
> Well, the case was:
> 
>   SELECT INTERVAL '0.1 months';
>interval
>   --
>3 days
>   
>   SELECT INTERVAL '0.1 months' + interval '0.9 months';
>?column?
>   --
>30 days
> 
> If you always truncate, you basically lose the ability to specify
> fractional internal units, which I think is useful.  I would say if you
> use fractional units of one of the internal units, you are basically
> knowing you are asking for an approximation --- that is not true of '3.5
> years', for example.

I’d argue that the fact that this:

('0.3 months'::interval) + ('0.7 months'::interval)

Is reported as '30 days' and not '1 month' is yet another bug—precisely because 
of what I said in my previous email (sorry that I forked the thread) where I 
referred to the fact that, in the right test, adding 1 month gets a different 
answer than adding 30 days. Yet another convincing reason to get rid of this 
flow down business altogether.

If some application wants to model flow-down, then it can do so with trivial 
programming and full control over its own definition of the rules.

Re: Have I found an interval arithmetic bug?

[Bryn Llewellyn]

> t...@sss.pgh.pa.us wrote:
> 
> br...@momjian.us writes:
>> b...@yugabyte.com wrote:
>>> After all, you've bitten the bullet now and changed the behavior. This 
>>> means that the semantics of some extant applications will change. So... in 
>>> for a penny, in for a pound?
> 
>> The docs now say:
> 
>> Field values can have fractional parts;  for example, '1.5
>> weeks' or '01:02:03.45'.  The fractional
>> -->  parts are used to compute appropriate values for the next lower-order
>> internal fields (months, days, seconds).
> 
>> meaning fractional years flows to the next lower internal unit, months, and 
>> no further.  Fractional months would flow to days.  The idea of not flowing 
>> past the next lower-order internal field is that the approximations between 
>> units are not precise enough to flow accurately.
> 
> Um, what's the argument for down-converting AT ALL?  The problem is precisely 
> that any such conversion is mostly fictional.
> 
>> With my patch, the output is now:
> 
>>  SELECT INTERVAL '3 years 10.241604 months';
>>  interval
>>  
>>   3 years 10 mons 7 days
> 
>> It used to flow to seconds.
> 
> Yeah, that's better than before, but I don't see any principled argument for 
> it not to be "3 years 10 months", full stop.

Tom, I fully support your suggestion to have no flow down at all. Please may 
this happen! However, the new rule must be described in terms of the three 
fields of the internal representation: [months, days, seconds]. This 
representation is already documented.

Don’t forget that '731.42587 hours’ is read back as "731:25:33.132" (or, if you 
prefer, 731 hours, 25 minutes, and 33.132 seconds if you use "extract" and your 
own pretty print). But we don’t think of this as “flow down”. Rather, it’s just 
a conventional representation of the seconds field of the internal 
representation. I could go on. But you all know what I mean.

By the way, I made a nice little demo (for my doc project). It shows that:

(1) if you pick the right date-time just before a DST change, and do the 
addition in the right time zone, then adding 24 hours gets a different answer 
than adding one day.

(2) if you pick the right date-time just before 29-Feb in a leap year, then 
adding 30 days gets a different answer than adding one month.

You all know why. And though the doc could explain and illustrate this better, 
it does tell you to expect this. It also says that the difference in semantics 
that these examples show is the reason for the three-field internal 
representation.

It seems to me that both the age-old behavior that vanilla 13.2 exhibits, and 
the behavior in the regime of Bruce’s patch are like adding 2.2 oranges to 1.3 
oranges and getting 3 oranges and 21 apples (because 1 orange is conventionally 
the same as 42 apples). Bruce made a step in the right direction by stopping 
oranges convert all the way down to bananas. But it would be so much better to 
get rid of this false equivalence business altogether.

Re: Have I found an interval arithmetic bug?

[Bryn Llewellyn]

> On 21-Jul-2021, at 02:58, Tom Lane  wrote:
> 
> Dean Rasheed  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.

Re: Have I found an interval arithmetic bug?

[Bryn Llewellyn]

> On 21-Jul-2021, at 01:23, Dean Rasheed  wrote:
> 
> On Wed, 21 Jul 2021 at 03:48, Bruce Momjian  wrote:
>> 
>> this example now gives me concern:
>> 
>>SELECT INTERVAL '1.06 months 1 hour';
>>   interval
>>---
>> 1 mon 2 days 01:00:00
>> 
>> Notice that it rounds the '1.06 months' to '1 mon 2 days', rather than
>> spilling to hours/minutes/seconds, even though hours is already
>> specified.  I don't see a better way to handle this than the current
>> code already does, but it is something odd.
> 
> 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'.
> 
> It's true that that leads to odd-looking results when the field value
> has lots of fractional digits, but that was at least explainable, and
> followed the documentation.
> 
> Looking for a general principle to follow, how about this -- the
> result of specifying a fractional value should be the same as
> multiplying an interval of 1 unit by that value. In other words,
> '1.8594 months'::interval should be the same as '1 month'::interval *
> 1.8594. (Actually, it probably can't easily be made exactly the same
> in all cases, due to differences in the floating point computations in
> the two cases, and rounding errors, but it's hopefully not far off,
> unlike the results obtained by not spilling down to lower units on
> input.)
> 
> The cases that are broken in master, in my opinion, are the larger
> units (year and above), which don't propagate down in the same way as
> fractional months and below. So, for example, '0.7 years' should be
> 8.4 months (with the conversion factor of 1 year = 12 months), giving
> '8 months 12 days', which is what '1 year'::interval * 0.7 produces.
> Sure, there are arguably more accurate ways of computing that.
> However, that's the result obtained using the documented conversion
> factors, so it's justifiable in those terms.
> 
> It's worth noting another case that is broken in master:
> 
> SELECT '1.7 decades'::interval;
> interval
> --
> 16 years 11 mons
> 
> which is surely not what anyone would expect. The current patch fixes
> this, but it would also be fixed by handling the fractional digits for
> these units in the same way as for smaller units. There was an earlier
> patch doing that, I think, though I didn't test it.
> 
> Regards,
> Dean

And try these two tests. (I’m using Version 13.3.) on current MacOS.

select
  '1.7 decades'::interval as i1, 
  ('1 decades'::interval)*1.7 as i2,
  ('10 years'::interval)*1.7 as i3;

   i1|i2|i3
--+--+--
 16 years 11 mons | 17 years | 17 years

select
  '1.7345 decades'::interval as i4, 
  ('1 decades'::interval)*1.7345 as i5,
  ('10 years'::interval)*1.7345 as i6;

   i4|   i5|   i6   
 
-+-+-
 17 years 4 mons | 17 years 4 mons 4 days 04:48:00 | 17 years 4 mons 4 days 
04:48:00

Shows only what we know already: mixed interval arithmetic is fishy.

Seems to me that units like “weeks”, “centuries”, “millennia”, and so on are a 
solution (broken in some cases) looking for a problem. Try this (and variants 
like I showed above):

select
  '1.7345 millennia'::interval as i7,
  '1.7345 centuries'::interval as i8,
  '1.7345 weeks'::interval as i9;

i7 |i8| i9 
---+--+
 1734 years 6 mons | 173 years 5 mons | 12 days 03:23:45.6

Re: Have I found an interval arithmetic bug?

[Bryn Llewellyn]

> On 21-Jul-2021, at 17:07, Bruce Momjian  wrote:
> 
> On Wed, Jul 21, 2021 at 01:29:49PM -0400, Tom Lane wrote:
>> Bryn Llewellyn  writes:
>>> 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.
>> 
>> Yeah, I think it's self-evident that your last four cases should
>> produce the same results.  Whether '9 mons 12 days' is the best
>> possible result is debatable --- in a perfect world, maybe we'd
>> produce '9 mons' exactly --- but given that the first two cases
>> produce what they do, that does seem self-consistent.  I think
>> we should be setting out to fix that outlier without causing
>> any of the other five results to change.
> 
> OK, I decided to reverse some of the changes I was proposing once I
> started to think about the inaccuracy of not spilling down from 'weeks'
> to seconds when hours also appear.  The fundamental issue is that the
> months-to-days conversion is almost always an approximation, while the
> days to seconds conversion is almost always accurate.  This means we are
> never going to have consistent spill-down that is useful.
> 
> Therefore, I went ahead and accepted that years and larger units spill
> only to months, months spill only to days, and weeks and lower spill all
> the way down to seconds.  I also spelled this out in the docs, and
> explained why we have this behavior.
> 
> Also, with my patch, the last four queries return the same result
> because of the proper rounding also added by the patch, attached.

Your statement

“months-to-days conversion is almost always an approximation, while the days to 
seconds conversion is almost always accurate.” 

is misleading. Any conversion like these (and also the “spill up” conversions 
that the justify_hours(), justify_days(), and justify_interval() built-in 
functions bring) are semantically dangerous because of the different rules for 
adding a pure months, a pure days, or a pure seconds interval to a timestamptz 
value.

Unless you avoid mixed interval values, then it’s so hard (even though it is 
possible) to predict the outcomes of interval arithmetic. Rather, all you get 
is emergent behavior that I fail to see can be relied upon in deliberately 
designed application code. Here’s a telling example:

set timezone = 'America/Los_Angeles';
with
  c as (
select
  '2021-03-13 19:00:00 America/Los_Angeles'::timestamptz as d,
  '25 hours'::interval   as i)
select
  d +   i  as "d + i",
  d + justify_hours(i) as "d + justify_hours(i)"
from c;

This is the result:

 d + i  |  d + justify_hours(i)  
+
 2021-03-14 21:00:00-07 | 2021-03-14 20:00:00-07

The two results are different, even though the native equality test shows that 
the two different interval values are the same:

with
  c as (select '25 hours'::interval as i)
select (i = justify_hours(i))::text
from c;

The result is TRUE.

The only route to sanity is to use only pure interval values (i.e. where only 
one of the fields of the internal [mm, dd, ss] representation is non-zero.

I mentioned that you can use a set of three domain types to enforce your 
intended practice here.

In other words, by programming application code defensively, it’s possible to 
insulate oneself entirely from the emergent behavior of the decades old PG code 
that implements the unconstrained native interval functionality and that brings 
what can only be considered to be unpredictable results.

Moreover, this defensive approach insulates you from any changes that Bruce’s 
patch might make.

Re: Have I found an interval arithmetic bug?

[Bryn Llewellyn]

> On 23-Jul-2021, at 08:05, Bruce Momjian  wrote:
> 
> On Thu, Jul 22, 2021 at 03:17:52PM -0700, Zhihong Yu wrote:
>> On Thu, Jul 22, 2021 at 2:59 PM Zhihong Yu  wrote:
>>Hi,
>> 
>>-   tm->tm_mon += (fval * MONTHS_PER_YEAR);
>>+   tm->tm_mon += rint(fval * MONTHS_PER_YEAR);
>> 
>>Should the handling for year use the same check as that for month ?
>> 
>>-   AdjustFractDays(fval, tm, fsec, DAYS_PER_MONTH);
>>+   /* round to a full month? */
>>+   if (rint(fval * DAYS_PER_MONTH) == DAYS_PER_MONTH)
>> 
>>Cheers 
>> 
>> Hi,
>> I guess the reason for current patch was that year to months conversion is
>> accurate.
> 
> Our internal units are hours/days/seconds, so the spill _up_ from months
> to years happens automatically:
> 
>   SELECT INTERVAL '23.99 months';
>interval
>   --
>2 years
> 
>> On the new test:
>> 
>> +SELECT INTERVAL '1.16 months 01:00:00' AS "One mon 5 days one hour";
>> 
>> 0.16 * 31 = 4.96 < 5
>> 
>> I wonder why 5 days were chosen in the test output.
> 
> We use 30 days/month, not 31.  However, I think you are missing the
> changes in the patch and I am just understanding them fully now.  There
> are two big changes:
> 
> 1.  The amount of spill from months only to days
> 2.  The _rounding_ of the result once we stop spilling at months or days
> 
> #2 is the part I think you missed.
> 
> One thing missing from my previous patch was the handling of negative
> units, which is now handled properly in the attached patch:
> 
>   SELECT INTERVAL '-1.99 years';
>interval
>   --
>-2 years
> 
>   SELECT INTERVAL '-1.99 months';
>interval
>   --
>-2 mons
> 
> I ended up creating a function to handle this, which allowed me to
> simplify some of the surrounding code.
> 
> -- 
>  Bruce Momjian  
> https://www.google.com/url?q=https://momjian.us=gmail-imap=162765755400=AOvVaw2pMx7QBd3qSjHK1L9oUnl0
>  EDB  
> https://www.google.com/url?q=https://enterprisedb.com=gmail-imap=162765755400=AOvVaw2Q92apfhXmqqFYz7aN16YL
> 
>  If only the physical world exists, free will is an illusion.
> 
> 
> 

Will the same new spilldown rules hold in the same way for interval 
multiplication and division as they will for the interpretation of an interval 
literal?

The semantics here are (at least as far as my limited search skills have shown 
me) simply undocumented. But my tests in 13.3 have to date not disproved this 
hypothesis:

* considering "new_i ◄— i * f"

* # notice that the internal representation is _months_, days, and seconds at 
odds with "Our internal units are hours/days/seconds,"
* let i’s internal representation be [mm, dd, ss] 

* new_i’s “intermediate” internal representation is [mm*f, dd*f, ss*f]

* input these values to the same spilldown algorithm that is applied when these 
same intermediate values are used in an interval literal

* so the result is [new_mm, new_dd, new_ss]

Here’s an example:

select
  '1.2345 months 1.2345 days 1.2345 seconds'::interval = 
  '1 month 1 day 1 second'::interval*1.2345;

In 13.3, the result is TRUE. (I know that this doesn’t guarantee that the 
internal representations of the two compared interval values are the same. But 
it’s a necessary condition for the outcome that I’m referring to and serves to 
indecate the pont I’m making. A more careful test can be made.

Re: Have I found an interval arithmetic bug?

[Bryn Llewellyn]

> On 27-Jul-2021, at 14:13, Bruce Momjian  wrote:
> 
> On Tue, Jul 27, 2021 at 04:01:54PM -0400, Tom Lane wrote:
>> Bruce Momjian  writes:
>>> I went ahead and modified the interval multiplication/division functions
>>> to use the same logic as fractional interval units:
>> 
>> Wait. A. Minute.
>> 
>> What I think we have consensus on is that interval_in is doing the
>> wrong thing in a particular corner case.  I have heard nobody but
>> you suggesting that we should start undertaking behavioral changes
>> in other interval functions, and I don't believe that that's a good
>> road to start going down.  These behaviors have stood for many years.
>> Moreover, since the whole thing is by definition operating with
>> inadequate information, it is inevitable that for every case you
>> make better there will be another one you make worse.
> 
> Bryn mentioned this so I thought I would see what the result looks like.
> I am fine to skip them.
> 
>> I'm really not on board with changing anything except interval_in,
>> and even there, we had better be certain that everything we change
>> is a case that is certainly being made better.
> 
> Well, I think what I had before the multiply/divide changes were
> acceptable to everyone except Bryn, who was looking for more
> consistency.
> 
>> BTW, please do not post patches as gzipped attachments, unless
>> they're enormous.  You're just adding another step making it
>> harder for people to look at them.
> 
> OK, what is large for you?  100k bytes?  I was using 10k bytes.

Before I say anything else, I’ll stress what I wrote recently (under the 
heading “summary”). I support Tom’s idea that the only appropriate change to 
make is to fix only the exactly self-evident bug that I reported at the start 
of this thread.

I fear that Bruce doesn’t understand my point about interval multiplication 
(which includes multiplying by a number whose absolute value lies between 0 and 
1). Here it is. I believe that the semantics are (and should be) defined like 
this:

[mm, dd, ss]*n == post_spilldown([mm*n, dd*n, ss*n])

where the function post_spilldown() applies the rules that are used when an 
interval literal that specifies only values for months, days, and seconds is 
converted to the internal [mm, dd, ss] representation—where mm and dd are 
4-byte integers and ss is an 80byte integer that represents microseconds.

Here’s a simple test that’s consistent with that hypothesis:

with
  c1 as (
select
  '1 month 1 day 1 second'::interval as i1,
  '1.234 month 1.234 day 1.234 second'::interval as i3),

  c2 as (
select i1*1.234 as i2, i3 from c1)

select i2::text as i2_txt, i3::text from c2 as i3_txt;

Here’s the result:

  i2_txt   |i3 
---+---
 1 mon 8 days 06:05:46.834 | 1 mon 8 days 06:05:46.834

So I’m so far happy.

But, like I said, I’d forgotten a orthogonal quirk. This test shows it. It’s 
informed by the fact that 1.2345*12.0 is 14.8140.

select
  ('1.2345 years'  ::interval)::text as i1_txt,
  ('14.8140 months'::interval)::text as i2_txt;

Here’s the result:

i1_txt | i2_txt 
---+
 1 year 2 mons | 1 year 2 mons 24 days 10:04:48

It seems to be to be crazy behavior. I haven’t found any account of it in the 
PG docs. Others have argued that it’s a sensible result. Anyway, I don’t 
believe that I’ve ever argued that it’s a bug. I wanted only to know what 
rationale informed the design. I agree that changing the behavior here would be 
problematic for extant code.
 
This quirk explains the outcome of this test:

select
  ('1.2345 years'::interval)::text as i1_txt,
  ('14.8140 months'::interval)::text as i2_txt,
  (1.2345*('1 years'::interval))::text as i3_txt;

This is the result:

i1_txt | i2_txt | i3_txt
 
---++
 1 year 2 mons | 1 year 2 mons 24 days 10:04:48 | 1 year 2 mons 24 days 10:04:48

Notice that the same text is reported for i2_txt as for i3_txt.

Re: Have I found an interval arithmetic bug?

[Bryn Llewellyn]

> On 23-Jul-2021, br...@momjian.us wrote:
> 
> On Fri, Jul 23, 2021 at 10:55:11AM -0700, Bryn Llewellyn wrote:
>> SELECT
>>  '1.2345 months 1.2345 days 1.2345 seconds'::interval = 
>>  '1 month 1 day 1 second'::interval*1.2345;
>> 
>> In 13.3, the result is TRUE. (I know that this doesn’t guarantee that the
>> internal representations of the two compared interval values are the same. 
>> But
>> it’s a necessary condition for the outcome that I’m referring to and serves 
>> to
>> indecate the pont I’m making. A more careful test can be made.
> 
> So you are saying fractional unit output should match multiplication
> output?  It doesn't now for all units:
> 
>   SELECT interval '1.3443 years';
>  interval
>   ---
>1 year 4 mons
>   
>   SELECT interval '1 years' * 1.3443;
>   ?column?
>   -
>1 year 4 mons 3 days 22:45:07.2
> 
> It is true this patch is further reducing that matching.  Do people
> think I should make them match as part of this patch?

Summary:


It seems to me that the best thing to do is fix the coarse bug about which 
there is unanimous agreement and to leave everything else (quirks and all) 
untouched.

Detail:
---

My previous email (to which Bruce replies here) was muddled. Sorry. The 
challenge is that are a number of mechanisms at work. Their effects are 
conflated. And it’s very hard to unscramble them.

The underlying problem is that the internal representation of an interval value 
is a [mm, dd, ss] tuple. This fact is documented. The representation is key to 
understanding the definitions of these operations:

— defining an interval value from a text literal that uses real number values 
for its various fields.

— defining an interval value from make_interval(). (This one is easy because 
the API requires integral values for all but the seconds argument. It would be 
interesting to know why this asymmetrical definition was implemented. It seems 
to imply that somebody though that spilldown was a bad idea and should be 
prevented before it might happen.)

— creating the text typecast of an extant interval value.

— creating an interval value by adding/subtracting an extant interval value 
to/from another

— creating an interval value by multiplying or dividing an extant interval 
value by a (real) number

— creating an interval value by subtracting a pair of moments of the same data 
type (timestamptz, plain timestamp, or time)

— creating a new moment value by adding or subtracting an extant interval value 
to an extant moment value of the same data type.

— creating an interval value by applying  justify_hours(i), justify_days(i), 
and justify_interval(i) to an extant interval value, i.

— creating a double precision value by applying extract(epoch from i) 
to an extant interval value, i.

— evaluating inequality and equality tests to compare two extant interval 
values.

Notice that, for example, this test:

select
  interval  '1.3443 years' as i1,
  interval '1 years' * 1.3443 as i2;

conflates three things: converting an interval literal to a [mm, dd, ss] tuple; 
multiplying a [mm, dd, ss] tuple by a real number; and converting a [mm, dd, 
ss] tuple to a text representation. Similarly, this test:

select
  interval  '1.3443 years' <
  interval '1 years' * 1.3443;

conflates three things: converting an interval literal to a [mm, dd, ss] tuple; 
multiplying a [mm, dd, ss] tuple by a real number; and inequality comparison of 
two [mm, dd, ss] two [mm, dd, ss] tuples.

As far as I’ve been able, the PG documentation doesn’t do a good job of 
defining the semantics of any of these operations. Some (like the “justify” 
functions” are sketched reasonably well. Others, like interval multiplication, 
are entirely undefined.

This makes discussion of simple test like the two I showed immediately above 
hard. It also makes any discussion of correctness, possible bugs, and proposed 
implementation changes very difficult.

Further, it also makes it hard to see how tests for application code that uses 
any of these operations can be designed. The normal approach relies on testing 
that you get what you expect. But here, you don't know what to expect—unless 
(as I’ve done) you first assert hypotheses for the undefined operations and 
test them with programmed simulations. Of course, this is, in general, an 
unreliable approach. The only way to know what code is intended to do is to 
read the prose specification that informs the implementation.

I had forgotten one piece of the long history of this thread. Soon after I 
presented the testcase that folks agree shows a clear bug, I asked about the 
rules for creating the internal [mm, dd, ss] tuple from a text literal that 
uses real numbers for the fields. My tests showed two things: (1) an 
intuitively clear mo