Sure,
I'm attaching v2 of the patch with "result" renamed to "week".
--
Sergey
On 7/12/25 18:15, Tom Lane wrote:
=?utf-8?q?=D0=A4=D1=83=D0=BA=D0=B0=D0=BD=D1=87=D0=B8=D0=BA_=D0=A1=D0=B5=D1=80=D0=B3=D0=B5=D0=B9?=
<s.fukanc...@postgrespro.ru> writes:
Hi PG hackers,
I found suspicious use of float8 in date2isoweek() and date2isoyear(). In both
cases float8 is only used for storing the value, while the entire calculation
on the right happens in integers:
float8 result = (dayn - (day4 - day0)) / 7 + 1;
At the end date2isoweek() returns `result' converted back to int:
return (int) result;
float8 here is confusing and a bit slow.
I looked into our git history to try to find out why it's like this.
The answer seems to be that commit dffd8cac3 created date2isoweek()
by splitting out pre-existing code that had been in timestamp_part().
In that context the code had been using a float8 "result" variable
that was shared with other switch cases, and that variable's type
was just blindly copied into date2isoweek(). Then 1c757c49f again
copied-and-pasted while creating date2isoyear().
I agree with getting rid of the unnecessary usage of float8 here,
but there's another aspect that's bugging me: "result" is a totally
misleading variable name in date2isoyear(), because it's *not*
the function's result. I'm inclined to rename it to "week", and
then to keep these functions looking as parallel as possible,
I'd probably do the same in date2isoweek().
I think there is no need in adding an extra test case here, because
date2isoweek and date2isoyear are covered by three regression tests:
Agreed, the code coverage report shows these are covered.
regards, tom lane
From 501da79f2a794b66cb4ebc3673a60859070a59df Mon Sep 17 00:00:00 2001
From: Sergey Fukanchik <s.fukanc...@postgrespro.ru>
Date: Sun, 13 Jul 2025 16:52:09 +0300
Subject: [PATCH v2] Replace float8 with int in date2isoweek and date2isoyear
---
src/backend/utils/adt/timestamp.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/src/backend/utils/adt/timestamp.c b/src/backend/utils/adt/timestamp.c
index 0a5848a4ab2..7420891092b 100644
--- a/src/backend/utils/adt/timestamp.c
+++ b/src/backend/utils/adt/timestamp.c
@@ -5312,7 +5312,7 @@ isoweekdate2date(int isoweek, int wday, int *year, int *mon, int *mday)
int
date2isoweek(int year, int mon, int mday)
{
- float8 result;
+ int week;
int day0,
day4,
dayn;
@@ -5338,13 +5338,13 @@ date2isoweek(int year, int mon, int mday)
day0 = j2day(day4 - 1);
}
- result = (dayn - (day4 - day0)) / 7 + 1;
+ week = (dayn - (day4 - day0)) / 7 + 1;
/*
* Sometimes the last few days in a year will fall into the first week of
* the next year, so check for this.
*/
- if (result >= 52)
+ if (week >= 52)
{
day4 = date2j(year + 1, 1, 4);
@@ -5352,10 +5352,10 @@ date2isoweek(int year, int mon, int mday)
day0 = j2day(day4 - 1);
if (dayn >= day4 - day0)
- result = (dayn - (day4 - day0)) / 7 + 1;
+ week = (dayn - (day4 - day0)) / 7 + 1;
}
- return (int) result;
+ return week;
}
@@ -5367,7 +5367,7 @@ date2isoweek(int year, int mon, int mday)
int
date2isoyear(int year, int mon, int mday)
{
- float8 result;
+ int week;
int day0,
day4,
dayn;
@@ -5395,13 +5395,13 @@ date2isoyear(int year, int mon, int mday)
year--;
}
- result = (dayn - (day4 - day0)) / 7 + 1;
+ week = (dayn - (day4 - day0)) / 7 + 1;
/*
* Sometimes the last few days in a year will fall into the first week of
* the next year, so check for this.
*/
- if (result >= 52)
+ if (week >= 52)
{
day4 = date2j(year + 1, 1, 4);
--
2.34.1
