[master][DB-2195] Reduce temp space needed for secondary index migration (#5692) - #5732
timothycoleman wants to merge 3 commits into
Conversation
Cherry picked from eb42bd7 * Reduce temp space needed for secondary index migration The V1 migration added idx_all.record_id with DEFAULT ''::BLOB. The cast makes the default a non-constant expression, which sends DuckDB down its workaround path and rewrites the statement into ADD COLUMN plus an UPDATE of every row (see transform_alter_table.cpp). That UPDATE needs ~20 bytes of transaction local memory per row rather than ~2, so on a large index it can exhaust max_temp_directory_size. Dropping the cast keeps the same value and the same backfill while taking the cheap metadata path. - V1 is frozen as V1A: internal, unregistered, and kept only so tests can reproduce the database state it left behind. It must not be edited. - V1B replaces it at version 1, identical except for the bare DEFAULT ''. Also identified other small descrepancies: - V2 gives record_id the shape a fresh install creates, not null with no default, on idx_all and on every user index table. DuckDB cannot add a column and a constraint in one statement, so a migrated column is necessarily nullable with a default until this runs. Both statements are catalog only and leave the database file unchanged. MigrationTests.SchemaParity asserts that a migrated schema matches what 1_Schema.sql and CreateUserIndex produce, via V1A and V1B paths, comparing every table including user index tables. Another test also checks that the migrations don't do something surprisingly expensive Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
PR Summary by QodoReduce DuckDB migration temp usage and align index schemas
AI Description
Diagram
High-Level Assessment
Files changed (10)
|
Code Review by Qodo🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)
Great, no issues found!Qodo reviewed your code and found no material issues that require reviewTip of the day💡 Did you know, you can enable the Remediation agent and Qodo fixes findings in a dedicated fix PR |
There was a problem hiding this comment.
🟡 Changes recommended
The tightened table-name predicate is shared by the supposedly frozen V1A migration, so legacy prefixed tables outside the current naming regex can be left unmigrated.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Reduces temporary space used by secondary-index migrations and aligns migrated schemas with fresh installs.
Changes:
- Replaces V1 with cheaper V1B and adds V2 schema reconciliation.
- Adds schema-parity, backfill, and temp-budget migration tests.
- Validates user-index table names before interpolating them into DDL.
File summaries
| File | Description |
|---|---|
IndexingDbSchema.Versioning.cs |
Registers V1B and V2. |
IndexingDbSchema.V1.A.cs |
Preserves the original V1 migration for tests. |
IndexingDbSchema.V1.B.cs |
Uses the cheaper default expression. |
IndexingDbSchema.V2.cs |
Aligns migrated record_id columns. |
IndexingDbSchema.Migration.cs |
Exposes migration helpers to tests. |
UserIndexSql.cs |
Validates migration table identifiers. |
| Migration test files | Add schema, backfill, and temp-budget coverage. |
Review details
- Files reviewed: 10/10 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Cherry picked from eb42bd7
The V1 migration added idx_all.record_id with DEFAULT ''::BLOB. The cast makes
the default a non-constant expression, which sends DuckDB down its workaround
path and rewrites the statement into ADD COLUMN plus an UPDATE of every row
(see transform_alter_table.cpp). That UPDATE needs ~20 bytes of transaction
local memory per row rather than ~2, so on a large index it can exhaust
max_temp_directory_size.
Dropping the cast keeps the same value and the same backfill while taking the
cheap metadata path.
reproduce the database state it left behind. It must not be edited.
Also identified other small descrepancies:
default, on idx_all and on every user index table. DuckDB cannot add a
column and a constraint in one statement, so a migrated column is
necessarily nullable with a default until this runs. Both statements are
catalog only and leave the database file unchanged.
MigrationTests.SchemaParity asserts that a migrated schema matches what
1_Schema.sql and CreateUserIndex produce, via V1A and V1B paths,
comparing every table including user index tables.
Another test also checks that the migrations don't do something surprisingly expensive
Co-Authored-By: Claude Opus 5 (1M context) noreply@anthropic.com