feat(migrations): atomic core + extension migrations#1768
Merged
c-thiel merged 10 commits intoMay 23, 2026
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
WalkthroughUpgrade sqlx to 0.9.0 and bump rust-version to 1.94; refactor Postgres migrations to support per-extension tracker tables with migrate_core_only(pool) and an extensions-aware migrate(pool, extensions); update callers, tests, fixtures, patch helper, and developer docs. ChangesExtension-scoped migration tracking system
Sequence DiagramsequenceDiagram
participant Caller
participant Migrate
participant Database
Caller->>Migrate: migrate(pool, extensions)
Migrate->>Database: BEGIN + advisory lock
Migrate->>Database: apply core using CORE_MIGRATIONS_TABLE
loop per extension
Migrate->>Database: apply extension using ext_<name>_sqlx_migrations
Migrate->>Database: run data_hooks for applied versions
end
Migrate->>Database: COMMIT
Caller->>Migrate: migrate_core_only(pool)
Migrate->>Database: apply core only
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
docs/docs/developer-guide.md (1)
185-239: ⚡ Quick winConsider adding view cleanup to the recovery script.
Line 161 states that "views attached to upstream-owned objects are not permitted under the prefix," but the recovery SQL script (lines 191-236) does not explicitly handle views with the
ext_*prefix. Although the convention forbids such views, a defensive sweep for them would align with the pattern established for triggers, types, and functions.🔍 Proposed addition to handle views
Add the following block after the function sweep (after line 234):
-- Views (defensive sweep) FOR r IN SELECT c.relname FROM pg_class c JOIN pg_namespace n ON n.oid = c.relnamespace WHERE n.nspname = current_schema() AND c.relkind = 'v' AND c.relname LIKE 'ext\_%' ESCAPE '\' LOOP EXECUTE format('DROP VIEW %I CASCADE', r.relname); END LOOP;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/docs/developer-guide.md` around lines 185 - 239, The recovery DO $$ block doesn't remove views with the ext_* prefix; add a defensive sweep after the function sweep in the same DO block that queries pg_class for c.relkind = 'v' in the current_schema() with c.relname LIKE 'ext\_%' ESCAPE '\' and then EXECUTE a DROP VIEW %I CASCADE for each matching r.relname so views named with the ext_ prefix are also removed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/lakekeeper/src/implementations/postgres/migrations/mod.rs`:
- Around line 54-56: The tracker_table builder uses ExtensionMigrations::name
verbatim which can produce invalid SQL identifiers or collisions; update the
code to first validate the extension name (e.g., require it match an identifier
regex like starting with letter/underscore and containing only
letters/digits/underscores) and surface a clear error when the name is invalid,
and for nonconforming names produce a deterministic safe identifier (for example
normalize/replace invalid chars or fallback to a hashed suffix derived from the
original name) so tracker_table() returns a SQL-safe, collision-resistant
string; apply the same validation/normalization wherever
ExtensionMigrations::name is used (including the other location referenced) so
all tracker-table names are validated or deterministically sanitized.
In `@docs/docs/developer-guide.md`:
- Around line 171-180: The docs snippet doesn't explain the data_hooks field on
ExtensionMigrations; update the guidance near the shown snippet to say that
data_hooks is a HashMap<i64, Box<dyn MigrationHook>> on ExtensionMigrations used
to run a MigrationHook immediately after a specific extension migration version
is applied (inside the same transaction), and that using HashMap::new() is
appropriate when no hooks are needed; reference ExtensionMigrations, data_hooks,
MigrationHook and the migrate(&pool, extensions) call so readers can locate the
example.
---
Nitpick comments:
In `@docs/docs/developer-guide.md`:
- Around line 185-239: The recovery DO $$ block doesn't remove views with the
ext_* prefix; add a defensive sweep after the function sweep in the same DO
block that queries pg_class for c.relkind = 'v' in the current_schema() with
c.relname LIKE 'ext\_%' ESCAPE '\' and then EXECUTE a DROP VIEW %I CASCADE for
each matching r.relname so views named with the ext_ prefix are also removed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 32f50a72-4045-43d1-b3ea-9b7a88d106d1
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
Cargo.tomlcrates/lakekeeper-bin/src/main.rscrates/lakekeeper/src/implementations/postgres/bootstrap.rscrates/lakekeeper/src/implementations/postgres/migrations/mod.rscrates/lakekeeper/src/implementations/postgres/namespace.rscrates/lakekeeper/src/implementations/postgres/tabular/table/commit.rscrates/lakekeeper/src/tests/mod.rscrates/lakekeeper/tests/extension_migrations_fixture/20260101000000_create_ext_demo.sqlcrates/lakekeeper/tests/extension_migrations_fixture_invalid/20260101000000_create_ok.sqlcrates/lakekeeper/tests/extension_migrations_fixture_invalid/20260101000001_fail.sqldocs/docs/developer-guide.md
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Cargo.toml`:
- Line 16: The rust-version field was bumped to "1.94" without justification;
inspect the change of rust-version and either revert it to the true minimum
(e.g., "1.86" to match sqlx v0.9.0 or the previous "1.92") or add a short
justification in the PR describing which dependency or language feature requires
1.94; update the rust-version line (rust-version = "1.94") accordingly and, if
keeping 1.94, list the exact symbol(s) or crate(s) (e.g., any upgraded crate or
use of new std/library features) that mandate the bump so reviewers can verify
the need.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Member
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/lakekeeper/src/implementations/postgres/migrations/mod.rs (1)
121-128:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate the rustdoc to match the actual ordering contract.
The implementation does not run extensions “after all core migrations”; it merges all sources by version and only uses source index as a tie-breaker. Leaving the current docs in place gives extension authors the wrong sequencing guarantee.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/lakekeeper/src/implementations/postgres/migrations/mod.rs` around lines 121 - 128, The rustdoc block in migrations/mod.rs incorrectly states "Extensions are applied in registration order, after all core migrations"; update the doc to reflect the actual ordering: explain that core and extension migrations are merged by migration version and that the implementation interleaves sources by version, using source index/registration order only as a tie-breaker for equal versions (so extensions are not guaranteed to run strictly after all core migrations). Mention the per-extension tracking table name pattern (`ext_<name>_sqlx_migrations`) and keep the transaction guarantee text intact.
♻️ Duplicate comments (1)
crates/lakekeeper/src/implementations/postgres/migrations/mod.rs (1)
136-140:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject duplicate extension names as well.
This only validates the identifier shape. Two registered extensions can still reuse the same
name, which makes them share oneext_<name>_sqlx_migrationstable and mixes pre-flight/apply state across distinct sources.Suggested fix
pub async fn migrate( pool: &sqlx::PgPool, mut extensions: Vec<ExtensionMigrations>, ) -> anyhow::Result<ServerId> { + let mut seen = HashSet::with_capacity(extensions.len()); // Fail fast on misconfigured extension names — before any DB work, before // the advisory lock is acquired. Catches typos in caller crate source. for ext in &extensions { ext.validate_name()?; + if !seen.insert(ext.name) { + return Err(anyhow!("duplicate extension migration name {:?}", ext.name)); + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/lakekeeper/src/implementations/postgres/migrations/mod.rs` around lines 136 - 140, The loop currently calls ext.validate_name() for each item but doesn’t detect duplicate extension names, which can cause different extensions to share the same ext_<name>_sqlx_migrations table; after validating shapes, iterate the same extensions collection (variable extensions) to collect each ext.name (or ext.get_name()) into a set/map and fail fast with an error if any name is seen twice (return a clear Err mentioning the duplicate name), so ensure you run this duplicate-name check immediately after validate_name() and before acquiring the advisory lock or proceeding with migrations.
🧹 Nitpick comments (2)
docs/docs/developer-guide.md (1)
160-160: ⚡ Quick winUse normative wording for the FK rule (
must, notshould).This line documents a hard contract, so
should beis too soft and can be interpreted as optional. Please change it tomust be ON DELETE CASCADE or ON DELETE SET NULL.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/docs/developer-guide.md` at line 160, Update the normative FK rule text under the "CASCADE required" entry so it uses mandatory language: replace "Every FK from an `ext_*` table to an upstream table should be `ON DELETE CASCADE` or `ON DELETE SET NULL`." with wording that says it must be `ON DELETE CASCADE` or `ON DELETE SET NULL` (e.g., "Every FK from an `ext_*` table to an upstream table must be ON DELETE CASCADE or ON DELETE SET NULL."), keeping the note about enforcing this in the extension crate's CI intact; locate the line labeled "CASCADE required" in the developer-guide.md to make the change.crates/lakekeeper/src/implementations/postgres/migrations/mod.rs (1)
628-637: ⚡ Quick winStrengthen the “core tracker untouched” assertion.
Counting
NULLcore checksums will miss accidental rewrites to a different non-NULLchecksum. Snapshot the core tracker’s(version, checksum)rows before the patched rerun and assert they are byte-for-byte identical afterward.Suggested test shape
let core_before: Vec<(i64, Vec<u8>)> = sqlx::query_as( "SELECT version, checksum FROM _sqlx_migrations ORDER BY version", ) .fetch_all(&pool) .await .unwrap(); // ... run Phase 3 ... let core_after: Vec<(i64, Vec<u8>)> = sqlx::query_as( "SELECT version, checksum FROM _sqlx_migrations ORDER BY version", ) .fetch_all(&pool) .await .unwrap(); assert_eq!(core_after, core_before, "extension sha patch must not touch core tracker");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/lakekeeper/src/implementations/postgres/migrations/mod.rs` around lines 628 - 637, Replace the weak NULL-count check of the core tracker with a byte-for-byte snapshot comparison: before running the Phase 3/patch logic, read and store the ordered rows (version, checksum) from the table `_sqlx_migrations` (use sqlx::query_as to fetch Vec<(i64, Vec<u8>)> into a variable like core_before), then after the patch re-read the same ordered rows into core_after and assert_eq!(core_after, core_before, "extension sha patch must not touch core tracker"); this ensures any non-NULL checksum changes are detected.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@crates/lakekeeper/src/implementations/postgres/migrations/patch_migration_hash.rs`:
- Around line 39-50: The current logic in patch_migration_hash.rs treats an
UPDATE that affects 0 rows as a success, but since the migration row is known to
exist this indicates the checksum was not patched; change the branch handling
q.rows_affected() so that: if q.rows_affected() > 1 keep the existing error, if
q.rows_affected() == 1 keep the info log for success, and if q.rows_affected()
== 0 return an Err (with a clear message including table_name and version)
instead of logging "No rows were updated", ensuring the caller does not consume
the SHA patch as successful; update the tracing::info/tracing::error and the
returned anyhow::anyhow accordingly around the q.rows_affected() checks.
In `@docs/docs/developer-guide.md`:
- Line 175: Update the example extension name to match runtime validation rules:
inspect the ExtensionMigrations struct and its name validation logic (search for
ExtensionMigrations, validate, or ext_<name>_sqlx_migrations in the migrations
code) and choose an example that passes those rules (e.g., snake_case with
allowed charset); then update the docs entry ("my-lakekeeper-extension") to the
validated form and, if helpful, add a short note indicating the charset/format
required by the ExtensionMigrations validation logic.
---
Outside diff comments:
In `@crates/lakekeeper/src/implementations/postgres/migrations/mod.rs`:
- Around line 121-128: The rustdoc block in migrations/mod.rs incorrectly states
"Extensions are applied in registration order, after all core migrations";
update the doc to reflect the actual ordering: explain that core and extension
migrations are merged by migration version and that the implementation
interleaves sources by version, using source index/registration order only as a
tie-breaker for equal versions (so extensions are not guaranteed to run strictly
after all core migrations). Mention the per-extension tracking table name
pattern (`ext_<name>_sqlx_migrations`) and keep the transaction guarantee text
intact.
---
Duplicate comments:
In `@crates/lakekeeper/src/implementations/postgres/migrations/mod.rs`:
- Around line 136-140: The loop currently calls ext.validate_name() for each
item but doesn’t detect duplicate extension names, which can cause different
extensions to share the same ext_<name>_sqlx_migrations table; after validating
shapes, iterate the same extensions collection (variable extensions) to collect
each ext.name (or ext.get_name()) into a set/map and fail fast with an error if
any name is seen twice (return a clear Err mentioning the duplicate name), so
ensure you run this duplicate-name check immediately after validate_name() and
before acquiring the advisory lock or proceeding with migrations.
---
Nitpick comments:
In `@crates/lakekeeper/src/implementations/postgres/migrations/mod.rs`:
- Around line 628-637: Replace the weak NULL-count check of the core tracker
with a byte-for-byte snapshot comparison: before running the Phase 3/patch
logic, read and store the ordered rows (version, checksum) from the table
`_sqlx_migrations` (use sqlx::query_as to fetch Vec<(i64, Vec<u8>)> into a
variable like core_before), then after the patch re-read the same ordered rows
into core_after and assert_eq!(core_after, core_before, "extension sha patch
must not touch core tracker"); this ensures any non-NULL checksum changes are
detected.
In `@docs/docs/developer-guide.md`:
- Line 160: Update the normative FK rule text under the "CASCADE required" entry
so it uses mandatory language: replace "Every FK from an `ext_*` table to an
upstream table should be `ON DELETE CASCADE` or `ON DELETE SET NULL`." with
wording that says it must be `ON DELETE CASCADE` or `ON DELETE SET NULL` (e.g.,
"Every FK from an `ext_*` table to an upstream table must be ON DELETE CASCADE
or ON DELETE SET NULL."), keeping the note about enforcing this in the extension
crate's CI intact; locate the line labeled "CASCADE required" in the
developer-guide.md to make the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a3f85d05-78ac-418c-b7ef-f00da43f8cd7
📒 Files selected for processing (4)
crates/lakekeeper/src/implementations/postgres/migrations/mod.rscrates/lakekeeper/src/implementations/postgres/migrations/patch_migration_hash.rscrates/lakekeeper/tests/extension_migrations_fixture_patched/20260101000000_create_ext_demo.sqldocs/docs/developer-guide.md
✅ Files skipped from review due to trivial changes (1)
- crates/lakekeeper/tests/extension_migrations_fixture_patched/20260101000000_create_ext_demo.sql
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
crates/lakekeeper/src/implementations/postgres/migrations/mod.rs (1)
136-140:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject duplicate extension names before pre-flight.
Two entries with the same
ExtensionMigrations::nametarget the same tracker table and can collide during merged application. Fail fast on duplicates before any DB work.Suggested fix
pub async fn migrate( pool: &sqlx::PgPool, mut extensions: Vec<ExtensionMigrations>, ) -> anyhow::Result<ServerId> { @@ - for ext in &extensions { - ext.validate_name()?; - } + let mut seen = HashSet::with_capacity(extensions.len()); + for ext in &extensions { + ext.validate_name()?; + anyhow::ensure!( + seen.insert(ext.name), + "duplicate extension migration name {:?}", + ext.name + ); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/lakekeeper/src/implementations/postgres/migrations/mod.rs` around lines 136 - 140, The code currently validates each extension name via ext.validate_name() but does not reject duplicate ExtensionMigrations::name entries; add a pre-flight duplicate check on the extensions collection (before calling validate_name() and before any DB work/advisory lock) that collects ExtensionMigrations::name values and returns an error if any name appears more than once so duplicate tracker-table targets are rejected early; locate the loop over extensions and the ExtensionMigrations type/name accessor to implement a fast set/count check and produce a clear error identifying the duplicated extension name(s).crates/lakekeeper/src/implementations/postgres/migrations/patch_migration_hash.rs (1)
47-50:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTreat zero-row checksum patches as failure.
If no row is updated, the checksum correction did not happen and migration flow should fail instead of logging success-like progress.
Suggested fix
- if q.rows_affected() == 1 { - tracing::info!("Patched checksum in {table_name} for version {version}"); - } else { - tracing::info!("No rows were updated in {table_name} for version {version}"); - } + match q.rows_affected() { + 1 => tracing::info!("Patched checksum in {table_name} for version {version}"), + 0 => { + let msg = + format!("Checksum patch updated no rows in {table_name} for version {version}"); + tracing::error!("{msg}"); + return Err(anyhow::anyhow!(msg)); + } + _ => unreachable!("handled by the >1 guard above"), + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/lakekeeper/src/implementations/postgres/migrations/patch_migration_hash.rs` around lines 47 - 50, The current logic treats zero-row updates as informational success; change it so a zero rows_affected() is treated as a failure: in the else branch (the check using q.rows_affected()) replace the tracing::info(...) with returning an Err that describes the failed checksum patch and includes table_name and version (e.g., construct an anyhow/error with context and return it from the function patch_migration_hash or the containing function), leaving the success branch to continue returning Ok; ensure the error propagates up instead of silently logging.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/docs/developer-guide.md`:
- Line 175: The example extension name "my-lakekeeper-extension" is invalid
because ExtensionMigrations::name must match the regex [a-z_][a-z0-9_]{0,40};
update the example in docs/docs/developer-guide.md to a compliant identifier
such as "my_lakekeeper_extension" (replace hyphens with underscores) and
optionally add a short note mentioning the allowed character set and that
migrate() uses validate_name to enforce it.
---
Duplicate comments:
In `@crates/lakekeeper/src/implementations/postgres/migrations/mod.rs`:
- Around line 136-140: The code currently validates each extension name via
ext.validate_name() but does not reject duplicate ExtensionMigrations::name
entries; add a pre-flight duplicate check on the extensions collection (before
calling validate_name() and before any DB work/advisory lock) that collects
ExtensionMigrations::name values and returns an error if any name appears more
than once so duplicate tracker-table targets are rejected early; locate the loop
over extensions and the ExtensionMigrations type/name accessor to implement a
fast set/count check and produce a clear error identifying the duplicated
extension name(s).
In
`@crates/lakekeeper/src/implementations/postgres/migrations/patch_migration_hash.rs`:
- Around line 47-50: The current logic treats zero-row updates as informational
success; change it so a zero rows_affected() is treated as a failure: in the
else branch (the check using q.rows_affected()) replace the tracing::info(...)
with returning an Err that describes the failed checksum patch and includes
table_name and version (e.g., construct an anyhow/error with context and return
it from the function patch_migration_hash or the containing function), leaving
the success branch to continue returning Ok; ensure the error propagates up
instead of silently logging.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bf34acc4-5de4-482a-9cb1-17eee06ac192
⛔ Files ignored due to path filters (88)
.sqlx/query-0086856941a82aa66a3006d5686fe6c34bed4571ffed5303998e655eae253581.jsonis excluded by!.sqlx/**.sqlx/query-04c6fd4a25005469a8342fe751db0302f8ff81b7046efaa73c0d2433e1867780.jsonis excluded by!.sqlx/**.sqlx/query-06344d50decb31166e6649525cce0cc8c263c7ad164d86148bf0b99c12b08007.jsonis excluded by!.sqlx/**.sqlx/query-08a9a63f9d7887c221ae406839a1cd9e76bae529948e76288748e4232f4b9ef5.jsonis excluded by!.sqlx/**.sqlx/query-0902b3c098641f60bcaf012a39082c879fd0b4ad7b2928c7c1ea33798216c053.jsonis excluded by!.sqlx/**.sqlx/query-097caa255e91c68d4590df9cd4370e4dcd902ad7d6f87a6de20cab523455bc19.jsonis excluded by!.sqlx/**.sqlx/query-11ef42e7d1210142e5a832c6217755e6f772d84b54bfa08b77b6c2f94ce5fd0e.jsonis excluded by!.sqlx/**.sqlx/query-12dba7d7e089c0c495fcd9e8d623fde4f3d3b5a2061bd40aea94c78df537276c.jsonis excluded by!.sqlx/**.sqlx/query-17e54b156bb6d9eee483e8ad4a2150526ee047a05358fa0adddb42c2dad0a746.jsonis excluded by!.sqlx/**.sqlx/query-18fb2ceff5fe5062c535dc1f918441580ea4a2aaf1fea9859d7cf4a5c1d079e1.jsonis excluded by!.sqlx/**.sqlx/query-1c1a32de33e3f54464a1107f7d4d4b8cbd99dd69aae1c740561c3cbbea2eaa19.jsonis excluded by!.sqlx/**.sqlx/query-1e36c7781b1223f732691c4d2e705521d3d078d90adedc3d183068526cf8c025.jsonis excluded by!.sqlx/**.sqlx/query-22ff599e546c056efbd47d933d78e92b224afd1d4b43e81b37c91480c2389c60.jsonis excluded by!.sqlx/**.sqlx/query-28268ee66edc1f897450ec98ca58a34f70cabbf2aba1eb604c5005c2f6f1acf7.jsonis excluded by!.sqlx/**.sqlx/query-298c38a2042a2b7d19ec8b3ea880e71dfdc087eb0b298637578c141e738cf0d2.jsonis excluded by!.sqlx/**.sqlx/query-2b838db363de7027ecb22b4f4d7d92ab06f417dcf82d587f08906d9fafae7cb8.jsonis excluded by!.sqlx/**.sqlx/query-2c33b05a0995b5390079e3039553a194b1cc7535e0a2f19653f00dd78c387e79.jsonis excluded by!.sqlx/**.sqlx/query-302f4eddf6068e4b36a6e2cf88724fc05463006008722a654a54ca59f6fafeab.jsonis excluded by!.sqlx/**.sqlx/query-32335b00416cdef4f4979f83eb7f6f9e67771f815b02eca66bf0805340eb7a27.jsonis excluded by!.sqlx/**.sqlx/query-3ccea3bc52e21ddb7d395ab7c7a6171bcb64ad2d33a6159e72f6a147dea3ae0f.jsonis excluded by!.sqlx/**.sqlx/query-3cd6aecd2f5bf47061065497d3cb7443c070533b15a4ac10649875443efbe6aa.jsonis excluded by!.sqlx/**.sqlx/query-3ea1e4d0a2a32c7e5131b3b518f0291e5fe844821e74e3acd4100f312158d400.jsonis excluded by!.sqlx/**.sqlx/query-3ec4e3932d25c44bf92556cf1323ff761c463285832bf78886730c7d1638623e.jsonis excluded by!.sqlx/**.sqlx/query-3f118743d59e0e035f57c9106816533c092fead83b8420cdab3149b040664910.jsonis excluded by!.sqlx/**.sqlx/query-415502c8533199d7246c64eeac7f2677f10c62f0fae1c48de99bf5471c1f2fb8.jsonis excluded by!.sqlx/**.sqlx/query-470b25047d1c0936dad4b560df9a0c3b40d501ec8ecb8f3823ea05dae4c3b68a.jsonis excluded by!.sqlx/**.sqlx/query-48475e35dc4e5363310fbc65d192a8e2f2b4f68e5239ebddbdfb74218b388266.jsonis excluded by!.sqlx/**.sqlx/query-4d30952e19a84710348564dcd0aa04fb7dbacfc6ffa3f163c8375f4cb990e7f0.jsonis excluded by!.sqlx/**.sqlx/query-4ecd137d2abeaf58285165aeec7a0374b34baf029d7c572b5247dfb29463a949.jsonis excluded by!.sqlx/**.sqlx/query-5691c6fcc6ff1ea68bcd4601370e857df265a645246572031bdf7b4265c0f575.jsonis excluded by!.sqlx/**.sqlx/query-56d8028355ea7526d7ea058fd5df0421f6917d9c9d03e311f6e07bfe29b84710.jsonis excluded by!.sqlx/**.sqlx/query-5a450894a677d31c8a80122908dbf49ad77d804682936d96a9142028ce5372f7.jsonis excluded by!.sqlx/**.sqlx/query-5bd88e2962ade07ae621b69644240f5c5e48aefc4990b3804bf23c8ee6c9df53.jsonis excluded by!.sqlx/**.sqlx/query-5bdeb64b6e903d29cb916d8503d7fab61f49359c1e0c79cfe1b380dfbe9e16b2.jsonis excluded by!.sqlx/**.sqlx/query-5bfefb745f98ac0bbbb7abaa6fb8ce979007e57adbad3992710106d1b6c32705.jsonis excluded by!.sqlx/**.sqlx/query-60e3a10848aa3053c13dfad61a4471bf5b34a9403713822863115071a9e895fb.jsonis excluded by!.sqlx/**.sqlx/query-62b9aeb0ed103e7f63a42f246d713b7727307c032df2c983e7ab8a72d569ddb1.jsonis excluded by!.sqlx/**.sqlx/query-63abbd953774fcc023cb26064d11a96dc35789e1b9ef84825208a4a2b02c58f8.jsonis excluded by!.sqlx/**.sqlx/query-6404eaa43100990cc71c82fad60e2c5303aa7bfa218324e34a5fb1d8f902d0a2.jsonis excluded by!.sqlx/**.sqlx/query-6b784d53e21df29bcfba893706b19cd31fcac2def0a8059dc1f509c16820a1b6.jsonis excluded by!.sqlx/**.sqlx/query-6e9d1aa75b2aac3abfceb93e949f30c3bfcd6d00f2f11122e03a7041e22c73dd.jsonis excluded by!.sqlx/**.sqlx/query-7006c3203e878fdde07018127ebe9067b77b99aae37fe6797a1a38988c24cc56.jsonis excluded by!.sqlx/**.sqlx/query-740f7b4789465c898d2461093a927462835c08bd027d1e8fbb1b9fd64d702583.jsonis excluded by!.sqlx/**.sqlx/query-763b066e0be1435303b875e81fe228e2aa7e421e9c65624695c8078f83d6b011.jsonis excluded by!.sqlx/**.sqlx/query-793788700b022096e85b161d3207b374cf941dc832f6e055b938e2fe7d02c819.jsonis excluded by!.sqlx/**.sqlx/query-7b380cbadf9c4987f27b4ff8d34f7341962a5a472ed80fb016ef826ca7f8a9d0.jsonis excluded by!.sqlx/**.sqlx/query-7ccf866841e291289987c72fd8cebdeec715e90d37db01f518b16893e3b74c58.jsonis excluded by!.sqlx/**.sqlx/query-80b88583b4395d85b351b1ffa80156b4514856e8bde1475e9fcab906cd45b670.jsonis excluded by!.sqlx/**.sqlx/query-81a5b11c053d23d4717e45bdc70c9d4a19716a40c994cfe952d78e30bccf53bb.jsonis excluded by!.sqlx/**.sqlx/query-859adda46e98e6b145d11a1d8d68cefdccb601de8b3d08c4b16ecd9591d599a0.jsonis excluded by!.sqlx/**.sqlx/query-866c21d361cb3edcb12f126e822c6b90ca6cdbc4b59429d04d2309d2367b9b3a.jsonis excluded by!.sqlx/**.sqlx/query-897cfb6691b671d2dba0472279a7b20274a5d3327374d55b92479387f8def564.jsonis excluded by!.sqlx/**.sqlx/query-92b403b07793061ed856e5db770137b5e076514670a5c45491feb00b8b82d3f4.jsonis excluded by!.sqlx/**.sqlx/query-99b67458585575905661f7a95662d17e4c50a1efbf96b0fb22b8b774045a934a.jsonis excluded by!.sqlx/**.sqlx/query-99e4e108d554390e9b6d96270cfbc29710601746acb3b6e4e085a9019dfa85aa.jsonis excluded by!.sqlx/**.sqlx/query-99e60de0f6aed1a2e16aa330765626307b20d78da6110e9b9721dca9e93d04fa.jsonis excluded by!.sqlx/**.sqlx/query-9a1d9b777d4d91c44114e91bbe76e8759d9b0133f4bc278266d3ed7521324a61.jsonis excluded by!.sqlx/**.sqlx/query-9c3a9664e2775d6cdbf1bff4941d5e5f09819823b70cbec2f077e32cb83acfda.jsonis excluded by!.sqlx/**.sqlx/query-9f25b4c1ec0e8b957922d23fec3d9eb01234ccd926961554ae25c238d1d990fa.jsonis excluded by!.sqlx/**.sqlx/query-a02df0ff30895ec09168d38c713f34b0966fdd3290a8fee6ac26323cd311808a.jsonis excluded by!.sqlx/**.sqlx/query-a31269da65f9ecbb7fbf9f75686e887d4f61e700a747d52856326fa620494347.jsonis excluded by!.sqlx/**.sqlx/query-a4c9aa19924c03216a18bcad5bc64ad14614ecf903eefb7c2330d7eb307a8c8b.jsonis excluded by!.sqlx/**.sqlx/query-a90b3a153a66147d554945f8ae2cb0b1e73400e846d3d0ca617604fc30fea185.jsonis excluded by!.sqlx/**.sqlx/query-abdbc91a03d5a5564ba259916c20563103fe2adb2daa6b50eafceec4f8afab99.jsonis excluded by!.sqlx/**.sqlx/query-b03b867cae1000c1aa7dd2da639731991cf91ec0c9c4c01bbdff4ece5ed5e025.jsonis excluded by!.sqlx/**.sqlx/query-b0a2bdb25f4d0e34016f2e596d6a3a59e81cdea114afcc11d60120b99802f673.jsonis excluded by!.sqlx/**.sqlx/query-b104f2da1e2ca05bdc4666363587c34601c97dc68635af8d901791be0aa356b2.jsonis excluded by!.sqlx/**.sqlx/query-b18be0a4a957c1b6c14cffc5ecdbd949c1f6dd29ff8d8e5a76c7442f4afbef2f.jsonis excluded by!.sqlx/**.sqlx/query-c0d53c1c4b9e30e933ada122f4f3fad8f7d01e5ddeb468ab9393f85845bf4df7.jsonis excluded by!.sqlx/**.sqlx/query-c131c632f9bfff038c5b7040c4f7de497151fde0f06dd2e701465a1b8a092361.jsonis excluded by!.sqlx/**.sqlx/query-cd40a8b8dd3e13b62aa248b2aaf00756c9e56e86a48538229b523e6a8b372b05.jsonis excluded by!.sqlx/**.sqlx/query-cddd1db59ca6ad6e3b662a23e5d95f87fd443302c8a520a51c74ecd6c36c8b2b.jsonis excluded by!.sqlx/**.sqlx/query-d4c2147c4c61845ec9f0f35727eca5b08987de0b06611eabb088beb042f77dbb.jsonis excluded by!.sqlx/**.sqlx/query-d55ad8ebe461380e54acc1c5ee93f02fa6006d29293a77bf201841eceb7522cd.jsonis excluded by!.sqlx/**.sqlx/query-d7dd04f6a1cd6cfc7d6105516f8dc4230ebc3d20fb42bc4ef718745eb7d4059e.jsonis excluded by!.sqlx/**.sqlx/query-d7de2e54a6361b87925688339671f84c7bcab3acaa11b5fa254d0e5290284c87.jsonis excluded by!.sqlx/**.sqlx/query-dd25d34dfbcd5a60c02d02b396040c0728f48297ee938a39c388260ac32ab58e.jsonis excluded by!.sqlx/**.sqlx/query-ddcd04a1474282004db831e24cac62ef3b642a81fafce2dd574cec9bd3463d57.jsonis excluded by!.sqlx/**.sqlx/query-de8db3913d1926ff41275b2611d6be2b0a67e204d9c37d17ebd3b0bf8bd29cb1.jsonis excluded by!.sqlx/**.sqlx/query-e037d81f4b4079f028ff434a6e3408491813c3428c813d2974606c95cd8a8085.jsonis excluded by!.sqlx/**.sqlx/query-e34c836cd691b4f3888052a016999ee88017cdbf7c8dd1b174401f6f81b5e60a.jsonis excluded by!.sqlx/**.sqlx/query-e811cd8f04a94ea594b81896b0ac22b6f63bef0094d17b14e28d69ebaf26259a.jsonis excluded by!.sqlx/**.sqlx/query-f01580cb4b0a2f3dbe051041b5c931e9e3a11d3a9187c75eef5eeffa37547f20.jsonis excluded by!.sqlx/**.sqlx/query-f0f4d3b6d69711b2bb9c94ea2ed752dfb1b220cf4ddb02f02c6a3b6ebebdf5d6.jsonis excluded by!.sqlx/**.sqlx/query-f546866de3d80ab873776e8c49a88e0f0918804478edc0b793b62980bc69f4a1.jsonis excluded by!.sqlx/**.sqlx/query-f6130cf73a09eaff96b23ad91ba846b641fce5e2dbb7f64fedfdfed487779672.jsonis excluded by!.sqlx/**.sqlx/query-fbc4ce3f128847954fff3b367a196bf3c5948c5036ccb8ef612e1c2d9d035f28.jsonis excluded by!.sqlx/**Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
Cargo.tomlcrates/lakekeeper-bin/src/main.rscrates/lakekeeper/src/implementations/postgres/bootstrap.rscrates/lakekeeper/src/implementations/postgres/migrations/mod.rscrates/lakekeeper/src/implementations/postgres/migrations/patch_migration_hash.rscrates/lakekeeper/src/implementations/postgres/namespace.rscrates/lakekeeper/src/implementations/postgres/tabular/table/commit.rscrates/lakekeeper/src/tests/mod.rscrates/lakekeeper/tests/extension_migrations_fixture/20260101000000_create_ext_demo.sqlcrates/lakekeeper/tests/extension_migrations_fixture_invalid/20260101000000_create_ok.sqlcrates/lakekeeper/tests/extension_migrations_fixture_invalid/20260101000001_fail.sqlcrates/lakekeeper/tests/extension_migrations_fixture_patched/20260101000000_create_ext_demo.sqldocs/docs/developer-guide.md
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/lakekeeper/src/implementations/postgres/migrations/mod.rs (1)
149-151:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMissing duplicate extension name check.
The loop validates each extension's name format individually but doesn't detect duplicates. If two
ExtensionMigrationswith identical names are passed (e.g., both"demo"), they would both target the same tracker tableext_demo_sqlx_migrations, causing version conflicts or migrations incorrectly marked as already-applied.Proposed fix
for ext in &extensions { ext.validate_name()?; } + let mut seen_names = std::collections::HashSet::new(); + for ext in &extensions { + if !seen_names.insert(ext.name) { + return Err(anyhow!( + "duplicate extension migration name {:?}", + ext.name + )); + } + }Or combine into a single loop:
+ let mut seen_names = std::collections::HashSet::new(); for ext in &extensions { ext.validate_name()?; + if !seen_names.insert(ext.name) { + return Err(anyhow!( + "duplicate extension migration name {:?}", + ext.name + )); + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/lakekeeper/src/implementations/postgres/migrations/mod.rs` around lines 149 - 151, Loop currently only calls ext.validate_name() for each ExtensionMigrations and doesn't detect duplicate extension names; update the loop over extensions to both validate and check for duplicates by tracking seen names (e.g., use a HashSet<String>), and return an error if insert into the set fails (duplicate found). Reference the extensions iterator and ext.validate_name() and ensure the error message mentions the conflicting extension name (which would map to the tracker table ext_<name>_sqlx_migrations) so duplicates are rejected early.
🧹 Nitpick comments (1)
docs/docs/developer-guide.md (1)
238-244: ⚡ Quick winConsider using
regprocedurefor robust function drops.The current
DROP FUNCTION %I CASCADEstatement uses onlyr.proname, which will fail if a non-conforming extension created overloaded functions (same name, different signatures). Although the contract forbids extensions from creating functions underext_*and this is defensive cleanup code, using the function OID with aregprocedurecast would make the recovery procedure more robust.♻️ Suggested improvement
- FOR r IN SELECT p.proname FROM pg_proc p + FOR r IN SELECT p.oid::regprocedure AS proc_sig FROM pg_proc p JOIN pg_namespace n ON n.oid = p.pronamespace WHERE n.nspname = current_schema() AND p.proname LIKE 'ext\_%' ESCAPE '\' LOOP - EXECUTE format('DROP FUNCTION %I CASCADE', r.proname); + EXECUTE format('DROP FUNCTION %s CASCADE', r.proc_sig); END LOOP;The
regprocedurecast produces a fully-qualified signature that unambiguously identifies the function even when overloaded.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/docs/developer-guide.md` around lines 238 - 244, The DROP loop currently uses r.proname and format('DROP FUNCTION %I CASCADE', r.proname) which will fail for overloaded functions; change the DROP target to use the function's OID cast to regprocedure (e.g., r.oid::regprocedure) so the DROP references the fully-qualified signature, locate the loop that iterates over pg_proc (r.proname) and replace the format/identifier approach with a command that drops the function by its regprocedure representation to unambiguously identify overloaded functions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@crates/lakekeeper/src/implementations/postgres/migrations/mod.rs`:
- Around line 149-151: Loop currently only calls ext.validate_name() for each
ExtensionMigrations and doesn't detect duplicate extension names; update the
loop over extensions to both validate and check for duplicates by tracking seen
names (e.g., use a HashSet<String>), and return an error if insert into the set
fails (duplicate found). Reference the extensions iterator and
ext.validate_name() and ensure the error message mentions the conflicting
extension name (which would map to the tracker table ext_<name>_sqlx_migrations)
so duplicates are rejected early.
---
Nitpick comments:
In `@docs/docs/developer-guide.md`:
- Around line 238-244: The DROP loop currently uses r.proname and format('DROP
FUNCTION %I CASCADE', r.proname) which will fail for overloaded functions;
change the DROP target to use the function's OID cast to regprocedure (e.g.,
r.oid::regprocedure) so the DROP references the fully-qualified signature,
locate the loop that iterates over pg_proc (r.proname) and replace the
format/identifier approach with a command that drops the function by its
regprocedure representation to unambiguously identify overloaded functions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1f44cc07-7ec0-4668-b0d0-915a902c33a8
📒 Files selected for processing (3)
crates/lakekeeper/src/implementations/postgres/migrations/mod.rscrates/lakekeeper/src/implementations/postgres/migrations/patch_migration_hash.rsdocs/docs/developer-guide.md
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/lakekeeper/src/implementations/postgres/migrations/mod.rs`:
- Around line 60-63: The doc comment currently advertises a 41-character pattern
but validate_name() actually rejects 41-character identifiers; update the
comment to document the real enforced limit (max 40 characters total) and the
exact character rules instead of the wider regex—refer to validate_name() and
migrate() so readers know this is the runtime-enforced constraint (e.g., "Must
be 1–40 characters: first char [a-z_], remaining chars [a-z0-9_]").
- Around line 8-11: The doc comment incorrectly implies consumers can use
sqlx::migrate! via this crate; update the comment around the re-exported
SqlxMigrator to clearly state that only the Migrator type is re-exported and
that the sqlx::migrate! macro must still be added as a direct dependency (or
explicitly re-exported) in downstream crates; reference the symbol SqlxMigrator
and the macro sqlx::migrate! in the updated wording so readers know the
limitation and the correct remedy.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: de703fd6-5654-4877-a0ea-15acd8ec2fb2
📒 Files selected for processing (3)
crates/lakekeeper/src/implementations/postgres/migrations/mod.rscrates/lakekeeper/src/implementations/postgres/migrations/patch_migration_hash.rsdocs/docs/developer-guide.md
✅ Files skipped from review due to trivial changes (1)
- docs/docs/developer-guide.md
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Member
Author
|
@coderabbitai resolve |
Contributor
✅ Actions performedComments resolved and changes approved. |
Contributor
✅ PR Title Formatted CorrectlyThe title of this PR match the correct format. Thank you! |
c-thiel
deleted the
christian/vak-407-extension-table-convention-postmigratehook
branch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
BEGIN_COMMIT_OVERRIDE
feat(migrations): atomic core + extension migrations
chore: Bump MSRV to 1.94
chore(deps): sqlx v0.9.0
END_COMMIT_OVERRIDE
Add
ExtensionMigrationsregistration to upstream'smigrate()so downstream crates can contribute SQL migrations that run inside the same outer transaction as core. Either every migration commits or the upgrade rolls back — partial state across core and extensions isimpossible.
Each extension tracks applied migrations in its own
ext_<name>_sqlx_migrationstable; core keeps_sqlx_migrations. Existing callers use the back-compatmigrate_core_only(pool)shim.Reserves the
ext_*table prefix:ext_*objects.ext_*to upstream must beON DELETE CASCADEorON DELETE SET NULL; upstream never FKs intoext_*.Recovery procedure (DO block scanning
pg_class/pg_trigger/pg_type/pg_proc) documented in the developer guide.Tests cover the happy path, cross-source atomicity on failure, the no-
ext_*-in-core invariant, and the FK direction/CASCADE rule.Prerequisite: sqlx 0.8.6 → 0.9.0 (the
Migratetrait now accepts an explicittable_name, enabling per-source trackers). Bump fallout limited to nullability annotations on two queries, oneQueryBuilderlifetime drop, andAssertSqlSafewraps in test-only format queries.Summary by CodeRabbit
New Features
Chores
Documentation
Tests
Bug Fixes