Re: BUG #2056: to_char no long takes time as input?

The following bug has been logged online:

Bug reference: 2056
Logged by: Nick Addington
Email address: adding(at)math(dot)wisc(dot)edu
PostgreSQL version: 8.1.0
Operating system: aix
Description: to_char no long takes time as input?
Details:

The following code works in 8.0.4 but fails in 8.1.0:

select to_char('1:00 pm'::time,'HH:MM AM');

8.1.0 gives this is the error message:
ERROR: invalid format specification for an interval value
HINT: Intervals are not tied to specific calendar dates.

I saw some discussion on the -hackers list about deprecating
to_char(interval, text), but do you really want to chuck to_char(time,
text)? That's a useful function. Or at least, I was using it...

Please advise,
Nick Addington

On Sun, Nov 20, 2005 at 07:53:50AM +0000, Nick Addington wrote:
> The following code works in 8.0.4 but fails in 8.1.0:
>
> select to_char('1:00 pm'::time,'HH:MM AM');
>
> 8.1.0 gives this is the error message:
> ERROR: invalid format specification for an interval value
> HINT: Intervals are not tied to specific calendar dates.
>
> I saw some discussion on the -hackers list about deprecating
> to_char(interval, text), but do you really want to chuck to_char(time,
> text)? That's a useful function. Or at least, I was using it...

to_char(time,text) doesn't exist, at least not in 7.3 and later --
you can see that with "\df to_char" in psql. If you set debug_print_parse
to on and set client_min_messages to debug1, you'll see that the
function being called is funcid 1768, which is

test=> select 1768::regprocedure;
regprocedure
------------------------
to_char(interval,text)
(1 row)

You'll also see that this function's first argument is a function
expression with funcid 1370, which is

test=> select 1370::regprocedure;
regprocedure
------------------------------------
"interval"(time without time zone)
(1 row)

So the time value is first converted to an interval and then passed
to to_char(interval,text).

test=> select "interval"('1:00 pm'::time);
interval
----------
13:00:00
(1 row)

test=> select to_char('13:00:00'::interval,'HH:MM AM');
ERROR: invalid format specification for an interval value
HINT: Intervals are not tied to specific calendar dates.

This looks like the commit that changed the behavior in 8.1 (the
hint was added later):

http://archives.postgresql.org/pgsql-committers/2005-08/msg00200.php

--
Michael Fuhr

"Nick Addington" <adding(at)math(dot)wisc(dot)edu> writes:
> The following code works in 8.0.4 but fails in 8.1.0:
> select to_char('1:00 pm'::time,'HH:MM AM');

I'm inclined to think that disallowing AM/PM for intervals was a mistake
--- if we allow both HH and HH24 (which we do) then the various flavors
of AM/PM seem reasonable to allow as well.

Bruce, did you have a specific rationale for that?

I notice the code now specifically forces HH to be treated as HH24 in
the interval case, which was not so before 8.1; we would need to revert
that change as well to continue supporting TIME cases reasonably.

regards, tom lane

Tom Lane wrote:
> "Nick Addington" <adding(at)math(dot)wisc(dot)edu> writes:
> > The following code works in 8.0.4 but fails in 8.1.0:
> > select to_char('1:00 pm'::time,'HH:MM AM');
>
> I'm inclined to think that disallowing AM/PM for intervals was a mistake
> --- if we allow both HH and HH24 (which we do) then the various flavors
> of AM/PM seem reasonable to allow as well.
>
> Bruce, did you have a specific rationale for that?
>
> I notice the code now specifically forces HH to be treated as HH24 in
> the interval case, which was not so before 8.1; we would need to revert
> that change as well to continue supporting TIME cases reasonably.

When I coded it I was thinking it doesn't make sense to allow "AM"/"PM" for
something like "5 hours". I disallowed anything that tied to a specific
timestamp value, even BC/AD.

I didn't realize "time" was used as input to to_char() as an interval.
I can see "time" as anchoring to midnight and we could then allow AM/PM.
I see your issue with HH/HH24, but I wanted this to work:

test=> select to_char('14 hours'::interval, 'HH');
to_char
---------
14
(1 row)

With the HH/HH24 change that is going to return 2. Do interval folks
know they would have to use HH24 for intervals? It doesn't seem 100%
clear, but I suppose we could just add a documentation about it, because
consider this:

test=> select to_char('44 hours'::interval, 'HH');
to_char
---------
44
(1 row)

With "HH" changed that would return 32. Ewe. Here is the formatting.c
code:

if (is_interval)
sprintf(inout, "%0*d", S_FM(suf) ? 0 : 2, tm->tm_hour);
else
sprintf(inout, "%0*d", S_FM(suf) ? 0 : 2,
tm->tm_hour == 0 ? 12 :
tm->tm_hour < 13 ? tm->tm_hour : tm->tm_hour - 12);

Should we subtract 12 only if the time is < 24. That also seems
strange. Also, a zero hour interval to HH would return 12, not 0.

It also seems strange to use HH24 for a value that might be greater than
24 (hours), while 'time' can not.

Anyway, I am thinking the easiest solution is to allow 'time' work
easily with HH, and to document that HH24 needs to be used for
intervals.

If this is what we want, I can make the change.

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> I see your issue with HH/HH24, but I wanted this to work:

> test=> select to_char('14 hours'::interval, 'HH');
> to_char
> ---------
> 14
> (1 row)

> With the HH/HH24 change that is going to return 2. Do interval folks
> know they would have to use HH24 for intervals?

