persist: register schema at write time, not writer open time #33902

teskje · 2025-10-21T14:49:52Z

This PR changes persist clients to defer calling register_schema on a shard from write handle creation time to the time of the first append operation. The rationale is that we don't need to enforce a schema if we are not attempting to write to a shard.

The plan is for 0dt upgrades to make good use of the new, more lenient behavior. Read-only environments can open write handles with evolved schemas without having to durably write down the new schemas. This will allow us to back out of version upgrades without the risk of permanently poisoning the persist state for lower versions.

Motivation

This PR adds a known-desirable feature.

Prepares for the design proposed in #33863.
Part of https://github.com/MaterializeInc/database-issues/issues/9793

Tips for reviewer

DataHandles::open_data_write_for_apply in txn-wal expects the given shard to have a schema registered. This PR ensures that by changing TxnsHandle::register to register the shard schema. This should be fine because according to this comment we can assume a shard is always registered (potentially by a previous envd version) prior to its ID being passed to open_data_write_for_apply.

Also, write handles currently only call ensure_schema_registered when doing a compare-and-append. I'm not sure this is the right place! Do we also need/want to register the schema when we write a batch?

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 · 2025-10-23T14:58:49Z

src/persist-client/src/internal/encoding.rs

-    // TODO: Remove the Option once this finishes rolling out and all shards
-    // have a registered schema.


Removed this TODO because we need the Option now to support WriteHandles who haven't yet registered their schema.

teskje · 2025-10-23T15:06:33Z

src/persist-client/src/write.rs

+        let Some(schema_id) = schema_id else {
+            panic!("unable to register schemas: {key:?} {val:?}");
+        };


Note that previously in open_writer we only soft-asserted that schema registration was successful. That seems dangerous to me. Presumably writing to a shard with an incompatible schema would lead to all kinds of issues? Perhaps it was fine because the CaA logic also performs validation.

Since ensure_schema_registered is only called when we know we will need the schema, it seems fine to panic here. Also fine to return an Option like register_schema does and then unwrap at the call site. Lmk which you prefer!

That seems dangerous to me.

Maybe! Note that prior to this stuff being added, the schema could change ~arbitrarily without Persist noticing, so it was a strict improvement in safety.

But anyways I don't think this assert has tripped at all in the year or so it's been in prod, so it seems safe to strengthen it now.

teskje · 2025-10-23T15:09:08Z

src/txn-wal/src/txns.rs

+                            mz_ore::soft_panic_or_log!(
+                                "encountered data shard without a schema; shard_id: {}",
+                                previous.shard_id(),
+                            );


If we made this a panic/unwrap, the control flow would be a bit cleaner. But it's perhaps a bit scary and the surrounding code also only does soft panics.

src/persist-client/src/write.rs

bkirwi

Do we also need/want to register the schema when we write a batch?

I don't think it's necessary personally... the append-time check preserves the shard-level invariant, and I can imagine cases where you'd want to start building up batches at a new schema before actually evolving the shard. (If we shifted the self-correction buffer to Persist, for example, that would allow a read-only replica to start spilling to Persist even if the schema it was using wasn't yet registered.)

src/persist-client/src/lib.rs

bkirwi · 2025-10-27T14:43:37Z

src/persist-client/src/write.rs

+        let Some(schema_id) = schema_id else {
+            panic!("unable to register schemas: {key:?} {val:?}");
+        };


That seems dangerous to me.

Maybe! Note that prior to this stuff being added, the schema could change ~arbitrarily without Persist noticing, so it was a strict improvement in safety.

But anyways I don't think this assert has tripped at all in the year or so it's been in prod, so it seems safe to strengthen it now.

src/persist-client/src/write.rs

bkirwi · 2025-10-27T14:55:25Z

src/txn-wal/src/txns.rs

+            // schema _before_ we publish it to the txns shard.
+            for data_write in &mut data_writes {
+                data_write.ensure_schema_registered().await;
+            }


