Skip to content

repr: fix strconv date/time and range parsing and rendering edge cases - #37977

Open
def- wants to merge 7 commits into
mainfrom
def/fuzz-07-repr-strconv
Open

repr: fix strconv date/time and range parsing and rendering edge cases#37977
def- wants to merge 7 commits into
mainfrom
def/fuzz-07-repr-strconv

Conversation

@def-

@def- def- commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

Seven fixes to mz-repr's strconv parsing and text rendering for date/time
and range types. Every one is reachable from user input, whether a bind
parameter, the HTTP SQL API, a string cast, COPY FROM, or a source cast. Several
are process aborts or silent data corruption rather than errors.

  • parse_timestamptz's offset subtraction was unchecked. It applied the
    offset with chrono's NaiveDateTime - FixedOffset, which panics on overflow.
    HIGH_DATE is exactly chrono::NaiveDate::MAX, so a westward offset on a
    late time that day left chrono's range and aborted the process. The
    subtraction runs before CheckedTimestamp::from_timestamplike, so the range
    check could not catch it. A 24-byte literal such as
    262142-12-31 23:00:00-01 was enough.

    The same subtraction also produced an unrepresentable value: chrono stores a
    parsed :60 as a leap second, legal only at a second-of-minute of 59, and
    copied that sub-second verbatim while shifting the second. An offset that is
    not a whole number of minutes, as in 1970-01-01 00:00:60+00:00:30, therefore
    yielded a leap second at :30, which panics in row encoding on every
    re-render. Folding into the next regular second matches PostgreSQL.

  • round_to_precision's rounding arithmetic was unbounded. It rounded with
    chrono's + and -, which panic on overflow, and rounding up off the last
    fraction of HIGH_DATE leaves chrono's range and aborted the process. The
    rounding runs after the CheckedTimestamp range check, so the check could not
    catch it. Every precision reaches this with its own quantum, and the lower the
    precision the fewer digits it takes: at precision 0 a single .5 rounds up a
    whole second, so SELECT '262142-12-31 23:59:59.5'::timestamptz(0) was
    enough. The precision is an ordinary type modifier on the column, so this is
    not a corner only the microsecond default can reach.

  • A rounded-up fraction did not carry into the seconds field.
    format_nanos_to_micros wrote the rounded microseconds as a fractional field
    and could not carry out of it, so .9999995 or more became 1000000
    microseconds, which the trailing-zero stripper reduced to 1. The value
    rendered about a second early with a nonsense fraction, so
    '... 00:00:00.9999999' displayed as 00:00:00.1. Nothing rounds on the way
    in, so this was silent corruption: the stored value displayed as something
    else, and the displayed text did not re-parse to it, so text round trips
    through COPY TO and sinks lost it.

  • A TIME string naming no time field was accepted. parse_time("")
    returned 00:00:00, as did " " and ":". PostgreSQL raises invalid input syntax for type time: "". Such a string tokenizes to nothing the grammar has
    to consume, so compute_time defaulted hour, minute and second to zero and
    the value looked deliberate rather than defaulted. It reached ''::time, the
    text-format bind parameter path, and COPY in TEXT and CSV format.

  • A quoted range bound kept its quotes. format_range quotes a bound
    whenever the element renderer reports MayNeedEscaping, but
    parse_range_inner extracted bounds with a bare take_while(|c| c != ',')
    and handed the substring to the element parser with the quotes still on. That
    mostly went unnoticed because the datetime parser tolerates a stray ". It
    stops being tolerable when the quote abuts a meaningful token, so the BC" of
    a pre-Common-Era bound is read as a time zone and a tsrange failed to
    survive a ::text::tsrange round trip.

  • A BC date was not reported as needing escaping. format_date appends a
    BC suffix and then returned Nestable::Yes regardless, which is documented
    as being for renderings that can never need escaping, and the whitespace in
    BC is exactly what every element escaper quotes for. stringify_datum
    returns this for the Date arm, and the Array, List, Map, Record and
    Range arms use it to decide whether to call escape_elem, as does the pgwire
    text encoder for a nested Value::Date. So quoting was skipped for every BC
    date inside a container, on both ::text and the wire text format.

  • Plus a test-only commit extending leap-second coverage in timezone() to the
    TIMESTAMP → TIMESTAMPTZ direction, which goes through
    checked_sub_with_leapsecond and had none.

Tests

Each fix carries a red test that fails without it, in src/repr/tests/strconv.rs
and in dates-times.slt, range.slt and timestamptz.slt. The SLT additions
have to be simple blocks where they assert on text encoding, since the
extended protocol that plain query uses returns TIME and TIMESTAMP in binary.
test/pgtest-mz/datums.pt moves with the carry fix: the DATUMS load generator's
leap-second TIMESTAMP is 23:59:59 plus 1234 milliseconds, which rendered as
23:59:60.1234 from an unreduced nanosecond count and is now
2019-07-24 23:59:60.234.

