Skip to content

Conversation

@teskje
Copy link
Contributor

@teskje teskje commented Nov 21, 2025

When applying an item StateUpdate, we had an optimization that would skip parsing the create_sql if it hadn't change, and reuse the old CatalogEntry instead. This produces the wrong outcome if the extra_versions change without the SQL changing: Any version updates are not reflected in the new CatalogEntry.

To fix this, this PR removes the optimization, under the assumption that DDL is sufficiently rare that we can live without it. If that turns out not the be the case, we can keep the optimization and refine the conditions under which SQL parsing is performed, though that seems brittle.

Motivation

  • This PR fixes a previously unreported bug.

This comes up in the ALTER MV work: A change adding new collection version without changing the create_sql doesn't get applied properly, the new version is ignored.

Part of https://github.com/MaterializeInc/database-issues/issues/9903

Tips for reviewer

I'm optimistically assuming that this was a premature optimization, but I don't know for sure. It was introduced in #27667 with no motivation given aside from "this is potentially wasteful". I couldn't find any further discussion anywhere else.

Istm that being inefficient when applying DDL changes should be fine, given that DDL should be a relatively rare occurrence. So doing more work here for the sake of robustness seems fine. But I'm missing a bunch of context and maybe apply_item_update is invoked more often than I assume.

Checklist

  • This PR has adequate test coverage / QA involvement has been duly considered. (trigger-ci for additional test/nightly runs)
  • This PR has an associated up-to-date design doc, is a design doc (template), or is sufficiently small to not require a design.
  • If this PR evolves an existing $T ⇔ Proto$T mapping (possibly in a backwards-incompatible way), then it is tagged with a T-proto label.
  • If this PR will require changes to cloud orchestration or tests, there is a companion cloud PR to account for those changes that is tagged with the release-blocker label (example).
  • If this PR includes major user-facing behavior changes, I have pinged the relevant PM to schedule a changelog post.

When applying an item `StateUpdate`, we had an optimization that would
skip parsing the `create_sql` if it hadn't change, and reuse the old
`CatalogEntry` instead. This produces the wrong outcome if the
`extra_versions` change without the SQL changing: Any version updates
are not reflected in the new `CatalogEntry`.

To fix this, this commit removes the optimization, under the assumption
that DDL is sufficiently rare that we can live without it. If that turns
out not the be the case, we can keep the optimization and refine the
conditions under which SQL parsing is performed, though that seems
brittle.
@teskje teskje marked this pull request as ready for review November 21, 2025 17:03
@teskje teskje requested a review from a team as a code owner November 21, 2025 17:03
@teskje teskje requested a review from ggevay November 21, 2025 17:03
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