From d6c6f9d492b8223dad11c573d299c6b25e1ccd36 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Fri, 13 Mar 2026 12:21:59 -0700 Subject: [PATCH 1/3] Auto-generated SQL instead of hand-written comments --- .../db-queries/src/db/datastore/deployment.rs | 105 +--------------- nexus/db-queries/src/db/datastore/ip_pool.rs | 46 ------- nexus/db-queries/src/db/queries/ip_pool.rs | 115 +++++------------- .../src/db/queries/network_interface.rs | 79 ------------ nexus/db-queries/src/db/queries/vpc_subnet.rs | 65 +++------- .../output/filter_overlapping_ip_ranges.sql | 91 ++++++++++++++ .../tests/output/insert_vpc_subnet_query.sql | 21 ++++ 7 files changed, 163 insertions(+), 359 deletions(-) create mode 100644 nexus/db-queries/tests/output/filter_overlapping_ip_ranges.sql create mode 100644 nexus/db-queries/tests/output/insert_vpc_subnet_query.sql diff --git a/nexus/db-queries/src/db/datastore/deployment.rs b/nexus/db-queries/src/db/datastore/deployment.rs index ae941f62b66..ec0ff47bf0c 100644 --- a/nexus/db-queries/src/db/datastore/deployment.rs +++ b/nexus/db-queries/src/db/datastore/deployment.rs @@ -3025,110 +3025,7 @@ fn decode_target_insert_error( /// direct child of the previous current target. Enforcing the above has some /// subtleties (particularly around handling the "first blueprint with no /// parent" case). These are expanded on below through inline comments on the -/// query we generate: -/// -/// ```sql -/// WITH -/// -- Subquery to fetch the current target (i.e., the row with the max -/// -- veresion in `bp_target`). -/// current_target AS ( -/// SELECT -/// version, -/// blueprint_id -/// FROM bp_target -/// 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. -/// -- -/// -- These checks are not required for correct behavior by the insert -/// -- below. If we removed them, the insert would insert 0 rows if -/// -- these checks would have failed. But they make it easier to report -/// -- specific problems to our caller. -/// -- -/// -- The specific cases we check here are noted below. -/// check_validity AS MATERIALIZED ( -/// SELECT CAST(IF( -/// -- Return `no-such-blueprint` if the ID we're being told to -/// -- set as the target doesn't exist in the blueprint table. -/// (SELECT "id" FROM "blueprint" WHERE "id" = ) IS NULL, -/// 'no-such-blueprint', -/// IF( -/// -- Check for whether our new target's parent matches our current -/// -- target. There are two cases here: The first is the common case -/// -- (i.e., the new target has a parent: does it match the current -/// -- target ID?). The second is the bootstrapping check: if we're -/// -- trying to insert a new target that does not have a parent, -/// -- we should not have a current target at all. -/// -- -/// -- If either of these cases fails, we return `parent-not-target`. -/// ( -/// SELECT "parent_blueprint_id" FROM "blueprint", current_target -/// WHERE -/// "id" = -/// AND current_target.blueprint_id = "parent_blueprint_id" -/// ) IS NOT NULL -/// OR -/// ( -/// SELECT 1 FROM "blueprint" -/// WHERE -/// "id" = -/// AND "parent_blueprint_id" IS NULL -/// AND NOT EXISTS (SELECT version FROM current_target) -/// ) = 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-target' -/// ) -/// ) AS UUID) -/// ), -/// -/// -- Determine the new version number to use: either 1 if this is the -/// -- first blueprint being made the current target, or 1 higher than -/// -- the previous target'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_target AS ( -/// SELECT 1 AS new_version FROM "blueprint" -/// WHERE -/// "id" = -/// AND "parent_blueprint_id" IS NULL -/// AND NOT EXISTS (SELECT version FROM current_target) -/// UNION -/// SELECT current_target.version + 1 FROM current_target, "blueprint" -/// WHERE -/// "id" = -/// AND "parent_blueprint_id" IS NOT NULL -/// AND "parent_blueprint_id" = current_target.blueprint_id -/// ) -/// -/// -- Perform the actual insertion. -/// INSERT INTO "bp_target"( -/// "version","blueprint_id","enabled","time_made_target" -/// ) -/// SELECT -/// new_target.new_version, -/// , -/// , -/// -/// FROM new_target -/// ``` +/// query we generate. fn insert_target_query( target_id: BlueprintUuid, enabled: bool, diff --git a/nexus/db-queries/src/db/datastore/ip_pool.rs b/nexus/db-queries/src/db/datastore/ip_pool.rs index 35b5190ae0f..edd13cd9733 100644 --- a/nexus/db-queries/src/db/datastore/ip_pool.rs +++ b/nexus/db-queries/src/db/datastore/ip_pool.rs @@ -2113,52 +2113,6 @@ fn extract_uuid_cast_sentinel(msg: &str) -> Option<&str> { // Pool and a Silo. It maintains the invariant that a pool can be reserved for // Oxide internal usage XOR linked to customer silos. It also checks that the // pool and silo still exist when the query is run. -// -// The full query is: -// -// ```sql -// WITH -// -- Select the IP Pool by ID, used to ensure it still exists when we run -// -- this query. Also select the reservation type, and fail if the pool is -// -- currently reserved for Oxide. Include `pool_type` and `ip_version` for -// -- denormalization into `ip_pool_resource`, which allows for a partial -// -- index we can constrain pool defaults on. -// ip_pool AS ( -// SELECT -// CAST(IF(reservation_type != 'external_silos', 'bad-link-type', $1) AS UUID) AS id, -// pool_type, -// ip_version -// FROM ip_pool -// WHERE id = $2 AND time_deleted IS NULL -// ), -// -- Select the Silo by ID, used to ensure it still exists when we run this -// -- query -// silo AS (SELECT id FROM silo WHERE id = $3 AND time_deleted IS NULL) -// INSERT -// INTO -// ip_pool_resource (ip_pool_id, resource_type, resource_id, is_default, pool_type, ip_version) -// SELECT -// -- If the pool exists, take its ID as a string. If it does not exist, take -// -- the string `'ip-pool-deleted'`. Attempt to cast the result to a UUID. -// -- This is the "true or cast error" trick we use in many places. -// CAST(COALESCE(CAST(ip.id AS STRING), 'ip-pool-deleted') AS UUID), -// -- The resource type, always 'silo' here. -// $4, -// -- If the silo exists, take its ID as a string. If it does not exist, take -// -- the string `'silo-deleted'`. Attempt to cast the result to a UUID. -// -- This is the "true or cast error" trick we use in many places. -// CAST(COALESCE(CAST(s.id AS STRING), 'silo-deleted') AS UUID), -// $5, -// -- Denormalized from ip_pool for the partial index constraint on defaults. -// ip.pool_type, -// ip.ip_version -// FROM -// (SELECT 1) AS dummy -// LEFT JOIN ip_pool AS ip ON true -// LEFT JOIN silo AS s ON true -// RETURNING -// * -// ``` fn link_ip_pool_to_external_silo_query( ip_pool_resource: &IncompleteIpPoolResource, ) -> TypedSqlQuery> { diff --git a/nexus/db-queries/src/db/queries/ip_pool.rs b/nexus/db-queries/src/db/queries/ip_pool.rs index 2a681d8a115..5bad7fcd206 100644 --- a/nexus/db-queries/src/db/queries/ip_pool.rs +++ b/nexus/db-queries/src/db/queries/ip_pool.rs @@ -24,92 +24,11 @@ use uuid::Uuid; /// This query is used when inserting a new IP range into an existing IP Pool. /// Those ranges must currently be unique globally, across all pools. This query /// selects the candidate range, _if_ it does not overlap with any existing -/// range. I.e., it filters out the candidate if it overlaps. The query looks -/// like +/// range. I.e., it filters out the candidate if it overlaps. /// -/// ```sql -/// SELECT -/// -/// WHERE -/// -- Check for ranges that contain the candidate first address -/// NOT EXISTS( -/// SELECT -/// id -/// FROM -/// ip_pool_range -/// WHERE -/// >= first_address AND -/// <= last_address AND -/// time_deleted IS NULL -/// LIMIT 1 -/// ) -/// AND -/// -- Check for ranges that contain the candidate last address -/// NOT EXISTS( -/// SELECT -/// id -/// FROM -/// ip_pool_range -/// WHERE -/// >= first_address AND -/// <= last_address AND -/// time_deleted IS NULL -/// LIMIT 1 -/// ) -/// AND -/// -- Check for ranges whose first address is contained by the candidate -/// -- range -/// NOT EXISTS( -/// SELECT -/// id -/// FROM -/// ip_pool_range -/// WHERE -/// first_address >= AND -/// first_address <= AND -/// time_deleted IS NULL -/// LIMIT 1 -/// ) -/// AND -/// -- Check for ranges whose last address is contained by the candidate -/// -- range -/// NOT EXISTS( -/// SELECT -/// id -/// FROM -/// ip_pool_range -/// WHERE -/// last_address >= AND -/// last_address <= AND -/// time_deleted IS NULL -/// LIMIT 1 -/// ) -/// -/// -- The query repeats the exact same 4 expressions above, but referring -/// -- to the `subnet_pool_member` table. This ensures we have no overlap -/// -- between external addresses within those tables or between them. -/// ``` -/// -/// That's a lot of duplication, but it's to help with the scalability of the -/// query. Collapsing those different `EXISTS()` subqueries into one set of -/// `WHERE` clauses would require an `OR`. For example: -/// -/// ```sql -/// WHERE -/// ( -/// >= first_address AND -/// <= last_address -/// ) -/// OR -/// ( -/// >= first_address AND -/// <= last_address -/// ) -/// AND -/// time_deleted IS NULL -/// ``` -/// -/// That `OR` means the database cannot use the indexes we've supplied on the +/// The query uses multiple separate `NOT EXISTS` subqueries rather than a +/// single subquery with `OR` clauses. That's a lot of duplication, but it's +/// to help with the scalability of the query. An `OR` means the database cannot use the indexes we've supplied on the /// `first_address` and `last_address` columns, and must resort to a full table /// scan. #[derive(Debug, Clone)] @@ -334,3 +253,29 @@ impl QueryFragment for FilterOverlappingIpRangesValues { self.0.walk_ast(out.reborrow()) } } + +#[cfg(test)] +mod tests { + use super::*; + use crate::db::raw_query_builder::expectorate_query_contents; + use omicron_common::address::Ipv4Range; + + #[tokio::test] + async fn expectorate_filter_overlapping_ip_ranges() { + let range = IpPoolRange::new( + &Ipv4Range::new( + std::net::Ipv4Addr::new(10, 0, 0, 1), + std::net::Ipv4Addr::new(10, 0, 0, 5), + ) + .unwrap() + .into(), + Uuid::nil(), + ); + let query = FilterOverlappingIpRanges { range }; + expectorate_query_contents( + &query, + "tests/output/filter_overlapping_ip_ranges.sql", + ) + .await; + } +} diff --git a/nexus/db-queries/src/db/queries/network_interface.rs b/nexus/db-queries/src/db/queries/network_interface.rs index baf499078c5..a7002a5f8f6 100644 --- a/nexus/db-queries/src/db/queries/network_interface.rs +++ b/nexus/db-queries/src/db/queries/network_interface.rs @@ -1601,85 +1601,6 @@ fn push_instance_state_verification_subquery<'a>( /// Second, while an instance may have zero or more interfaces, if it has one /// or more, exactly one of those must be the primary interface. That means /// we can only delete the primary interface if there are no secondary interfaces. -/// The full query is: -/// -/// ```sql -/// WITH -/// instance AS MATERIALIZED (SELECT CAST( -/// CASE -/// COALESCE( -/// (SELECT -/// state -/// FROM -/// instance -/// WHERE -/// id = AND -/// time_deleted IS NULL -/// ), -/// 'destroyed' -/// ) -/// WHEN 'stopped' THEN '' -/// WHEN 'creating' THEN '' -/// WHEN 'failed' THEN '' -/// WHEN 'destroyed' THEN 'no-instance' -/// ELSE 'bad-state' -/// END -/// AS UUID)), -/// interface AS MATERIALIZED ( -/// SELECT CAST(IF( -/// ( -/// SELECT -/// NOT is_primary -/// FROM -/// network_interface -/// WHERE -/// id = AND -/// time_deleted IS NULL -/// ) -/// OR -/// ( -/// SELECT -/// COUNT(*) -/// FROM -/// network_interface -/// WHERE -/// parent_id = AND -/// kind = AND -/// time_deleted IS NULL -/// ) <= 1, -/// '', -/// 'secondaries' -/// ) AS UUID) -/// ), -/// found_interface AS ( -/// SELECT -/// id -/// FROM -/// network_interface -/// WHERE -/// id = -/// ), -/// updated AS ( -/// UPDATE -/// network_interface -/// SET -/// time_deleted = NOW() -/// WHERE -/// id = AND -/// time_deleted IS NULL -/// RETURNING -/// id -/// ) -/// SELECT -/// found_interface.id, -/// updated.id -/// FROM -/// found_interface -/// LEFT JOIN -/// updated -/// ON -/// found_interface.id = updated.id -/// ``` /// /// Notes /// ----- diff --git a/nexus/db-queries/src/db/queries/vpc_subnet.rs b/nexus/db-queries/src/db/queries/vpc_subnet.rs index f3105948881..ab4b15171f5 100644 --- a/nexus/db-queries/src/db/queries/vpc_subnet.rs +++ b/nexus/db-queries/src/db/queries/vpc_subnet.rs @@ -30,51 +30,6 @@ use uuid::Uuid; /// other modification or alteration. If callers care, they can inspect the /// record to make sure it's what they expected, though that's usually a fraught /// endeavor. -/// -/// Here is the entire query: -/// -/// ```sql -/// WITH -/// -- This CTE generates a casting error if any live records, other than _this_ -/// -- record, have overlapping IP blocks of either family. -/// overlap AS MATERIALIZED ( -/// SELECT -/// -- NOTE: This cast always fails, we just use _how_ it fails to -/// -- learn which IP block overlaps. The filter `id != ` below -/// -- means we're explicitly ignoring an existing, identical record. -/// -- So this cast is only run if there is another record in the same -/// -- VPC with an overlapping subnet, which is exactly the error case -/// -- we're trying to cacth. -/// CAST( -/// IF( -/// ( && ipv4_block), -/// 'ipv4', -/// 'ipv6' -/// ) -/// AS BOOL -/// ) -/// FROM -/// vpc_subnet -/// WHERE -/// vpc_id = AND -/// time_deleted IS NULL AND -/// id != AND -/// ( -/// (ipv4_block && ) OR -/// (ipv6_block && ) -/// ) -/// ) -/// INSERT INTO -/// vpc_subnet -/// VALUES ( -/// -/// ) -/// ON CONFLICT (id) -/// -- We use this "no-op" update to allow us to return the actual row from the -/// -- DB, either the existing or inserted one. -/// DO UPDATE SET id = id -/// RETURNING *; -/// ``` #[derive(Clone, Debug)] pub struct InsertVpcSubnetQuery { /// The subnet to insert @@ -296,11 +251,31 @@ mod test { use crate::db::explain::ExplainableAsync as _; use crate::db::model::VpcSubnet; use crate::db::pub_test_utils::TestDatabase; + use crate::db::raw_query_builder::expectorate_query_contents; use omicron_common::api::external::IdentityMetadataCreateParams; use omicron_common::api::external::Name; use omicron_test_utils::dev; use std::convert::TryInto; + #[tokio::test] + async fn expectorate_insert_vpc_subnet_query() { + let ipv4_block = "172.30.0.0/24".parse().unwrap(); + let ipv6_block = "fd12:3456:7890::/64".parse().unwrap(); + let name = "a-name".to_string().try_into().unwrap(); + let description = "some description".to_string(); + let identity = IdentityMetadataCreateParams { name, description }; + let vpc_id = "d402369d-c9ec-c5ad-9138-9fbee732d53e".parse().unwrap(); + let subnet_id = "093ad2db-769b-e3c2-bc1c-b46e84ce5532".parse().unwrap(); + let row = + VpcSubnet::new(subnet_id, vpc_id, identity, ipv4_block, ipv6_block); + let query = InsertVpcSubnetQuery::new(row); + expectorate_query_contents( + &query, + "tests/output/insert_vpc_subnet_query.sql", + ) + .await; + } + #[tokio::test] async fn explain_insert_query() { let ipv4_block = "172.30.0.0/24".parse().unwrap(); diff --git a/nexus/db-queries/tests/output/filter_overlapping_ip_ranges.sql b/nexus/db-queries/tests/output/filter_overlapping_ip_ranges.sql new file mode 100644 index 00000000000..6f5a3cc5dc2 --- /dev/null +++ b/nexus/db-queries/tests/output/filter_overlapping_ip_ranges.sql @@ -0,0 +1,91 @@ +SELECT + $1, $2, $3, $4, $5, $6, $7, $8 +WHERE + NOT + EXISTS( + SELECT + 1 + FROM + ip_pool_range + WHERE + first_address >= $9 AND last_address <= $10 AND time_deleted IS NULL + LIMIT + 1 + ) + AND NOT + EXISTS( + SELECT + 1 + FROM + ip_pool_range + WHERE + first_address >= $11 AND last_address <= $12 AND time_deleted IS NULL + LIMIT + 1 + ) + AND NOT + EXISTS( + SELECT + 1 + FROM + ip_pool_range + WHERE + $13 >= first_address AND $14 <= last_address AND time_deleted IS NULL + LIMIT + 1 + ) + AND NOT + EXISTS( + SELECT + 1 + FROM + ip_pool_range + WHERE + $15 >= first_address AND $16 <= last_address AND time_deleted IS NULL + LIMIT + 1 + ) + AND NOT + EXISTS( + SELECT + 1 + FROM + subnet_pool_member + WHERE + first_address >= $17 AND last_address <= $18 AND time_deleted IS NULL + LIMIT + 1 + ) + AND NOT + EXISTS( + SELECT + 1 + FROM + subnet_pool_member + WHERE + first_address >= $19 AND last_address <= $20 AND time_deleted IS NULL + LIMIT + 1 + ) + AND NOT + EXISTS( + SELECT + 1 + FROM + subnet_pool_member + WHERE + $21 >= first_address AND $22 <= last_address AND time_deleted IS NULL + LIMIT + 1 + ) + AND NOT + EXISTS( + SELECT + 1 + FROM + subnet_pool_member + WHERE + $23 >= first_address AND $24 <= last_address AND time_deleted IS NULL + LIMIT + 1 + ) diff --git a/nexus/db-queries/tests/output/insert_vpc_subnet_query.sql b/nexus/db-queries/tests/output/insert_vpc_subnet_query.sql new file mode 100644 index 00000000000..93c57c344d6 --- /dev/null +++ b/nexus/db-queries/tests/output/insert_vpc_subnet_query.sql @@ -0,0 +1,21 @@ +WITH + overlap + AS MATERIALIZED ( + SELECT + CAST(IF((ipv4_block && $1), $2, $3) AS BOOL) + FROM + vpc_subnet + WHERE + vpc_id = $4 AND time_deleted IS NULL AND id != $5 AND (ipv4_block && $6 OR ipv6_block && $7) + ) +INSERT +INTO + vpc_subnet +VALUES + ($8, $9, $10, $11, $12, $13, $14, $15, $16, $17, $18) +ON CONFLICT + (id) +DO + UPDATE SET id = $19 +RETURNING + * From 9552e76a7978ac50f92615e9e8d778d09525f3f4 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Fri, 13 Mar 2026 13:01:19 -0700 Subject: [PATCH 2/3] reference output files, preserve comments --- .../db-queries/src/db/datastore/deployment.rs | 102 ++++++++++++------ nexus/db-queries/src/db/datastore/ip_pool.rs | 27 +++-- nexus/db-queries/src/db/queries/ip_pool.rs | 2 + .../src/db/queries/network_interface.rs | 7 ++ nexus/db-queries/src/db/queries/vpc_subnet.rs | 11 ++ 5 files changed, 111 insertions(+), 38 deletions(-) diff --git a/nexus/db-queries/src/db/datastore/deployment.rs b/nexus/db-queries/src/db/datastore/deployment.rs index ec0ff47bf0c..9447547771d 100644 --- a/nexus/db-queries/src/db/datastore/deployment.rs +++ b/nexus/db-queries/src/db/datastore/deployment.rs @@ -3025,7 +3025,8 @@ fn decode_target_insert_error( /// direct child of the previous current target. Enforcing the above has some /// subtleties (particularly around handling the "first blueprint with no /// parent" case). These are expanded on below through inline comments on the -/// query we generate. +/// query we generate. See `tests/output/insert_target_blueprint_query.sql` +/// for the full generated SQL. fn insert_target_query( target_id: BlueprintUuid, enabled: bool, @@ -3034,6 +3035,8 @@ fn insert_target_query( let mut builder = QueryBuilder::new(); let target_id = *target_id.as_untyped_uuid(); + // `current_target`: fetch the current target (row with max version + // in bp_target). builder.sql( "WITH \ current_target AS ( \ @@ -3043,53 +3046,90 @@ fn insert_target_query( FROM bp_target \ ORDER BY version DESC \ LIMIT 1 \ - ), \ - check_validity AS MATERIALIZED ( \ + ), ", + ); + + // `check_validity`: error-checking CTE. Uses the CAST(... AS UUID) + // trick with non-UUID sentinel values to produce runtime errors for + // specific failure cases. These checks aren't required for correct + // behavior by the insert below — if we removed them, the insert would + // simply insert 0 rows — but they make it easier to report specific + // problems to our caller. + // + // Cases checked: + // - 'no-such-blueprint': the blueprint ID we're being told to set + // as the target doesn't exist in the blueprint table. + // - 'parent-not-target': checks whether the new target's parent + // matches the current target. Two sub-cases: (1) common case — + // the new target has a parent; does it match the current target + // ID? (2) bootstrap — the new target has no parent; there must + // be no current target at all. If either check fails, we return + // 'parent-not-target'. + builder + .sql( + "check_validity AS MATERIALIZED ( \ SELECT \ CAST( \ IF( \ (SELECT id FROM blueprint WHERE id = ", - ) - .param() - .bind::(target_id) - .sql(") IS NULL, '") - .sql(NO_SUCH_BLUEPRINT_SENTINEL) - .sql( - "', \ + ) + .param() + .bind::(target_id) + .sql(") IS NULL, '") + .sql(NO_SUCH_BLUEPRINT_SENTINEL) + .sql( + "', \ IF( \ (SELECT parent_blueprint_id FROM blueprint, current_target \ WHERE id = ", - ) - .param() - .bind::(target_id) - .sql( - " \ + ) + .param() + .bind::(target_id) + .sql( + " \ AND current_target.blueprint_id = parent_blueprint_id \ ) IS NOT NULL \ OR \ (SELECT 1 FROM blueprint \ WHERE id = ", - ) - .param() - .bind::(target_id) - .sql( - " \ + ) + .param() + .bind::(target_id) + .sql( + " \ AND parent_blueprint_id IS NULL \ AND NOT EXISTS (SELECT version FROM current_target) \ - ) = 1, \ - CAST(", - ) - .param() - .bind::(target_id) - .sql(" AS text), '") - .sql(PARENT_NOT_TARGET_SENTINEL) - .sql( - "' \ + ) = 1, ", + ); + + // Workaround: sometime between CRDB v22.1.9 and v22.2.19, the type + // checker became too smart for the CAST(... AS UUID) error-checking + // gadget — it infers that the target_id must be a UUID and tries to + // parse the sentinel strings as UUIDs during type checking, causing + // the query to always fail. Casting the UUID to text here defeats + // this inference, allowing the sentinels to survive type checking and + // only be cast to UUIDs at runtime in the failure cases. + builder + .sql("CAST(") + .param() + .bind::(target_id) + .sql(" AS text), '") + .sql(PARENT_NOT_TARGET_SENTINEL) + .sql( + "' \ ) \ ) AS UUID \ ) \ - ), \ - new_target AS ( \ + ), ", + ); + + // `new_target`: determine the new version number to use — either 1 + // if this is the first blueprint being made the current target, or + // 1 higher than the previous target's version. The final clauses of + // each WHERE repeat the checks from `check_validity`, and will cause + // this subquery to return no rows if the new target should not be set. + builder.sql( + "new_target AS ( \ SELECT 1 AS new_version FROM blueprint \ WHERE id = ", ) diff --git a/nexus/db-queries/src/db/datastore/ip_pool.rs b/nexus/db-queries/src/db/datastore/ip_pool.rs index edd13cd9733..ebe436da1b8 100644 --- a/nexus/db-queries/src/db/datastore/ip_pool.rs +++ b/nexus/db-queries/src/db/datastore/ip_pool.rs @@ -2113,10 +2113,17 @@ fn extract_uuid_cast_sentinel(msg: &str) -> Option<&str> { // Pool and a Silo. It maintains the invariant that a pool can be reserved for // Oxide internal usage XOR linked to customer silos. It also checks that the // pool and silo still exist when the query is run. +// +// See `tests/output/ip_pool_external_silo_link.sql` for the full generated SQL. fn link_ip_pool_to_external_silo_query( ip_pool_resource: &IncompleteIpPoolResource, ) -> TypedSqlQuery> { let mut builder = QueryBuilder::new(); + // `ip_pool` CTE: select the pool by ID, ensuring it still exists. + // Fail with a 'bad-link-type' sentinel (via CAST-to-UUID trick) if + // the pool is reserved for Oxide rather than external silos. + // Also selects pool_type and ip_version for denormalization into + // ip_pool_resource (needed for a partial index constraint on defaults). builder .sql("WITH ip_pool AS (SELECT CAST(IF(reservation_type != ") .param() @@ -2131,16 +2138,22 @@ fn link_ip_pool_to_external_silo_query( .sql(") AS UUID) AS id, pool_type, ip_version FROM ip_pool WHERE id = ") .param() .bind::(ip_pool_resource.ip_pool_id) - .sql( - " \ - AND time_deleted IS NULL), \ - silo AS (SELECT id FROM silo WHERE id = ", - ) + .sql(" AND time_deleted IS NULL), "); + + // `silo` CTE: select the silo by ID, ensuring it still exists. + builder + .sql("silo AS (SELECT id FROM silo WHERE id = ") .param() .bind::(ip_pool_resource.resource_id) + .sql(" AND time_deleted IS NULL) "); + + // Use COALESCE(CAST(id AS STRING), '') to detect missing + // pool/silo: if the CTE returned no rows the LEFT JOIN produces NULL, + // COALESCE substitutes the sentinel, and CAST-to-UUID fails at runtime + // with a recognizable error message. + builder .sql( - " AND time_deleted IS NULL) \ - INSERT INTO ip_pool_resource (\ + "INSERT INTO ip_pool_resource (\ ip_pool_id, \ resource_type, \ resource_id, \ diff --git a/nexus/db-queries/src/db/queries/ip_pool.rs b/nexus/db-queries/src/db/queries/ip_pool.rs index 5bad7fcd206..5fbd2c3785e 100644 --- a/nexus/db-queries/src/db/queries/ip_pool.rs +++ b/nexus/db-queries/src/db/queries/ip_pool.rs @@ -31,6 +31,8 @@ use uuid::Uuid; /// to help with the scalability of the query. An `OR` means the database cannot use the indexes we've supplied on the /// `first_address` and `last_address` columns, and must resort to a full table /// scan. +/// +/// See `tests/output/filter_overlapping_ip_ranges.sql` for the full generated SQL. #[derive(Debug, Clone)] pub struct FilterOverlappingIpRanges { pub range: IpPoolRange, diff --git a/nexus/db-queries/src/db/queries/network_interface.rs b/nexus/db-queries/src/db/queries/network_interface.rs index a7002a5f8f6..a4360dff22f 100644 --- a/nexus/db-queries/src/db/queries/network_interface.rs +++ b/nexus/db-queries/src/db/queries/network_interface.rs @@ -1610,6 +1610,8 @@ fn push_instance_state_verification_subquery<'a>( /// `parent_id` as a string in this type. /// /// The `instance` CTE is only present if the interface is an instance-kind. +/// +/// See `tests/output/delete_vnic_*_query.sql` for the full generated SQL. #[derive(Debug, Clone)] pub struct DeleteQuery { interface_id: Uuid, @@ -1715,6 +1717,11 @@ impl QueryFragment for DeleteQuery { out.push_bind_param::( &DeleteError::HAS_SECONDARIES_SENTINEL, )?; + // `found_interface` selects the interface (even if deleted) and + // `updated` performs the actual soft-delete. The final SELECT + // LEFT JOINs them so the caller can distinguish three cases: + // (found + deleted) = success, (found + NULL) = existed but + // couldn't delete, (NULL + NULL) = not found. out.push_sql(") AS UUID)), found_interface AS (SELECT "); out.push_identifier(dsl::id::NAME)?; out.push_sql(" FROM "); diff --git a/nexus/db-queries/src/db/queries/vpc_subnet.rs b/nexus/db-queries/src/db/queries/vpc_subnet.rs index ab4b15171f5..b9013444d09 100644 --- a/nexus/db-queries/src/db/queries/vpc_subnet.rs +++ b/nexus/db-queries/src/db/queries/vpc_subnet.rs @@ -30,6 +30,8 @@ use uuid::Uuid; /// other modification or alteration. If callers care, they can inspect the /// record to make sure it's what they expected, though that's usually a fraught /// endeavor. +/// +/// See `tests/output/insert_vpc_subnet_query.sql` for the full generated SQL. #[derive(Clone, Debug)] pub struct InsertVpcSubnetQuery { /// The subnet to insert @@ -59,6 +61,12 @@ impl QueryFragment for InsertVpcSubnetQuery { &'a self, mut out: AstPass<'_, 'a, Pg>, ) -> diesel::QueryResult<()> { + // The `overlap` CTE selects any live records in the same VPC with + // overlapping IP blocks. It generates a casting error if any overlap + // is found: CAST(IF(, 'ipv4', 'ipv6') AS BOOL) always + // fails, but *how* it fails tells us which IP family overlapped. + // The `id != ` filter ignores the existing identical record, so + // that re-inserting the same row (idempotency) doesn't flag as overlap. out.push_sql("WITH overlap AS MATERIALIZED (SELECT CAST(IF(("); out.push_identifier(dsl::ipv4_block::NAME)?; out.push_sql(" && "); @@ -128,6 +136,9 @@ impl QueryFragment for InsertVpcSubnetQuery { out.push_bind_param::, _>( &self.subnet.custom_router_id, )?; + // ON CONFLICT: perform a no-op update (set id = id) so that + // RETURNING * gives back the actual DB row whether it was newly + // inserted or already existed. This is what makes the query idempotent. out.push_sql(") ON CONFLICT ("); out.push_identifier(dsl::id::NAME)?; out.push_sql(") DO UPDATE SET "); From 5390943e7e40d841fc09ccacf998e8a7f23917d0 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Fri, 13 Mar 2026 13:16:48 -0700 Subject: [PATCH 3/3] linebreak --- nexus/db-queries/src/db/queries/ip_pool.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/nexus/db-queries/src/db/queries/ip_pool.rs b/nexus/db-queries/src/db/queries/ip_pool.rs index 5fbd2c3785e..e20848453ad 100644 --- a/nexus/db-queries/src/db/queries/ip_pool.rs +++ b/nexus/db-queries/src/db/queries/ip_pool.rs @@ -28,9 +28,9 @@ use uuid::Uuid; /// /// The query uses multiple separate `NOT EXISTS` subqueries rather than a /// single subquery with `OR` clauses. That's a lot of duplication, but it's -/// to help with the scalability of the query. An `OR` means the database cannot use the indexes we've supplied on the -/// `first_address` and `last_address` columns, and must resort to a full table -/// scan. +/// to help with the scalability of the query. An `OR` means the database +/// cannot use the indexes we've supplied on the `first_address` and +/// `last_address` columns, and must resort to a full table scan. /// /// See `tests/output/filter_overlapping_ip_ranges.sql` for the full generated SQL. #[derive(Debug, Clone)]