date_trunc invalid units with infinite value

On Sun, Dec 01, 2024 at 03:09:17PM -0500, Joseph Koshakow wrote:

When looking at the date_trunc function, I noticed that it wasn't
rejecting invalid units if the value was infinite. It's slightly
annoying to fix this, but we already do something very similar for
date_part. I have attached a patch that would fix this issue, let me
know if you think it's worth pursuing.

I agree that it is inconsistent that we allow infinite values to go
through this function call even for fields that are not listed as
supported by the documentation. So, yes, I think that what you are
doing the right thing by applying the check based on the units
supported, but I also doubt that it is something that we could
backpatch as it would cause queries to work now to suddenly break.

Thoughts and comments from others are welcome.
--
Michael

On Thu, Dec 05, 2024 at 02:34:41PM +0900, Michael Paquier wrote:

I agree that it is inconsistent that we allow infinite values to go
through this function call even for fields that are not listed as
supported by the documentation. So, yes, I think that what you are
doing the right thing by applying the check based on the units
supported, but I also doubt that it is something that we could
backpatch as it would cause queries to work now to suddenly break.

Thoughts and comments from others are welcome.

Hearing nothing, I have looked at this patch again and I think that
I'm OK with your proposal. While the discrepancy is annoying for
back-branches, this causes a slight change of behavior, so I have no
backpatch in mind.

I am planning to get this one applied around the end of this week on
Friday for HEAD, that should be enough if there are comments and/or
objections.
--
Michael

On Tue, Dec 24, 2024 at 04:33:47PM +0900, Michael Paquier wrote:

I am planning to get this one applied around the end of this week on
Friday for HEAD, that should be enough if there are comments and/or
objections.

And done for now. If there are any remarks and/or objections, of
course feel free.
--
Michael

On 27.12.24 05:42, Michael Paquier wrote:

On Tue, Dec 24, 2024 at 04:33:47PM +0900, Michael Paquier wrote:

I am planning to get this one applied around the end of this week on
Friday for HEAD, that should be enough if there are comments and/or
objections.

And done for now. If there are any remarks and/or objections, of
course feel free.

It turned out this had a bug, and also the newly added test cases didn't
actually cover the new code, otherwise this would have shown up.

Please review the attached patches with additional test cases and the fix.

See also [0]/messages/by-id/8246d7ff-f4b7-4363-913e-827dadfeb145@eisentraut.org for further context:

[0]: /messages/by-id/8246d7ff-f4b7-4363-913e-827dadfeb145@eisentraut.org
/messages/by-id/8246d7ff-f4b7-4363-913e-827dadfeb145@eisentraut.org

On Wed, Aug 06, 2025 at 01:07:25PM +0200, Peter Eisentraut wrote:

It turned out this had a bug, and also the newly added test cases didn't
actually cover the new code, otherwise this would have shown up.

Please review the attached patches with additional test cases and the fix.

See also [0] for further context:

[0]: /messages/by-id/8246d7ff-f4b7-4363-913e-827dadfeb145@eisentraut.org

Yes, confirmed the broken case on 32-bit builds with the incorrect
result returned by timestamptz_trunc_internal():
SELECT date_trunc( 'week', timestamp with time zone 'infinity' );

And confirmed that we don't have any coverage for two code paths as of
HEAD:
- "not supported" in timestamp_trunc() for the entire new section of
where TIMESTAMP_NOT_FINITE() is satisfied.
- "not supported" in timestamptz_trunc_internal() for the entire
section where TIMESTAMP_NOT_FINITE() is satisfied.

0001 adds three new tests for timestamp:
- TIMESTAMP_NOT_FINITE + a valid unit, new path.
- TIMESTAMP_NOT_FINITE + "not supported" unit, new error path
- !TIMESTAMP_NOT_FINITE + "not supported" unit, old error path

0001 four new tests for timestamptz:
1) Three tests for timestamptz_trunc():
- TIMESTAMP_NOT_FINITE + a valid unit, new path.
- TIMESTAMP_NOT_FINITE + "not supported" unit, new path.
- !TIMESTAMP_NOT_FINITE +
2) One test for timestamptz_trunc_zone():
- !TIMESTAMP_NOT_FINITE + "not supported" unit

With what I am reading in your patch, what you are suggesting to add,
and a double-check at the interval tests, that seems complete to me.

This is a v18 open item, for something that I am an owner of as the
committer of d85ce012f99f, so I'll go take care of it. Thanks!
--
Michael

On Thu, Aug 07, 2025 at 09:06:25AM +0900, Michael Paquier wrote:

Yes, confirmed the broken case on 32-bit builds with the incorrect
result returned by timestamptz_trunc_internal():
SELECT date_trunc( 'week', timestamp with time zone 'infinity' );

0001 four new tests for timestamptz:
1) Three tests for timestamptz_trunc():
- TIMESTAMP_NOT_FINITE + a valid unit, new path.
- TIMESTAMP_NOT_FINITE + "not supported" unit, new path.
- !TIMESTAMP_NOT_FINITE +

Blurp here. I meant !TIMESTAMP_NOT_FINITE with unsupported unit, old
code path.

2) One test for timestamptz_trunc_zone():
- !TIMESTAMP_NOT_FINITE + "not supported" unit

There can be an extra test here, for the case of an infinite value
with a valid unit and a time zone specified, which would also have
failed with the bug as timestamptz_trunc_internal() is also used by
timestamptz_trunc_zone(), like:
SELECT date_trunc( 'week', timestamp with time zone 'infinity', 'GMT')
AS inf_zone_trunc;

I have added one more test, reversed the order to avoid spurious
failures should one have the idea to do a bisect with a 32b build, and
applied both things.

Thanks for the report!
--
Michael

Sorry about that and thanks for the fix!

- Joe Koshakow