On Wed, Feb 23, 2022 at 7:42 PM Joseph Koshakow <kosh...@gmail.com> wrote:
>
> Hi all,
>
> I noticed something odd when going through some
> of the Interval code. The DAYS_PER_YEAR constant
> is defined in src/include/datatype/timestamp.h.
> > #define DAYS_PER_YEAR 365.25 /* assumes leap year every four years */
>
> We execute the EXTRACT and date_part functions in
> src/backend/utils/adt/timestamp.c in
> > static Datum
> > interval_part_common(PG_FUNCTION_ARGS, bool retnumeric)
>
> When executing date_part we multiply the total
> years in the Interval by DAYS_PER_YEAR
> > result += ((double) DAYS_PER_YEAR * SECS_PER_DAY) * (interval->month /
> > MONTHS_PER_YEAR);
> > result += ((double) DAYS_PER_MONTH * SECS_PER_DAY) * (interval->month %
> > MONTHS_PER_YEAR);
> > result += ((double) SECS_PER_DAY) * interval->day;
>
>
> However when executing EXTRACT we first truncate
> DAYS_PER_YEAR to an integer, and then multiply it
> by the total years in the Interval
> /* this always fits into int64 */
> > secs_from_day_month = ((int64) DAYS_PER_YEAR * (interval->month /
> > MONTHS_PER_YEAR) +
> > (int64) DAYS_PER_MONTH * (interval->month %
> > MONTHS_PER_YEAR) +
> > interval->day) * SECS_PER_DAY;
>
> Is this truncation on purpose? It seems like
> EXTRACT is not accounting for leap years in
> it's calculation.
>
> - Joe Koshakow
Oops I sent that to the wrong email. If this isn't intented I've created a patch
that fixes it, with the following two open questions
* DAYS_PER_YEAR_NUM is recalculated every time. Is there anyway
to convert a float directly to a numeric to avoid this?
* For some reason the change adds a lot of trailing zeros to the result. I'm
not sure why that is.
- Joe Koshakow
From 9ac8bc7e0fcd56343a9b659aa5c6624ec81c5ff2 Mon Sep 17 00:00:00 2001
From: Joseph Koshakow <kosh...@gmail.com>
Date: Wed, 23 Feb 2022 21:25:13 -0500
Subject: [PATCH] Consider leap years when extracting epoch from interval
---
src/backend/utils/adt/timestamp.c | 28 ++++++++++++++++++--------
src/include/datatype/timestamp.h | 5 +++--
src/test/regress/expected/interval.out | 18 ++++++++++++++---
src/test/regress/sql/interval.sql | 3 +++
4 files changed, 41 insertions(+), 13 deletions(-)
diff --git a/src/backend/utils/adt/timestamp.c b/src/backend/utils/adt/timestamp.c
index 36f8a84bcc..84396e385a 100644
--- a/src/backend/utils/adt/timestamp.c
+++ b/src/backend/utils/adt/timestamp.c
@@ -5264,13 +5264,17 @@ interval_part_common(PG_FUNCTION_ARGS, bool retnumeric)
if (retnumeric)
{
Numeric result;
+ Numeric secs_from_year;
int64 secs_from_day_month;
int64 val;
/* this always fits into int64 */
- secs_from_day_month = ((int64) DAYS_PER_YEAR * (interval->month / MONTHS_PER_YEAR) +
- (int64) DAYS_PER_MONTH * (interval->month % MONTHS_PER_YEAR) +
- interval->day) * SECS_PER_DAY;
+ secs_from_day_month = ((int64) DAYS_PER_MONTH * (interval->month % MONTHS_PER_YEAR) +
+ interval->day) * SECS_PER_DAY;
+ secs_from_year = numeric_mul_opt_error(
+ DAYS_PER_YEAR_NUM,
+ int64_to_numeric(interval->month / MONTHS_PER_YEAR * SECS_PER_DAY),
+ NULL);
/*---
* result = secs_from_day_month + interval->time / 1'000'000
@@ -5284,12 +5288,20 @@ interval_part_common(PG_FUNCTION_ARGS, bool retnumeric)
*/
if (!pg_mul_s64_overflow(secs_from_day_month, 1000000, &val) &&
!pg_add_s64_overflow(val, interval->time, &val))
- result = int64_div_fast_to_numeric(val, 6);
+ {
+ result = numeric_add_opt_error(
+ secs_from_year,
+ int64_div_fast_to_numeric(val,6),
+ NULL);
+ }
else
- result =
- numeric_add_opt_error(int64_div_fast_to_numeric(interval->time, 6),
- int64_to_numeric(secs_from_day_month),
- NULL);
+ {
+ result = numeric_add_opt_error(
+ int64_div_fast_to_numeric(interval->time, 6),
+ int64_to_numeric(secs_from_day_month),
+ NULL);
+ result = numeric_add_opt_error(result, secs_from_year, NULL);
+ }
PG_RETURN_NUMERIC(result);
}
diff --git a/src/include/datatype/timestamp.h b/src/include/datatype/timestamp.h
index 5fa38d20d8..457f10dafc 100644
--- a/src/include/datatype/timestamp.h
+++ b/src/include/datatype/timestamp.h
@@ -65,8 +65,9 @@ typedef struct
* Assorted constants for datetime-related calculations
*/
-#define DAYS_PER_YEAR 365.25 /* assumes leap year every four years */
-#define MONTHS_PER_YEAR 12
+#define DAYS_PER_YEAR 365.25 /* assumes leap year every four years */
+#define DAYS_PER_YEAR_NUM (numeric_add_opt_error(int64_to_numeric(365), numeric_div_opt_error(int64_to_numeric(1), int64_to_numeric(4), NULL), NULL))
+#define MONTHS_PER_YEAR 12
/*
* DAYS_PER_MONTH is very imprecise. The more accurate value is
* 365.2425/12 = 30.436875, or '30 days 10:29:06'. Right now we only
diff --git a/src/test/regress/expected/interval.out b/src/test/regress/expected/interval.out
index accd4a7d90..98e5cb6967 100644
--- a/src/test/regress/expected/interval.out
+++ b/src/test/regress/expected/interval.out
@@ -1038,8 +1038,20 @@ SELECT f1,
-- internal overflow test case
SELECT extract(epoch from interval '1000000000 days');
- extract
------------------------
- 86400000000000.000000
+ extract
+-------------------------------------
+ 86400000000000.00000000000000000000
+(1 row)
+
+SELECT extract(epoch from interval '4 years');
+ extract
+--------------------------------
+ 126230400.00000000000000000000
+(1 row)
+
+SELECT date_part('epoch', interval '4 years');
+ date_part
+-----------
+ 126230400
(1 row)
diff --git a/src/test/regress/sql/interval.sql b/src/test/regress/sql/interval.sql
index 6d532398bd..f2abf1f7e4 100644
--- a/src/test/regress/sql/interval.sql
+++ b/src/test/regress/sql/interval.sql
@@ -355,3 +355,6 @@ SELECT f1,
-- internal overflow test case
SELECT extract(epoch from interval '1000000000 days');
+
+SELECT extract(epoch from interval '4 years');
+SELECT date_part('epoch', interval '4 years');
--
2.25.1
