Conversation
mergeconflict
force-pushed
the
mergeconflict/fm-comments
branch
from
d2bf8fc to
b6ee77e
Compare
| // (not allocating) but insert one at a time? Note that a batched insert | ||
| // would need to use `ON CONFLICT DO NOTHING` rather than checking for | ||
| // `Conflict` errors from individual inserts, since multiple Nexus | ||
| // instances may run this task concurrently. | ||
| // | ||
| // Currently, these `alert_create` calls have no guard against a stale | ||
| // Nexus inserting alerts from an outdated sitrep. This is fine for now | ||
| // because alert requests are carried forward into newer sitreps, so a | ||
| // stale insert is redundant rather than incorrect. However, if alerts | ||
| // are ever hard-deleted (e.g. when a case is closed), a lagging Nexus | ||
| // could re-create "zombie" alert records after deletion. At that point, | ||
| // the INSERT should be guarded by a CTE that checks the sitrep | ||
| // generation matches the current one. |
There was a problem hiding this comment.
Thanks for writing this down. I kinda wonder if we ought to write up an issue for some of this...
mergeconflict
force-pushed
the
mergeconflict/fm-comments
branch
from
cdb8cb5 to
2dbbfa3
Compare
mergeconflict
force-pushed
the
mergeconflict/fm-comments
branch
from
2dbbfa3 to
c463d46
Compare
Move DB model conventions (field ordering, Diesel derives) from nexus/db-model/src/fm.rs to the nexus-db-model crate-level docs in lib.rs, with a vmm example showing schema-to-model type mapping. Move _on_conn/_in_txn naming conventions from the FM datastore module to nexus/db-queries/src/db/datastore/mod.rs. Fix the framing: connection sharing is primarily about pool efficiency, not consistency. Keep the FM-specific correctness detail (concurrent delete detection, #9594) inline with a pointer to the general docs.
mergeconflict
force-pushed
the
mergeconflict/fm-comments
branch
from
c463d46 to
b65d715
Compare
| //! column list, so the result columns are always in the order `Queryable` | ||
| //! expects. **Always derive both.** | ||
| //! | ||
| //! # Field ordering convention |
There was a problem hiding this comment.
If we care: #9927 catches the ordering if it's incorrect.
| // sitrep_id (which is inserted first above). If we crash partway | ||
| // through, orphaned child records will be cleaned up when the orphaned | ||
| // sitrep is garbage collected. |
|
|
||
| //! Primary control plane interface for database read and write operations | ||
| //! | ||
| //! # Method naming conventions |
There was a problem hiding this comment.
nit: i believe the typical markdown convention is that a single # is the title of the entire markdown file/document/doc comment block, so I think this should be:
| //! # Method naming conventions | |
| //! ## Method naming conventions |
| //! Primary control plane interface for database read and write operations | ||
| //! | ||
| //! # Method naming conventions | ||
| //! |
There was a problem hiding this comment.
Another high-level note on naming conventions that I think might be worth writing down is that it's important to remember that almost all the database queries in this crate are defined as methods on the DataStore type, so everything is datastore.method() rather than namespaced in a module. Therefore, we tend to follow a naming convention with the name of the resource/object/type of data that the query operates on first, followed by the verb, even if this feels less like a grammatically correct English sentence than verb-first. For example, we tend to write things like ereports_insert or instance_delete, rather than insert_ereports or delete_instance. One reason for doing this is that it's a lot more friendly to search and IDE tab-completion; if you type datastore.ereport_, your editor shows you all the methods that operate on ereports, which sis typically more useful than datastore.insert_ suggesting every single insert operation for stuff that you don't care about at the moment.
| //! Rust types. `nexus_db_queries` then uses both to build and execute | ||
| //! queries in its `DataStore` methods. | ||
| //! | ||
| //! # How the mapping works |
There was a problem hiding this comment.
This should be a level 2 heading since it's not the title of the entire doc comment.
| //! # How the mapping works | |
| //! ## How the mapping works |
| //! ```text | ||
| //! // in nexus-db-schema |
There was a problem hiding this comment.
i think it wouldn't be too hard to turn this into rust, we'd just need to import a few types first. that way, the compiler could check it...
| //! queries in its `DataStore` methods. | ||
| //! | ||
| //! # How the mapping works | ||
| //! |
There was a problem hiding this comment.
I think there's probably some Diesel documentation we could also link to here?
| //! | ||
| //! The model struct provides the Rust representation of a row: | ||
| //! | ||
| //! ```text |
There was a problem hiding this comment.
similarly, if we added some imports, we could probably turn this into rust and get the compiler to check it?
| //! # Field ordering convention | ||
| //! | ||
| //! Fields in `Queryable`/`Insertable` structs should be ordered to match the | ||
| //! column order in `dbinit.sql`. This keeps the Rust types easy to | ||
| //! cross-reference with the schema definition and prevents subtle bugs if a | ||
| //! query ever omits `Selectable`. | ||
| //! | ||
| //! # Representing enums |
There was a problem hiding this comment.
these should probably be level 2 or level 3 headings
| //! # Field ordering convention | |
| //! | |
| //! Fields in `Queryable`/`Insertable` structs should be ordered to match the | |
| //! column order in `dbinit.sql`. This keeps the Rust types easy to | |
| //! cross-reference with the schema definition and prevents subtle bugs if a | |
| //! query ever omits `Selectable`. | |
| //! | |
| //! # Representing enums | |
| //! ## Field ordering convention | |
| //! | |
| //! Fields in `Queryable`/`Insertable` structs should be ordered to match the | |
| //! column order in `dbinit.sql`. This keeps the Rust types easy to | |
| //! cross-reference with the schema definition and prevents subtle bugs if a | |
| //! query ever omits `Selectable`. | |
| //! | |
| //! ## Representing enums |
| //! - **Type translation.** Each field's Rust type must implement Diesel's | ||
| //! [`FromSql`](diesel::deserialize::FromSql) and | ||
| //! [`ToSql`](diesel::serialize::ToSql) traits for the corresponding column's | ||
| //! SQL type. Diesel provides these impls for common types (`Uuid`, | ||
| //! `DateTime<Utc>`, `String`, etc.); this crate defines wrapper types like | ||
| //! [`DbTypedUuid`] for domain-specific conversions | ||
| //! (e.g. distinguishing a sled UUID from any other UUID). | ||
| //! - **Column renaming.** Field names must match column names by default, but | ||
| //! `#[diesel(column_name = ...)]` allows the Rust name to differ (e.g. | ||
| //! `generation` for the `state_generation` column). | ||
| //! - **Positional mapping.** [`diesel::Queryable`] maps SQL result columns to | ||
| //! struct fields by position, not by name. If the field order doesn't match | ||
| //! the column order in the query, fields will silently receive wrong values. | ||
| //! Deriving [`diesel::Selectable`] mitigates this by generating an explicit | ||
| //! column list, so the result columns are always in the order `Queryable` | ||
| //! expects. **Always derive both.** |
There was a problem hiding this comment.
Personally, I feel like these might be nicer as level 3 subheadings rather than bullet points, at least because then we can link to them from elsewhere?
3 participants
Capture a few comments from the conversation between @hawkw and @smklein in #9552 and previous PRs so I don't mess things up later.