oxidecomputer / oxidecomputer/omicron
Diesel/DB skew: Nullability
Nobody has claimed this yet.
- Dominant language
- Rust
- Stars
- 572
- Forks
- 97
- Avg merge
- 2d 12h
- Merged PRs (30d)
- 96
Description
This is a related issue of https://github.com/oxidecomputer/omicron/issues/9925
TL;DR:
- There exist columns where Diesel has a representation in
schema.rswhich does not matchdbinit.sql - This issue focuses on cases where "Diesel says the type cannot be nullable, but Cockroachdb says the type could be nullable"
- This is problematic, because it can lead to serialization errors if we use Diesel to read these columns from the databse for null rows
There are a handful of different categories here, which I'll describe below:
Case 1: Needs investigation.
This type should maybe become non-nullable in CockroachDB?
switch_port_settings_bgp_peer_config.port_settings_id
- Diesel:
Uuid(non-nullable) | CRDB:UUID(nullable) - Rust model:
Uuid(non-Option) - API: Internal — derived from parent port settings
- Insertion: Always provided from parent
port_settings_id - Reads: Part of partial unique indexes for both numbered and unnumbered peers
- Migration history: Originally NOT NULL (part of composite PK). Migration
bgp-unnumbered-peers/up03.sqldropped NOT NULL because "constraints were implicitly added when columns were p
art of the PK, and they persist after dropping the PK." The NOT NULL was an artifact of the old PK, and was dropped as part of migrating to anid-based PK. No code path was changed to allo
w NULLs. - Can NULL exist? No — all existing rows had values (was PK), all new rows provide values
- Would NULL crash? Yes
- Proposal: Make NOT NULL. The NOT NULL drop was a migration artifact, not an intentional design choice. No backfill needed.
switch_port_settings_bgp_peer_config.interface_name
- Diesel:
Text(non-nullable) | CRDB:TEXT(nullable) - Rust model:
Name(non-Option) - API: Required field (
BgpPeer.interface_name: Name) - Insertion: Always provided from API
- Reads: Part of partial unique indexes
- Migration history: Same as port_settings_id — NOT NULL dropped as PK migration artifact in
bgp-unnumbered-peers/up03.sql - Can NULL exist? No
- Would NULL crash? Yes
- Proposal: Make NOT NULL. No backfill needed.
switch_port_settings_bgp_peer_config.hold_time
- Diesel:
Int8(non-nullable) | CRDB:INT8(nullable) - Rust model:
SqlU32(non-Option) - API: Required field (
BgpPeer.hold_time: u32) - Insertion: Always provided from API params
- Reads: Accessed directly, also read via
bgp_peer_view. BgpPeerView struct usesSqlU32(non-Option). - Migration history: Added in
8.0.0/up08.sqlasALTER TABLE ADD COLUMN hold_time INT8(nullable, no DEFAULT). Rows that existed before 8.0.0 would have NULL. - Can NULL exist? Possible — pre-8.0.0 rows would have NULL if they were never updated
- Would NULL crash? Yes — both the direct struct and BgpPeerView would fail deserialization
- Proposal: Make NOT NULL, but requires backfill migration (e.g.
UPDATE ... SET hold_time = 0 WHERE hold_time IS NULL). Need to confirm a sensible default with networking team.
switch_port_settings_bgp_peer_config.idle_hold_time
- Same pattern as
hold_time - Migration: Added in
8.0.0/up09.sql(nullable, no DEFAULT) - Can NULL exist? Possible — pre-8.0.0 rows
- Proposal: Make NOT NULL with backfill. Same concern as hold_time.
switch_port_settings_bgp_peer_config.delay_open
- Same pattern as
hold_time - Migration: Added in
8.0.0/up10.sql(nullable, no DEFAULT) - Can NULL exist? Possible — pre-8.0.0 rows
- Proposal: Make NOT NULL with backfill.
switch_port_settings_bgp_peer_config.connect_retry
- Same pattern as
hold_time - Migration: Added in
8.0.0/up11.sql(nullable, no DEFAULT) - Can NULL exist? Possible — pre-8.0.0 rows
- Proposal: Make NOT NULL with backfill.
switch_port_settings_bgp_peer_config.keepalive
- Same pattern as
hold_time - Migration: Added in
8.0.0/up12.sql(nullable, no DEFAULT) - Can NULL exist? Possible — pre-8.0.0 rows
- Proposal: Make NOT NULL with backfill.
switch_port_settings_interface_config.port_settings_id
- Diesel:
Uuid(non-nullable) | CRDB:UUID(nullable) - Rust model:
Uuid(non-Option) - API: Internal — derived from parent port settings
- Insertion:
SwitchInterfaceConfig::new()requires it - Reads: Part of composite unique index
(port_settings_id, interface_name) - Migration history: Nullable since initial CREATE TABLE (1.0.0)
- Can NULL exist? No
- Would NULL crash? Yes
- Proposal: Make NOT NULL. No backfill needed.
switch_port_settings_interface_config.kind
- Diesel:
SwitchInterfaceKindEnum(non-nullable) | CRDB:switch_interface_kind(nullable) - Rust model:
DbSwitchInterfaceKind(non-Option). Values: Primary, Vlan, Loopback. - API: Required enum field (
SwitchInterfaceKind) - Insertion: Always provided from API
- Reads: Used in switch configuration logic
- Migration history: Nullable since 1.0.0
- Can NULL exist? No
- Would NULL crash? Yes
- Proposal: Make NOT NULL. No backfill needed.
switch_port_settings_link_config.mtu
- Diesel:
Int4(non-nullable) | CRDB:INT4(nullable) - Rust model:
SqlU16(non-Option) - API: Required field (
LinkConfigCreate.mtu: u16) - Insertion: Always provided. Tests use explicit values (1500, 4700).
- Reads: Accessed directly, returned in API responses
- Migration history: Nullable since 1.0.0
- Can NULL exist? No
- Would NULL crash? Yes
- Proposal: Make NOT NULL. No backfill needed.
switch_port_settings_link_config.speed
- Diesel:
SwitchLinkSpeedEnum(non-nullable) | CRDB:switch_link_speed(nullable) - Rust model:
SwitchLinkSpeed(non-Option). Values: Speed0G through Speed400G. - API: Required enum field (
LinkConfigCreate.speed: LinkSpeed) - Insertion: Always provided
- Reads: Used in link negotiation, returned in API
- Migration history: Nullable since 1.0.0
- Can NULL exist? No
- Would NULL crash? Yes
- Proposal: Make NOT NULL. No backfill needed.
switch_port_settings_port_config.geometry
- Diesel:
SwitchPortGeometryEnum(non-nullable) | CRDB:switch_port_geometry(nullable) - Rust model:
SwitchPortGeometry(non-Option). Values: Qsfp28x1, Qsfp28x2, Sfp28x4. - API: Required enum field (
SwitchPortConfigCreate.geometry) - Insertion: Always provided. One row per port_settings (port_settings_id is PK).
- Reads: Used to validate link configs against physical port breakout
- Migration history: Nullable since 1.0.0
- Can NULL exist? No
- Would NULL crash? Yes
- Proposal: Make NOT NULL. No backfill needed.
Case 2: This type should not be "NULLABLE" in CRDB
So far: nothing
Case 3: This type should be "NULLABLE" in CRDB
The following cases are modified by https://github.com/oxidecomputer/omicron/pull/9976
bfd_session.mode
- Diesel:
BfdModeEnum(non-nullable) | CRDB:omicron.public.bfd_mode(nullable) - Rust model:
BfdMode(non-Option) - API: Required field (
BfdSessionEnable.mode: BfdMode) - Insertion: Single path —
bfd_session_create()always setsmode: config.mode.into() - Reads: All consumers (BfdManager, sync_switch_configuration) access
.modedirectly with no NULL handling - Migration history: Column has been nullable since initial CREATE TABLE in 31.0.0. No migrations ever touched it.
- Can NULL exist? No — no code path can produce one
- Would NULL crash? Yes — Diesel deserialization fails; pattern match in sync_switch_configuration has no wildcard
- Proposal: Make NOT NULL. No backfill needed.
internet_gateway_ip_address.address
- Diesel:
Inet(non-nullable) | CRDB:INET(nullable) - Rust model:
IpNetwork(non-Option) - API: Required field (
InternetGatewayIpAddressCreate.address: IpAddr) - Insertion: Constructor always sets from API params. No raw SQL or CTE insertions.
- Reads: Accessed directly, no NULL handling
- Migration history: Nullable since initial CREATE TABLE. No migrations touched it.
- Can NULL exist? No
- Would NULL crash? Yes — Diesel deserialization fails
- Proposal: Make NOT NULL. No backfill needed.
internet_gateway_ip_address.internet_gateway_id
- Diesel:
Uuid(non-nullable) | CRDB:UUID(nullable) - Rust model:
Uuid(non-Option) - API: Required (set from parent gateway during creation)
- Insertion: Constructor requires it. No raw SQL insertions.
- Reads: Accessed directly
- Migration history: Nullable since initial CREATE TABLE
- Can NULL exist? No
- Would NULL crash? Yes
- Proposal: Make NOT NULL. No backfill needed.
internet_gateway_ip_pool.internet_gateway_id
- Diesel:
Uuid(non-nullable) | CRDB:UUID(nullable) - Rust model:
Uuid(non-Option) - API: Required (set from parent gateway)
- Insertion: Constructor requires it. Migration
internet-gateway/up13.sqlalso always provides it viaSELECT igw.id. - Reads: Accessed directly
- Migration history: Nullable since initial CREATE TABLE
- Can NULL exist? No — both Diesel and migration INSERT paths always provide
- Would NULL crash? Yes
- Proposal: Make NOT NULL. No backfill needed.
internet_gateway_ip_pool.ip_pool_id
- Diesel:
Uuid(non-nullable) | CRDB:UUID(nullable) - Rust model:
Uuid(non-Option) - API: Required (set from IP pool association)
- Insertion: Constructor requires it. Migration
up13.sqlprovides viaSELECT ipp.ip_pool_id. - Reads: Accessed directly
- Migration history: Nullable since initial CREATE TABLE
- Can NULL exist? No
- Would NULL crash? Yes
- Proposal: Make NOT NULL. No backfill needed.
inv_collection_error.message
- Diesel:
Text(non-nullable) | CRDB:TEXT(nullable) - Rust model:
String(non-Option) - API: Internal only — always populated from collection error strings
- Insertion:
InvCollectionError::new()takesmessage: String. Single insertion path. - Reads:
e.messageaccessed directly, fed into error lists - Migration history: Nullable since initial CREATE TABLE
- Can NULL exist? No — error messages are always strings
- Would NULL crash? Yes — Diesel can't deserialize NULL into String
- Proposal: Make NOT NULL. No backfill needed.
inv_nvme_disk_firmware.slot1_is_read_only
- Diesel:
Bool(non-nullable) | CRDB:BOOLEAN(nullable) - Rust model:
bool(non-Option) - API: Internal — populated from sled agent firmware inventory
- Insertion:
InvNvmeDiskFirmware::new()always sets fromNvmeFirmwarestruct - Reads: Accessed as part of full struct deserialization
- Migration history: Nullable since initial CREATE TABLE
- Can NULL exist? No — firmware inventory always reports this field
- Would NULL crash? Yes — Diesel can't deserialize NULL into bool
- Proposal: Make NOT NULL. No backfill needed.
inv_nvme_disk_firmware.slot_firmware_versions
- Diesel:
Array<Nullable<Text>>(non-nullable outer) | CRDB:STRING(8)[](nullable outer, with CHECK constraint) - Rust model:
Vec<Option<String>>(non-Option outer Vec) - API: Internal — populated from firmware inventory
- Insertion: Always populated from
NvmeFirmware.slot_firmware_versions. Constructor validates length. - Reads: Accessed as Vec, individual slots can be None (empty firmware slot)
- Migration history: Nullable since initial CREATE TABLE. CHECK constraint ensures 1-7 elements when non-NULL.
- Can NULL exist? No — always populated
- Would NULL crash? Yes — Diesel expects a non-nullable array
- Proposal: Make NOT NULL. No backfill needed. The inner
Nullable<Text>is correct (individual slots can be empty).
multicast_group_member.source_ips
- Diesel:
Array<Inet>(non-nullable) | CRDB:INET[] DEFAULT ARRAY[]::INET[](nullable) - Rust model:
Vec<IpNetwork>(non-Option) - API: Optional in API — but defaults to empty array (ASM mode) when not provided
- Insertion: Constructor does
source_ips.unwrap_or_default(). CTE insertion always binds via param. Column has DEFAULT. - Reads: Code branches on
source_ips.is_empty()(ASM vs SSM), never checks for NULL - Migration history: Added via
multicast-implicit-lifecycle/up06.sqlwithDEFAULT ARRAY[]::INET[] - Can NULL exist? No — DEFAULT ensures non-NULL even if not explicitly provided
- Would NULL crash? Yes — Vec deserialization expects array, not NULL
- Proposal: Make NOT NULL. No backfill needed (DEFAULT already prevents NULLs).
vmm.cpu_platform
- Diesel:
VmmCpuPlatformEnum(non-nullable) | CRDB:omicron.public.vmm_cpu_platform(nullable) - Rust model:
VmmCpuPlatform(non-Option) - API: Internal — always set during VMM creation
- Insertion:
Vmm::new()requirescpu_platformparameter. All 8+ call sites provideVmmCpuPlatform::SledDefault. - Reads: Accessed directly —
vmm.cpu_platform.compatible_sled_cpu_families(), pattern matching with no wildcard - Migration history: Added via
add-instance-cpu-platform/up04.sqlwithDEFAULT 'sled_default', thenup05.sqldropped the DEFAULT. All existing rows were backfilled by the DEFAULT b
efore it was dropped. - Can NULL exist? No — migration backfilled existing rows, all new rows provide a value
- Would NULL crash? Yes — enum deserialization and direct pattern matching would fail
- Proposal: Make NOT NULL. No backfill needed (migration already handled it).
switch_port.port_name
- Diesel:
Text(non-nullable) | CRDB:TEXT(nullable) - Rust model:
Name(non-Option) - API: Required field
- Insertion:
SwitchPort::new()requiresport_name. Also set duringpopulate_switch_ports()in rack init. - Reads: Accessed directly, part of UNIQUE constraint
(rack_id, switch_location, port_name) - Migration history: Nullable since initial CREATE TABLE (1.0.0)
- Can NULL exist? No — always required by constructor
- Would NULL crash? Yes
- Proposal: Make NOT NULL. No backfill needed.
switch_port.rack_id
- Diesel:
Uuid(non-nullable) | CRDB:UUID(nullable) - Rust model:
Uuid(non-Option) - API: Required — validated against rack table before insertion
- Insertion: Always provided and validated
- Reads: Part of UNIQUE constraint, used in lookups
- Migration history: Nullable since 1.0.0
- Can NULL exist? No
- Would NULL crash? Yes
- Proposal: Make NOT NULL. No backfill needed.
switch_port.switch_location
- Diesel:
Text(non-nullable) | CRDB:TEXT(nullable) - Rust model:
String(non-Option) - API: Required
- Insertion: Always provided
- Reads: Part of UNIQUE constraint
- Migration history: Nullable since 1.0.0
- Can NULL exist? No
- Would NULL crash? Yes
- Proposal: Make NOT NULL. No backfill needed.
For the user_provision_type values: https://github.com/oxidecomputer/omicron/pull/9985
| Column | Diesel | CRDB | Can NULLs exist? | Notes |
|---|---|---|---|---|
silo_user.user_provision_type |
UserProvisionTypeEnum |
user_provision_type (nullable) |
Only for deleted rows | CHECK: (NOT NULL AND time_deleted IS NULL) OR (time_deleted IS NOT NULL) |
silo_group.user_provision_type |
UserProvisionTypeEnum |
user_provision_type (nullable) |
Only for deleted rows | Same CHECK constraint |
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Compare the Diesel definitions in schema.rs with dbinit.sql and the listed migrations, including bgp-unnumbered-peers/up03.sql and 8.0.0/up08.sql through up12.sql. Review the affected Rust models and BgpPeerView, then determine which nullable columns can contain existing NULLs and which require a confirmed backfill default. Done means the database constraints and Diesel representations agree without serialization failures.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- rust, sql
- Domain
- databases
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 35/100