BUG #17866: behavior does not match documentation

PG Bug reporting form <noreply@postgresql.org> writes:

pgc=# SELECT EXTRACT(EPOCH FROM INTERVAL '5 days 3 hours');
extract
---------------
442800.000000
(1 row)

But in documentation we can see:
SELECT EXTRACT(EPOCH FROM INTERVAL '5 days 3 hours');
Result: 442800

Hmm. That's not wrong exactly, but it's fair to question whether it
satisfies the POLA. This behavior change happened with commit a2da77cdb
(Change return type of EXTRACT to numeric), and it was intentional
according to the commit log:

- Return values when extracting fields with possibly fractional
values, such as second and epoch, now have the full scale that the
value has internally (so, for example, '1.000000' instead of just
'1').

But exactly nothing was mentioned of that in user-facing docs.
I wonder if we should rethink that and have these operations strip
insignificant trailing zeroes. It looks like that could be done as
practically a one-liner change, since int64_div_fast_to_numeric isn't
yet used anywhere except in these datetime extraction functions.

(The test cases that change behavior are either reverting to their
pre-a2da77cdb output, or were newly added in that commit.)

Thoughts?

regards, tom lane

On 23 Mar 2023, at 19:31, Tom Lane <tgl@sss.pgh.pa.us> wrote:

This behavior change happened with commit a2da77cdb
(Change return type of EXTRACT to numeric), and it was intentional
according to the commit log:

- Return values when extracting fields with possibly fractional
values, such as second and epoch, now have the full scale that the
value has internally (so, for example, '1.000000' instead of just
'1').

But exactly nothing was mentioned of that in user-facing docs.

Skimming the thread it's also not really discussed much AFAICT. It's first
brought up in [0]/messages/by-id/a3be61d9-f44b-7fce-3dc8-d700fdfb6f48@2ndquadrant.com as:

When extracting seconds or microseconds, I made it always produce 6 or
3 decimal places, even if they are zero. I don't know if we want that
or what behavior we want. That's what all the changes in the
regression tests are about. Everything else passes unchanged.

There are no follow-ups to that though.

I wonder if we should rethink that and have these operations strip
insignificant trailing zeroes.

Any app relying on insignificant trailing zeroes seems broken, but the inverse
can be argued as well. It's however quite easy to argue for the stripped
output being more readable though.

--
Daniel Gustafsson

[0]: /messages/by-id/a3be61d9-f44b-7fce-3dc8-d700fdfb6f48@2ndquadrant.com

Daniel Gustafsson <daniel@yesql.se> writes:

On 23 Mar 2023, at 19:31, Tom Lane <tgl@sss.pgh.pa.us> wrote:

I wonder if we should rethink that and have these operations strip
insignificant trailing zeroes.

Any app relying on insignificant trailing zeroes seems broken, but the inverse
can be argued as well. It's however quite easy to argue for the stripped
output being more readable though.

It's more readable for sure, and it duplicates what you got in pre-v14
versions, at least textually. I'm not sure I'd propose back-patching
this, but it feels like a good idea for HEAD.

The argument for the current behavior probably goes like "we're exposing
the actual precision of the value". But I don't believe that we are;
we're exposing the maximum possible precision. We have no way to know
how precise the original timestamp or interval input was. So I don't
think there's much basis for claiming that the result is good to six
fractional digits.

regards, tom lane

On 23 Mar 2023, at 22:54, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Daniel Gustafsson <daniel@yesql.se> writes:

On 23 Mar 2023, at 19:31, Tom Lane <tgl@sss.pgh.pa.us> wrote:

I wonder if we should rethink that and have these operations strip
insignificant trailing zeroes.

Any app relying on insignificant trailing zeroes seems broken, but the inverse
can be argued as well. It's however quite easy to argue for the stripped
output being more readable though.

It's more readable for sure, and it duplicates what you got in pre-v14
versions, at least textually. I'm not sure I'd propose back-patching
this, but it feels like a good idea for HEAD.

Agreed, I don't think this is material for backpatching.

--
Daniel Gustafsson

Daniel Gustafsson <daniel@yesql.se> writes:

On 23 Mar 2023, at 22:54, Tom Lane <tgl@sss.pgh.pa.us> wrote:

It's more readable for sure, and it duplicates what you got in pre-v14
versions, at least textually. I'm not sure I'd propose back-patching
this, but it feels like a good idea for HEAD.

Agreed, I don't think this is material for backpatching.

Since we didn't get anything done about that for v16, I concluded
that adjusting the docs is a better idea. If anyone takes this
idea up in v17 or beyond, they can always revert fbbd7edca at that
time.

regards, tom lane

On 10 Apr 2023, at 19:11, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Daniel Gustafsson <daniel@yesql.se> writes:

On 23 Mar 2023, at 22:54, Tom Lane <tgl@sss.pgh.pa.us> wrote:

It's more readable for sure, and it duplicates what you got in pre-v14
versions, at least textually. I'm not sure I'd propose back-patching
this, but it feels like a good idea for HEAD.

Agreed, I don't think this is material for backpatching.

Since we didn't get anything done about that for v16, I concluded
that adjusting the docs is a better idea. If anyone takes this
idea up in v17 or beyond, they can always revert fbbd7edca at that
time.

The more I think about it the more I lean towards not changing anything
(besides the docs which you did). It's been in two releases already and hardly
any complaints, so it seems to be have been quite well accepted.

--
Daniel Gustafsson