feat: Allow toggling event flag during auto-migration for empty tables#4875
feat: Allow toggling event flag during auto-migration for empty tables#4875Ludv1gL wants to merge 2 commits intoclockworklabs:masterfrom
Conversation
Centril
added
enhancement
| let old = create_v10_module_def(|builder| { | ||
| builder | ||
| .build_table_with_new_type("events", ProductType::from([("id", AlgebraicType::U64)]), true) | ||
| .with_event(true) | ||
| .finish(); | ||
| }); | ||
| let new = create_v10_module_def(|builder| { | ||
| builder | ||
| .build_table_with_new_type("events", ProductType::from([("id", AlgebraicType::U64)]), true) | ||
| .finish(); | ||
| }); |
There was a problem hiding this comment.
The only difference is the bool, so please make a closure out out of this, called twice, that takes a parameter is_event where `is_event: bool.
There was a problem hiding this comment.
Done in ead511c3f4. Merged the two test_change_event_flag_produces_step_* tests into one test_change_event_flag_produces_step that defines a build = |is_event: bool| ... closure and an assert_flip = |old_is_event, new_is_event| ... closure, then calls assert_flip(false, true) and assert_flip(true, false).
| let old = create_v10_module_def(|builder| { | ||
| builder | ||
| .build_table_with_new_type("events", ProductType::from([("id", AlgebraicType::U64)]), true) | ||
| .with_unique_constraint(0) | ||
| .with_index(btree(0), "events_id_idx", "events_id_idx") | ||
| .with_primary_key(0) | ||
| .finish(); | ||
| }); | ||
| let new = create_v10_module_def(|builder| { | ||
| builder | ||
| .build_table_with_new_type("events", ProductType::from([("id", AlgebraicType::U64)]), true) | ||
| .with_unique_constraint(0) | ||
| .with_index(btree(0), "events_id_idx", "events_id_idx") | ||
| .with_primary_key(0) | ||
| .with_event(true) | ||
| .finish(); | ||
| }); |
There was a problem hiding this comment.
Only difference here is the parameter to .with_event(is_event) with an implicit .with_event(false) in the old case. Let's introduce a closure here too with a single parameter is_event.
There was a problem hiding this comment.
Done in ead511c3f4. test_change_event_flag_does_not_produce_orphan_sub_object_steps now uses the same build = |is_event: bool| ... closure pattern, with an explicit .with_event(is_event) in both the old and new branches — no more implicit-false asymmetry.
| // We only need to update if we've already constructed the in-memory table structure. | ||
| // If we haven't yet, then `self.get_table_and_blob_store_or_create` will see the correct schema | ||
| // (via `schema_for_table_raw`'s live `st_event_table` lookup) when it eventually runs. | ||
| if let Ok((table, ..)) = self.get_table_and_blob_store_mut(table_id) { |
There was a problem hiding this comment.
| if let Ok((table, ..)) = self.get_table_and_blob_store_mut(table_id) { | |
| if let Ok((table, ..)) = self.get_table_and_blob_store_mut(table_id) { | |
| assert_eq!(table.num_rows(), 0); |
There was a problem hiding this comment.
Applied in ead511c3f4.
| // Pre-validate: flipping is only safe when the table has no committed rows. | ||
| if stdb.table_row_count_mut(tx, table_id).unwrap_or(0) > 0 { | ||
| anyhow::bail!( | ||
| "Cannot change `event` flag on table `{table_name}`: table contains data. \ | ||
| Clear the table's rows (e.g. via a reducer) before toggling the `event` annotation." | ||
| ); | ||
| } | ||
|
|
There was a problem hiding this comment.
Hmm, feels like this should really be a AutoMigratePrecheck::CheckTableEmpty(...) so that we do all validation before we do any mutations.
There was a problem hiding this comment.
Acknowledged — per our offline discussion, deferring this to a follow-up PR that adds AutoMigratePrecheck::CheckTableEmpty and moves the row-count check into the precheck pass alongside the existing CheckAddSequenceRangeValid etc.
| let t = builder.build_table_with_new_type("events", [("id", U64)], true); | ||
| let t = if is_event { t.with_event(true) } else { t }; | ||
| t.finish(); |
There was a problem hiding this comment.
| let t = builder.build_table_with_new_type("events", [("id", U64)], true); | |
| let t = if is_event { t.with_event(true) } else { t }; | |
| t.finish(); | |
| builder.build_table_with_new_type("events", [("id", U64)], true) | |
| .with_event(is_event) | |
| .finish(); |
There was a problem hiding this comment.
Done in ead511c3f4: single_event_table_module_v10 now chains .build_table_with_new_type(...).with_event(is_event).finish() directly, no more intermediate if is_event { ... } else { ... } rebind.
| .map(|s| s.is_event) | ||
| .expect("schema should exist"), | ||
| false | ||
| ); |
There was a problem hiding this comment.
Here again is the test helper function I asked about for update.rs. The function should probably live in crates/datastore and be used in update.rs too.
There was a problem hiding this comment.
Done in ead511c3f4. The helpers live in the new crates/datastore/src/locking_tx_datastore/test_helpers.rs module (pub, gated by #[cfg(any(test, feature = "test"))], re-exported from locking_tx_datastore::mod). spacetimedb-core's update.rs tests import them via the existing features = ["test"] dev-dep already in Cargo.toml.
| assert_matches!( | ||
| tx.pending_schema_changes(), | ||
| &[PendingSchemaChange::TableAlterEventFlag(t, false)] if t == table_id | ||
| ); |
There was a problem hiding this comment.
Repeated 3x; Let's make a function out of this: check_table_event_flag_altered(&tx, table_id, state: bool)
There was a problem hiding this comment.
Done in ead511c3f4. The new check_table_event_flag_altered(datastore, tx, table_id, expected_is_event) helper in test_helpers.rs bundles the schema is_event check with the st_event_table row-presence check. The 4 test_alter_table_event_flag_* tests each call it in lieu of the inline pattern.
| assert!( | ||
| tx.pending_schema_changes().is_empty(), | ||
| "rollback should clear pending schema changes" | ||
| ); |
There was a problem hiding this comment.
| assert!( | |
| tx.pending_schema_changes().is_empty(), | |
| "rollback should clear pending schema changes" | |
| ); | |
| assert_eq!(tx.pending_schema_changes(), []); |
There was a problem hiding this comment.
Done in ead511c3f4.
| } | ||
|
|
||
| #[test] | ||
| fn test_alter_table_event_flag_non_event_to_event() -> ResultTest<()> { |
There was a problem hiding this comment.
In these tests, I'd like to see some assertions about what happens to TxData when committing changes that successfully altered the system tables.
There was a problem hiding this comment.
Done in ead511c3f4. test_alter_table_event_flag_non_event_to_event now captures TxData from commit(&datastore, tx)? and asserts tx_data.inserts_for_table(ST_EVENT_TABLE_ID).map(<[_]>::len) == Some(1) plus the user-table has None inserts and None deletes. The event→non test does the symmetric deletes_for_table assertion.
| // Flipping to the same value must be a no-op with no pending change. | ||
| let mut tx = begin_mut_tx(&datastore); | ||
| tx.alter_table_event_flag(table_id, false)?; | ||
| assert_matches!(tx.pending_schema_changes(), &[]); |
There was a problem hiding this comment.
| assert_matches!(tx.pending_schema_changes(), &[]); | |
| assert_eq!(tx.pending_schema_changes(), []); |
There was a problem hiding this comment.
Done in ead511c3f4.
Currently, toggling `#[spacetimedb::table(event)]` on an existing table fails with `AutoMigrateError::ChangeTableEventFlag` and requires a manual migration. This PR allows the flip in either direction as a live auto-migration step when the table has zero committed rows. Non-empty tables fail with an actionable error guiding the user to clear the table first. Clients are disconnected on the flip because the change is observable to subscribers (event tables have no committed state). - **`tx_state.rs`**: New `PendingSchemaChange::TableAlterEventFlag` variant storing the old `is_event` value for rollback. - **`committed_state.rs`**: Rollback branch restores the old value on the live schema. - **`mut_tx.rs`**: New `alter_table_event_flag` — dual-write to the tx + commit table schemas and to `st_event_table`. Idempotent no-op early returns before pushing a pending change. New `delete_st_event_table_row` helper using the existing `delete_col_eq` utility (hits the unique btree index on col 0). - **`replay.rs`**: New hook on `ST_EVENT_TABLE_ID` insert/delete mirrors `reschema_table_for_st_table_update` — flips `is_event` on the referenced user-table's cached schema during replay. This is load-bearing for cold replay across the flip point. - **`relational_db.rs`**: Thin `alter_table_event_flag` wrapper. - **`auto_migrate.rs`**: `event_ok` error branch replaced with `AutoMigrateStep::ChangeEventFlag` + `ensure_disconnect_all_users`. Removed dead `AutoMigrateError::ChangeTableEventFlag` variant. - **`formatter.rs` / `termcolor_formatter.rs`**: New `format_change_event_flag` mirroring `format_change_access`. - **`update.rs`**: New `ChangeEventFlag` handler with an O(1) row-count precheck before any mutation. - **Transaction safety**: Precheck (row count) and all three writes (st_event_table, tx schema, commit schema) run in the same `MutTx`. - **Rollback**: `TableAlterEventFlag` stores the old flag value so failed txs revert `is_event` on the live schema. Idempotent flips do not push a pending change. - **Replay correctness**: Without the replay hook, cold replay from a pre-migration snapshot would miss the schema flip and post-migration inserts would silently land in committed state. The hook mirrors the existing `st_table`/`st_column` pattern. - **Client contract**: Flipping `event` changes observability — v1 subscribers stop seeing updates; v2 subscribers see a different message variant. `ensure_disconnect_all_users` forces reconnection. ``` Cannot change `event` flag on table `my_table`: table contains data. Clear the table's rows (e.g. via a reducer) before toggling the `event` annotation. ``` - [x] `cargo test -p spacetimedb-datastore --features test` — 87 pass (including 4 new `alter_table_event_flag` tests) - [x] `cargo test -p spacetimedb-schema` — 103 pass (including 3 new `change_event_flag` plan tests) - [x] `cargo test -p spacetimedb-core` — 192 pass (including 2 new empty/non-empty integration tests) - [x] `cargo clippy -p spacetimedb-datastore -p spacetimedb-schema -p spacetimedb-core --tests` clean - [x] Pre-existing event-table tests still pass (10 tests)
Address all 16 actionable items from the review — deferred item D1
(moving the row-count precheck into `AutoMigratePrecheck::CheckTableEmpty`)
is not addressed here and will follow in a separate PR as agreed.
- `schema/auto_migrate.rs`: merge `test_change_event_flag_produces_step_{non_to_event,event_to_non}`
into a single test with a closure called twice; make the sub-object-steps
test use the same closure pattern instead of duplicating the old/new
builder with an implicit `false` vs explicit `true`.
- `datastore/replay.rs`: assert `num_rows()==0` inside the
`reschema_table_for_st_event_table_update` hook. This enforces the
feature's core invariant during cold replay.
- `datastore/committed_state.rs`: same assertion on the `TableAlterEventFlag`
rollback arm.
- `datastore/mut_tx.rs`: extract `insert_st_event_table_row(table_id)`
shared by `create_table` and `alter_table_event_flag`.
- `datastore/locking_tx_datastore/test_helpers.rs` (new): host the
cross-crate-reachable `assert_is_event_state`, `st_event_table_has_row`,
and `check_table_event_flag_altered` helpers. Gated by the existing
`test` feature. Re-exported from `locking_tx_datastore::mod`.
- `datastore/datastore.rs`: replace the 4 `test_alter_table_event_flag_*`
tests' inline is-event + st_event_table-row-presence duplication with
`check_table_event_flag_altered` (was repeated 3x by the reviewer's
count; actual count after deduplication drops to 5 call sites via the
helper). Add `TxData` assertions on the successful commits proving the
schema change materializes as an `st_event_table` insert/delete and
does not touch the user-table row data. Swap `is_empty()` /
`assert_matches!(&[])` for `assert_eq!(tx.pending_schema_changes(), [])`.
- `core/update.rs`: cleaner v10 builder via `.with_event(is_event)` chain;
extract `setup_events_table` returning `TableId`; move the row insert
into a separate tx in the non-empty-fails test; replace
`.any(|c| matches!(...))` with `assert_matches!([pat, ..])`; replace
`.is_empty()` with `assert_eq!([])`; adopt the shared
`assert_is_event_state` helper from `spacetimedb-datastore`.
Ludv1gL
force-pushed
the
feat/auto-migrate-event-flag-empty-table
branch
from
40af039 to
ead511c
Compare
|
Thanks for the review! Addressed all 16 actionable items — the deferred Rebased onto current
Summary of changes in
|
2 participants
Summary
Currently, toggling
#[spacetimedb::table(event)]on an existing tablefails auto-migration with
AutoMigrateError::ChangeTableEventFlagandforces a manual migration. This PR allows the flip in either direction
as a live auto-migration step when the table has zero committed
rows. Non-empty tables fail with an actionable error telling the user
to clear the table first. Clients are disconnected on the flip because
the change is observable to subscribers (event tables have no committed
state; v2 subscribers see a different message variant; v1 subscribers
stop seeing updates).
Changes
crates/datastore/src/locking_tx_datastore/tx_state.rs: newPendingSchemaChange::TableAlterEventFlag(TableId, bool)variantstoring the old
is_eventvalue for rollback.crates/datastore/src/locking_tx_datastore/committed_state.rs:rollback branch restoring the old value on the live schema.
crates/datastore/src/locking_tx_datastore/mut_tx.rs: newalter_table_event_flag(table_id, is_event)mirroringalter_table_access— dual-writes to the tx + commit table schemasvia
with_mut_schema_and_clone, and tost_event_tableviainsert_via_serialize_bsatn/delete_col_eq. Idempotent no-opearly-returns before pushing a pending change. New
delete_st_event_table_rowhelper.crates/datastore/src/locking_tx_datastore/replay.rs: new hooks onST_EVENT_TABLE_IDinsert/delete that flipis_eventon thereferenced user-table's cached schema, mirroring
reschema_table_for_st_table_update. This is load-bearing for coldreplay across the flip — without it, a snapshot predating the flip
plus commitlog replay would leave the cached schema stale, and
post-flip inserts would incorrectly land in committed state.
crates/core/src/db/relational_db.rs: thinalter_table_event_flagwrapper analogous toalter_table_access.crates/schema/src/auto_migrate.rs:event_okerror branchreplaced with
AutoMigrateStep::ChangeEventFlag+plan.ensure_disconnect_all_users(). Removed the now-unusedAutoMigrateError::ChangeTableEventFlagvariant. Replacedtest_change_event_flag_rejectedwith three new tests asserting theplan shape in both directions and confirming no orphan sub-object
steps.
crates/schema/src/auto_migrate/formatter.rs/termcolor_formatter.rs: newformat_change_event_flag+EventFlagChangeInfomirroringformat_change_access.crates/core/src/db/update.rs: newChangeEventFlaghandler withan O(1) row-count precheck via
table_row_count_mutbefore anymutation. Fails with a clear message if the table has data.
Safety
(
st_event_tablerow, tx table schema, commit table schema) run inthe same
MutTx. No window for concurrent inserts between check andflip.
TableAlterEventFlagstores the pre-flip flag value sofailed transactions revert
is_eventon the live schema via theexisting
rollback_pending_schema_changepath. Idempotent flips donot push a pending change and thus require no rollback work.
st_event_tablereschema hook is thepiece with no prior analog.
st_event_tablewas already in thecommitlog, but the existing reschema path only covered
st_tableandst_column, so flippingis_eventmid-life would have been invisibleto cold replay. Added hooks in both
replay_insertandreplay_delete_by_relcall the same helper, reusing the existingSelf::read_table_id(row)pattern (no new unsafe code).eventchanges what subscribers see —v1 subscribers stop receiving updates (event tables aren't in default
subscriptions); v2 subscribers receive a different message variant
(
TableUpdateRows::EventTable).ensure_disconnect_all_usersforcesreconnection so clients observe a consistent state.
Example error output
Test plan
cargo test -p spacetimedb-datastore --features test— 87 pass,including four new
alter_table_event_flag_*tests covering bothdirections, rollback, and the idempotent no-op case
cargo test -p spacetimedb-schema— 103 pass, including three newplan-shape tests (both directions + "no orphan sub-object steps")
cargo test -p spacetimedb-core— 192 pass, including two newend-to-end update-execution tests (empty succeeds, non-empty fails)
cargo clippy -p spacetimedb-datastore -p spacetimedb-schema -p spacetimedb-core --testscleaneventflag on an empty user table viaspacetime publish; clientsdisconnected as expected, subsequent inserts correctly routed to the
commitlog only.