Duplicate function call on timestamp2tm
Hi,
I find there is a duplicate function call on timestamp2tm in timestamptz_part and timestamp_part.
Is that necessary? I remove the latter one and it also works.
Best,
Japin.
Attachments:
remove-duplicate-timestamp2tm-function-call.patchapplication/octet-stream; name=remove-duplicate-timestamp2tm-function-call.patchDownload
diff --git a/src/backend/utils/adt/timestamp.c b/src/backend/utils/adt/timestamp.c
index 31bdb37c55..edeaee4cb1 100644
--- a/src/backend/utils/adt/timestamp.c
+++ b/src/backend/utils/adt/timestamp.c
@@ -4651,20 +4651,12 @@ timestamp_part(PG_FUNCTION_ARGS)
case DTK_DOW:
case DTK_ISODOW:
- if (timestamp2tm(timestamp, NULL, tm, &fsec, NULL, NULL) != 0)
- ereport(ERROR,
- (errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
- errmsg("timestamp out of range")));
result = j2day(date2j(tm->tm_year, tm->tm_mon, tm->tm_mday));
if (val == DTK_ISODOW && result == 0)
result = 7;
break;
case DTK_DOY:
- if (timestamp2tm(timestamp, NULL, tm, &fsec, NULL, NULL) != 0)
- ereport(ERROR,
- (errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
- errmsg("timestamp out of range")));
result = (date2j(tm->tm_year, tm->tm_mon, tm->tm_mday)
- date2j(tm->tm_year, 1, 1) + 1);
break;
@@ -4855,20 +4847,12 @@ timestamptz_part(PG_FUNCTION_ARGS)
case DTK_DOW:
case DTK_ISODOW:
- if (timestamp2tm(timestamp, &tz, tm, &fsec, NULL, NULL) != 0)
- ereport(ERROR,
- (errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
- errmsg("timestamp out of range")));
result = j2day(date2j(tm->tm_year, tm->tm_mon, tm->tm_mday));
if (val == DTK_ISODOW && result == 0)
result = 7;
break;
case DTK_DOY:
- if (timestamp2tm(timestamp, &tz, tm, &fsec, NULL, NULL) != 0)
- ereport(ERROR,
- (errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
- errmsg("timestamp out of range")));
result = (date2j(tm->tm_year, tm->tm_mon, tm->tm_mday)
- date2j(tm->tm_year, 1, 1) + 1);
break;
Li Japin <japinli@hotmail.com> writes:
I find there is a duplicate function call on timestamp2tm in timestamptz_part and timestamp_part.
Is that necessary? I remove the latter one and it also works.
Huh. I do believe you're right. Must be an ancient copy-and-paste
mistake?
regards, tom lane
I wrote:
Li Japin <japinli@hotmail.com> writes:
I find there is a duplicate function call on timestamp2tm in timestamptz_part and timestamp_part.
Is that necessary? I remove the latter one and it also works.
Huh. I do believe you're right. Must be an ancient copy-and-paste
mistake?
Ah, after looking in the git history, not quite that ancient:
this duplication dates to commit 258ee1b63, which moved these
switch cases from the "if (type == RESERV)" switches in the
same functions. In the previous coding these function calls
were actually necessary, but here they're redundant. I guess
that's just additional ammunition for Greg's point that the
keywords were misclassified ;-).
I see from the code coverage report that we're missing coverage
for these and some other paths in timestamp[tz]_part. Think
I'll go add some more test cases while I'm at it.
Thanks again for the report!
regards, tom lane
Thanks for your confirm. Is there anything I can do?
On Dec 12, 2019, at 11:13 PM, Tom Lane <tgl@sss.pgh.pa.us<mailto:tgl@sss.pgh.pa.us>> wrote:
Ah, after looking in the git history, not quite that ancient:
this duplication dates to commit 258ee1b63, which moved these
switch cases from the "if (type == RESERV)" switches in the
same functions. In the previous coding these function calls
were actually necessary, but here they're redundant. I guess
that's just additional ammunition for Greg's point that the
keywords were misclassified ;-).
Li Japin <japinli@hotmail.com> writes:
Thanks for your confirm. Is there anything I can do?
No, I've got it.
In adding the test coverage I spoke of, I thought we should allow
the date_part tests to check all the entries in timestamp[tz]_tbl
not just those around current time, and I found an independent
problem:
timestamp | isoyear | week | isodow | dow | doy
-----------------------------+-----------+------+--------+-----+-----
...
Tue Feb 16 17:32:01 0097 BC | -96 | 7 | 2 | 2 | 47
Sat Feb 16 17:32:01 0097 | 97 | 7 | 6 | 6 | 47
that is, the ISOYEAR case is failing to correct for BC years.
We could imagine fixing this in date2isoyear() but I think it's
safer to leave that function alone and do the corrections
in timestamp[tz]_part. Note for example that formatting.c
already applies a BC correction to the result; and I think the
usage in date2isoyearday() requires sticking to the year-zero-exists
convention, too.
regards, tom lane