From 3e7187f8ee18b782d76a1d4b8eeabc8fe93aa7a7 Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Mon, 10 Nov 2025 12:56:35 -0800 Subject: [PATCH 1/7] wip --- nexus/db-queries/src/db/datastore/fm.rs | 585 ++++++++++++------------ 1 file changed, 290 insertions(+), 295 deletions(-) diff --git a/nexus/db-queries/src/db/datastore/fm.rs b/nexus/db-queries/src/db/datastore/fm.rs index 480193b215..35647c7f84 100644 --- a/nexus/db-queries/src/db/datastore/fm.rs +++ b/nexus/db-queries/src/db/datastore/fm.rs @@ -18,11 +18,7 @@ use crate::db::pagination::paginated; use crate::db::raw_query_builder::QueryBuilder; use crate::db::raw_query_builder::TypedSqlQuery; use async_bb8_diesel::AsyncRunQueryDsl; -use diesel::pg::Pg; use diesel::prelude::*; -use diesel::query_builder::AstPass; -use diesel::query_builder::QueryFragment; -use diesel::query_builder::QueryId; use diesel::result::DatabaseErrorKind; use diesel::result::Error as DieselError; use diesel::sql_types; @@ -195,14 +191,290 @@ impl DataStore { // TODO(eliza): other sitrep records would be inserted here... // Now, try to make the sitrep current. - let query = InsertSitrepVersionQuery { sitrep_id: sitrep.id() }; + let query = Self::insert_sitrep_version_query(sitrep.id()); query .execute_async(&*conn) .await - .map_err(|e| query.decode_error(e)) + .map_err(|err| match err { + DieselError::DatabaseError( + DatabaseErrorKind::Unknown, + info, + ) if info.message() == PARENT_NOT_CURRENT_ERROR_MESSAGE => { + InsertSitrepError::ParentNotCurrent(sitrep.id()) + } + err => { + let err = + public_error_from_diesel(err, ErrorHandler::Server) + .internal_context( + "failed to insert new sitrep version", + ); + InsertSitrepError::Other(err) + } + }) .map(|_| ()) } + + /// Query to insert a new sitrep version into the `fm_sitrep_history` table, + /// making it the current sitrep. + /// + /// This implements the "compare-and-swap" operation [described in RFD + /// 603](https://rfd.shared.oxide.computer/rfd/0603#_creating_sitreps). In + /// particular, this query will insert a new sitrep version into the + /// `fm_sitrep_history` table IF AND ONLY IF one of the following conditions + /// are true: + /// + /// 1. The new sitrep's parent sitrep ID is the current sitrep (i.e. the sitrep + /// with the highest version number in `fm_sitrep_history`) + /// 2. The new sitrep's parent sitrep ID is `NULL`, AND there are no other + /// sitreps in `fm_sitrep_history` (i.e., we are inserting the first-ever + /// sitrep) + /// + /// Upholding these invariants ensures that sitreps are sequentially consistent, + /// and `fm_sitrep_history` always contains a linear history of sitreps which + /// were generated based on the previous current sitrep. + /// + /// The CTE used to perform this operation is based on the one used in the + /// `deployment` module to insert blueprints into the `bp_target` table. It + /// differs in that it does not perform an existence check on the sitrep to be + /// made current. This is because the `db::datastore::deployment` module's + /// public API treats inserting a new blueprint and setting it as the current + /// target as separate operations, so it is possible for a consumer of the API + /// to try and set a blueprint as the target without first having created it. + /// Here, however, we only ever set a sitrep as the current sitrep in the + /// `Datastore::fm_sitrep_insert` method, which also creates the sitrep. So, it + /// is impossible for a consumer of this API to attempt to make a sitrep current + /// without having first created it. + /// + /// The SQL generated for this CTE looks like this: + /// + /// ```sql + /// WITH + /// -- Subquery to fetch the current sitrep (i.e., the row with the max + /// -- version). + /// current_sitrep AS ( + /// SELECT + /// "version" AS version, + /// "sitrep_id" AS sitrep_id, + /// FROM "fm_sitrep_history" + /// ORDER BY "version" DESC + /// LIMIT 1 + /// ), + /// + /// -- Error checking subquery: This uses similar tricks as elsewhere in + /// -- this crate to `CAST(... AS UUID)` with non-UUID values that result + /// -- in runtime errors in specific cases, allowing us to give accurate + /// -- error messages. + /// -- + /// -- This checks that the sitrep descends directly from the current + /// -- sitrep, and will fail the query if it does not. + /// check_validity AS MATERIALIZED ( + /// SELECT CAST(IF( + /// -- Check for whether our new sitrep's parent matches our current + /// -- sitrep. There are two cases here: The first is the common case + /// -- (i.e., the new sitrep has a parent: does it match the current + /// -- sitrep ID?). The second is the bootstrapping check: if we're + /// -- trying to insert a new sitrep that does not have a parent, + /// -- we should not have a sitrep target at all. + /// -- + /// -- If either of these cases fails, we return `parent-not-current`. + /// ( + /// SELECT "parent_sitrep_id" FROM "sitrep", current_sitrep + /// WHERE + /// "id" = + /// AND current_sitrep.sitrep_id = "parent_sitrep_id" + /// ) IS NOT NULL + /// OR + /// ( + /// SELECT 1 FROM "sitrep" + /// WHERE + /// "id" = + /// AND "parent_sitrep_id" IS NULL + /// AND NOT EXISTS (SELECT version FROM current_sitrep) + /// ) = 1, + /// -- Sometime between v22.1.9 and v22.2.19, Cockroach's type checker + /// -- became too smart for our `CAST(... as UUID)` error checking + /// -- gadget: it can infer that `` must be a UUID, so + /// -- then tries to parse 'parent-not-target' and 'no-such-blueprint' + /// -- as UUIDs _during typechecking_, which causes the query to always + /// -- fail. We can defeat this by casting the UUID to text here, which + /// -- will allow the 'parent-not-target' and 'no-such-blueprint' + /// -- sentinels to survive type checking, making it to query execution + /// -- where they will only be cast to UUIDs at runtime in the failure + /// -- cases they're supposed to catch. + /// CAST( AS text), + /// 'parent-not-current' + /// ) AS UUID) + /// ), + /// + /// -- Determine the new version number to use: either 1 if this is the + /// -- first sitrep being made the current sitrep, or 1 higher than + /// -- the previous sitrep's version. + /// -- + /// -- The final clauses of each of these WHERE clauses repeat the + /// -- checks performed above in `check_validity`, and will cause this + /// -- subquery to return no rows if we should not allow the new + /// -- target to be set. + /// new_sitrep AS ( + /// SELECT 1 AS new_version FROM "sitrep" + /// WHERE + /// "id" = + /// AND "parent_sitrep_id" IS NULL + /// AND NOT EXISTS (SELECT version FROM current_sitrep) + /// UNION + /// SELECT current_sitrep.version + 1 FROM current_sitrep, "sitrep" + /// WHERE + /// "id" = + /// AND "parent_sitrep_id" IS NOT NULL + /// AND "parent_sitrep_id" = current_sitrep.sitrep_id + /// ) + /// + /// -- Perform the actual insertion. + /// INSERT INTO "sitrep_history"( + /// "version","sitrep_id","time_made_current" + /// ) + /// SELECT + /// new_sitrep.new_version, + /// , + /// NOW() + /// FROM new_sitrep + /// ``` + fn insert_sitrep_version_query( + sitrep_id: SitrepUuid, + ) -> TypedSqlQuery { + let mut builder = QueryBuilder::new(); + + // Subquery to fetch the current sitrep (i.e., the row with the max + // version). + builder.sql( + "WITH current_sitrep AS ( \ + SELECT version AS version, sitrep_id AS sitrep_id \ + FROM omicron.public.fm_sitrep_history \ + ORDER BY version DESC \ + LIMIT 1 \ + ), ", + ); + + // Error checking subquery: This uses similar tricks as elsewhere in + // this crate to `CAST(... AS UUID)` with non-UUID values that result + // in runtime errors in specific cases, allowing us to give accurate + // error messages. + // + // This checks that the sitrep descends directly from the current + // sitrep, and will fail the query if it does not. + builder + .sql("check_validity AS MATERIALIZED ( ") + // Check for whether our new sitrep's parent matches our current + // sitrep. There are two cases here: The first is the common case + // (i.e., the new sitrep has a parent: does it match the current + // sitrep ID?). The second is the bootstrapping check: if we're + // trying to insert a new sitrep that does not have a parent, we + // should not have a sitrep target at all. + // + // If either of these cases fails, we return `parent-not-current`. + .sql( + "SELECT CAST( \ + IF( \ + (SELECT parent_sitrep_id \ + FROM omicron.public.fm_sitrep, current_sitrep \ + WHERE id = ", + ) + .param() + .bind::(sitrep_id.into_untyped_uuid()) + .sql( + " AND current_sitrep.sitrep_id = parent_sitrep_id) IS NOT NULL \ + OR \ + (SELECT 1 FROM omicron.public.fm_sitrep \ + WHERE id = ", + ) + .param() + .bind::(sitrep_id.into_untyped_uuid()) + .sql( + " AND parent_sitrep_id IS NULL \ + AND NOT EXISTS (SELECT version FROM current_sitrep)) = 1,", + ) + // Sometime between v22.1.9 and v22.2.19, Cockroach's type checker + // became too smart for our `CAST(... as UUID)` error checking + // gadget: it can infer that `` must be a UUID, so + // then tries to parse 'parent-not-target' as UUIDs _during + // typechecking_, which causes the query to always fail. We can + // defeat this by casting the UUID to text here, which will allow + // the 'parent-not-target' sentinel to survive type checking, making + // it to query execution where they will only be cast to UUIDs at + // runtime in the failure cases they're supposed to catch. + .sql(" CAST(") + .param() + .bind::(sitrep_id.into_untyped_uuid()) + .sql(" AS text), ") + .sql(PARENT_NOT_CURRENT) + .sql( + ") AS UUID \ + ) \ + ), ", + ); + + // Determine the new version number to use: either 1 if this is the + // first sitrep being made the current sitrep, or 1 higher than + // the previous sitrep's version. + // + // The final clauses of each of these WHERE clauses repeat the + // checks performed above in `check_validity`, and will cause this + // subquery to return no rows if we should not allow the new + // target to be set. + // + // new_sitrep AS ( + // SELECT 1 AS new_version FROM omicron.public.fm_sitrep + // WHERE + // id = $5 AND parent_sitrep_id IS NULL + // AND + // NOT EXISTS (SELECT version FROM current_sitrep) + // UNION + // SELECT + // current_sitrep.version + 1 + // FROM + // current_sitrep, omicron.public.fm_sitrep + // WHERE + // id = $6 AND parent_sitrep_id IS NOT NULL + // AND parent_sitrep_id = current_sitrep.sitrep_id + // ) + builder + .sql( + "new_sitrep AS ( \ + SELECT 1 AS new_version FROM omicron.public.fm_sitrep \ + WHERE id = ", + ) + .param() + .bind::(sitrep_id.into_untyped_uuid()) + .sql( + "AND parent_sitrep_id IS NULL \ + AND NOT EXISTS (SELECT version FROM current_sitrep) \ + UNION \ + SELECT current_sitrep.version + 1 \ + FROM current_sitrep, omicron.public.fm_sitrep \ + WHERE id = ", + ) + .param() + .bind::(sitrep_id.into_untyped_uuid()) + .sql( + " AND parent_sitrep_id IS NOT NULL \ + AND parent_sitrep_id = current_sitrep.sitrep_id \ + ) ", + ); + + //Finally, perform the actual insertion. + builder + .sql( + "INSERT INTO omicron.public.fm_sitrep_history \ + (version, sitrep_id, time_made_current) \ + SELECT new_sitrep.new_version, ", + ) + .param() + .bind::(sitrep_id.into_untyped_uuid()) + .sql(", NOW() FROM new_sitrep"); + + builder.query() + } + /// Lists all orphaned alternative sitreps (paginated by sitrep UUID). /// /// Orphaned sitreps are those which can never be committed to the @@ -401,135 +673,6 @@ pub enum InsertSitrepError { ParentNotCurrent(SitrepUuid), } -/// Query to insert a new sitrep version into the `fm_sitrep_history` table, -/// making it the current sitrep. -/// -/// This implements the "compare-and-swap" operation [described in RFD -/// 603](https://rfd.shared.oxide.computer/rfd/0603#_creating_sitreps). In -/// particular, this query will insert a new sitrep version into the -/// `fm_sitrep_history` table IF AND ONLY IF one of the following conditions -/// are true: -/// -/// 1. The new sitrep's parent sitrep ID is the current sitrep (i.e. the sitrep -/// with the highest version number in `fm_sitrep_history`) -/// 2. The new sitrep's parent sitrep ID is `NULL`, AND there are no other -/// sitreps in `fm_sitrep_history` (i.e., we are inserting the first-ever -/// sitrep) -/// -/// Upholding these invariants ensures that sitreps are sequentially consistent, -/// and `fm_sitrep_history` always contains a linear history of sitreps which -/// were generated based on the previous current sitrep. -/// -/// The CTE used to perform this operation is based on the one used in the -/// `deployment` module to insert blueprints into the `bp_target` table. It -/// differs in that it does not perform an existence check on the sitrep to be -/// made current. This is because the `db::datastore::deployment` module's -/// public API treats inserting a new blueprint and setting it as the current -/// target as separate operations, so it is possible for a consumer of the API -/// to try and set a blueprint as the target without first having created it. -/// Here, however, we only ever set a sitrep as the current sitrep in the -/// `Datastore::fm_sitrep_insert` method, which also creates the sitrep. So, it -/// is impossible for a consumer of this API to attempt to make a sitrep current -/// without having first created it. -/// -/// The SQL generated for this CTE looks like this: -/// -/// ```sql -/// WITH -/// -- Subquery to fetch the current sitrep (i.e., the row with the max -/// -- version). -/// current_sitrep AS ( -/// SELECT -/// "version" AS version, -/// "sitrep_id" AS sitrep_id, -/// FROM "fm_sitrep_history" -/// ORDER BY "version" DESC -/// LIMIT 1 -/// ), -/// -/// -- Error checking subquery: This uses similar tricks as elsewhere in -/// -- this crate to `CAST(... AS UUID)` with non-UUID values that result -/// -- in runtime errors in specific cases, allowing us to give accurate -/// -- error messages. -/// -- -/// -- This checks that the sitrep descends directly from the current -/// -- sitrep, and will fail the query if it does not. -/// check_validity AS MATERIALIZED ( -/// SELECT CAST(IF( -/// -- Check for whether our new sitrep's parent matches our current -/// -- sitrep. There are two cases here: The first is the common case -/// -- (i.e., the new sitrep has a parent: does it match the current -/// -- sitrep ID?). The second is the bootstrapping check: if we're -/// -- trying to insert a new sitrep that does not have a parent, -/// -- we should not have a sitrep target at all. -/// -- -/// -- If either of these cases fails, we return `parent-not-current`. -/// ( -/// SELECT "parent_sitrep_id" FROM "sitrep", current_sitrep -/// WHERE -/// "id" = -/// AND current_sitrep.sitrep_id = "parent_sitrep_id" -/// ) IS NOT NULL -/// OR -/// ( -/// SELECT 1 FROM "sitrep" -/// WHERE -/// "id" = -/// AND "parent_sitrep_id" IS NULL -/// AND NOT EXISTS (SELECT version FROM current_sitrep) -/// ) = 1, -/// -- Sometime between v22.1.9 and v22.2.19, Cockroach's type checker -/// -- became too smart for our `CAST(... as UUID)` error checking -/// -- gadget: it can infer that `` must be a UUID, so -/// -- then tries to parse 'parent-not-target' and 'no-such-blueprint' -/// -- as UUIDs _during typechecking_, which causes the query to always -/// -- fail. We can defeat this by casting the UUID to text here, which -/// -- will allow the 'parent-not-target' and 'no-such-blueprint' -/// -- sentinels to survive type checking, making it to query execution -/// -- where they will only be cast to UUIDs at runtime in the failure -/// -- cases they're supposed to catch. -/// CAST( AS text), -/// 'parent-not-current' -/// ) AS UUID) -/// ), -/// -/// -- Determine the new version number to use: either 1 if this is the -/// -- first sitrep being made the current sitrep, or 1 higher than -/// -- the previous sitrep's version. -/// -- -/// -- The final clauses of each of these WHERE clauses repeat the -/// -- checks performed above in `check_validity`, and will cause this -/// -- subquery to return no rows if we should not allow the new -/// -- target to be set. -/// new_sitrep AS ( -/// SELECT 1 AS new_version FROM "sitrep" -/// WHERE -/// "id" = -/// AND "parent_sitrep_id" IS NULL -/// AND NOT EXISTS (SELECT version FROM current_sitrep) -/// UNION -/// SELECT current_sitrep.version + 1 FROM current_sitrep, "sitrep" -/// WHERE -/// "id" = -/// AND "parent_sitrep_id" IS NOT NULL -/// AND "parent_sitrep_id" = current_sitrep.sitrep_id -/// ) -/// -/// -- Perform the actual insertion. -/// INSERT INTO "sitrep_history"( -/// "version","sitrep_id","time_made_current" -/// ) -/// SELECT -/// new_sitrep.new_version, -/// , -/// NOW() -/// FROM new_sitrep -/// ``` -#[derive(Debug, Clone, Copy)] -struct InsertSitrepVersionQuery { - sitrep_id: SitrepUuid, -} - // Uncastable sentinel used to detect we attempt to make a sitrep current when // its parent sitrep ID is no longer the current sitrep. const PARENT_NOT_CURRENT: &str = "parent-not-current"; @@ -539,165 +682,6 @@ const PARENT_NOT_CURRENT_ERROR_MESSAGE: &str = "could not parse \ \"parent-not-current\" as type uuid: \ uuid: incorrect UUID length: parent-not-current"; -impl InsertSitrepVersionQuery { - fn decode_error(&self, err: DieselError) -> InsertSitrepError { - match err { - DieselError::DatabaseError(DatabaseErrorKind::Unknown, info) - if info.message() == PARENT_NOT_CURRENT_ERROR_MESSAGE => - { - InsertSitrepError::ParentNotCurrent(self.sitrep_id) - } - err => { - let err = public_error_from_diesel(err, ErrorHandler::Server) - .internal_context("failed to insert new sitrep version"); - InsertSitrepError::Other(err) - } - } - } -} - -impl QueryId for InsertSitrepVersionQuery { - type QueryId = (); - const HAS_STATIC_QUERY_ID: bool = false; -} -impl QueryFragment for InsertSitrepVersionQuery { - fn walk_ast<'a>( - &'a self, - mut out: AstPass<'_, 'a, Pg>, - ) -> diesel::QueryResult<()> { - use nexus_db_schema::schema; - - type FromClause = - diesel::internal::table_macro::StaticQueryFragmentInstance; - const SITREP_FROM_CLAUSE: FromClause = - FromClause::new(); - const SITREP_HISTORY_FROM_CLAUSE: FromClause< - schema::fm_sitrep_history::table, - > = FromClause::new(); - - const CURRENT_SITREP: &'static str = "current_sitrep"; - - out.push_sql("WITH "); - out.push_identifier(CURRENT_SITREP)?; - out.push_sql(" AS (SELECT "); - out.push_identifier(history_dsl::version::NAME)?; - out.push_sql(" AS version,"); - out.push_identifier(history_dsl::sitrep_id::NAME)?; - out.push_sql(" AS sitrep_id"); - out.push_sql(" FROM "); - SITREP_HISTORY_FROM_CLAUSE.walk_ast(out.reborrow())?; - out.push_sql(" ORDER BY "); - out.push_identifier(history_dsl::version::NAME)?; - out.push_sql(" DESC LIMIT 1),"); - - out.push_sql( - "check_validity AS MATERIALIZED ( \ - SELECT \ - CAST( \ - IF(", - ); - out.push_sql("(SELECT "); - out.push_identifier(sitrep_dsl::parent_sitrep_id::NAME)?; - out.push_sql(" FROM "); - SITREP_FROM_CLAUSE.walk_ast(out.reborrow())?; - out.push_sql(", "); - out.push_identifier(CURRENT_SITREP)?; - out.push_sql(" WHERE "); - out.push_identifier(sitrep_dsl::id::NAME)?; - out.push_sql(" = "); - out.push_bind_param::( - self.sitrep_id.as_untyped_uuid(), - )?; - out.push_sql(" AND "); - out.push_identifier(CURRENT_SITREP)?; - out.push_sql(".sitrep_id = "); - out.push_identifier(sitrep_dsl::parent_sitrep_id::NAME)?; - out.push_sql( - ") IS NOT NULL \ - OR \ - (SELECT 1 FROM ", - ); - SITREP_FROM_CLAUSE.walk_ast(out.reborrow())?; - out.push_sql(" WHERE "); - out.push_identifier(sitrep_dsl::id::NAME)?; - out.push_sql(" = "); - out.push_bind_param::( - self.sitrep_id.as_untyped_uuid(), - )?; - out.push_sql(" AND "); - out.push_identifier(sitrep_dsl::parent_sitrep_id::NAME)?; - out.push_sql( - "IS NULL \ - AND NOT EXISTS ( \ - SELECT version FROM current_sitrep) \ - ) = 1, ", - ); - out.push_sql(" CAST("); - out.push_bind_param::( - self.sitrep_id.as_untyped_uuid(), - )?; - out.push_sql(" AS text), "); - out.push_bind_param::( - &PARENT_NOT_CURRENT, - )?; - out.push_sql( - ") \ - AS UUID) \ - ), ", - ); - - out.push_sql("new_sitrep AS (SELECT 1 AS new_version FROM "); - SITREP_FROM_CLAUSE.walk_ast(out.reborrow())?; - out.push_sql(" WHERE "); - out.push_identifier(sitrep_dsl::id::NAME)?; - out.push_sql(" = "); - out.push_bind_param::( - self.sitrep_id.as_untyped_uuid(), - )?; - out.push_sql(" AND "); - out.push_identifier(sitrep_dsl::parent_sitrep_id::NAME)?; - out.push_sql( - " IS NULL \ - AND NOT EXISTS \ - (SELECT version FROM current_sitrep) \ - UNION \ - SELECT current_sitrep.version + 1 FROM \ - current_sitrep, ", - ); - SITREP_FROM_CLAUSE.walk_ast(out.reborrow())?; - out.push_sql(" WHERE "); - out.push_identifier(sitrep_dsl::id::NAME)?; - out.push_sql(" = "); - out.push_bind_param::( - self.sitrep_id.as_untyped_uuid(), - )?; - out.push_sql(" AND "); - out.push_identifier(sitrep_dsl::parent_sitrep_id::NAME)?; - out.push_sql(" IS NOT NULL AND "); - out.push_identifier(sitrep_dsl::parent_sitrep_id::NAME)?; - out.push_sql(" = current_sitrep.sitrep_id) "); - - out.push_sql("INSERT INTO "); - SITREP_HISTORY_FROM_CLAUSE.walk_ast(out.reborrow())?; - out.push_sql("("); - out.push_identifier(history_dsl::version::NAME)?; - out.push_sql(","); - out.push_identifier(history_dsl::sitrep_id::NAME)?; - out.push_sql(","); - out.push_identifier(history_dsl::time_made_current::NAME)?; - out.push_sql(") SELECT new_sitrep.new_version, "); - out.push_bind_param::( - self.sitrep_id.as_untyped_uuid(), - )?; - out.push_sql(", NOW()"); - out.push_sql(" FROM new_sitrep"); - - Ok(()) - } -} - -impl RunQueryDsl for InsertSitrepVersionQuery {} - #[cfg(test)] mod tests { use super::*; @@ -706,6 +690,7 @@ mod tests { use crate::db::pub_test_utils::TestDatabase; use crate::db::raw_query_builder::expectorate_query_contents; use chrono::Utc; + use diesel::pg::Pg; use nexus_types::fm; use omicron_test_utils::dev; use omicron_uuid_kinds::CollectionUuid; @@ -719,7 +704,7 @@ mod tests { let pool = db.pool(); let conn = pool.claim().await.unwrap(); - let query = InsertSitrepVersionQuery { sitrep_id: SitrepUuid::nil() }; + let query = DataStore::insert_sitrep_version_query(SitrepUuid::nil()); // Before trying to explain the query, let's start by making sure it's // valid SQL... @@ -744,6 +729,16 @@ mod tests { logctx.cleanup_successful(); } + #[tokio::test] + async fn expectorate_insert_sitrep_version_query() { + let query = DataStore::insert_sitrep_version_query(SitrepUuid::nil()); + expectorate_query_contents( + &query, + "tests/output/insert_sitrep_version_query.sql", + ) + .await; + } + // Ensure we have the right query contents. #[tokio::test] async fn expectorate_sitrep_list_orphans_no_marker() { From 7cdabb434b2bef564a429c0117ca0eff7fac7298 Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Mon, 10 Nov 2025 12:58:54 -0800 Subject: [PATCH 2/7] tidy stuff up --- nexus/db-queries/src/db/datastore/fm.rs | 135 ++++-------------------- 1 file changed, 20 insertions(+), 115 deletions(-) diff --git a/nexus/db-queries/src/db/datastore/fm.rs b/nexus/db-queries/src/db/datastore/fm.rs index 35647c7f84..2b1d41cf42 100644 --- a/nexus/db-queries/src/db/datastore/fm.rs +++ b/nexus/db-queries/src/db/datastore/fm.rs @@ -199,7 +199,7 @@ impl DataStore { DieselError::DatabaseError( DatabaseErrorKind::Unknown, info, - ) if info.message() == PARENT_NOT_CURRENT_ERROR_MESSAGE => { + ) if info.message() == Self::PARENT_NOT_CURRENT_ERROR_MESSAGE => { InsertSitrepError::ParentNotCurrent(sitrep.id()) } err => { @@ -214,6 +214,14 @@ impl DataStore { .map(|_| ()) } + // Uncastable sentinel used to detect we attempt to make a sitrep current when + // its parent sitrep ID is no longer the current sitrep. + const PARENT_NOT_CURRENT: &str = "parent-not-current"; + + // Error messages generated from the above sentinel values. + const PARENT_NOT_CURRENT_ERROR_MESSAGE: &str = "could not parse \ + \"parent-not-current\" as type uuid: \ + uuid: incorrect UUID length: parent-not-current"; /// Query to insert a new sitrep version into the `fm_sitrep_history` table, /// making it the current sitrep. @@ -245,100 +253,6 @@ impl DataStore { /// `Datastore::fm_sitrep_insert` method, which also creates the sitrep. So, it /// is impossible for a consumer of this API to attempt to make a sitrep current /// without having first created it. - /// - /// The SQL generated for this CTE looks like this: - /// - /// ```sql - /// WITH - /// -- Subquery to fetch the current sitrep (i.e., the row with the max - /// -- version). - /// current_sitrep AS ( - /// SELECT - /// "version" AS version, - /// "sitrep_id" AS sitrep_id, - /// FROM "fm_sitrep_history" - /// ORDER BY "version" DESC - /// LIMIT 1 - /// ), - /// - /// -- Error checking subquery: This uses similar tricks as elsewhere in - /// -- this crate to `CAST(... AS UUID)` with non-UUID values that result - /// -- in runtime errors in specific cases, allowing us to give accurate - /// -- error messages. - /// -- - /// -- This checks that the sitrep descends directly from the current - /// -- sitrep, and will fail the query if it does not. - /// check_validity AS MATERIALIZED ( - /// SELECT CAST(IF( - /// -- Check for whether our new sitrep's parent matches our current - /// -- sitrep. There are two cases here: The first is the common case - /// -- (i.e., the new sitrep has a parent: does it match the current - /// -- sitrep ID?). The second is the bootstrapping check: if we're - /// -- trying to insert a new sitrep that does not have a parent, - /// -- we should not have a sitrep target at all. - /// -- - /// -- If either of these cases fails, we return `parent-not-current`. - /// ( - /// SELECT "parent_sitrep_id" FROM "sitrep", current_sitrep - /// WHERE - /// "id" = - /// AND current_sitrep.sitrep_id = "parent_sitrep_id" - /// ) IS NOT NULL - /// OR - /// ( - /// SELECT 1 FROM "sitrep" - /// WHERE - /// "id" = - /// AND "parent_sitrep_id" IS NULL - /// AND NOT EXISTS (SELECT version FROM current_sitrep) - /// ) = 1, - /// -- Sometime between v22.1.9 and v22.2.19, Cockroach's type checker - /// -- became too smart for our `CAST(... as UUID)` error checking - /// -- gadget: it can infer that `` must be a UUID, so - /// -- then tries to parse 'parent-not-target' and 'no-such-blueprint' - /// -- as UUIDs _during typechecking_, which causes the query to always - /// -- fail. We can defeat this by casting the UUID to text here, which - /// -- will allow the 'parent-not-target' and 'no-such-blueprint' - /// -- sentinels to survive type checking, making it to query execution - /// -- where they will only be cast to UUIDs at runtime in the failure - /// -- cases they're supposed to catch. - /// CAST( AS text), - /// 'parent-not-current' - /// ) AS UUID) - /// ), - /// - /// -- Determine the new version number to use: either 1 if this is the - /// -- first sitrep being made the current sitrep, or 1 higher than - /// -- the previous sitrep's version. - /// -- - /// -- The final clauses of each of these WHERE clauses repeat the - /// -- checks performed above in `check_validity`, and will cause this - /// -- subquery to return no rows if we should not allow the new - /// -- target to be set. - /// new_sitrep AS ( - /// SELECT 1 AS new_version FROM "sitrep" - /// WHERE - /// "id" = - /// AND "parent_sitrep_id" IS NULL - /// AND NOT EXISTS (SELECT version FROM current_sitrep) - /// UNION - /// SELECT current_sitrep.version + 1 FROM current_sitrep, "sitrep" - /// WHERE - /// "id" = - /// AND "parent_sitrep_id" IS NOT NULL - /// AND "parent_sitrep_id" = current_sitrep.sitrep_id - /// ) - /// - /// -- Perform the actual insertion. - /// INSERT INTO "sitrep_history"( - /// "version","sitrep_id","time_made_current" - /// ) - /// SELECT - /// new_sitrep.new_version, - /// , - /// NOW() - /// FROM new_sitrep - /// ``` fn insert_sitrep_version_query( sitrep_id: SitrepUuid, ) -> TypedSqlQuery { @@ -406,7 +320,7 @@ impl DataStore { .param() .bind::(sitrep_id.into_untyped_uuid()) .sql(" AS text), ") - .sql(PARENT_NOT_CURRENT) + .sql(Self::PARENT_NOT_CURRENT) .sql( ") AS UUID \ ) \ @@ -673,15 +587,6 @@ pub enum InsertSitrepError { ParentNotCurrent(SitrepUuid), } -// Uncastable sentinel used to detect we attempt to make a sitrep current when -// its parent sitrep ID is no longer the current sitrep. -const PARENT_NOT_CURRENT: &str = "parent-not-current"; - -// Error messages generated from the above sentinel values. -const PARENT_NOT_CURRENT_ERROR_MESSAGE: &str = "could not parse \ - \"parent-not-current\" as type uuid: \ - uuid: incorrect UUID length: parent-not-current"; - #[cfg(test)] mod tests { use super::*; @@ -697,6 +602,16 @@ mod tests { use omicron_uuid_kinds::OmicronZoneUuid; use std::collections::BTreeSet; + #[tokio::test] + async fn expectorate_insert_sitrep_version_query() { + let query = DataStore::insert_sitrep_version_query(SitrepUuid::nil()); + expectorate_query_contents( + &query, + "tests/output/insert_sitrep_version_query.sql", + ) + .await; + } + #[tokio::test] async fn explain_insert_sitrep_version_query() { let logctx = dev::test_setup_log("explain_insert_sitrep_version_query"); @@ -729,16 +644,6 @@ mod tests { logctx.cleanup_successful(); } - #[tokio::test] - async fn expectorate_insert_sitrep_version_query() { - let query = DataStore::insert_sitrep_version_query(SitrepUuid::nil()); - expectorate_query_contents( - &query, - "tests/output/insert_sitrep_version_query.sql", - ) - .await; - } - // Ensure we have the right query contents. #[tokio::test] async fn expectorate_sitrep_list_orphans_no_marker() { From cf0c78e7b7e5ed6b9dcc144d75d252d4ac5bff28 Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Mon, 10 Nov 2025 13:07:08 -0800 Subject: [PATCH 3/7] fixy --- nexus/db-queries/src/db/datastore/fm.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/nexus/db-queries/src/db/datastore/fm.rs b/nexus/db-queries/src/db/datastore/fm.rs index 2b1d41cf42..03685767d2 100644 --- a/nexus/db-queries/src/db/datastore/fm.rs +++ b/nexus/db-queries/src/db/datastore/fm.rs @@ -371,7 +371,7 @@ impl DataStore { .bind::(sitrep_id.into_untyped_uuid()) .sql( " AND parent_sitrep_id IS NOT NULL \ - AND parent_sitrep_id = current_sitrep.sitrep_id \ + AND parent_sitrep_id = current_sitrep.sitrep_id \ ) ", ); @@ -379,8 +379,8 @@ impl DataStore { builder .sql( "INSERT INTO omicron.public.fm_sitrep_history \ - (version, sitrep_id, time_made_current) \ - SELECT new_sitrep.new_version, ", + (version, sitrep_id, time_made_current) \ + SELECT new_sitrep.new_version, ", ) .param() .bind::(sitrep_id.into_untyped_uuid()) From 80bf5f62318d6ac5dc7ab8845fd8877fe8c66414 Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Mon, 10 Nov 2025 13:22:50 -0800 Subject: [PATCH 4/7] fix accidentally unquoted sql string --- nexus/db-queries/src/db/datastore/fm.rs | 5 +- .../output/insert_sitrep_version_query.sql | 65 +++++++++++++++++++ 2 files changed, 68 insertions(+), 2 deletions(-) create mode 100644 nexus/db-queries/tests/output/insert_sitrep_version_query.sql diff --git a/nexus/db-queries/src/db/datastore/fm.rs b/nexus/db-queries/src/db/datastore/fm.rs index 03685767d2..5a2958c232 100644 --- a/nexus/db-queries/src/db/datastore/fm.rs +++ b/nexus/db-queries/src/db/datastore/fm.rs @@ -319,10 +319,11 @@ impl DataStore { .sql(" CAST(") .param() .bind::(sitrep_id.into_untyped_uuid()) - .sql(" AS text), ") + // opening single quote for `PARENT_NOT_CURRENT` string literal + .sql(" AS text), '") .sql(Self::PARENT_NOT_CURRENT) .sql( - ") AS UUID \ + "') AS UUID \ ) \ ), ", ); diff --git a/nexus/db-queries/tests/output/insert_sitrep_version_query.sql b/nexus/db-queries/tests/output/insert_sitrep_version_query.sql new file mode 100644 index 0000000000..0227f48455 --- /dev/null +++ b/nexus/db-queries/tests/output/insert_sitrep_version_query.sql @@ -0,0 +1,65 @@ +WITH + current_sitrep + AS ( + SELECT + version AS version, sitrep_id AS sitrep_id + FROM + omicron.public.fm_sitrep_history + ORDER BY + version DESC + LIMIT + 1 + ), + check_validity + AS MATERIALIZED ( + SELECT + CAST( + IF( + ( + SELECT + parent_sitrep_id + FROM + omicron.public.fm_sitrep, current_sitrep + WHERE + id = $1 AND current_sitrep.sitrep_id = parent_sitrep_id + ) IS NOT NULL + OR ( + SELECT + 1 + FROM + omicron.public.fm_sitrep + WHERE + id = $2 + AND parent_sitrep_id IS NULL + AND NOT EXISTS(SELECT version FROM current_sitrep) + ) + = 1, + CAST($3 AS STRING), + 'parent-not-current' + ) + AS UUID + ) + ), + new_sitrep + AS ( + SELECT + 1 AS new_version + FROM + omicron.public.fm_sitrep + WHERE + id = $4 AND parent_sitrep_id IS NULL AND NOT EXISTS(SELECT version FROM current_sitrep) + UNION + SELECT + current_sitrep.version + 1 + FROM + current_sitrep, omicron.public.fm_sitrep + WHERE + id = $5 AND parent_sitrep_id IS NOT NULL AND parent_sitrep_id = current_sitrep.sitrep_id + ) +INSERT +INTO + omicron.public.fm_sitrep_history (version, sitrep_id, time_made_current) +SELECT + new_sitrep.new_version, $6, now() +FROM + new_sitrep From b3663cd5d76d2c9889be98d7671cde2eb95340c0 Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Mon, 10 Nov 2025 13:23:26 -0800 Subject: [PATCH 5/7] rustfmt --- nexus/db-queries/src/db/datastore/fm.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/nexus/db-queries/src/db/datastore/fm.rs b/nexus/db-queries/src/db/datastore/fm.rs index 5a2958c232..12a078abe8 100644 --- a/nexus/db-queries/src/db/datastore/fm.rs +++ b/nexus/db-queries/src/db/datastore/fm.rs @@ -199,7 +199,9 @@ impl DataStore { DieselError::DatabaseError( DatabaseErrorKind::Unknown, info, - ) if info.message() == Self::PARENT_NOT_CURRENT_ERROR_MESSAGE => { + ) if info.message() + == Self::PARENT_NOT_CURRENT_ERROR_MESSAGE => + { InsertSitrepError::ParentNotCurrent(sitrep.id()) } err => { From dbc2e9f124cfa708d778c6b9caeb02a6947859a5 Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Mon, 10 Nov 2025 14:17:14 -0800 Subject: [PATCH 6/7] dont SELECT foo AS foo --- nexus/db-queries/src/db/datastore/fm.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/nexus/db-queries/src/db/datastore/fm.rs b/nexus/db-queries/src/db/datastore/fm.rs index 12a078abe8..04d44c72e2 100644 --- a/nexus/db-queries/src/db/datastore/fm.rs +++ b/nexus/db-queries/src/db/datastore/fm.rs @@ -264,7 +264,7 @@ impl DataStore { // version). builder.sql( "WITH current_sitrep AS ( \ - SELECT version AS version, sitrep_id AS sitrep_id \ + SELECT version, sitrep_id \ FROM omicron.public.fm_sitrep_history \ ORDER BY version DESC \ LIMIT 1 \ From 2eeffb6858ce2d66f12959c696d6e87d7fdbd9e3 Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Mon, 10 Nov 2025 15:51:39 -0800 Subject: [PATCH 7/7] fixup expected output --- .../tests/output/insert_sitrep_version_query.sql | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/nexus/db-queries/tests/output/insert_sitrep_version_query.sql b/nexus/db-queries/tests/output/insert_sitrep_version_query.sql index 0227f48455..0b749431d4 100644 --- a/nexus/db-queries/tests/output/insert_sitrep_version_query.sql +++ b/nexus/db-queries/tests/output/insert_sitrep_version_query.sql @@ -1,14 +1,7 @@ WITH current_sitrep AS ( - SELECT - version AS version, sitrep_id AS sitrep_id - FROM - omicron.public.fm_sitrep_history - ORDER BY - version DESC - LIMIT - 1 + SELECT version, sitrep_id FROM omicron.public.fm_sitrep_history ORDER BY version DESC LIMIT 1 ), check_validity AS MATERIALIZED (