[PATCH] replace float8 with int in date2isoweek() and date2isoyear()
Hackorum builds and tests every patch posted to the lists, not only commitfest submissions. This is Hackorum's own CI rather than the PostgreSQL project's, and it is still under testing - please report anything that looks wrong.
You can run a PostgreSQL built from this patch straight from Docker, with no checkout and no build:
docker run --rm -p 5432:5432 ghcr.io/hackorum-dev/postgres-patch:t51907psql -h localhost -U postgresBuilt from patchset v3 (message #3), August 18, 2026 at 02:33 PM.
Every patchset is also pushed to a branch of our PostgreSQL fork, so you can check out the same tree CI built. Without a PostgreSQL checkout:
git clone --branch t51907_3 https://github.com/hackorum-dev/postgres.gitIn a checkout you already have, add the fork once:
git remote add hackorum https://github.com/hackorum-dev/postgres.gitthen, for this patchset and every later one:
git fetch hackorum t51907_3 && git checkout t51907_3Patchset v3 (message #3) is on t51907_3
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 run a benchmark with optimization -O3, and found that using int is 3% faster:
BM_Float/1000/3/8_mean 5.12 ns 5.12 ns 100
BM_Int/1000/3/8_mean 4.95 ns 4.95 ns 100
BM_Long/1000/3/8_mean 4.96 ns 4.96 ns 100
Without optimization (-O0), speedup even better around 30%:
BM_Float/1000/3/8_mean 19.6 ns 19.6 ns 100
BM_Int/1000/3/8_mean 14.8 ns 14.8 ns 100
BM_Long/1000/3/8_mean 16.7 ns 16.7 ns 100
Additionally, as a paranoia measure I run a comparison test with the following
ranges: year={-100000,100000}, month={-100,+100}, day={-100,+100}.
As expected float and int did not produce any difference.
Attached patch replaces `float8 result' with `int result'.
I think there is no need in adding an extra test case here, because
date2isoweek and date2isoyear are covered by three regression tests:
* date
SELECT f1 as "date",
...
date_part('isoyear', f1) AS isoyear,
date_part('week', f1) AS week,
date_part('dow', f1) AS dow,
date_part('isodow', f1) AS isodow,
...
FROM date_tbl;
* timestamp
SELECT date_trunc( 'week', timestamp '2004-02-29 15:44:17.71393' ) AS week_trunc;
* timestamptz
SELECT date_trunc( 'week', timestamp with time zone '2004-02-29 15:44:17.71393' ) AS week_trunc;
---
Sergey
Attachments:
0001-Replace-float8-with-int-in-date2isoweek-and-date2iso.patchtext/x-patchDownload+3-4
=?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.fukanchik@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
Sure,
I'm attaching v2 of the patch with "result" renamed to "week".
--
Sergey
Show quoted text
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.fukanchik@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
Sergey Fukanchik <s.fukanchik@postgrespro.ru> writes:
I'm attaching v2 of the patch with "result" renamed to "week".
I pushed it already, actually.
regards, tom lane
Thank you!
--
Sergey
Show quoted text
On 7/13/25 18:39, Tom Lane wrote:
Sergey Fukanchik <s.fukanchik@postgrespro.ru> writes:
I'm attaching v2 of the patch with "result" renamed to "week".
I pushed it already, actually.
regards, tom lane