Dunno if they know it, but they always had to do it that way before 8.1,
so it's not a change to require it. I get this in everything back to
7.2:

regression=# select to_char('14 hours'::interval, 'HH');
to_char
---------
02
(1 row)

regression=# select to_char('14 hours'::interval, 'HH24');
to_char
---------
14
(1 row)

and I don't see anything especially wrong with that behavior, as long as
it's documented.

> Should we subtract 12 only if the time is < 24. That also seems
> strange. Also, a zero hour interval to HH would return 12, not 0.

Offhand I'd vote for making the HH code use a "mod 12" calculation,
and making AM/PM depend on the value "mod 24". This gives at least a
slightly sane behavior for intervals > 24 hours.

regards, tom lane

Here is a patch that allows HH and HH12 to be used with interval, and
hence time. I added documentation to warn users that using those with
intervals are mapped to single-day values.

I will soon apply this to HEAD and 8.1.X.

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

Tom Lane wrote:
> Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> > I see your issue with HH/HH24, but I wanted this to work:
>
> > test=> select to_char('14 hours'::interval, 'HH');
> > to_char
> > ---------
> > 14
> > (1 row)
>
> > With the HH/HH24 change that is going to return 2. Do interval folks
> > know they would have to use HH24 for intervals?
>
> Dunno if they know it, but they always had to do it that way before 8.1,
> so it's not a change to require it. I get this in everything back to
> 7.2:
>
> regression=# select to_char('14 hours'::interval, 'HH');
> to_char
> ---------
> 02
> (1 row)
>
> regression=# select to_char('14 hours'::interval, 'HH24');
> to_char
> ---------
> 14
> (1 row)
>
> and I don't see anything especially wrong with that behavior, as long as
> it's documented.
>
> > Should we subtract 12 only if the time is < 24. That also seems
> > strange. Also, a zero hour interval to HH would return 12, not 0.
>
> Offhand I'd vote for making the HH code use a "mod 12" calculation,
> and making AM/PM depend on the value "mod 24". This gives at least a
> slightly sane behavior for intervals > 24 hours.
>
> regards, tom lane
>
> ---------------------------(end of broadcast)---------------------------
> TIP 4: Have you searched our list archives?
>
> http://archives.postgresql.org
>

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

Oh, one more thing. With the new patch I just posted, '0 hours' and
'HH' returns 12:

test=> select to_char('0 hours'::interval, 'HH');
to_char
---------
12
(1 row)

Of course HH24 works fine.

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

Tom Lane wrote:
> Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> > I see your issue with HH/HH24, but I wanted this to work:
>
> > test=> select to_char('14 hours'::interval, 'HH');
> > to_char
> > ---------
> > 14
> > (1 row)
>
> > With the HH/HH24 change that is going to return 2. Do interval folks
> > know they would have to use HH24 for intervals?
>
> Dunno if they know it, but they always had to do it that way before 8.1,
> so it's not a change to require it. I get this in everything back to
> 7.2:
>
> regression=# select to_char('14 hours'::interval, 'HH');
> to_char
> ---------
> 02
> (1 row)
>
> regression=# select to_char('14 hours'::interval, 'HH24');
> to_char
> ---------
> 14
> (1 row)
>
> and I don't see anything especially wrong with that behavior, as long as
> it's documented.
>
> > Should we subtract 12 only if the time is < 24. That also seems
> > strange. Also, a zero hour interval to HH would return 12, not 0.
>
> Offhand I'd vote for making the HH code use a "mod 12" calculation,
> and making AM/PM depend on the value "mod 24". This gives at least a
> slightly sane behavior for intervals > 24 hours.
>
> regards, tom lane
>
> ---------------------------(end of broadcast)---------------------------
> TIP 4: Have you searched our list archives?
>
> http://archives.postgresql.org
>

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> Oh, one more thing. With the new patch I just posted, '0 hours' and
> 'HH' returns 12:

> test=> select to_char('0 hours'::interval, 'HH');
> to_char
> ---------
> 12
> (1 row)

Yeah, it's done that in every release since 7.2. 8.1.0 is the only
release that thinks 00 is correct.

regards, tom lane

Patch applied to HEAD and 8.1.X.

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

Tom Lane wrote:
> Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> > I see your issue with HH/HH24, but I wanted this to work:
>
> > test=> select to_char('14 hours'::interval, 'HH');
> > to_char
> > ---------
> > 14
> > (1 row)
>
> > With the HH/HH24 change that is going to return 2. Do interval folks
> > know they would have to use HH24 for intervals?
>
> Dunno if they know it, but they always had to do it that way before 8.1,
> so it's not a change to require it. I get this in everything back to
> 7.2:
>
> regression=# select to_char('14 hours'::interval, 'HH');
> to_char
> ---------
> 02
> (1 row)
>
> regression=# select to_char('14 hours'::interval, 'HH24');
> to_char
> ---------
> 14
> (1 row)
>
> and I don't see anything especially wrong with that behavior, as long as
> it's documented.
>
> > Should we subtract 12 only if the time is < 24. That also seems
> > strange. Also, a zero hour interval to HH would return 12, not 0.
>
> Offhand I'd vote for making the HH code use a "mod 12" calculation,
> and making AM/PM depend on the value "mod 24". This gives at least a
> slightly sane behavior for intervals > 24 hours.
>
> regards, tom lane
>
> ---------------------------(end of broadcast)---------------------------
> TIP 4: Have you searched our list archives?
>
> http://archives.postgresql.org
>

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073