All of these were found by the strconv_parse_* cargo-fuzz targets, which are
sharpened in #37979. That PR should land after this one.

Release notes

This release will fix several date/time text-format bugs: a TIMESTAMPTZ
literal near the end of the representable range could abort the server, a
sub-second value rounding up to a full second rendered roughly one second early,
a TIME string naming no time field was accepted, a quoted range bound kept its
quotes, and a BC date rendered incorrectly inside a composite value.

@def- def- changed the title def/fuzz 07 repr strconv repr: fix strconv date/time and range parsing and rendering edge cases Jul 31, 2026
@def-
def- marked this pull request as ready for review July 31, 2026 09:58
@def-
def- requested review from a team as code owners July 31, 2026 09:58
@def-
def- changed the base branch from def/fuzz-06-repr-regex to main July 31, 2026 10:05
@def-
def- requested review from a team as code owners July 31, 2026 10:05
def- and others added 7 commits July 31, 2026 10:06
…ection

Leap-second handling in `timezone()` was covered only in the
TIMESTAMPTZ -> TIMESTAMP direction, which goes through
`checked_add_with_leapsecond`. The mirror direction uses
`checked_sub_with_leapsecond` and needs the same fold.

`timezone()` reads a numeric offset as POSIX, where a positive offset is west of
Greenwich, so `+00:00:01` has a `local_minus_utc` of -1 and moves the leap second
forward past the minute boundary, while `-00:00:01` moves it back onto `:58`.
Neither lands on `:59`, so both take the folding branch. A zero offset does keep
the result on `:59`, exercising the `else` branch that restores the leap-second
nanos.
`parse_timestamptz` applied the time-zone offset with chrono's
`NaiveDateTime - FixedOffset`, which panics on overflow. `HIGH_DATE` is
exactly `chrono::NaiveDate::MAX`, so a westward offset on a late time that
day left chrono's range and aborted the process. The subtraction runs
before `CheckedTimestamp::from_timestamplike`, so the range check could
not catch it. A 24-byte literal such as `262142-12-31 23:00:00-01` was
enough, reachable from a bind parameter, the HTTP SQL API, a string cast,
or a source cast.

The same subtraction also produced a value that cannot be represented.
chrono stores a parsed `:60` as a leap second, legal only at a
second-of-minute of 59, and copied that sub-second verbatim while
shifting the second. An offset that is not a whole number of minutes,
as in `1970-01-01 00:00:60+00:00:30`, therefore yielded a leap second at
`:30`, which panics in row encoding on every re-render. Folding it into
the next regular second matches what PostgreSQL does with `:60`.

Both are fixed by the leap-aware checked offset helpers that
`timezone_timestamp` already used for this defect class. They move from
`mz_expr` to `mz_repr::adt::timestamp`, next to the range they enforce,
so the two crates share one copy rather than keeping the leap-second
reasoning in two places.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`round_to_precision` rounded with chrono's `+` and `-`, which panic on
overflow. Rounding *up* off the last fraction of `HIGH_DATE`, itself
exactly `chrono::NaiveDate::MAX`, leaves chrono's range and aborted the
process. The rounding runs after the `CheckedTimestamp` range check, so
the check could not catch it either.

Every precision reaches this, each with its own quantum, and the lower the
precision the fewer fractional digits it takes: at precision 0 a single
`.5` rounds up a whole second, so
`SELECT '262142-12-31 23:59:59.5'::timestamptz(0)` was enough. The
precision is an ordinary type modifier on the column, so this is not a
corner only the microsecond default can reach.

Report `TimestampError::OutOfRange` instead, which the callers already
propagate.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`format_nanos_to_micros` rounded a nanosecond fraction to microseconds and
wrote the result as a fractional field. It could not carry out of that
field, so a fraction of `.9999995` or more became `1000000` microseconds,
which the trailing-zero stripper then reduced to `1`. The value rendered
about a second early with a nonsense fraction:

    '2020-01-01 00:00:00.9999999'  ->  2020-01-01 00:00:00.1
    '12:34:56.9999999'             ->  12:34:56.1

The same happened to chrono's leap-second representation, a sub-second of
one second or more, which `%S` has already rendered as `60` by then.

Nothing rounds on the way in, so these values reach the renderer as
stored. `mz_pgrepr::Value::decode_text` pushes the parsed nanoseconds
straight into the `Row` behind COPY in TEXT and CSV format, COPY FROM S3,
and text-format bind parameters, and `Row` keeps all of them. The TIME
cast does not round either, nor does DATE + TIME. Only the TIMESTAMP cast
rounds, which is why it hid this. The result was silent corruption: the
stored value displayed as something else, and the displayed text did not
re-parse to it, so text round trips through COPY TO and sinks lost it.

Split the helper in two. `split_nanos_to_micros` returns the whole-second
carry separately from a fraction that is guaranteed below one second, and
`format_micros` asserts that bound before writing. The carry is applied to
the timestamp itself, so it reaches `%H:%M:%S` and rolls the minute, day,
month or year as needed. Clearing the fraction before adding the second is
what makes this correct for a leap second too: the second after `23:59:60`
is `00:00:00` of the next minute.

