Conversation
…2954) Captures the measured evidence behind #2961 for future schema-refactor designs: cache-size sweep (flat once the copy is chunked), production-scale redo timings, crash-resumption proof, the 24-minute boot-path VACUUM the reuse-only reclaim removes, synthetic-fixture bias, and benchmark methodology notes.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ffe509db02
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| On a production DB (29 GB file, 4.4M `sdk_messages` rows), migration 212 — a | ||
| `sdk_messages` PK rewrite (`id TEXT` → `seq INTEGER PRIMARY KEY`) — blocked | ||
| daemon startup for ~80 minutes inside one `BEGIN…COMMIT`, writing ~19.7 GB of | ||
| WAL at ~6 rows/sec. CPU sat at 20–40% of one core while the physical disk was |
There was a problem hiding this comment.
The stated figures are arithmetically incompatible: processing 4,420,697 rows in roughly 80 minutes averages about 921 rows/sec, whereas 6 rows/sec would take more than eight days. Because the report is intended to guide estimates for future migrations and repeats the 6 rows/sec figure later, correct either the duration or throughput so readers do not plan from contradictory production evidence.
Useful? React with 👍 / 👎.
| the atomic swap. Resumption verified the partial table's schema before | ||
| trusting its watermark and dropped an incompatible leftover instead of |
There was a problem hiding this comment.
When a partial sdk_messages_m212_new table has the expected ordered column names but different types, constraints, generated expressions, or foreign keys, runMigration212 accepts it because its compatibility check compares only the PRAGMA table_xinfo names and order, then trusts MAX(seq) and eventually swaps that table into place. Describing this as verifying the table's schema overstates the implemented correctness guarantee; either document it as a column-layout check or validate the complete staging schema.
Useful? React with 👍 / 👎.
| All numbers from copies of the real DB (readonly snapshot via `VACUUM INTO`), | ||
| 4,420,697 rows / 23.2 GiB live, Bun 1.3.x on Apple Silicon-class hardware. |
There was a problem hiding this comment.
The blanket statement that all measurements came from copies of the real database is contradicted immediately by the following section's 1M-row synthetic fixture. Readers cannot tell which conclusions reflect production-shaped data—especially since the report later warns that synthetic fixtures drastically flatter the legacy path—so scope this provenance statement to the production-scale measurements or explicitly identify both datasets.
Useful? React with 👍 / 👎.
|
|
||
| | cache_size | end-to-end | | ||
| | --- | --- | | ||
| | default (~8 MiB) | 114 s | |
There was a problem hiding this comment.
This row conflicts with the report's earlier statement that SQLite's default cache is about 2 MB: the usual PRAGMA cache_size = -2000 is interpreted as roughly 2,000 KiB, while approximately 8 MiB corresponds to a positive 2,000-page setting with 4 KiB pages. Unless this benchmark explicitly overrode the cache to 2000, calling the result the default misstates the tested baseline and makes the cache-size comparison difficult to reproduce.
Useful? React with 👍 / 👎.
| rows. Consolidating redundant indexes (three of ours led with `session_id`) | ||
| is worth more than any pragma — measure hot paths with `EXPLAIN QUERY PLAN` | ||
| before the migration ships, not after it stalls. |
There was a problem hiding this comment.
At migration 212, six of the ten rebuilt indexes—not three—lead with session_id: session_timestamp_id, parent_tool_use_id, renderable_terminal, session_subtype_parent, send_status_timestamp, and session_uuid. The repository's index sweep also classifies these as serving distinct required query shapes, so presenting the shared prefix as evidence of redundancy gives future migration authors a misleading consolidation target despite the subsequent instruction to measure plans.
Useful? React with 👍 / 👎.
| | --- | --- | --- | | ||
| | copy + swap | ~80 min, crash = restart at zero | ~43 min clean-path on a fresh compacted copy | | ||
| | WAL peak during copy | 19.7 GB | ~0.16–0.83 GB (autocheckpoint between chunks) | | ||
| | crash mid-migration | full restart | lost ≤1 chunk; resume skipped the finished copy instantly | |
There was a problem hiding this comment.
The claim that any mid-migration crash loses at most one chunk is only true while a copy chunk is running. The final drop/rename/index work is one transaction, and the report states that its DROP takes about 4.6 minutes followed by ten index rebuilds taking 3–13 minutes each; a crash during that phase rolls back and loses all completed swap/index work, which must be repeated on restart even though the copied rows survive. Qualify this row as a copy-phase guarantee so operators do not expect uniformly bounded recovery work.
Useful? React with 👍 / 👎.
|
Closing as stale — open since 2026-08-26 with no approval on any head and no activity in over three weeks. Not a judgement on the content. It is a single new file, Reopen if the postmortem should land; it should rebase cleanly given it adds one file and touches nothing existing. |
1 participant
Records the measured evidence behind #2961 as docs/reports/db-migration-stall-postmortem-2954.md so future schema refactors start from data: the cache-size sweep (flat once copying is chunked), production-scale redo timings and crash-resumption proof, the 24-minute boot-path VACUUM the reuse-only reclaim removes, synthetic-fixture bias, and benchmark methodology notes.