Обсуждение: Re: date_trunc invalid units with infinite value

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] for further context:

[0]: 
https://www.postgresql.org/message-id/8246d7ff-f4b7-4363-913e-827dadfeb145%40eisentraut.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]: https://www.postgresql.org/message-id/8246d7ff-f4b7-4363-913e-827dadfeb145%40eisentraut.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