Skip to content

metric sink: skip rows with a null label value (SQL-645) - #38375

Merged
mtabebe merged 1 commit into
MaterializeInc:mainfrom
mtabebe:fix/metric-sink-null-label
Aug 24, 2026
Merged

metric sink: skip rows with a null label value (SQL-645)#38375
mtabebe merged 1 commit into
MaterializeInc:mainfrom
mtabebe:fix/metric-sink-null-label

Conversation

@mtabebe

@mtabebe mtabebe commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Problem:
A map[text=>text] has no per-value nullability, so a metric sink's
labels map can hold nulls. Such a row hit extract_row, which unwrapped the
value as a string and panicked the worker. That takes down clusterd, and the
sink re-renders over the same persisted row on restart, crash-looping the whole
cluster.

Separately, drop_optimizer_notices handled only Index and
MaterializedView, so a dropped sink's notices were never retracted.

Solution:
Skip a row whose label set is not representable, counting it in
mz_compute_metric_sink_skipped. A null value has nothing to encode, and an
empty string is not a stand-in either, since Prometheus reads it as absent and
would fold {a => ''} into {}.
Retract a dropped sink's notices through a new dataflow_metainfo_mut, the
mutable twin of the existing dataflow_metainfo getter, shared by both drop
sites.

Testing:

  • unit tests
  • a sqllogictest over a filtered, indexed view that exercises both the
    panic and the notice retraction on DROP METRIC SINK.

Closes: SQL-645

@mtabebe
mtabebe force-pushed the fix/metric-sink-null-label branch from 4503157 to 4346c1b Compare August 20, 2026 17:54
Problem:
A `map[text=>text]` has no per-value nullability, so a metric sink's
`labels` map can hold nulls. Such a row hit `extract_row`, which unwrapped the
value as a string and panicked the worker. That takes down clusterd, and the
sink re-renders over the same persisted row on restart, crash-looping the whole
cluster.

Separately, `drop_optimizer_notices` handled only `Index` and
`MaterializedView`, so a dropped sink's notices were never retracted.

Solution:
Skip a row whose label set is not representable, counting it in
`mz_compute_metric_sink_skipped`. A null value has nothing to encode, and an
empty string is not a stand-in either, since Prometheus reads it as absent and
would fold `{a => ''}` into `{}`.
Retract a dropped sink's notices through a new `dataflow_metainfo_mut`, the
mutable twin of the existing `dataflow_metainfo` getter, shared by both drop
sites.

Testing:
- unit tests
- a sqllogictest over a filtered, indexed view that exercises both the
panic and the notice retraction on `DROP METRIC SINK`.

Closes: SQL-645
@mtabebe
mtabebe force-pushed the fix/metric-sink-null-label branch from 4346c1b to 6edb149 Compare August 20, 2026 18:15
@mtabebe
mtabebe marked this pull request as ready for review August 20, 2026 18:40
@mtabebe
mtabebe requested review from a team as code owners August 20, 2026 18:40
@mtabebe
mtabebe requested review from SangJunBak and antiguru August 20, 2026 18:40
Comment on lines -256 to +261
.map(|(k, v)| (k, v.unwrap_str()))
.map(|(k, v)| (k, (!v.is_null()).then(|| v.unwrap_str())))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It appears to me the alternative to dropping rows with null label values would be to just discard this label? This seems to be more consistent to me than dropping the whole row, but I also think it's a policy question, and both ways are acceptable.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

You are right it is a policy decision. The reason why I opted for this policy is that if we collapse the label then we could have two rows that are the same. And then we will have a collision.

E.g., {foo='a', bar=NULL} and {foo='a'} would then collide.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Makes sense, and we can always revisit if needed. Values are string, so another option would be canonicalize as "NULL", which might be equally surprising :)

@mtabebe
mtabebe requested a review from antiguru August 21, 2026 17:11
@mtabebe
mtabebe merged commit d7b0d9e into MaterializeInc:main Aug 24, 2026
86 checks passed
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.

2 participants