[fm] Introduce SitrepGuardedInsert.#10532
Conversation
Add the `SitrepGuardedInsert` Diesel combinator and the
`SitrepGuardedResource` trait: a generic primitive for FM rendezvous to
insert a resource row idempotently and guarded against stale-sitrep
execution.
The combinator wraps a caller-supplied resource INSERT in a single CTE
statement that:
- aborts (StaleSitrep) unless the executor's expected generation still
equals the latest sitrep's generation column;
- short-circuits (AlreadyExists) if a creation marker already exists for
the resource id;
- on a successful insert, atomically writes a creation marker, gated by
`WHERE EXISTS (SELECT 1 FROM new_resource)` so a marker is never
fabricated for a pre-existing row.
All spliced SQL identifiers come from the trait's `&'static str` consts, so
the query is injection-safe. The result is surfaced as a
`SitrepGuardedInsertOutcome` of Created / AlreadyExists / StaleSitrep.
6663e70 to
c6e5904
Compare
| Err(e) => { | ||
| if matches_sentinel(&e, &[STALE_GENERATION_SENTINEL]).is_some() | ||
| { | ||
| Ok(SitrepGuardedInsertOutcome::StaleSitrep) | ||
| } else if matches_sentinel(&e, &[ALREADY_EXISTS_SENTINEL]) | ||
| .is_some() | ||
| { | ||
| Ok(SitrepGuardedInsertOutcome::AlreadyExists) | ||
| } else { | ||
| Err(e) | ||
| } | ||
| } |
There was a problem hiding this comment.
style nit, feel free to ignore me: personally, I prefer to stuff the if conditions into the match arms so it feels less nested, like:
| Err(e) => { | |
| if matches_sentinel(&e, &[STALE_GENERATION_SENTINEL]).is_some() | |
| { | |
| Ok(SitrepGuardedInsertOutcome::StaleSitrep) | |
| } else if matches_sentinel(&e, &[ALREADY_EXISTS_SENTINEL]) | |
| .is_some() | |
| { | |
| Ok(SitrepGuardedInsertOutcome::AlreadyExists) | |
| } else { | |
| Err(e) | |
| } | |
| } | |
| Err(e) if matches_sentinel(&e, &[STALE_GENERATION_SENTINEL]).is_some() => { | |
| Ok(SitrepGuardedInsertOutcome::StaleSitrep) | |
| } | |
| Err(e) if matches_sentinel(&e, &[ALREADY_EXISTS_SENTINEL]).is_some() => { | |
| Ok(SitrepGuardedInsertOutcome::AlreadyExists) | |
| }, | |
| Err(e) => Err(e), |
| /// 5. `new_marker`: the marker INSERT, emitted inline by the | ||
| /// combinator using R's `&'static str` consts. This only runs when | ||
| /// `new_resource` actually produced a row. | ||
| fn walk_ast<'b>(&'b self, mut out: AstPass<'_, 'b, Pg>) -> QueryResult<()> { |
There was a problem hiding this comment.
It would be nice to have an expectorate test (and maybe also an EXPLAIN test) for the kind of queries that this generates.
| /// Column on `fm_sitrep` carrying this resource's generation counter. | ||
| const GENERATION_COLUMN: &'static str; | ||
| /// Creation table tracked alongside the resource. | ||
| const MARKER_TABLE: &'static str; | ||
| /// Resource-id column in the marker table. | ||
| const MARKER_ID_COLUMN: &'static str; |
There was a problem hiding this comment.
I don't love that these are all just &'static strs that have to be the name of the table/column, which you could easily imagine someone typoing and silently breaking something. I wonder if we can instead use the generated diesel schema types, so that you could instead write something like
impl SitrepGuardedResource for nexus_db_model::Alert {
type MarkerTable = schema::rendezvous_alert_created::table;
type MarkerIdColumn = schema::rendezvous_alert_created::dsl::alert_id;
type GenerationColumn = schema::fm_sitrep::dsl::alert_generation;
}This way, you get the actual table name from the schema and can't mistype it --- and we can probably leverage diesel to enforce that MarkerIdColumn comes from the same table as MarkerTable?
I notice that this is what the DatastoreCollectionConfig trait does:
omicron/nexus/db-model/src/collection.rs
Lines 11 to 77 in 8341849
| /// turn the "row already exists" outcome into a hard error. | ||
| pub fn new( | ||
| resource_id: Uuid, | ||
| expected_generation: i64, |
There was a problem hiding this comment.
i feel like this should probably be a Generation?
Add the
SitrepGuardedInsertDiesel combinator and theSitrepGuardedResourcetrait: a generic primitive for FM rendezvous to insert a resource row idempotently and guarded against stale-sitrep execution.The combinator wraps a caller-supplied resource INSERT in a single CTE statement that:
The result is surfaced as a
SitrepGuardedInsertOutcomeofCreated/AlreadyExists/StaleSitrep.Context: #10248. This is used in #10533 and #10535 which are split out in hopes of making the review somewhat less miserable.