Duplicate function call on timestamp2tm

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