Skip to content

Fix panic when attempting to set default_timestamp_interval to 0 #36853

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
peterdukelarsen merged 4 commits into from
Jun 3, 2026
+41 −3
Merged

Fix panic when attempting to set default_timestamp_interval to 0 #36853

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
96caef4
Fix panic when attempting to set default_timestamp_interval to 0
peterdukelarsen Jun 1, 2026
8779c9d
Fix tests
peterdukelarsen Jun 1, 2026
ffddae1
fix
peterdukelarsen Jun 1, 2026
7628edb
clean up error reason
peterdukelarsen Jun 3, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions src/sql/src/session/vars/constraints.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@

use std::fmt::Debug;
use std::ops::{RangeBounds, RangeFrom, RangeInclusive};
use std::time::Duration;

use mz_repr::adt::numeric::Numeric;
use mz_repr::bytes::ByteSize;
Expand All @@ -19,6 +20,8 @@ use super::{Value, Var, VarError};

pub static NUMERIC_NON_NEGATIVE: NumericNonNegNonNan = NumericNonNegNonNan;

pub static NON_ZERO_DURATION: NonZeroDuration = NonZeroDuration;

pub static NUMERIC_BOUNDED_0_1_INCLUSIVE: NumericInRange<RangeInclusive<f64>> =
NumericInRange(0.0f64..=1.0);

Expand Down Expand Up @@ -114,6 +117,25 @@ impl DomainConstraint for NumericNonNegNonNan {
}
}

#[derive(Debug, Clone, Eq, PartialEq)]
pub struct NonZeroDuration;

impl DomainConstraint for NonZeroDuration {
type Value = Duration;

fn check(&self, var: &dyn Var, d: &Duration) -> Result<(), VarError> {
if d.is_zero() {
Err(VarError::InvalidParameterValue {
name: var.name(),
invalid_values: vec![format!("{:?}", d)],
reason: "only supports durations greater than zero".to_string(),
Copy link
Copy Markdown
Contributor

@ublubu ublubu Jun 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😅

Copy link
Copy Markdown
Contributor Author

@peterdukelarsen peterdukelarsen Jun 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

})
} else {
Ok(())
}
}
}

#[derive(Debug, Clone, Eq, PartialEq)]
pub struct NumericInRange<R>(pub R);

Expand Down
7 changes: 4 additions & 3 deletions src/sql/src/session/vars/definitions.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,8 @@ use uncased::UncasedStr;

use crate::session::user::{SUPPORT_USER, SYSTEM_USER, User};
use crate::session::vars::constraints::{
BYTESIZE_AT_LEAST_1MB, DomainConstraint, NUMERIC_BOUNDED_0_1_INCLUSIVE, NUMERIC_NON_NEGATIVE,
ValueConstraint,
BYTESIZE_AT_LEAST_1MB, DomainConstraint, NON_ZERO_DURATION, NUMERIC_BOUNDED_0_1_INCLUSIVE,
NUMERIC_NON_NEGATIVE, ValueConstraint,
};
use crate::session::vars::errors::VarError;
use crate::session::vars::polyfill::{LazyValueFn, lazy_value, value};
Expand Down Expand Up @@ -1441,7 +1441,8 @@ pub static DEFAULT_TIMESTAMP_INTERVAL: VarDefinition = VarDefinition::new(
value!(Duration; Duration::from_millis(1000)),
"The interval at which timestamps are assigned to data from sources and tables.",
false,
);
)
.with_constraint(&NON_ZERO_DURATION);

pub static MIN_TIMESTAMP_INTERVAL: VarDefinition = VarDefinition::new(
"min_timestamp_interval",
Expand Down
15 changes: 15 additions & 0 deletions test/sqllogictest/alter.slt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,21 @@ ALTER SYSTEM SET max_timestamp_interval = '10s';
----
COMPLETE 0

simple conn=mz_system,user=mz_system
ALTER SYSTEM SET default_timestamp_interval = '1';
----
COMPLETE 0

simple conn=mz_system,user=mz_system
ALTER SYSTEM SET default_timestamp_interval = '0';
----
db error: ERROR: parameter "default_timestamp_interval" cannot have value "0ns": only supports durations greater than zero

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
Copy link
Copy Markdown
Contributor

@ublubu ublubu Jun 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

@peterdukelarsen peterdukelarsen Jun 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// Default unit is milliseconds
"ms" | "" => (Duration::from_millis, 1),
.

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.


statement ok
ALTER SOURCE s SET (TIMESTAMP INTERVAL '2s')

Expand Down
Loading