Fix panic when attempting to set default_timestamp_interval to 0#36853
Conversation
ublubu
left a comment
ublubu
left a comment
There was a problem hiding this comment.
Looks good. It does give me a new question, though. (inline)
| if d.is_zero() { | ||
| Err(VarError::InvalidParameterValue { | ||
| name: var.name(), | ||
| invalid_values: vec![format!("{:?}", d)], | ||
| reason: "only supports durations greater than zero".to_string(), |
There was a problem hiding this comment.
It looks a little weird to is_zero() and then say greater than zero. I had to double-check that Duration can't be negative 😅
There was a problem hiding this comment.
Ah whoops, I'll change this to "only supports non-zero durations" - I had originally named this struct PositiveDuration which I later realized was a little weird/redundant given Durations are never negative. But then I forgot to update this reason field.
| simple conn=mz_system,user=mz_system | ||
| ALTER SYSTEM SET default_timestamp_interval = '-1'; | ||
| ---- | ||
| db error: ERROR: parameter "default_timestamp_interval" requires a "duration" value |
There was a problem hiding this comment.
I realize this isn't a regression, but this error message is interesting, especially given that the above tests use values like 1 and 0, which don't appear to be durations either.
There was a problem hiding this comment.
Poked around here a bit, and it looks like default unit for a Duration in our parsing logic is "ms", see here:
materialize/src/sql/src/session/vars/value.rs
Lines 309 to 310 in 8f3bf9d
This seems to be consistent across durations, but is documented on a per-field level. I.e. see docs for "idle_in_transaction_session_timeout" under https://materialize.com/docs/sql/alter-system-set/.
Discussed offline that this may also align with postgres.
3 participants
Motivation
Closes ss-147
Description
Adds a non-zero constraint to the "default_timestamp_interval" system parameter. This prevents an issue where setting it to 0 causes a panic and the database won't start up until you go in and update the parameter by different means.
Verification
ALTER ...statement is issued