Skip to content

fix: backfill missing request_stats columns on existing databases - #179

Open
marcuslannister wants to merge 1 commit into
yym68686:mainfrom
marcuslannister:fix/sqlite-stats-column-migration
Open

fix: backfill missing request_stats columns on existing databases#179
marcuslannister wants to merge 1 commit into
yym68686:mainfrom
marcuslannister:fix/sqlite-stats-column-migration

Conversation

@marcuslannister

Copy link
Copy Markdown

Summary

Databases created before the trace_id and timing_spans columns existed on request_stats never got them, because the schema init uses CREATE TABLE IF NOT EXISTS — a no-op when the table already exists:

  • On SQLite this crashed the service at startup: CREATE INDEX ix_request_stats_trace_id ON request_stats(trace_id) fails outright when the column doesn't exist.
  • On Postgres (and SQLite once past startup) it silently failed every insert into request_stats with timing_spans set (logged as rust_persistence_write_error, not fatal, but stats stopped recording).

Hit this in production on an upgrade from an older image against a pre-existing stats.db.

Changes

  • Split each backend's schema constant into *_SCHEMA_TABLES / *_SCHEMA_INDEXES, and run a column-backfill migration in between table creation and index creation.
  • REQUEST_STATS_COLUMNS is a single (name, sqlite_type, postgres_type) list shared by both backends — adding a future column only needs one entry here instead of a new one-off ALTER TABLE in two places.
  • Both migrate_sqlite_columns and migrate_postgres_columns check existing columns first (PRAGMA table_info / information_schema.columns) and only issue ALTER TABLE ADD COLUMN for what's actually missing. This matters for Postgres in particular: an unconditional ALTER TABLE ... ADD COLUMN IF NOT EXISTS still requires ALTER privilege even when the column already exists, which would break startup for a least-privilege application role on an already-migrated database.
  • Pure additive ALTER TABLE ADD COLUMN, no drops/rewrites — no data loss.

Test plan

  • cargo test --bin uni-api-front persistence — existing tests pass, plus a new test covering backfill from an old schema and idempotency on re-run.
  • cargo clippy --bin uni-api-front — clean on this file.
  • Verified against a real production SQLite database that predated both columns: migration backfills, service starts, and subsequent requests write stats without error.

CREATE TABLE IF NOT EXISTS is a no-op on an existing table, so trace_id
and timing_spans were never added to databases created before those
columns existed. On SQLite this crashed the service at startup
(CREATE INDEX on the missing column); on Postgres it would silently
fail per-request inserts.

Split each backend's schema into tables/indexes and run a column
migration in between, driven by one shared column list so future
columns only need one entry instead of a new backend-specific patch.
Both migrations check existing columns first, so an already-migrated
database never issues an ALTER (avoids requiring ALTER privilege on
every startup for least-privilege Postgres roles).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant