Horton
Menu

Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Fix extraction of week and quarter fields from intervals.

  2. Doc: improve explanation of type interval, especially extract().

  1. BUG #18348: Inconsistency with EXTRACT([field] from INTERVAL);

    PG Bug reporting form <noreply@postgresql.org> — 2024-02-16T12:06:55Z

    The following bug has been logged on the website:

Bug reference:      18348
Logged by:          Michael Bondarenko
Email address:      work.michael.2956@gmail.com
PostgreSQL version: 14.10
Operating system:   macOS
Description:        

Hello,

I'm building a random semantically-correct SQL code generator for PostgreSQL
and I stumbled upon an inconsistency:

tpch=# select extract(year from interval '3 years');
 extract 
---------
       3
(1 row)

tpch=# select extract(week from interval '3 weeks');
ERROR:  interval units "week" not supported

In the documentation it's mentioned that 'week' is an ISO 8601 week, so it
makes sense why it's not applicable to INTERVAL, which is the same for
isoyear. However, the field is named week and not isoweek, so I expect it to
work like the `select extract(year from interval '3 years');` does.
Moreover, the documentation does not mention that the field cannot be
extracted from INTERVAL, like it does for isoyear:
https://www.postgresql.org/docs/current/functions-datetime.html#FUNCTIONS-DATETIME-EXTRACT
.

  2. Re: BUG #18348: Inconsistency with EXTRACT([field] from INTERVAL);

    Michael Bondarenko <work.michael.2956@gmail.com> — 2024-02-16T12:21:57Z

    Adding another inconsistency I found in the docs to this thread (
https://www.postgresql.org/docs/current/functions-datetime.html#FUNCTIONS-DATETIME-EXTRACT
):

The docs say: "source must be a value expression of type *timestamp*, *time*,
or *interval*. (Expressions of type *date* are *cast to timestamp* and can
therefore be used as well.)"

Which implies that the following two results must be the same:

tpch=# select extract(microseconds from date '1924.01.01');
ERROR:  date units "microseconds" not supported

tpch=# select extract(microseconds from (date '1924.01.01')::timestamp);
 extract
---------
       0
(1 row)

However, the behaviour is different, which suggests that the date is indeed
treated as its own type in EXTRACT, and not cast to timestamp.

On Fri, Feb 16, 2024 at 2:07 PM PG Bug reporting form <
noreply@postgresql.org> wrote:

> The following bug has been logged on the website:
>
> Bug reference:      18348
> Logged by:          Michael Bondarenko
> Email address:      work.michael.2956@gmail.com
> PostgreSQL version: 14.10
> Operating system:   macOS
> Description:
>
> Hello,
>
> I'm building a random semantically-correct SQL code generator for
> PostgreSQL
> and I stumbled upon an inconsistency:
>
> tpch=# select extract(year from interval '3 years');
>  extract
> ---------
>        3
> (1 row)
>
> tpch=# select extract(week from interval '3 weeks');
> ERROR:  interval units "week" not supported
>
> In the documentation it's mentioned that 'week' is an ISO 8601 week, so it
> makes sense why it's not applicable to INTERVAL, which is the same for
> isoyear. However, the field is named week and not isoweek, so I expect it
> to
> work like the `select extract(year from interval '3 years');` does.
> Moreover, the documentation does not mention that the field cannot be
> extracted from INTERVAL, like it does for isoyear:
>
> https://www.postgresql.org/docs/current/functions-datetime.html#FUNCTIONS-DATETIME-EXTRACT
> .
>
>

  3. Re: BUG #18348: Inconsistency with EXTRACT([field] from INTERVAL);

    David Rowley <dgrowleyml@gmail.com> — 2024-02-16T12:44:01Z

    On Sat, 17 Feb 2024 at 01:27, Michael Bondarenko
