Skip to content

Diesel/DB skew: Nullability #9966

@smklein

Description

@smklein

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type
    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions