This is a related issue of #9925
TL;DR:
- There exist columns where Diesel has a representation in
schema.rs which does not match dbinit.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.sql dropped 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 an id-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 uses SqlU32 (non-Option).
- Migration history: Added in
8.0.0/up08.sql as ALTER 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 #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 sets mode: config.mode.into()
- Reads: All consumers (BfdManager, sync_switch_configuration) access
.mode directly 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.sql also always provides it via SELECT 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.sql provides via SELECT 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() takes message: String. Single insertion path.
- Reads:
e.message accessed 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 from NvmeFirmware struct
- 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.sql with DEFAULT 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() requires cpu_platform parameter. All 8+ call sites provide VmmCpuPlatform::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.sql with DEFAULT 'sled_default', then up05.sql dropped 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() requires port_name. Also set during populate_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: #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 |
This is a related issue of #9925
TL;DR:
schema.rswhich does not matchdbinit.sqlThere 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_idUuid(non-nullable) | CRDB:UUID(nullable)Uuid(non-Option)port_settings_idbgp-unnumbered-peers/up03.sqldropped NOT NULL because "constraints were implicitly added when columns were part 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 an
id-based PK. No code path was changed to allow NULLs.
switch_port_settings_bgp_peer_config.interface_nameText(non-nullable) | CRDB:TEXT(nullable)Name(non-Option)BgpPeer.interface_name: Name)bgp-unnumbered-peers/up03.sqlswitch_port_settings_bgp_peer_config.hold_timeInt8(non-nullable) | CRDB:INT8(nullable)SqlU32(non-Option)BgpPeer.hold_time: u32)bgp_peer_view. BgpPeerView struct usesSqlU32(non-Option).8.0.0/up08.sqlasALTER TABLE ADD COLUMN hold_time INT8(nullable, no DEFAULT). Rows that existed before 8.0.0 would have NULL.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_timehold_time8.0.0/up09.sql(nullable, no DEFAULT)switch_port_settings_bgp_peer_config.delay_openhold_time8.0.0/up10.sql(nullable, no DEFAULT)switch_port_settings_bgp_peer_config.connect_retryhold_time8.0.0/up11.sql(nullable, no DEFAULT)switch_port_settings_bgp_peer_config.keepalivehold_time8.0.0/up12.sql(nullable, no DEFAULT)switch_port_settings_interface_config.port_settings_idUuid(non-nullable) | CRDB:UUID(nullable)Uuid(non-Option)SwitchInterfaceConfig::new()requires it(port_settings_id, interface_name)switch_port_settings_interface_config.kindSwitchInterfaceKindEnum(non-nullable) | CRDB:switch_interface_kind(nullable)DbSwitchInterfaceKind(non-Option). Values: Primary, Vlan, Loopback.SwitchInterfaceKind)switch_port_settings_link_config.mtuInt4(non-nullable) | CRDB:INT4(nullable)SqlU16(non-Option)LinkConfigCreate.mtu: u16)switch_port_settings_link_config.speedSwitchLinkSpeedEnum(non-nullable) | CRDB:switch_link_speed(nullable)SwitchLinkSpeed(non-Option). Values: Speed0G through Speed400G.LinkConfigCreate.speed: LinkSpeed)switch_port_settings_port_config.geometrySwitchPortGeometryEnum(non-nullable) | CRDB:switch_port_geometry(nullable)SwitchPortGeometry(non-Option). Values: Qsfp28x1, Qsfp28x2, Sfp28x4.SwitchPortConfigCreate.geometry)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 #9976
bfd_session.modeBfdModeEnum(non-nullable) | CRDB:omicron.public.bfd_mode(nullable)BfdMode(non-Option)BfdSessionEnable.mode: BfdMode)bfd_session_create()always setsmode: config.mode.into().modedirectly with no NULL handlinginternet_gateway_ip_address.addressInet(non-nullable) | CRDB:INET(nullable)IpNetwork(non-Option)InternetGatewayIpAddressCreate.address: IpAddr)internet_gateway_ip_address.internet_gateway_idUuid(non-nullable) | CRDB:UUID(nullable)Uuid(non-Option)internet_gateway_ip_pool.internet_gateway_idUuid(non-nullable) | CRDB:UUID(nullable)Uuid(non-Option)internet-gateway/up13.sqlalso always provides it viaSELECT igw.id.internet_gateway_ip_pool.ip_pool_idUuid(non-nullable) | CRDB:UUID(nullable)Uuid(non-Option)up13.sqlprovides viaSELECT ipp.ip_pool_id.inv_collection_error.messageText(non-nullable) | CRDB:TEXT(nullable)String(non-Option)InvCollectionError::new()takesmessage: String. Single insertion path.e.messageaccessed directly, fed into error listsinv_nvme_disk_firmware.slot1_is_read_onlyBool(non-nullable) | CRDB:BOOLEAN(nullable)bool(non-Option)InvNvmeDiskFirmware::new()always sets fromNvmeFirmwarestructinv_nvme_disk_firmware.slot_firmware_versionsArray<Nullable<Text>>(non-nullable outer) | CRDB:STRING(8)[](nullable outer, with CHECK constraint)Vec<Option<String>>(non-Option outer Vec)NvmeFirmware.slot_firmware_versions. Constructor validates length.Nullable<Text>is correct (individual slots can be empty).multicast_group_member.source_ipsArray<Inet>(non-nullable) | CRDB:INET[] DEFAULT ARRAY[]::INET[](nullable)Vec<IpNetwork>(non-Option)source_ips.unwrap_or_default(). CTE insertion always binds via param. Column has DEFAULT.source_ips.is_empty()(ASM vs SSM), never checks for NULLmulticast-implicit-lifecycle/up06.sqlwithDEFAULT ARRAY[]::INET[]vmm.cpu_platformVmmCpuPlatformEnum(non-nullable) | CRDB:omicron.public.vmm_cpu_platform(nullable)VmmCpuPlatform(non-Option)Vmm::new()requirescpu_platformparameter. All 8+ call sites provideVmmCpuPlatform::SledDefault.vmm.cpu_platform.compatible_sled_cpu_families(), pattern matching with no wildcardadd-instance-cpu-platform/up04.sqlwithDEFAULT 'sled_default', thenup05.sqldropped the DEFAULT. All existing rows were backfilled by the DEFAULT before it was dropped.
switch_port.port_nameText(non-nullable) | CRDB:TEXT(nullable)Name(non-Option)SwitchPort::new()requiresport_name. Also set duringpopulate_switch_ports()in rack init.(rack_id, switch_location, port_name)switch_port.rack_idUuid(non-nullable) | CRDB:UUID(nullable)Uuid(non-Option)switch_port.switch_locationText(non-nullable) | CRDB:TEXT(nullable)String(non-Option)For the
user_provision_typevalues: #9985silo_user.user_provision_typeUserProvisionTypeEnumuser_provision_type(nullable)(NOT NULL AND time_deleted IS NULL) OR (time_deleted IS NOT NULL)silo_group.user_provision_typeUserProvisionTypeEnumuser_provision_type(nullable)