<work.michael.2956@gmail.com> wrote:
>
> Adding another inconsistency I found in the docs to this thread (https://www.postgresql.org/docs/current/functions-datetime.html#FUNCTIONS-DATETIME-EXTRACT):
>
> The docs say: "source must be a value expression of type timestamp, time, or interval. (Expressions of type date are cast to timestamp and can therefore be used as well.)"
>
> Which implies that the following two results must be the same:
>
> tpch=# select extract(microseconds from date '1924.01.01');
> ERROR:  date units "microseconds" not supported
>
> tpch=# select extract(microseconds from (date '1924.01.01')::timestamp);
>  extract
> ---------
>        0

It looks like a2da77cdb should have updated the documentation for this.

David

  4. Re: BUG #18348: Inconsistency with EXTRACT([field] from INTERVAL);

    David Rowley <dgrowleyml@gmail.com> — 2024-02-16T13:02:20Z

    On Sat, 17 Feb 2024 at 01:27, PG Bug reporting form
<noreply@postgresql.org> wrote:
> tpch=# select extract(week from interval '3 weeks');
> ERROR:  interval units "week" not supported
>
> In the documentation it's mentioned that 'week' is an ISO 8601 week, so it
> makes sense why it's not applicable to INTERVAL, which is the same for
> isoyear. However, the field is named week and not isoweek, so I expect it to
> work like the `select extract(year from interval '3 years');` does.
> Moreover, the documentation does not mention that the field cannot be
> extracted from INTERVAL, like it does for isoyear:
> https://www.postgresql.org/docs/current/functions-datetime.html#FUNCTIONS-DATETIME-EXTRACT

Maybe that table should specify which type(s) each of the items listed
is applicable to. Seems better than mentioning which types they're not
applicable to.

David

  5. Re: BUG #18348: Inconsistency with EXTRACT([field] from INTERVAL);

    jian he <jian.universality@gmail.com> — 2024-02-17T01:47:58Z

    in `9.9.1. EXTRACT, date_part`
EXTRACT(field FROM source)

I saw more inconsistencies with the doc when `source` is an interval.

the `minute` field
select extract(minute from interval '2011 year 16 month 35 day 48 hour
1005 min 71 sec 11 ms');
select extract(minute from interval '2011 year 16 month 35 day 48 hour
1005 min 2 sec 11 ms');
select extract(minute from interval '2011 year 16 month 35 day 48 hour
1005 min 2 sec 11 ms');

the `hour` field:
select extract(hour from interval '2011 year 16 month 35 day 48 hour
1005 min 71 sec 11 ms');
select extract(hour from interval '2011 year 16 month 35 day 48 hour
1005 min 2 sec 11 ms');
select extract(hour from interval '2011 year 16 month 35 day 48 hour
1005 min 71 sec 11111111111 ms');

the `quarter` field:
select extract(quarter from interval '2011 year 12 month 48 hour 1005
min 2 sec 11 ms');
SELECT EXTRACT(QUARTER FROM TIMESTAMP '2001-12-16 20:38:40');

  6. Re: BUG #18348: Inconsistency with EXTRACT([field] from INTERVAL);

    Michael Bondarenko <work.michael.2956@gmail.com> — 2024-02-17T08:00:39Z

    When testing I stumbled upon that too, but I thought no calculation was
happening in the interval field. However, it's different with the days and
months etc. It seems no calculation for day and month and more:

tpch=# select extract(day from interval '86400000 seconds');
 extract
---------
       0
(1 row)

tpch=# select extract(month from interval '86400000 seconds');
 extract
---------
       0
(1 row)

tpch=# select extract(year from interval '86400000 seconds');
 extract
---------
       0
(1 row)

But calculation is present for hour, and minutes and seconds (90061 sec is
1 day 1 hour 1 minute 1 second):

tpch=# select extract(minute from interval '90061 seconds');
 extract
---------
       1
(1 row)

tpch=# select extract(hour from interval '90061 seconds');
 extract
---------
      25
(1 row)

tpch=# select extract(second from interval '90061 seconds');
 extract
----------
 1.000000
(1 row)

The docs mention *The hour field (0–23)* for the hours, which is not true
because it's not the field at all, but the calculated amount, and the value
is not 0-23.

On Sat, Feb 17, 2024 at 3:48 AM jian he <jian.universality@gmail.com> wrote:

> in `9.9.1. EXTRACT, date_part`
> EXTRACT(field FROM source)
>
> I saw more inconsistencies with the doc when `source` is an interval.
>
> the `minute` field
> select extract(minute from interval '2011 year 16 month 35 day 48 hour
> 1005 min 71 sec 11 ms');
> select extract(minute from interval '2011 year 16 month 35 day 48 hour
> 1005 min 2 sec 11 ms');
> select extract(minute from interval '2011 year 16 month 35 day 48 hour
> 1005 min 2 sec 11 ms');
>
> the `hour` field:
> select extract(hour from interval '2011 year 16 month 35 day 48 hour
> 1005 min 71 sec 11 ms');
> select extract(hour from interval '2011 year 16 month 35 day 48 hour
> 1005 min 2 sec 11 ms');
> select extract(hour from interval '2011 year 16 month 35 day 48 hour
> 1005 min 71 sec 11111111111 ms');
>
> the `quarter` field:
> select extract(quarter from interval '2011 year 12 month 48 hour 1005
> min 2 sec 11 ms');
> SELECT EXTRACT(QUARTER FROM TIMESTAMP '2001-12-16 20:38:40');
>

  7. Re: BUG #18348: Inconsistency with EXTRACT([field] from INTERVAL);

    Francisco Olarte <folarte@peoplecall.com> — 2024-02-17T15:12:15Z

    On Sat, 17 Feb 2024 at 09:01, Michael Bondarenko
<work.michael.2956@gmail.com> wrote:
> When testing I stumbled upon that too, but I thought no calculation was happening in the interval field. However, it's different with the days and months etc. It seems no calculation for day and month and more:
...
> But calculation is present for hour, and minutes and seconds (90061 sec is 1 day 1 hour 1 minute 1 second):

No, intervals have seconds, days and months. This is because not all
days have 24 hours, due to DST they can have 23 or 25, or even more
extreme values if some country decides to change its time zone
definition. And not all months have 30 days, so 90061 is 0 months, 0
days, 25 hours, 1 minute, 1 second ( IIRC leap second are not handled
).

It is done that way so when you add one day across a dst jump you get
the same hour on the next day, and when you add one month you get the
same day in the next month independent of how many days the month has.
This is great for things like "schedule a meeting one month and one
week from now", but it bites you sometimes, like when you need a
duration to bill for a long event like a phone call, where I always
end up extracting epoch and substracting them.

Francisco Olarte.

  8. Re: BUG #18348: Inconsistency with EXTRACT([field] from INTERVAL);

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-02-17T18:14:19Z

    David Rowley <dgrowleyml@gmail.com> writes:
> On Sat, 17 Feb 2024 at 01:27, PG Bug reporting form
> <noreply@postgresql.org> wrote:
>> Moreover, the documentation does not mention that the field cannot be
>> extracted from INTERVAL, like it does for isoyear:
>> https://www.postgresql.org/docs/current/functions-datetime.html#FUNCTIONS-DATETIME-EXTRACT

> Maybe that table should specify which type(s) each of the items listed
> is applicable to. Seems better than mentioning which types they're not
> applicable to.

The thing's not laid out as a table though, and converting it seems
like more trouble than this is worth.  The rejected cases hardly seem
surprising.  I propose just mentioning that not all fields apply for
all data types, as in 0001 attached.

(Parenthetically, one case that perhaps is surprising is
	ERROR:  unit "week" not supported for type interval
Why not just return the day field divided by 7?)

Unrelated but adjacent, the discussion of the century field seems
more than a bit flippant when I read it now.  In other places we
are typically content to use examples to make similar points.
I propose doing so here too, as in 0002 attached.

Lastly, the entire page is quite schizophrenic about whether to leave
a blank line between adjacent examples.  I could go either way on
whether to have that whitespace or not, but I do think it would be
better to make it uniform.  Any votes on what to do there?

			regards, tom lane

  9. Re: BUG #18348: Inconsistency with EXTRACT([field] from INTERVAL);

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-02-17T20:30:25Z

    Francisco Olarte <folarte@peoplecall.com> writes:
> On Sat, 17 Feb 2024 at 09:01, Michael Bondarenko
> <work.michael.2956@gmail.com> wrote:
>> When testing I stumbled upon that too, but I thought no calculation was happening in the interval field. However, it's different with the days and months etc. It seems no calculation for day and month and more:
>> ...
>> But calculation is present for hour, and minutes and seconds (90061 sec is 1 day 1 hour 1 minute 1 second):

> No, intervals have seconds, days and months.

Yeah.  I think much of the confusion here comes from starting with
non-normalized interval input.  Sure you can write "2011 year 12 month
48 hour 1005 min 2 sec 11 ms", but that's not how it's stored:

regression=# select interval '2011 year 12 month 48 hour 1005 min 2 sec 11 ms';
        interval         
-------------------------
 2012 years 64:45:02.011
(1 row)

(Actually, what's stored is 2012*12 months, 0 days, and some number
of microseconds that I don't feel like working out.  Conversion of
the microseconds to HH:MM:SS.SSS happens on output.)

Once you look at the normalized value, the results of extract()
are far less surprising.

Probably the right place to enlarge on this point is not in the
extract() section at all, but in 8.5.4. Interval Input.  That does
mention the months/days/microseconds representation, but it doesn't
follow through by illustrating how other input is combined.  Perhaps
we'd want to adopt something like the attached (this is separate from
the other patches I posted in the thread).

			regards, tom lane

  10. Re: BUG #18348: Inconsistency with EXTRACT([field] from INTERVAL);

    jian he <jian.universality@gmail.com> — 2024-02-18T01:48:27Z

    On Sun, Feb 18, 2024 at 4:30 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
>
>
> Once you look at the normalized value, the results of extract()
> are far less surprising.
>
> Probably the right place to enlarge on this point is not in the
> extract() section at all, but in 8.5.4. Interval Input.  That does
> mention the months/days/microseconds representation, but it doesn't
> follow through by illustrating how other input is combined.  Perhaps
> we'd want to adopt something like the attached (this is separate from
> the other patches I posted in the thread).
>

--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -10040,13 +10040,19 @@ EXTRACT(<replaceable>field</replaceable>
FROM <replaceable>source</replaceable>)
     The <function>extract</function> function retrieves subfields
     such as year or hour from date/time values.
     <replaceable>source</replaceable> must be a value expression of
-    type <type>timestamp</type>, <type>time</type>, or <type>interval</type>.
-    (Expressions of type <type>date</type> are
-    cast to <type>timestamp</type> and can therefore be used as
-    well.)  <replaceable>field</replaceable> is an identifier or
+    type <type>timestamp</type>, <type>date</type>, <type>time</type>,
+    or <type>interval</type>.  (Timestamps and times can be with or
+    without time zone.)
+    <replaceable>field</replaceable> is an identifier or
     string that selects what field to extract from the source value.
+    Not all fields are valid for every input data type; for example, fields
+    smaller than a day cannot be extracted from a <type>date</type>, while
+    fields of a day or more cannot be extracted from a <type>time</type>.
     The <function>extract</function> function returns values of type
     <type>numeric</type>.
+   </para>

you already mentioned "Not all fields are valid for every input data type".
interval data type don't even have a unit "quarter",
so the following should generate an error?
select extract(quarter from interval '2011 year 12 month 48 hour
1005min 2 sec 11 ms');

9.9.1. EXTRACT, date_part
hour field description as
`
The hour field (0–23)
`
Do we need to update for the EXTRACT(INTERVAL) case?

  11. Re: BUG #18348: Inconsistency with EXTRACT([field] from INTERVAL);

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-02-18T02:19:08Z

    jian he <jian.universality@gmail.com> writes:
> you already mentioned "Not all fields are valid for every input data type".
> interval data type don't even have a unit "quarter",
> so the following should generate an error?
> select extract(quarter from interval '2011 year 12 month 48 hour
> 1005min 2 sec 11 ms');

I'm not especially persuaded by that reasoning.  Intervals don't have
century or millisecond fields either, but we allow extracting those.

If your argument is that we shouldn't allow it because we don't take
the input INTERVAL '1 quarter', I'd be much more inclined to add that
as valid input than to take away existing extract functionality.
But I'm dubious about the proposition that extract's list of valid
fields should exactly match the set of allowed input units.  The
semantics aren't really the same (as per the '80 minutes' example)
so such a restriction doesn't seem to have much basis in reality.

			regards, tom lane

  12. Re: BUG #18348: Inconsistency with EXTRACT([field] from INTERVAL);

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-02-19T17:22:33Z

    jian he <jian.universality@gmail.com> writes:
> 9.9.1. EXTRACT, date_part
> hour field description as
> `
> The hour field (0–23)
> `
> Do we need to update for the EXTRACT(INTERVAL) case?

Yeah, probably.  I did a bit more wordsmithing too.
Here's a rolled-up patch.

			regards, tom lane

  13. Re: BUG #18348: Inconsistency with EXTRACT([field] from INTERVAL);

    jian he <jian.universality@gmail.com> — 2024-02-20T03:56:29Z

    On Sun, Feb 18, 2024 at 2:14 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
>
> (Parenthetically, one case that perhaps is surprising is
>         ERROR:  unit "week" not supported for type interval
> Why not just return the day field divided by 7?)
>
seems pretty simple?
diff --git a/src/backend/utils/adt/timestamp.c
b/src/backend/utils/adt/timestamp.c
index ed03c50a..5e69e258 100644
--- a/src/backend/utils/adt/timestamp.c
+++ b/src/backend/utils/adt/timestamp.c
@@ -5992,6 +5992,10 @@ interval_part_common(PG_FUNCTION_ARGS, bool retnumeric)
                                intresult = tm->tm_mday;
                                break;

+                       case DTK_WEEK:
+                               intresult = (tm->tm_mday  - 1) / 7 + 1;
+                               break;
but I am not sure not sure how to write the doc.

On Sun, Feb 18, 2024 at 10:19 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> jian he <jian.universality@gmail.com> writes:
> > you already mentioned "Not all fields are valid for every input data type".
> > interval data type don't even have a unit "quarter",
> > so the following should generate an error?
> > select extract(quarter from interval '2011 year 12 month 48 hour
> > 1005min 2 sec 11 ms');
>
> I'm not especially persuaded by that reasoning.  Intervals don't have
> century or millisecond fields either, but we allow extracting those.
>
> If your argument is that we shouldn't allow it because we don't take
> the input INTERVAL '1 quarter', I'd be much more inclined to add that
> as valid input than to take away existing extract functionality.
> But I'm dubious about the proposition that extract's list of valid
> fields should exactly match the set of allowed input units.  The
> semantics aren't really the same (as per the '80 minutes' example)
> so such a restriction doesn't seem to have much basis in reality.
>

in interval_part_common:
case DTK_QUARTER:
intresult = (tm->tm_mon / 3) + 1;
break;

in timestamp_part_common:
case DTK_QUARTER:
intresult = (tm->tm_mon - 1) / 3 + 1;
break;

So in section 9.9.1. EXTRACT, date_part
we may need to document extract(quarter from interval) case.
intervals can be negative, which will make the issue more complicated.
except the "quarter" field , EXTRACT other fields from intervals, the
output seems sane.

for example:
drop table s;
create table s(a interval);
insert into s select ( g * 1000 || 'year ' ||  g || 'month ' || g || '
day ' || g || 'hour ' || g || 'min ' || g || 'sec' )::interval
from generate_series(-20, 20) g;

select
        extract(century from a) as century,
        extract(millennium from a) as millennium,
        extract(decade from a) as decade,
        extract(year from a) as year,
        extract(quarter from a) as quarter,
        extract(month from a) as mon,
        extract(day from a) as day,
        extract(hour from a) as hour,
        extract(min from a) as min,
        extract(second from a) as sec,
        extract(microseconds from a) as microseconds
        -- a
from s order by 2 asc;

  14. Re: BUG #18348: Inconsistency with EXTRACT([field] from INTERVAL);

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-02-20T19:42:30Z

    jian he <jian.universality@gmail.com> writes:
> On Sun, Feb 18, 2024 at 2:14 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> (Parenthetically, one case that perhaps is surprising is
>> ERROR:  unit "week" not supported for type interval
>> Why not just return the day field divided by 7?)

> seems pretty simple?

Hm, maybe, but does this behave desirably for zero or negative days?

> So in section 9.9.1. EXTRACT, date_part
> we may need to document extract(quarter from interval) case.
> intervals can be negative, which will make the issue more complicated.
> except the "quarter" field , EXTRACT other fields from intervals, the
> output seems sane.

Yeah, I see what you mean: the output for negative month counts is
very bizarre, whereas other fields seem to all produce the negative
of what they'd produce for the absolute value of the interval.
We could either try to fix that or decide that rejecting "quarter"
for intervals is the saner answer.

I went ahead and pushed the docs changes after adding more explicit
descriptions of interval's behavior for the field types where it
seemed important.  If we make any changes to the behavior for
week or quarter fields, ISTM that should be a HEAD-only change.

			regards, tom lane

  15. Re: BUG #18348: Inconsistency with EXTRACT([field] from INTERVAL);

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-02-20T20:56:04Z

    I wrote:
> jian he <jian.universality@gmail.com> writes:
>> On Sun, Feb 18, 2024 at 2:14 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> (Parenthetically, one case that perhaps is surprising is
>>> ERROR:  unit "week" not supported for type interval
>>> Why not just return the day field divided by 7?)

>> seems pretty simple?

> Hm, maybe, but does this behave desirably for zero or negative days?

>> So in section 9.9.1. EXTRACT, date_part
>> we may need to document extract(quarter from interval) case.
>> intervals can be negative, which will make the issue more complicated.
>> except the "quarter" field , EXTRACT other fields from intervals, the
>> output seems sane.

> Yeah, I see what you mean: the output for negative month counts is
> very bizarre, whereas other fields seem to all produce the negative
> of what they'd produce for the absolute value of the interval.
> We could either try to fix that or decide that rejecting "quarter"
> for intervals is the saner answer.

After fooling with these cases for a little I'm inclined to think
we should do it as attached (no test or docs changes yet).

			regards, tom lane

  16. Re: BUG #18348: Inconsistency with EXTRACT([field] from INTERVAL);

    jian he <jian.universality@gmail.com> — 2024-02-29T10:30:17Z

    On Wed, Feb 21, 2024 at 4:56 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> I wrote:
> > jian he <jian.universality@gmail.com> writes:
> >> On Sun, Feb 18, 2024 at 2:14 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >>> (Parenthetically, one case that perhaps is surprising is
> >>> ERROR:  unit "week" not supported for type interval
> >>> Why not just return the day field divided by 7?)
>
> >> seems pretty simple?
>
> > Hm, maybe, but does this behave desirably for zero or negative days?
>
> >> So in section 9.9.1. EXTRACT, date_part
> >> we may need to document extract(quarter from interval) case.
> >> intervals can be negative, which will make the issue more complicated.
> >> except the "quarter" field , EXTRACT other fields from intervals, the
> >> output seems sane.
>
> > Yeah, I see what you mean: the output for negative month counts is
> > very bizarre, whereas other fields seem to all produce the negative
> > of what they'd produce for the absolute value of the interval.
> > We could either try to fix that or decide that rejecting "quarter"
> > for intervals is the saner answer.
>
> After fooling with these cases for a little I'm inclined to think
> we should do it as attached (no test or docs changes yet).
>
>                         regards, tom lane
>

for `week`, we can do following for the doc:

diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index e5fa82c1..a21eb9f8 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -10422,7 +10422,7 @@ SELECT EXTRACT(SECOND FROM TIME '17:12:28.5');
         The number of the <acronym>ISO</acronym> 8601 week-numbering week of
         the year.  By definition, ISO weeks start on Mondays and the first
         week of a year contains January 4 of that year.  In other words, the
-        first Thursday of a year is in week 1 of that year.
+        first Thursday of a year is in week 1 of that year. For
<type>interval</type> values, divide the number of days by 7.

Actually, it's not totally correct, since "the number of days is a
numeric value. need to cast "the number of days" to int.

for positive interval value, we can
+ For positive <type>interval</type> values, divide the number of days
by 3 then plus 1.
I don't know how to write the documentation for the `quarter` when
it's negative.

  17. Re: BUG #18348: Inconsistency with EXTRACT([field] from INTERVAL);

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-05-07T21:27:08Z

    jian he <jian.universality@gmail.com> writes:
> On Wed, Feb 21, 2024 at 4:56 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> Yeah, I see what you mean: the output for negative month counts is
>>> very bizarre, whereas other fields seem to all produce the negative
>>> of what they'd produce for the absolute value of the interval.
>>> We could either try to fix that or decide that rejecting "quarter"
>>> for intervals is the saner answer.

>> After fooling with these cases for a little I'm inclined to think
>> we should do it as attached (no test or docs changes yet).

> ... I don't know how to write the documentation for the `quarter` when
> it's negative.

After poking at it some more, I realized that my draft patch was still
wrong about that.  We really have to look at interval->month if we
want to behave plausibly for negative months.

Here's a more fleshed-out patch.  I don't think we really need to
document the behavior for negative intervals; at least, we haven't
done that so far for any other fields.  I did add testing of such
cases though.

			regards, tom lane

  18. BUG #18348: Inconsistency with EXTRACT([field] from INTERVAL);

    Wetmore, Matthew (CTR) <matthew.wetmore@evernorth.com> — 2024-05-07T22:09:34Z

    Devils advocating here, feel free to ignore.

Is there a real need for a negative month?  Sounds like high level this could be disastrous if I screw up the syntax. (Ah, memories of DD)

I have done this in data warehousing with dimensions tables.

Just process on the INT and translate into the name.

I was thinking on how a negative month could impact this side (data warehousing) side of querying. 

I could be chicken little on this, but wanted it in the conversation.

workaround for negative months:

CREATE TABLE dim_biz_hours( year INT(4)
, doy INT(3)
, dow INT(7)
, month INT(2)
, day INT(2)
, hour INT(2)
, minute INT(2)
, second INT(2)
, utc_offset INT(2)
, utc_offset_dst INT(2)
);

INSERT INTO biz_hours (year)
SELECT * FROM generate_series(2000, 2099);

INSERT INTO biz_hours (doy)
SELECT * FROM generate_series(1, 366);

INSERT INTO biz_hours (dow)
SELECT * FROM generate_series(1, 7);

INSERT INTO biz_hours (month)
SELECT * FROM generate_series(1, 12);

INSERT INTO biz_hours (day)
SELECT * FROM generate_series(1, 31) ;

INSERT INTO biz_hours (hour)
SELECT * FROM generate_series(1, 24);

INSERT INTO biz_hours (minute)
SELECT * FROM generate_series(1, 60);

INSERT INTO biz_hours (second
SELECT * FROM generate_series(1, 60);

INSERT INTO biz_hours (utc_offset)
SELECT * FROM generate_series(1, 24);

INSERT INTO biz_hours (utc_offset_dst)
SELECT * FROM generate_series(1, 24);


-----Original Message-----
From: Tom Lane <tgl@sss.pgh.pa.us> 
Sent: Tuesday, May 7, 2024 2:27 PM
To: jian he <jian.universality@gmail.com>
Cc: Francisco Olarte <folarte@peoplecall.com>; Michael Bondarenko <work.michael.2956@gmail.com>; pgsql-bugs@lists.postgresql.org; dgrowleyml@gmail.com; Peter Eisentraut <peter.eisentraut@enterprisedb.com>
Subject: [EXTERNAL] Re: BUG #18348: Inconsistency with EXTRACT([field] from INTERVAL);

jian he <jian.universality@gmail.com> writes:
> On Wed, Feb 21, 2024 at 4:56 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> Yeah, I see what you mean: the output for negative month counts is 
>>> very bizarre, whereas other fields seem to all produce the negative 
>>> of what they'd produce for the absolute value of the interval.
>>> We could either try to fix that or decide that rejecting "quarter"
>>> for intervals is the saner answer.

>> After fooling with these cases for a little I'm inclined to think we 
>> should do it as attached (no test or docs changes yet).

> ... I don't know how to write the documentation for the `quarter` when 
> it's negative.

After poking at it some more, I realized that my draft patch was still wrong about that.  We really have to look at interval->month if we want to behave plausibly for negative months.

Here's a more fleshed-out patch.  I don't think we really need to document the behavior for negative intervals; at least, we haven't done that so far for any other fields.  I did add testing of such cases though.

			regards, tom lane

  19. Re: BUG #18348: Inconsistency with EXTRACT([field] from INTERVAL);

    jian he <jian.universality@gmail.com> — 2024-05-08T01:03:56Z

    On Wed, May 8, 2024 at 5:27 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Here's a more fleshed-out patch.  I don't think we really need to
> document the behavior for negative intervals; at least, we haven't
> done that so far for any other fields.  I did add testing of such
> cases though.
>

the doc looks good to me.
extract quarter from the interval makes sense to me.

but in real life, for week, we generally begin with 1?
like "the first week", "second week"

so should
select extract(week from interval '1 day');
return 1
?

  20. Re: BUG #18348: Inconsistency with EXTRACT([field] from INTERVAL);

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-05-08T13:55:50Z

    "Wetmore, Matthew  (CTR)" <Matthew.Wetmore@evernorth.com> writes:
> Devils advocating here, feel free to ignore.
> Is there a real need for a negative month?  Sounds like high level this could be disastrous if I screw up the syntax. (Ah, memories of DD)

What are you objecting to the "need for"?  That intervals can store
negative months at all?  I think that ship sailed a couple decades
ago.  It's hard to use interval as the output of, say,
timestamp minus timestamp if it refuses to allow negative values.

The next fallback position perhaps could be that extract(quarter ...)
could throw error for negative input, but that seems like mostly a
foot-gun.  We've striven elsewhere to not have it throw error, even
if there's not any very sane choice to make.  For instance, these
are pre-existing behaviors:

regression=# select extract(quarter from interval 'infinity');
 extract 
---------
        
(1 row)

regression=# select extract(quarter from interval '-infinity');
 extract 
---------
        
(1 row)

Maybe there's a case for returning null for "quarter" for any negative
months value, but that seems inconsistent with other behaviors of
extract().  The pattern I see for finite values is that negating
the input interval negates each output of extract().

			regards, tom lane

  21. Re: BUG #18348: Inconsistency with EXTRACT([field] from INTERVAL);

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-05-08T14:10:48Z

    jian he <jian.universality@gmail.com> writes:
> but in real life, for week, we generally begin with 1?
> like "the first week", "second week"
> so should
> select extract(week from interval '1 day');
> return 1
> ?

Hmm, I read it as being "the number of (whole) weeks in the
interval".  Starting with week 1 is what happens in the timestamp
case, true, but I don't find that appropriate for interval.
By analogy,

regression=# select extract(day from interval '23 hours');
 extract 
---------
       0
(1 row)

There's no such thing as "day 0" in the timestamp case,
but that doesn't make this wrong.

In any case, I'm starting to wonder why this issue is on the v17
open items list.  These are hardly new bugs in 17.  If there's
still differences of opinion about what the definition should be,
I think cramming in a change post-feature-freeze is not appropriate.
Let's just queue the issue for the next commitfest (already done
at [1]) and take it off the open items list.

			regards, tom lane

[1] https://commitfest.postgresql.org/48/4979/

  22. Re: BUG #18348: Inconsistency with EXTRACT([field] from INTERVAL);

    Martijn Wallet <martijn@dbre.nu> — 2024-05-18T14:02:17Z

    The following review has been posted through the commitfest application:
make installcheck-world:  tested, failed
Implements feature:       tested, failed
Spec compliant:           tested, failed
Documentation:            tested, failed

Hi, works out well everything. This is my first review, so if I should add more content here let me know.
Cheers, Martijn.

The new status of this patch is: Ready for Committer

  23. Re: BUG #18348: Inconsistency with EXTRACT([field] from INTERVAL);

    Martijn Wallet <martijn@dbre.nu> — 2024-05-18T14:47:29Z

    For some reason the review indicated "failed".
It should of course read:
make installcheck-world:  tested, passed
Implements feature:       tested, passed
Spec compliant:           tested, passed
Documentation:            tested, passed

  24. Re: BUG #18348: Inconsistency with EXTRACT([field] from INTERVAL);

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-07-08T17:03:28Z

    I took another look at this issue and got annoyed by the fact that the
proposed coding for "quarter" still doesn't satisfy the rule that
the output for a negative interval should be the negative of the
output for the sign-reversed interval.  Specifically, if the month
field is zero, the v2 patch always emits 1:

regression=# select extract(quarter from interval '1 day');
 extract 
---------
       1
(1 row)

regression=# select extract(quarter from interval '-1 day');
 extract 
---------
       1
(1 row)

We could fix that by examining the sign of the lower-order fields
when month is zero, as in the v3 patch attached.  However, I'm not
at all sure this is really better than v2.  Notably, it makes the
documentation's statement that the result is "the month field
divided by 3 plus 1" even more incomplete.  I still don't really
want to go into details about the behavior for negative intervals.
OTOH if we did do that, I'd rather write a blanket statement
about the result being the negative of the result for a positive
interval.

Thoughts?

			regards, tom lane

  25. Re: BUG #18348: Inconsistency with EXTRACT([field] from INTERVAL);

    jian he <jian.universality@gmail.com> — 2024-07-12T16:35:19Z

    On Tue, Jul 9, 2024 at 1:03 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> I took another look at this issue and got annoyed by the fact that the
> proposed coding for "quarter" still doesn't satisfy the rule that
> the output for a negative interval should be the negative of the
> output for the sign-reversed interval.  Specifically, if the month
> field is zero, the v2 patch always emits 1:
>
> regression=# select extract(quarter from interval '1 day');
>  extract
> ---------
>        1
> (1 row)
>
> regression=# select extract(quarter from interval '-1 day');
>  extract
> ---------
>        1
> (1 row)
>
> We could fix that by examining the sign of the lower-order fields
> when month is zero, as in the v3 patch attached.  However, I'm not
> at all sure this is really better than v2.  Notably, it makes the
> documentation's statement that the result is "the month field
> divided by 3 plus 1" even more incomplete.  I still don't really
> want to go into details about the behavior for negative intervals.
> OTOH if we did do that, I'd rather write a blanket statement
> about the result being the negative of the result for a positive
> interval.
>
> Thoughts?
>
>                         regards, tom lane
>


+       <para>
+        For <type>interval</type> values, the week field is simply the number
+        of integral days divided by 7.
+       </para>


+SELECT EXTRACT(WEEK FROM INTERVAL '13 days 24 hours');
+<lineannotation>Result: </lineannotation><computeroutput>1</computeroutput>

not sure the doc example will vividly demonstrate the explanation ("integral")
or confuse people, given that
SELECT EXTRACT(WEEK FROM INTERVAL '14 days');
returns 2.

and
SELECT  INTERVAL '14 days' = INTERVAL '13 days 24 hours';
is true.

  26. Re: BUG #18348: Inconsistency with EXTRACT([field] from INTERVAL);

    Bruce Momjian <bruce@momjian.us> — 2024-08-16T02:45:58Z

    On Mon, Jul  8, 2024 at 01:03:28PM -0400, Tom Lane wrote:
> I took another look at this issue and got annoyed by the fact that the
> proposed coding for "quarter" still doesn't satisfy the rule that
> the output for a negative interval should be the negative of the
> output for the sign-reversed interval.  Specifically, if the month
> field is zero, the v2 patch always emits 1:
> 
> regression=# select extract(quarter from interval '1 day');
>  extract 
> ---------
>        1
> (1 row)
> 
> regression=# select extract(quarter from interval '-1 day');
>  extract 
> ---------
>        1
> (1 row)
> 
> We could fix that by examining the sign of the lower-order fields
> when month is zero, as in the v3 patch attached.  However, I'm not
> at all sure this is really better than v2.  Notably, it makes the
> documentation's statement that the result is "the month field
> divided by 3 plus 1" even more incomplete.  I still don't really
> want to go into details about the behavior for negative intervals.
> OTOH if we did do that, I'd rather write a blanket statement
> about the result being the negative of the result for a positive
> interval.
> 
> Thoughts?

I tested master, patch version 2 and patch version 3 with some sample
extract() queires, attached.  I like patch version 2.  Patch version 3
bothers me because "-600 days" is ignored if months is non-zero, and
used for its sign for zero month values, which seems odd to me;  better
to ignore it.

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EDB                                      https://enterprisedb.com

  Only you can decide what is important to you.

  27. Re: BUG #18348: Inconsistency with EXTRACT([field] from INTERVAL);

    Bruce Momjian <bruce@momjian.us> — 2024-08-16T15:26:58Z

    On Thu, Aug 15, 2024 at 10:45:58PM -0400, Bruce Momjian wrote:
> > We could fix that by examining the sign of the lower-order fields
> > when month is zero, as in the v3 patch attached.  However, I'm not
> > at all sure this is really better than v2.  Notably, it makes the
> > documentation's statement that the result is "the month field
> > divided by 3 plus 1" even more incomplete.  I still don't really
> > want to go into details about the behavior for negative intervals.
> > OTOH if we did do that, I'd rather write a blanket statement
> > about the result being the negative of the result for a positive
> > interval.
> > 
> > Thoughts?
> 
> I tested master, patch version 2 and patch version 3 with some sample
> extract() queires, attached.  I like patch version 2.  Patch version 3
> bothers me because "-600 days" is ignored if months is non-zero, and
> used for its sign for zero month values, which seems odd to me;  better
> to ignore it.

I think there are two more issues.  In patch version 3, when months is
zero and you check days, you should also check seconds if days is zero.

I think the other issue is that zero months is a valid Q1 value, since
months 0-2 are Q1;  from master:

	SELECT extract(quarter FROM interval '0 months');
	 extract
	---------
	       1
	
	SELECT extract(quarter FROM interval '2 months');
	 extract
	---------
	       1
	
	SELECT extract(quarter FROM interval '3 months');
	 extract
	---------
	       2

so the idea that we should adjust the sign for zero months quarter
extract doesn't seem logical to me.

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EDB                                      https://enterprisedb.com

  Only you can decide what is important to you.

  28. Re: BUG #18348: Inconsistency with EXTRACT([field] from INTERVAL);

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-08-16T15:37:55Z

    Bruce Momjian <bruce@momjian.us> writes:
> On Thu, Aug 15, 2024 at 10:45:58PM -0400, Bruce Momjian wrote:
>> I tested master, patch version 2 and patch version 3 with some sample
>> extract() queires, attached.  I like patch version 2.

I'm still pretty dissatisfied with both versions :-(

> I think there are two more issues.  In patch version 3, when months is
> zero and you check days, you should also check seconds if days is zero.

Eh?  v3 does that:

+                else if (interval->day > 0 ||
+                         (interval->day == 0 && interval->time >= 0))

But I'm starting to despair of reaching a solution that's actually
self-consistent.  Maybe we should leave the DTK_QUARTER behavior
alone, and content ourselves with adding DTK_WEEK.

			regards, tom lane

  29. Re: BUG #18348: Inconsistency with EXTRACT([field] from INTERVAL);

    Bruce Momjian <bruce@momjian.us> — 2024-08-16T15:52:38Z

    On Fri, Aug 16, 2024 at 11:37:55AM -0400, Tom Lane wrote:
> Bruce Momjian <bruce@momjian.us> writes:
> > On Thu, Aug 15, 2024 at 10:45:58PM -0400, Bruce Momjian wrote:
> >> I tested master, patch version 2 and patch version 3 with some sample
> >> extract() queries, attached.  I like patch version 2.
> 
> I'm still pretty dissatisfied with both versions :-(
> 
> > I think there are two more issues.  In patch version 3, when months is
> > zero and you check days, you should also check seconds if days is zero.
> 
> Eh?  v3 does that:
> 
> +                else if (interval->day > 0 ||
> +                         (interval->day == 0 && interval->time >= 0))

Oh, sorry, I missed that detail.

> But I'm starting to despair of reaching a solution that's actually
> self-consistent.  Maybe we should leave the DTK_QUARTER behavior
> alone, and content ourselves with adding DTK_WEEK.

Well, I liked that -4 months actually was in -2 quarter. I see your
point that if 0-2 is Q1, why is only -1 to -2 in minus Q1, but I think I
can live with that on the assumption that negative months can be handled
differently.

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EDB                                      https://enterprisedb.com

  Only you can decide what is important to you.

  30. Re: BUG #18348: Inconsistency with EXTRACT([field] from INTERVAL);

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-08-16T16:06:35Z

    Bruce Momjian <bruce@momjian.us> writes:
> On Fri, Aug 16, 2024 at 11:37:55AM -0400, Tom Lane wrote:
>> But I'm starting to despair of reaching a solution that's actually
>> self-consistent.  Maybe we should leave the DTK_QUARTER behavior
>> alone, and content ourselves with adding DTK_WEEK.

> Well, I liked that -4 months actually was in -2 quarter.

Yeah.  On further reflection, I agree it's a bad idea for the
DTK_QUARTER computation to depend on anything but the months field.
So that lets out v3.  However, what we have historically is

regression=# select n, extract(quarter from interval '1 mon' * n) from generate_series(-12,12) n;
  n  | extract 
-----+---------
 -12 |       1
 -11 |      -2
 -10 |      -2
  -9 |      -2
  -8 |      -1
  -7 |      -1
  -6 |      -1
  -5 |       0
  -4 |       0
  -3 |       0
  -2 |       1
  -1 |       1
   0 |       1
   1 |       1
   2 |       1
   3 |       2
   4 |       2
   5 |       2
   6 |       3
   7 |       3
   8 |       3
   9 |       4
  10 |       4
  11 |       4
  12 |       1
(25 rows)

which is fine on the positive side but it's hard to describe the
results for negative months as anything but wacko.  The v2 patch
gives

regression=# select n, extract(quarter from interval '1 mon' * n) from generate_series(-12,12) n;
  n  | extract 
-----+---------
 -12 |      -1
 -11 |      -4
 -10 |      -4
  -9 |      -4
  -8 |      -3
  -7 |      -3
  -6 |      -3
  -5 |      -2
  -4 |      -2
  -3 |      -2
  -2 |      -1
  -1 |      -1
   0 |       1
   1 |       1
   2 |       1
   3 |       2
   4 |       2
   5 |       2
   6 |       3
   7 |       3
   8 |       3
   9 |       4
  10 |       4
  11 |       4
  12 |       1
(25 rows)

which is a whole lot saner.  So let's run with v2.

			regards, tom lane

  31. Re: BUG #18348: Inconsistency with EXTRACT([field] from INTERVAL);

    Bruce Momjian <bruce@momjian.us> — 2024-08-16T16:15:58Z

    On Fri, Aug 16, 2024 at 12:06:35PM -0400, Tom Lane wrote:
> regression=# select n, extract(quarter from interval '1 mon' * n) from generate_series(-12,12) n;
>   n  | extract 
> -----+---------
>  -12 |       1
>  -11 |      -2
>  -10 |      -2
>   -9 |      -2

Wow, that "1" is weird to see.

> which is fine on the positive side but it's hard to describe the
> results for negative months as anything but wacko.  The v2 patch
> gives
> 
> regression=# select n, extract(quarter from interval '1 mon' * n) from generate_series(-12,12) n;
>   n  | extract 
> -----+---------
>  -12 |      -1
>  -11 |      -4
>  -10 |      -4
>   -9 |      -4
>   -8 |      -3
>   -7 |      -3
>   -6 |      -3
>   -5 |      -2
>   -4 |      -2
>   -3 |      -2
>   -2 |      -1
>   -1 |      -1
>    0 |       1
>    1 |       1
>    2 |       1
>    3 |       2
>    4 |       2
>    5 |       2
>    6 |       3
>    7 |       3
>    8 |       3
>    9 |       4
>   10 |       4
>   11 |       4
>   12 |       1
> (25 rows)
> 
> which is a whole lot saner.  So let's run with v2.

Yes, that v2 output looks very clean.  I had to really dig my head into
this so I am not surprised it was confusing to find the right solution.

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EDB                                      https://enterprisedb.com

  Only you can decide what is important to you.