Are we confident that read-only replicas don't even register handles for existing shards, even though they'll never actually write to them?

Yes, based on the current code: TxnsHandle::register is only called by TxnsTableWorker, which is only instantiated in leader mode (read-only mode uses read_only_mode_table_worker instead).

We don't know what future code will want to do of course. That's why it would be nice to have some way to assert that "write" operations (e.g. CaA, schema registration) are only performed against shards that have been created by, or upgraded to the current version.

bkirwi

The new method seems good and looks safer to me - thanks for that!

One open thread about the dead code, but however that ends up getting resolved I think we're good to merge.

bkirwi · 2025-10-28T17:42:27Z

src/persist-client/src/lib.rs

+        // We defer registering the schema until write time, to allow opening
+        // write handles in a "read-only" mode where they don't implicitly
+        // modify persist state. But it might already be registered, in which
+        // case we can fetch its ID.


I was a little nervous about how this changed the semantics of the schema_id method... but it turns out that's only used by txn-wal in what looks like a safe pattern. Still a possible footgun, but hopefully one we can mitigate in the future...

This commit changes persist clients to defer calling `register_schema` on a shard from write handle creation time to the time of the first append operation. The rationale is that we don't need to enforce a schema if we are not attempting to write to a shard. The plan is for 0dt upgrades to make good use of the new, more lenient behavior. Read-only environments can open write handles with evolved schemas without having to durably write down the new schemas. This will allow us to back out of version upgrades without the risk of permanently poisoning the persist state for lower versions.

... and use it to enable shard finalization without a known shard schema.

teskje · 2025-10-29T09:17:52Z

TFTR!

…efer-register-schema" This reverts commit d2b1b58, reversing changes made to dd3402f.

teskje force-pushed the persist-defer-register-schema branch 8 times, most recently from 395a329 to 04664bf Compare October 23, 2025 13:30

teskje commented Oct 23, 2025

View reviewed changes

src/persist-client/src/write.rs Outdated Show resolved Hide resolved

teskje marked this pull request as ready for review October 23, 2025 15:49

teskje requested review from a team and aljoscha as code owners October 23, 2025 15:49

teskje requested a review from bkirwi October 23, 2025 15:49

bkirwi reviewed Oct 27, 2025

View reviewed changes

teskje requested a review from a team as a code owner October 28, 2025 12:29

teskje force-pushed the persist-defer-register-schema branch from 69c6796 to 29577af Compare October 28, 2025 16:01

teskje requested a review from bkirwi October 28, 2025 16:37

bkirwi approved these changes Oct 28, 2025

View reviewed changes

teskje force-pushed the persist-defer-register-schema branch from 29577af to 1233630 Compare October 28, 2025 18:30

teskje added 2 commits October 28, 2025 19:39

persist: provide WriteHandle::advance_upper

990ffbb

... and use it to enable shard finalization without a known shard schema.

teskje force-pushed the persist-defer-register-schema branch from 1233630 to 990ffbb Compare October 28, 2025 18:39

teskje merged commit d2b1b58 into MaterializeInc:main Oct 29, 2025
129 checks passed

teskje deleted the persist-defer-register-schema branch October 29, 2025 09:18

teskje added a commit to teskje/materialize that referenced this pull request Oct 30, 2025

Revert "Merge pull request MaterializeInc#33902 from teskje/persist-d…

7a8fa3c

…efer-register-schema" This reverts commit d2b1b58, reversing changes made to dd3402f.

teskje mentioned this pull request Oct 30, 2025

Revert "persist: register schema at write time, not writer open time" #33984

Open

5 tasks

		// TODO: Remove the Option once this finishes rolling out and all shards
		// have a registered schema.

persist: register schema at write time, not writer open time #33902

persist: register schema at write time, not writer open time #33902

Uh oh!

Conversation

teskje commented Oct 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Tips for reviewer

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bkirwi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bkirwi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

teskje commented Oct 29, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

teskje commented Oct 21, 2025 •

edited

Loading