Where the carry cannot be represented it is dropped and the fraction
saturates at `.999999`, rather than the value rolling over to something
unrepresentable. That is the last microsecond of `HIGH_DATE`, which is
exactly `chrono::NaiveDate::MAX`, and the last second of the day for
`NaiveTime`, which wraps to midnight rather than reaching PostgreSQL's
`24:00:00`.

Covered by `test_format_subsecond_carry`, which drives all three renderers
across the second, minute, day, month and year boundaries plus both
saturation edges, by `test_split_nanos_to_micros` on the helper, and by
`simple` blocks in `dates-times.slt` for the SQL-visible TIME and
DATE + TIME paths. Those have to be `simple`: they assert on the text
encoding, and the extended protocol that plain `query` uses returns TIME
and TIMESTAMP in binary.

The `datums.pt` golden moves with the fix. The DATUMS load generator's
leap-second TIMESTAMP is `23:59:59` plus 1234 milliseconds, whose whole
nanosecond count divided straight down to 1234000 microseconds and stripped to
a `.1234` fraction. Reducing it below one second first leaves the leap in the
seconds field where `%S` already put it, so the value renders as
`2019-07-24 23:59:60.234`.
`parse_time("")` returned `00:00:00`. PostgreSQL raises `invalid input
syntax for type time: ""`. The same held for `" "` and `":"`.

Such a string tokenizes to nothing the time grammar has to consume:
`determine_format_w_datetimefield` returns no format, `fill_pdt_time`
falls into its catch-all arm without consuming anything, the
leftover-token check passes because the deque is empty, and
`compute_time` then defaults hour, minute and second to zero. The value
looked deliberate rather than defaulted, and it reached `''::time`, the
text-format bind parameter path, and COPY in TEXT and CSV format.

Reject it in `parse_time` instead. Hour, minute and second are the only
fields the time grammar fills, and the only ones `compute_time` reads, so
requiring one of the three to be set is the whole condition.

The storage source cast `CastStringToTime` keeps the old reading via a new
frozen `parse_time_legacy`, as `CastStringToDate` and `CastStringToOid`
already do. Its stability contract is not stylistic: replication re-casts
the old tuple on delete, so a row ingested pre-upgrade as `00:00:00` would
post-upgrade be retracted as a cast error, leaving a phantom value in the
ok shard and a negative multiplicity in the err shard. Nothing is lost by
freezing it, because a PostgreSQL `time` column never renders as one of
these strings, so no export could have held one.

`parity_time` drops its fieldless input, since storage and SQL now
intentionally diverge there, and
`test_cast_string_to_time_frozen_fieldless` pins the frozen side.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`format_range` emits a bound quoted whenever the element renderer reports
`Nestable::MayNeedEscaping` and the rendering needs escaping, but
`parse_range_inner` extracted bounds with a bare
`take_while(|c| c != ',')` and handed the substring to the element parser
with the quotes still on it.

That mostly went unnoticed because the datetime parser tolerates a stray
`"`. It stops being tolerable the moment the quote abuts a token the
parser attributes meaning to: the `BC"` of a pre-Common-Era bound is read
as a time zone, so

    SELECT tsrange('0001-02-24 BC'::timestamp, '2020-01-01'::timestamp)::text::tsrange

failed with "invalid input syntax for type range: 'BC' is not a valid
timezone". A `tsrange` or `tstzrange` with a pre-Common-Era bound could
not be read back from its own rendering.

Use `lex_quoted_element`, the same helper the list and array parsers use,
so the quotes come off and `\` escapes are processed. Unquoted, absent and
`empty` bounds take the path they always did.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`format_date` appends a ` BC` suffix for a pre-Common-Era date and then
returned `Nestable::Yes` regardless. `Nestable::Yes` is documented as
being for renderings that can *never* need escaping, and the whitespace in
` BC` is exactly what every element escaper quotes for, so the promise was
false.

It is not bookkeeping. `stringify_datum` returns this value for the `Date`
arm and the `Array`, `List`, `Map`, `Record` and `Range` arms use it to
decide whether to call `escape_elem`, as does the pgwire text encoder for
`Value::Date` nested in a container. So the quoting step was skipped for
every BC date inside a container, on both `::text` and the wire text
format:

    SELECT ARRAY[('0001-02-24'::date - interval '1 YEAR')::date];
    -- was {0001-02-24 BC}, PostgreSQL gives {"0001-02-24 BC"}

`format_timestamp`, `format_timestamptz` and `format_interval` all return
`MayNeedEscaping` for the same reason. `format_date` was the outlier, and
the same value already quoted correctly when rendered as a timestamp.
Same class as SQL-466, whose fix extended `needs_escaping` and left this
side untouched.

Return `MayNeedEscaping` only for the BC case, so a Common Era date keeps
the cheaper answer and its rendering is unchanged.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant