diff --git a/crates/datastore/src/error.rs b/crates/datastore/src/error.rs index 912abca4fce..25388f26af2 100644 --- a/crates/datastore/src/error.rs +++ b/crates/datastore/src/error.rs @@ -82,8 +82,8 @@ pub enum TableError { ChangeColumnsError(#[from] Box), #[error(transparent)] AddColumnsError(#[from] Box), - #[error("Event table with ID `{0}` is not empty")] - EventTableNotEmpty(TableId), + #[error("Table with ID `{0}` attempted a reschema requiring an empty table, but it is not empty")] + TableNotEmpty(TableId), #[error( "Table with ID `{0}` attempted to reschema using `alter_event_table_row_type`, but it is not an event table" )] diff --git a/crates/datastore/src/locking_tx_datastore/committed_state.rs b/crates/datastore/src/locking_tx_datastore/committed_state.rs index 46e2e60131f..cfefcbc86be 100644 --- a/crates/datastore/src/locking_tx_datastore/committed_state.rs +++ b/crates/datastore/src/locking_tx_datastore/committed_state.rs @@ -853,7 +853,7 @@ impl CommittedState { unsafe { table.change_columns_to_unchecked(column_schemas, |_, _, _| Ok::<_, Infallible>(())) } .unwrap_or_else(|e| match e {}); } - ReschemaEventTable(table_id, column_schemas) => { + ReschemaEmptyTable(table_id, column_schemas) => { let table = self.tables.get_mut(&table_id)?; // SAFETY: // Same argument as in `TableAlterRowType` applies, diff --git a/crates/datastore/src/locking_tx_datastore/datastore.rs b/crates/datastore/src/locking_tx_datastore/datastore.rs index a219c14d548..d49cb021d6e 100644 --- a/crates/datastore/src/locking_tx_datastore/datastore.rs +++ b/crates/datastore/src/locking_tx_datastore/datastore.rs @@ -327,6 +327,15 @@ impl Locking { tx.alter_event_table_row_type(table_id, column_schemas) } + pub fn alter_empty_table_row_type_mut_tx( + &self, + tx: &mut MutTxId, + table_id: TableId, + column_schemas: Vec, + ) -> Result<()> { + tx.alter_empty_table_row_type(table_id, column_schemas) + } + pub fn add_columns_to_table_mut_tx( &self, tx: &mut MutTxId, diff --git a/crates/datastore/src/locking_tx_datastore/mut_tx.rs b/crates/datastore/src/locking_tx_datastore/mut_tx.rs index 3ba90142faf..63d0bb181f5 100644 --- a/crates/datastore/src/locking_tx_datastore/mut_tx.rs +++ b/crates/datastore/src/locking_tx_datastore/mut_tx.rs @@ -1462,21 +1462,35 @@ impl MutTxId { return Err(TableError::ReschemaNotAnEventTable(table_id).into()); } + self.alter_empty_table_row_type(table_id, column_schemas) + } + + /// Change the row type of the table identified by `table_id` to `column_schemas`, + /// without requiring the new row type to be layout-compatible with the old + /// (e.g. columns may be reordered). + /// + /// This is only valid on a table with no resident rows; + /// errors with [`TableError::TableNotEmpty`] otherwise. + pub(crate) fn alter_empty_table_row_type( + &mut self, + table_id: TableId, + column_schemas: Vec, + ) -> Result<()> { // Write to the table in the tx state. let ((tx_table, ..), (commit_table, ..)) = self.get_or_create_insert_table_mut(table_id)?; if tx_table.row_count != 0 || commit_table.row_count != 0 { // N.b. the delete table must also be empty, 'cause the committed table is empty. - return Err(TableError::EventTableNotEmpty(table_id).into()); + return Err(TableError::TableNotEmpty(table_id).into()); } let old_column_schemas = tx_table .change_columns_of_empty_table_to(column_schemas.clone()) - .map_err(|_| TableError::EventTableNotEmpty(table_id))?; + .map_err(|_| TableError::TableNotEmpty(table_id))?; commit_table .change_columns_of_empty_table_to(column_schemas.clone()) - .map_err(|_| TableError::EventTableNotEmpty(table_id))?; + .map_err(|_| TableError::TableNotEmpty(table_id))?; // Update system tables. // We'll simply remove all rows in `st_columns` and then add the new ones. @@ -1487,7 +1501,7 @@ impl MutTxId { self.insert_st_column(&table_name, &column_schemas)?; // Remember the pending change so we can undo if necessary. - self.push_schema_change(PendingSchemaChange::ReschemaEventTable(table_id, old_column_schemas)); + self.push_schema_change(PendingSchemaChange::ReschemaEmptyTable(table_id, old_column_schemas)); Ok(()) } diff --git a/crates/datastore/src/locking_tx_datastore/replay.rs b/crates/datastore/src/locking_tx_datastore/replay.rs index c42185e2bb2..3e8c0b5e76b 100644 --- a/crates/datastore/src/locking_tx_datastore/replay.rs +++ b/crates/datastore/src/locking_tx_datastore/replay.rs @@ -930,10 +930,14 @@ impl<'cs> ReplayCommittedState<'cs> { let is_event = self.is_event_table_for_replay(table_id)?; // Update the columns and layout of the the in-memory table. if let Some(table) = self.tables.get_mut(&table_id) { - if is_event { + if is_event || table.row_count == 0 { + // Layout-incompatible reschemas (e.g. reordering the columns of an empty + // table, `AutoMigrateStep::ReschemaEmptyTable`) are only ever committed + // against a table with no resident rows, so when the table is empty at + // this point in the log, mirror that and skip layout-compatibility checks. table .change_columns_of_empty_table_to(columns) - .map_err(|_| TableError::EventTableNotEmpty(table_id))?; + .map_err(|_| TableError::TableNotEmpty(table_id))?; } else { table.change_columns_to(columns).map_err(TableError::from)?; } diff --git a/crates/datastore/src/locking_tx_datastore/tx_state.rs b/crates/datastore/src/locking_tx_datastore/tx_state.rs index 6b47948d8a3..3d03cae125a 100644 --- a/crates/datastore/src/locking_tx_datastore/tx_state.rs +++ b/crates/datastore/src/locking_tx_datastore/tx_state.rs @@ -123,12 +123,13 @@ pub enum PendingSchemaChange { /// Only non-representational row-type changes are allowed here, /// so existing rows in the table will be compatible with the new row type. TableAlterRowType(TableId, Vec), - /// The row type of the event table with [`TableId`] was changed. + /// The row type of the empty table with [`TableId`] was changed. /// The old column schemas was stored. /// - /// As event tables never have rows resident across transactions or during automigrations, - /// we're fine to allow representational/layout-incompatible changes here. - ReschemaEventTable(TableId, Vec), + /// The table was verified to have no resident rows at the time of the change + /// (event tables are rowless by construction), + /// so we're fine to allow representational/layout-incompatible changes here. + ReschemaEmptyTable(TableId, Vec), /// The primary key of the table with [`TableId`] was changed. /// The old primary key was stored. TableAlterPrimaryKey(TableId, Option), @@ -168,7 +169,7 @@ impl MemoryUsage for PendingSchemaChange { table_id.heap_usage() + sequence.heap_usage() + sequence_schema.heap_usage() } Self::SequenceAdded(table_id, sequence_id) => table_id.heap_usage() + sequence_id.heap_usage(), - Self::ReschemaEventTable(table_id, column_schemas) => table_id.heap_usage() + column_schemas.heap_usage(), + Self::ReschemaEmptyTable(table_id, column_schemas) => table_id.heap_usage() + column_schemas.heap_usage(), } } } diff --git a/crates/engine/src/relational_db.rs b/crates/engine/src/relational_db.rs index 29c04fe0316..1b5d119f0a1 100644 --- a/crates/engine/src/relational_db.rs +++ b/crates/engine/src/relational_db.rs @@ -1092,6 +1092,17 @@ impl RelationalDB { .alter_event_table_row_type_mut_tx(tx, table_id, column_schemas)?) } + pub(crate) fn alter_empty_table_row_type( + &self, + tx: &mut MutTx, + table_id: TableId, + column_schemas: Vec, + ) -> Result<(), DBError> { + Ok(self + .inner + .alter_empty_table_row_type_mut_tx(tx, table_id, column_schemas)?) + } + pub(crate) fn add_columns_to_table_mut_tx( &self, tx: &mut MutTx, diff --git a/crates/engine/src/update.rs b/crates/engine/src/update.rs index e09ae1c076e..03ead33dc07 100644 --- a/crates/engine/src/update.rs +++ b/crates/engine/src/update.rs @@ -257,6 +257,21 @@ fn auto_migrate_database( anyhow::bail!("Precheck failed: added sequence {sequence_name} already has values in range",); } } + spacetimedb_schema::auto_migrate::AutoMigratePrecheck::CheckTableEmpty(table_name_key) => { + let (namespace, local) = table_name_key; + let table_name = joined(namespace, local); + let table_id = stdb + .table_id_from_name_mut(tx, &table_name)? + .ok_or_else(|| anyhow::anyhow!("Precheck: table `{table_name}` not found in database"))?; + let row_count = stdb.table_row_count_mut(tx, table_id).unwrap_or(0); + if row_count > 0 { + anyhow::bail!( + "Precheck failed: table `{table_name}` contains data ({row_count} rows), \ + but this migration requires it to be empty. \ + Clear the table's rows (e.g. via a reducer) before publishing." + ); + } + } } } @@ -270,13 +285,8 @@ fn auto_migrate_database( let table_name = joined(namespace, local); let table_id = stdb.table_id_from_name_mut(tx, &table_name)?.unwrap(); - if stdb.table_row_count_mut(tx, table_id).unwrap_or(0) > 0 { - anyhow::bail!( - "Cannot remove table `{table_name}`: table contains data. \ - Clear the table's rows (e.g. via a reducer) before removing it from your schema." - ); - } - + // Emptiness was already validated by the matching `CheckTableEmpty` precheck, + // before any mutations were performed. log!(logger, "Dropping table `{table_name}`"); stdb.drop_table(tx, table_id)?; } @@ -481,6 +491,21 @@ fn auto_migrate_database( stdb.alter_event_table_row_type(tx, table_id, column_schemas)?; } + spacetimedb_schema::auto_migrate::AutoMigrateStep::ReschemaEmptyTable(table_name_key) => { + let (namespace, local) = table_name_key; + let table_name = joined(namespace, local); + let (owning_def, table_def) = plan.new.find_table(table_name_key).ok_or_else(|| { + anyhow::anyhow!("ReschemaEmptyTable: table `{table_name}` not found in new module def") + })?; + let table_id = stdb.table_id_from_name_mut(tx, &table_name).unwrap().unwrap(); + let column_schemas = column_schemas_from_defs(owning_def, &table_def.columns, table_id); + + log!(logger, "Changing column layout of empty table `{}`", table_name); + + // Emptiness was already validated by the matching `CheckTableEmpty` precheck; + // the datastore re-checks it as a backstop. + stdb.alter_empty_table_row_type(tx, table_id, column_schemas)?; + } spacetimedb_schema::auto_migrate::AutoMigrateStep::ChangeAccess(table_name_key) => { let (namespace, local) = table_name_key; let table_name = joined(namespace, local); @@ -646,6 +671,7 @@ mod test { use crate::relational_db::{ open_snapshot_repo, tests_utils::{begin_mut_tx, insert, TestDB}, + MutTx, }; use spacetimedb_datastore::locking_tx_datastore::PendingSchemaChange; use spacetimedb_datastore::system_tables::ST_EVENT_TABLE_ID; @@ -656,7 +682,8 @@ mod test { }, Identity, }; - use spacetimedb_sats::{product, AlgebraicType, AlgebraicType::U64, ProductType}; + use spacetimedb_primitives::ColId; + use spacetimedb_sats::{product, AlgebraicType, AlgebraicType::U64, ProductType, ProductValue}; use spacetimedb_schema::{auto_migrate::ponder_migrate, def::ModuleDef}; struct TestLogger; @@ -1293,17 +1320,223 @@ mod test { let result = update_database(&stdb, &mut tx, auth_ctx, plan, &TestLogger); let err = result.err().expect("removing a non-empty table should fail"); assert!( - err.to_string().contains("table contains data"), + err.to_string().contains("contains data"), "error should mention that the table contains data, got: {err}" ); + assert_eq!(tx.pending_schema_changes(), []); + Ok(()) + } + + /// A module with a single table `points` whose columns are `(id, name)`, + /// or `(name, id)` when `swapped`, with a primary key, unique constraint, + /// and index on `id` in both cases. + fn points_module(swapped: bool) -> ModuleDef { + let mut builder = RawModuleDefV9Builder::new(); + let (product_type, id_col) = if swapped { + (ProductType::from([("name", AlgebraicType::String), ("id", U64)]), 1) + } else { + (ProductType::from([("id", U64), ("name", AlgebraicType::String)]), 0) + }; + builder + .build_table_with_new_type("points", product_type, true) + .with_unique_constraint(id_col) + .with_index(btree(id_col), "points_id_idx") + .with_primary_key(id_col) + .with_access(TableAccess::Public) + .finish(); + builder + .finish() + .try_into() + .expect("should be a valid module definition") + } + + /// Creates the tables of `module` in `stdb`, returning the [`TableId`] of `points`. + fn create_points_table(stdb: &TestDB, module: &ModuleDef) -> anyhow::Result { + let mut tx = begin_mut_tx(stdb); + for def in module.tables() { + create_table_from_def(stdb, &mut tx, module, def)?; + } + let table_id = stdb + .table_id_from_name_mut(&tx, "points")? + .expect("`points` table should exist"); + stdb.commit_tx(tx)?; + Ok(table_id) + } + + /// Asserts that the stored schema of `table_id` has exactly the column names + /// `columns`, in order, and the primary key `primary_key`. + fn assert_column_order( + stdb: &TestDB, + tx: &MutTx, + table_id: TableId, + columns: &[&str], + primary_key: Option, + ) -> anyhow::Result<()> { + let schema = stdb.schema_for_table_mut(tx, table_id)?; + let names: Vec<&str> = schema.columns().iter().map(|c| &*c.col_name).collect(); + assert_eq!(names, columns); + assert_eq!(schema.primary_key, primary_key); + Ok(()) + } + + /// Returns all rows of `table_id` as [`ProductValue`]s. + fn collect_rows(stdb: &TestDB, tx: &MutTx, table_id: TableId) -> anyhow::Result> { + Ok(stdb.iter_mut(tx, table_id)?.map(|r| r.to_product_value()).collect()) + } + + #[test] + fn reorder_empty_table_succeeds() -> anyhow::Result<()> { + let auth_ctx = AuthCtx::for_testing(); + let stdb = TestDB::durable()?; + + let old = points_module(false); + let new = points_module(true); + let table_id = create_points_table(&stdb, &old)?; + + // Insert a row and delete it again: a table which previously contained rows + // keeps residual (row-empty) pages, which the reschema must handle. + let mut tx = begin_mut_tx(&stdb); + insert(&stdb, &mut tx, table_id, &product![7u64, "gone"])?; + stdb.commit_tx(tx)?; + let mut tx = begin_mut_tx(&stdb); + assert_eq!(stdb.delete_by_rel(&mut tx, table_id, [product![7u64, "gone"]]), 1); + stdb.commit_tx(tx)?; + + let mut tx = begin_mut_tx(&stdb); + let plan = ponder_migrate(&old, &new)?; + let res = update_database(&stdb, &mut tx, auth_ctx, plan, &TestLogger)?; + assert!( + matches!(res, UpdateResult::RequiresClientDisconnect), + "reordering columns should disconnect clients" + ); + + // The stored schema now has the new column order, and the sub-objects + // on the moved column were re-created against its new position. + assert_column_order(&stdb, &tx, table_id, &["name", "id"], Some(ColId(1)))?; assert!( - tx.pending_schema_changes().is_empty(), - "failed migration should leave no pending schema changes: {:?}", + matches!( + tx.pending_schema_changes(), + [ + PendingSchemaChange::IndexRemoved(..), + PendingSchemaChange::ConstraintRemoved(..), + PendingSchemaChange::ReschemaEmptyTable(..), + PendingSchemaChange::IndexAdded(..), + PendingSchemaChange::ConstraintAdded(..), + PendingSchemaChange::TableAlterPrimaryKey(..), + ] + ), + "{:?}", tx.pending_schema_changes() ); + stdb.commit_tx(tx)?; + + // The table is usable with the new layout: insert a row and read it back. + let mut tx = begin_mut_tx(&stdb); + insert(&stdb, &mut tx, table_id, &product!["p1", 42u64])?; + assert_eq!(collect_rows(&stdb, &tx, table_id)?, [product!["p1", 42u64]]); + stdb.commit_tx(tx)?; + + Ok(()) + } + + #[test] + fn reorder_nonempty_table_fails() -> anyhow::Result<()> { + let auth_ctx = AuthCtx::for_testing(); + let stdb = TestDB::durable()?; + + let old = points_module(false); + let new = points_module(true); + let table_id = create_points_table(&stdb, &old)?; + + let mut tx = begin_mut_tx(&stdb); + insert(&stdb, &mut tx, table_id, &product![7u64, "p1"])?; + stdb.commit_tx(tx)?; + + let mut tx = begin_mut_tx(&stdb); + let plan = ponder_migrate(&old, &new)?; + let result = update_database(&stdb, &mut tx, auth_ctx, plan, &TestLogger); + let err = result.err().expect("reordering a non-empty table should fail"); + assert!( + err.to_string().contains("contains data"), + "error should mention that the table contains data, got: {err}" + ); + assert_eq!(tx.pending_schema_changes(), []); + + // The table keeps its old layout and contents. + assert_column_order(&stdb, &tx, table_id, &["id", "name"], Some(ColId(0)))?; + assert_eq!(collect_rows(&stdb, &tx, table_id)?, [product![7u64, "p1"]]); + + Ok(()) + } + + /// Reorders the columns of the (empty) `points` table, then replays the commitlog. + /// + /// Prior to the accompanying fix in `replay.rs` (`st_column_changed`), + /// replaying the reorder failed with a layout-compatibility error, + /// as replay used the layout-checked `change_columns_to` for non-event tables. + fn replay_reordered_table(snapshot: TakeSnapshot) -> anyhow::Result<()> { + let auth_ctx = AuthCtx::for_testing(); + let with_snapshot = matches!(snapshot, TakeSnapshot::BeforeAutomigration); + let stdb = with_snapshotting(with_snapshot)?; + + let old = points_module(false); + let new = points_module(true); + let table_id = create_points_table(&stdb, &old)?; + + // Insert a row and delete it again, so the commitlog contains writes to the + // table in the old layout, while the table is empty at migration time. + { + let mut tx = begin_mut_tx(&stdb); + insert(&stdb, &mut tx, table_id, &product![7u64, "gone"])?; + stdb.commit_tx(tx)?; + + let mut tx = begin_mut_tx(&stdb); + assert_eq!(stdb.delete_by_rel(&mut tx, table_id, [product![7u64, "gone"]]), 1); + stdb.commit_tx(tx)?; + } + + if with_snapshot { + take_snapshot(&stdb)?; + } + + // Migrate, reordering `points`' columns. + { + let mut tx = begin_mut_tx(&stdb); + let plan = ponder_migrate(&old, &new)?; + let res = update_database(&stdb, &mut tx, auth_ctx, plan, &TestLogger)?; + assert!( + matches!(res, UpdateResult::RequiresClientDisconnect), + "reordering columns should disconnect clients" + ); + stdb.commit_tx(tx)?; + } + + // Insert a row in the new layout. + { + let mut tx = begin_mut_tx(&stdb); + insert(&stdb, &mut tx, table_id, &product!["p1", 42u64])?; + stdb.commit_tx(tx)?; + } + + // Replay the commitlog and verify the reordered schema and its rows survived. + let stdb = stdb.reopen()?; + let tx = begin_mut_tx(&stdb); + assert_column_order(&stdb, &tx, table_id, &["name", "id"], Some(ColId(1)))?; + assert_eq!(collect_rows(&stdb, &tx, table_id)?, [product!["p1", 42u64]]); + Ok(()) } + #[test] + fn replay_reordered_table_no_snapshot() -> anyhow::Result<()> { + replay_reordered_table(TakeSnapshot::None) + } + + #[test] + fn replay_reordered_table_after_snapshot() -> anyhow::Result<()> { + replay_reordered_table(TakeSnapshot::BeforeAutomigration) + } + #[test] fn add_sequence_precheck_rejects_existing_column_max_value() -> anyhow::Result<()> { let auth_ctx = AuthCtx::for_testing(); diff --git a/crates/schema/src/auto_migrate.rs b/crates/schema/src/auto_migrate.rs index a516815fea1..4ba7f622eaf 100644 --- a/crates/schema/src/auto_migrate.rs +++ b/crates/schema/src/auto_migrate.rs @@ -244,6 +244,14 @@ pub enum AutoMigratePrecheck<'def> { /// Perform a check that adding a sequence is valid (the relevant column contains no values /// greater than the sequence's start value). CheckAddSequenceRangeValid(::Key<'def>), + + /// Perform a check that the table contains no rows. + /// + /// Emitted for migration steps that are only valid on an empty table, + /// such as [`AutoMigrateStep::RemoveTable`] and [`AutoMigrateStep::ReschemaEmptyTable`]. + /// The planner cannot see table contents, so the check is performed at execution time, + /// before any mutations. + CheckTableEmpty(::Key<'def>), } /// A step in an automatic migration. @@ -285,7 +293,9 @@ pub enum AutoMigrateStep<'def> { RemoveRowLevelSecurity(::Key<'def>), /// Remove an empty table and all its sub-objects (indexes, constraints, sequences). - /// Validated at execution time: fails if the table contains data. + /// + /// Only valid on an empty table; the plan will contain a matching + /// [`AutoMigratePrecheck::CheckTableEmpty`], and execution fails if the table contains data. RemoveTable(::Key<'def>), /// Change the column types of a table, in a layout compatible way. @@ -294,6 +304,13 @@ pub enum AutoMigrateStep<'def> { /// Change the column types of an event table, in a way that may not be layout-compatible. ReschemaEventTable(::Key<'def>), + /// Change the columns of a table, in a way that may not be layout-compatible + /// (e.g. reordering columns). + /// + /// Only valid on an empty table; the plan will contain a matching + /// [`AutoMigratePrecheck::CheckTableEmpty`], and execution fails if the table contains data. + ReschemaEmptyTable(::Key<'def>), + /// Add columns to a table, in a layout-INCOMPATIBLE way. /// /// This is a destructive operation that requires first running a `DisconnectAllUsers`. @@ -352,9 +369,6 @@ pub enum AutoMigrateError { #[error("Removing a column {column} from table {table} requires a manual migration")] RemoveColumn { table: Identifier, column: Identifier }, - #[error("Reordering table {table} requires a manual migration")] - ReorderTable { table: Identifier }, - #[error( "Changing the type of column {} in table {} from {:?} to {:?} requires a manual migration", .0.column, .0.table, .0.type1, .0.type2 @@ -696,6 +710,7 @@ fn auto_migrate_tables<'def>(plan: &mut AutoMigratePlan<'def>) -> Result<()> { for key in old_tables.keys() { if !new_tables.contains_key(key) { + plan.prechecks.push(AutoMigratePrecheck::CheckTableEmpty(*key)); plan.steps.push(AutoMigrateStep::RemoveTable(*key)); plan.ensure_disconnect_all_users(); } @@ -781,14 +796,14 @@ fn auto_migrate_table<'def>( let columns_ok = old .columns .iter() - .map(|old_col| -> Result> { + .map(|old_col| -> Result> { match new_col_by_name.get(&old_col.name) { None => { if is_event { // Event tables never have any resident rows, so removing a column is not a // data migration. However, changing the schema will break clients. - // `row_type_changed`, `columns_added`, `event_schema_changed` - Ok(ArrayMonoid([Any(false), Any(false), Any(true)])) + // `row_type_changed`, `columns_added`, `event_schema_changed`, `columns_reordered` + Ok(ArrayMonoid([Any(false), Any(false), Any(true), Any(false)])) } else { Err(AutoMigrateError::RemoveColumn { table: old_col.table_name.clone(), @@ -819,42 +834,33 @@ fn auto_migrate_table<'def>( Err(err) } }); - // Reject reordering of existing columns (unless it's an event table). - let positions_ok = if old_col.col_id == new_col.col_id { - Ok(Any(false)) - } else if is_event { - Ok(Any(true)) - } else { - Err(AutoMigrateError::ReorderTable { - table: old_col.table_name.clone(), - } - .into()) - }; - (types_ok, positions_ok) - .combine_errors() - // `row_type_changed`, `columns_added`, `event_schema_changed` - .map(|(types_changed, positions_changed)| { + // Reordering existing columns changes the row layout, which is only + // possible when the table has no resident rows. Event tables are + // rowless by construction; other tables get an emptiness precheck. + let positions_changed = Any(old_col.col_id != new_col.col_id); + types_ok + // `row_type_changed`, `columns_added`, `event_schema_changed`, `columns_reordered` + .map(|types_changed| { if is_event { - ArrayMonoid([Any(false), Any(false), types_changed | positions_changed]) + ArrayMonoid([Any(false), Any(false), types_changed | positions_changed, Any(false)]) } else { - assert!(!positions_changed.0); - ArrayMonoid([types_changed, Any(false), Any(false)]) + ArrayMonoid([types_changed, Any(false), Any(false), positions_changed]) } }) } } }) - .chain(new.columns.iter().map(|new_col| -> Result> { + .chain(new.columns.iter().map(|new_col| -> Result> { if old_col_by_name.contains_key(&new_col.name) { - Ok(ArrayMonoid([Any(false), Any(false), Any(false)])) + Ok(ArrayMonoid([Any(false), Any(false), Any(false), Any(false)])) } else if is_event { // Event tables never have any resident rows, so adding a column is not a data // migration. However, changing the schema will break clients. - // `row_type_changed`, `columns_added`, `event_schema_changed` - Ok(ArrayMonoid([Any(false), Any(false), Any(true)])) + // `row_type_changed`, `columns_added`, `event_schema_changed`, `columns_reordered` + Ok(ArrayMonoid([Any(false), Any(false), Any(true), Any(false)])) } else if new_col.default_value.is_some() { - // `row_type_changed`, `columns_added`, `event_schema_changed` - Ok(ArrayMonoid([Any(false), Any(true), Any(false)])) + // `row_type_changed`, `columns_added`, `event_schema_changed`, `columns_reordered` + Ok(ArrayMonoid([Any(false), Any(true), Any(false), Any(false)])) } else { Err(AutoMigrateError::AddColumn { table: new_col.table_name.clone(), @@ -863,16 +869,28 @@ fn auto_migrate_table<'def>( .into()) } })) - .collect_all_errors::>(); + .collect_all_errors::>(); - let ((), (), ArrayMonoid([Any(row_type_changed), Any(columns_added), Any(event_schema_changed)])) = - (type_ok, event_ok, columns_ok).combine_errors()?; + let ( + (), + (), + ArrayMonoid([Any(row_type_changed), Any(columns_added), Any(event_schema_changed), Any(columns_reordered)]), + ) = (type_ok, event_ok, columns_ok).combine_errors()?; if event_schema_changed { // If we're rewriting an event table, there's no data migration to do. // But incompatibly changing the schema can break clients. plan.ensure_disconnect_all_users(); plan.steps.push(AutoMigrateStep::ReschemaEventTable(key)); + } else if columns_reordered { + // Reordering columns rewrites the row layout in place, which is only valid on an + // empty table. The planner cannot see table contents, so emptiness is validated + // at execution time, before any mutations. This subsumes any `ChangeColumns` or + // `AddColumns` for the same table: the reschema rebuilds the full new layout, and + // with no resident rows there is no data to migrate or default-fill. + plan.prechecks.push(AutoMigratePrecheck::CheckTableEmpty(key)); + plan.ensure_disconnect_all_users(); + plan.steps.push(AutoMigrateStep::ReschemaEmptyTable(key)); } else if columns_added { // If we're adding a column, we'll rewrite the whole table. // That makes any `ChangeColumns` moot, so we can skip it. @@ -1168,7 +1186,9 @@ fn auto_migrate_sequences<'def>( // Added or changed sequences. for (sequence_key, (table_key, new_seq)) in &new_seqs { if let Some((_, old_seq)) = old_seqs.get(sequence_key) { - // we do not need to check column ids, since in an automigrate, column ids are not changed. + // Column ids can change in an automigrate (reordering an empty table); since + // `SequenceDef` includes the column id, such a sequence diffs as changed here + // and is removed and re-added against the new column position. if *old_seq != *new_seq { plan.prechecks .push(AutoMigratePrecheck::CheckAddSequenceRangeValid(*sequence_key)); @@ -1226,14 +1246,23 @@ fn auto_migrate_constraints<'def>( } // Changed constraints. - for (constraint_key, (_, new_constraint)) in &new_constraints { + for (constraint_key, (table_key, new_constraint)) in &new_constraints { if let Some((_, old_constraint)) = old_constraints.get(constraint_key) && *old_constraint != *new_constraint { - results.push(Err(AutoMigrateError::ChangeUniqueConstraint { - constraint: old_constraint.name.clone(), + // A constraint on a reordered column keeps its name but changes its column ids. + // When the owning table is being reschema'd empty (`ReschemaEmptyTable`), + // re-adding the constraint against the new column positions is trivially valid, + // as the table contains no rows. + if plan.any_step(|step| matches!(step, AutoMigrateStep::ReschemaEmptyTable(key) if key == table_key)) { + plan.steps.push(AutoMigrateStep::RemoveConstraint(*constraint_key)); + plan.steps.push(AutoMigrateStep::AddConstraint(*constraint_key)); + } else { + results.push(Err(AutoMigrateError::ChangeUniqueConstraint { + constraint: old_constraint.name.clone(), + } + .into())); } - .into())); } } @@ -1767,11 +1796,6 @@ mod tests { } => table == &apples && column == &count ); - expect_error_matching!( - result, - AutoMigrateError::ReorderTable { table } => table == &apples - ); - expect_error_matching!( result, AutoMigrateError::ChangeColumnType(ChangeColumnTypeParts { @@ -1961,11 +1985,77 @@ mod tests { } => &index[..] == apples_id_index && old_accessor.as_ref() == Some(&accessor_old) && new_accessor.as_ref() == Some(&accessor_new) ); - // It is not currently possible to test for `ChangeUniqueConstraint`, because unique constraint names are now generated during validation, - // and are determined by their columns and table name. So it's impossible to create a unique constraint with the same name - // but different columns from an old one. + // It is not currently possible to test for `ChangeUniqueConstraint` on a table that isn't + // being reschema'd empty, because unique constraint names are now generated during validation, + // and are determined by their columns and table name. So it's impossible to create a unique constraint + // with the same name but different columns from an old one. // We've left the check in, just in case this changes in the future. } + + #[test] + fn reorder_columns_of_empty_table() { + fn points_module(swapped: bool) -> ModuleDef { + let mut builder = RawModuleDefV9Builder::new(); + let (product_type, id_col) = if swapped { + ( + ProductType::from([("name", AlgebraicType::String), ("id", AlgebraicType::U64)]), + ColId(1), + ) + } else { + ( + ProductType::from([("id", AlgebraicType::U64), ("name", AlgebraicType::String)]), + ColId(0), + ) + }; + builder + .build_table_with_new_type("Points", product_type, true) + .with_column_sequence(id_col) + .with_unique_constraint(id_col) + .with_index(btree(id_col), "id_index") + .with_primary_key(id_col) + .finish(); + builder.finish().try_into().expect("should be a valid module def") + } + + let old_def = points_module(false); + let new_def = points_module(true); + + let plan = ponder_auto_migrate(&old_def, &new_def).expect("reordering columns should plan an auto-migration"); + + let points = key("", "Points"); + let points_sequence = sub_key("", "Points_id_seq"); + let points_constraint = sub_key("", "Points_id_key"); + let points_index = sub_key("", "Points_id_idx_btree"); + + // Reordering requires the table to be empty, validated before any mutations. + // The re-added sequence also gets its usual range precheck. + assert_eq!( + &plan.prechecks[..], + &[ + AutoMigratePrecheck::CheckAddSequenceRangeValid(points_sequence), + AutoMigratePrecheck::CheckTableEmpty(points), + ], + ); + + // Sub-objects on the moved column are removed and re-added against the new position, + // around the `ReschemaEmptyTable` step. There are no `ChangeColumns`/`AddColumns` steps: + // the full-layout reschema subsumes them. + assert_eq!( + &plan.steps[..], + &[ + AutoMigrateStep::RemoveIndex(points_index), + AutoMigrateStep::RemoveConstraint(points_constraint), + AutoMigrateStep::RemoveSequence(points_sequence), + AutoMigrateStep::ReschemaEmptyTable(points), + AutoMigrateStep::AddIndex(points_index), + AutoMigrateStep::AddConstraint(points_constraint), + AutoMigrateStep::AddSequence(points_sequence), + AutoMigrateStep::ChangePrimaryKey(points), + AutoMigrateStep::DisconnectAllUsers, + ], + ); + } + #[test] fn print_empty_to_populated_schema_migration() { // Start with completely empty schema diff --git a/crates/schema/src/auto_migrate/formatter.rs b/crates/schema/src/auto_migrate/formatter.rs index ede9d9e44fa..ca03ac992bf 100644 --- a/crates/schema/src/auto_migrate/formatter.rs +++ b/crates/schema/src/auto_migrate/formatter.rs @@ -129,6 +129,8 @@ fn format_step( // TODO(format-event-table-reschema): I (pgoldman 2026-06-10) didn't have time to meaningfully format event table reschemas, // so for now we're just printing the table name. AutoMigrateStep::ReschemaEventTable(table) => f.format_event_table_reschema(&joined(*table)), + + AutoMigrateStep::ReschemaEmptyTable(table) => f.format_empty_table_reschema(&joined(*table)), }?; Ok(()) @@ -190,6 +192,8 @@ pub trait MigrationFormatter { // TODO(format-event-table-reschema): I (pgoldman 2026-06-10) didn't have time to meaningfully format event table reschemas, // so for now we're just printing the table name. fn format_event_table_reschema(&mut self, table_name: &NamespacedIdentifier) -> io::Result<()>; + /// Format a layout-incompatible reschema of an empty (non-event) table, e.g. a column reorder. + fn format_empty_table_reschema(&mut self, table_name: &NamespacedIdentifier) -> io::Result<()>; } #[derive(Debug, Clone, PartialEq)] diff --git a/crates/schema/src/auto_migrate/termcolor_formatter.rs b/crates/schema/src/auto_migrate/termcolor_formatter.rs index f19d19ec7ad..30f7de56c14 100644 --- a/crates/schema/src/auto_migrate/termcolor_formatter.rs +++ b/crates/schema/src/auto_migrate/termcolor_formatter.rs @@ -438,6 +438,15 @@ impl MigrationFormatter for TermColorFormatter { Ok(()) } + + fn format_empty_table_reschema(&mut self, table_name: &NamespacedIdentifier) -> io::Result<()> { + self.write_action_prefix(&Action::Changed)?; + self.buffer.write_all(b" column layout of table ")?; + self.write_colored(table_name, Some(self.colors.table_name), true)?; + self.buffer.write_all(b" (requires the table to be empty)\n")?; + + Ok(()) + } } trait ActionColorExt { diff --git a/crates/schema/tests/ensure_same_schema.rs b/crates/schema/tests/ensure_same_schema.rs index fb6d572b2a1..79cf00faaf3 100644 --- a/crates/schema/tests/ensure_same_schema.rs +++ b/crates/schema/tests/ensure_same_schema.rs @@ -22,6 +22,7 @@ fn step_namespace<'a, 'def>(step: &'a AutoMigrateStep<'def>) -> Option<&'a Names | AutoMigrateStep::RemoveTable((ns, _)) | AutoMigrateStep::ChangeColumns((ns, _)) | AutoMigrateStep::ReschemaEventTable((ns, _)) + | AutoMigrateStep::ReschemaEmptyTable((ns, _)) | AutoMigrateStep::AddColumns((ns, _)) | AutoMigrateStep::AddTable((ns, _)) | AutoMigrateStep::AddSchedule((ns, _)) diff --git a/crates/table/src/pages.rs b/crates/table/src/pages.rs index b815b6415e2..31c0491a3f1 100644 --- a/crates/table/src/pages.rs +++ b/crates/table/src/pages.rs @@ -144,6 +144,16 @@ impl Pages { .collect(); } + /// Drops all pages, resetting `self` to its initial empty state. + /// + /// Unlike [`Self::clear`], which empties each page but keeps it allocated, + /// this removes the pages themselves, + /// so that [`Self::set_contents`] can be called afterwards. + pub fn reset(&mut self) { + self.pages.clear(); + self.non_full_pages.clear(); + } + /// Get a reference to fixed-len row data. /// /// Used in benchmarks. diff --git a/crates/table/src/table.rs b/crates/table/src/table.rs index 7fb63a2e429..000a9a5e78e 100644 --- a/crates/table/src/table.rs +++ b/crates/table/src/table.rs @@ -382,6 +382,9 @@ impl Table { } // Remove and drop any pages, as even though they must be empty, // they may have residual layout-derived data which conflicts with the new schema. + // A table which previously contained rows keeps its (now row-empty) pages around, + // so drop them first; `set_pages` requires that no pages are present. + self.inner.pages.reset(); // Safety: there aren't any pages here, so they cannot conflict with the schema or row layout. unsafe { self.set_pages(Vec::new(), &NullBlobStore) }; diff --git a/docs/docs/00200-core-concepts/00100-databases/00500-migrations/00200-automatic-migrations.md b/docs/docs/00200-core-concepts/00100-databases/00500-migrations/00200-automatic-migrations.md index 4a29ff7465f..857ad06eb66 100644 --- a/docs/docs/00200-core-concepts/00100-databases/00500-migrations/00200-automatic-migrations.md +++ b/docs/docs/00200-core-concepts/00100-databases/00500-migrations/00200-automatic-migrations.md @@ -33,6 +33,8 @@ These changes are allowed by automatic migration, but may cause runtime errors f - **Changing or removing reducers.** Clients attempting to call the old version of a changed reducer or a removed reducer will receive runtime errors. - **Changing tables from public to private.** Clients subscribed to a newly-private table will receive runtime errors. - **Removing `Primary Key` annotations.** Non-updated clients will still use the old primary key as a unique key in their local cache, which can result in non-deterministic behavior when updates are received. +- **Removing an empty table.** The publish fails if the table still contains rows; clear the table's rows first (e.g. via a reducer). All clients are disconnected, and non-updated clients subscribed to the removed table will receive runtime errors. +- **Reordering the columns of an empty table.** The publish fails if the table contains rows; clear the table's rows first (e.g. via a reducer). All clients are disconnected, and clients must regenerate their bindings to read the table correctly. - **Removing indexes.** This is only breaking in specific situations. The main issue occurs with subscription queries involving semijoins, such as: ```typescript @@ -48,8 +50,8 @@ These changes are allowed by automatic migration, but may cause runtime errors f The following changes cannot be performed with automatic migration and will cause the publish to fail: -- **Removing tables.** -- **Removing or modifying existing columns.** This includes changing the type, renaming, or reordering columns. +- **Removing tables that contain data.** Empty tables can be removed (see above). +- **Removing or modifying existing columns.** This includes changing the type or renaming columns. Reordering columns is only possible while the table is empty (see above). - **Adding columns without a default value.** New columns must have a default value so existing rows can be populated. - **Adding columns in the middle of a table.** New columns must be added at the end of the table definition. - **Changing whether a table is used for `scheduling`.**