Re: [PATCH] Fix interval input at minimum signed field values (BUG #19746)

Hi Tinh, Ishjaap,

Thank you for the patch. I tried v2 (CF entry 7620) and would like to
share what I saw. I can send the scripts and the output if they are
useful. I am new to reviewing here, so please correct me if I misread
something.

Setup
- Master at debfb3c8a8c9 (12 Nov 2026) plus the v2 attachment from the
first message of the thread (3 files: datetime.c, interval.sql,
interval.out). `git apply --check` was clean on that master. (log
00-setup.log)
- configure with --enable-cassert --enable-debug --enable-tap-tests
--without-icu, on Linux x86_64 (WSL Ubuntu, gcc). I built the
unpatched tree and the patched tree separately. Both builds had 0
compiler warnings. (logs 01-*, 02-*)

Tests that passed
- `make check` with the patch: all 243 tests passed, including
interval. (log 10-make-check-patched.log)
- With datetime.c left unpatched and the new test files (interval.sql
and interval.out) kept, `make check` fails in interval only (1 of 243
tests failed). The diff shows the new round trip tests failing, for
example `ERROR: invalid input syntax for type interval:
"-2562047788:00:54.775808"` and `ERROR: interval field value out of
range: "@ 2562047788 hours 54.775808 secs ago"`, and pg_input_is_valid
returning f for '-2562047788:00:54.775808'. So the new tests do
exercise the fix. (logs 11-make-check-tests-only-nofix.log,
11-nofix-regression.diffs)

Manual tests (temporary clusters, one run each on the unpatched and
the patched build; script repro.sql, outputs 22-repro-base.out and
22-repro-patched.out, differences in 23-repro-diff.txt)
- Cases from the bug report. Unpatched:
`'-2562047788:00:54.775808'::interval` gives "invalid input syntax"
and `'@ 2147483648 days ago'` (postgres_verbose) gives "interval field
value out of range". Patched: both are accepted and read back to the
same value, and `'@ 2147483648 months ago'` also works.
- Round trip `interval_send(i) = interval_send(i::text::interval)`
over six values at the limits (INT64_MIN and INT64_MAX microseconds,
INT_MIN days, INT_MIN months, and two combined values), for
intervalstyle postgres, postgres_verbose, sql_standard and iso_8601.
Unpatched: the first three styles raise an error, iso_8601 is fine.
Patched: all four styles return true for all six values.
- Inputs that should stay invalid still give "interval field value out
of range" on the patched build: '-2147483649 days', '2147483649 days
ago', '2147483649 months ago', '-2562047788:00:54.775809' (invalid
input syntax), '2562047788:00:54.775808', '-9223372036854775809 us'.
- '2147483648 days ago', '2147483648 months ago' and
'2562047788:00:54.775808 ago' are now accepted. These are -INT_MIN
days, -INT_MIN months and the minimum time value, so I think that is
right.
- Valid values near the limits ('2147483647 days', '-2147483647
months', '-1 2:03:04', '1 day 02:03:04 ago', '1 -01:02:03', and the
sql_standard examples) gave the same output on both builds.
- The cancellation cases from Ishjaap's message. On unpatched master
they all return true. On the patched build, when the minimum value
comes first in the string they return true (`'-9223372036854775808 us
1 millisecond ago'`, `'-2147483648 days 1 week ago'`), but when it
comes last they fail with "interval field value out of range":
- `'1 millisecond -9223372036854775808 us ago'`
- `'1 hour -9223372036854775808 us ago'`
- `'1 week -2147483648 days ago'`
- `'-2147483648 months 0.5 years ago'`
So I can confirm the problem Ishjaap reported, and I did not see it
fixed in v2. The new tests only cover the minimum-first order.
- `'2147483647 days 1 days ago'` now says "invalid input syntax"
instead of "field value out of range". Same as Ishjaap noted; I agree
this seems fine.
- The server did not crash and there was no PANIC or TRAP line in
either server log (assert-enabled build). (logs 21-server-base.log,
21-server-patched.log)

Questions
- Could the "ago" handling for the minimum value be done so that the
other field can still cancel it? Since the fields are read right to
left, the minimum field is negated before the other field has been
added. One idea (only an idea, I have not tried it) is to accumulate
in a wider type, or to remember the "ago" and apply it once at the end
with a check that does not overflow on the minimum. Please ignore this
if you already have a better plan.
- Could the tests cover both field orders, as Ishjaap suggested? The
four cases above would fit in the existing cancellation block.
- Is the different error for repeated units ("invalid input syntax")
intended? I think it is fine, but it is a visible change in the
message.

Not tested
- Back-patching, and PG 14 to 19 branches (I only used master).
- The pg_dump and restore example from the bug report (I tested the
text round trip only).
- Binary input and output (interval_recv), the ISO 8601 "P" input
parser beyond the round trip above, and other callers of
DecodeTimeForInterval and AdjustDays.
- Fractional values with time fields at the limits beyond the cases
above, and the non-US datestyle or locale settings.
- make check-world, the isolation tests and the other TAP suites (I
ran only make check).
- Meson build, non-assert builds, Windows, 32-bit, other compilers, ICU builds.
- pgindent and a full code review.

Thanks again, and please let me know if you want me to rerun anything
with different settings.

Best regards,
João Rito