Skip to content

Conversation

@teskje
Copy link
Contributor

@teskje teskje commented Nov 4, 2025

Second attempt at getting lazy write schema registration working. This was previously attempted in #33902 but had to be reverted because it caused type confusion during compaction. Details in Slack.

We believe that the confusion came from batches not having schema IDs set, so compaction would fall back the write handle's schema, which in the case of shard finalization would be a dummy schema. #33995 removes that fallback logic. In this PR we add additional logic that populates the part schema IDs in compare_and_append_batch, so compaction should always be able to find a correct schema.

Motivation

  • This PR fixes a recognized bug.

Fixes https://github.com/MaterializeInc/database-issues/issues/9874
Part of https://github.com/MaterializeInc/database-issues/issues/9793

Tips for reviewer

Here is a branch where I test this under a configuration known to tickle the bug (shard finalization with a non-trivial schema): #33992

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.

@teskje teskje marked this pull request as ready for review November 4, 2025 15:33
@teskje teskje requested review from a team and aljoscha as code owners November 4, 2025 15:33
@teskje teskje mentioned this pull request Nov 5, 2025
5 tasks
This way if the append fails we haven't modified the inputs; probably
not harmful but easier to reason about.
Copy link
Contributor

@bkirwi bkirwi left a comment

Choose a reason for hiding this comment

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

Looks good!

I pushed one tweak, moving the schema enforcement to the combined batch instead of the inputs... under the theory that if the append fails for whatever reason it's better to not have modified our inputs, which might be reused. Hopefully noncontroversial, but let me know if otherwise!

@teskje teskje enabled auto-merge November 6, 2025 17:45
@teskje
Copy link
Contributor Author

teskje commented Nov 6, 2025

TFTR (and tweaking)!

@teskje teskje merged commit 78f5039 into MaterializeInc:main Nov 6, 2025
128 of 129 checks passed
@teskje teskje deleted the persist-lazy-schema branch November 7, 2025 13:00
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