migratekvdb: add Postgres bulk migration path#94
Conversation
debdd4c to
72cbef2
Compare
|
/gateway review |
72cbef2 to
345f0c5
Compare
|
/gateway review |
There was a problem hiding this comment.
Gateway review — 9 findings
🔴 0 Blocker · 🟠 4 Major · 🟡 5 Minor · 🔵 0 Nit
Summary
This PR wires lndinit to lnd's new Postgres-only bulk SQL-KV migration API, adding a MigrateBulk/VerifyMigrationBulk path behind a --bulk-writes flag plus a BulkInProgress recovery marker. The orchestration code is clean and the new logic is unit-tested for recovery, fresh-target rejection, and target-corruption detection.
The central risk is in the verification path, which is the gate that lets an operator tombstone a live source DB. VerifyMigrationBulk compares the bolt source cursor positionally against the rows returned by the lnd-owned FetchChildren, so its soundness rests on an ordering contract that is defined only in the (currently vendored-by-replace) fork and cannot be audited here. Separately, the interrupted-run recovery path unconditionally truncates the destination based on source-side state alone, and an explicitly-requested --bulk-writes is silently downgraded on non-Postgres destinations. Verdict is REQUEST_CHANGES.
Bot commands
/gateway re-review— re-run after pushing changes/gateway dismiss <id>— silence a finding (maintainers)/gateway explain <id>— elaborate on a finding (anyone)
|
🤖 gateway audit metadata for this PR — auto-generated, please don't edit. |
2f7a506 to
485971a
Compare
|
/gateway re-review |
|
🔁 gateway re-review starting… |
|
A benchmark I did so far locally just for info |
|
/gateway re-review |
29af421 to
b9d25f6
Compare
Bump the Go directive in both the main module and the tools module to 1.25.11 so the whole project builds and lints against a single, current toolchain. The tools module was still pinned to 1.25.4 and is now aligned with the main module.
b9d25f6 to
b48557c
Compare
|
Version check passed (not a version issue). The migrate phase was flying (~114k keys/sec) then postgres ran out of shared memory. The likely root cause: the upstream BeginBulk opens the whole migration as one SERIALIZABLE transaction (migration_bulk_postgres.go:91-92), and a serializable transaction inserting millions of rows accumulates predicate locks until it exhausts shared memory on default postgres settings. The original path I laid out used the default READ COMMITTED and completed on the same vanilla config. |
|
No-tag build breaks: cmd_migrate_db.go is untagged but references sqlbase.MigrationBackend, postgres.NewMigrationBackend, and the kvdb_postgres-only MigrateBulk/VerifyMigrationBulk. go build ./... without tags fails. Fix: move the bulk wiring into a //go:build kvdb_postgres file with a small non-postgres stub. (Mine avoided this by using database/sql generically in the untagged cmd.) |
780018d to
5d53429
Compare
btcd has split several of its packages into independently versioned modules. Move the direct imports over to the new module paths -- chaincfg/v2 in cmd_init_wallet.go and btcutil/v2 in cmd_migrate_db.go -- and update go.mod/go.sum accordingly. This pulls in the v2 lines of the btcd sub-modules (btcutil, chaincfg, btcec, chainhash, psbt, txscript, wire, address) together with the matching btcwallet 0.18.0 and kvdb 1.6.1, plus a broad refresh of the indirect dependency set. Note that lnd is pinned to a pseudo-version (v0.21.0-beta.rc2.0.20260717013942-f02cf4c470c9) rather than a tagged release -- a commit-hashed build is required until the split-module work lands in a tagged lnd. The kvdb bump is what exposes the SQL KV bulk migration API used by the following commits.
Add a BulkInProgress flag to the persisted migration state. The fresh-only bulk path sets this marker before it writes any rows to the target database and clears it once the write completes. The marker is what makes interrupted bulk runs recoverable: if the process dies mid-write, the flag is still set on the next start, which signals that the target holds a partial migration and must be reset before the bulk migration can start over from the source.
Add MigrateBulk and VerifyMigrationBulk, a faster bolt-to-SQL KV migration that goes through the destination backend's migration-only bulk API instead of the generic per-key kvdb writes. Migration walks the source bucket tree depth-first. Bucket rows are inserted immediately so their generated SQL ids can serve as parent ids for their children, while leaf rows are buffered in a bulkWriter and flushed in batches to amortize the insert cost. The path is intentionally not resumable mid-bucket. Before writing it sets the BulkInProgress marker; if a previous run left that marker behind, the target is reset through the lnd-owned API and the migration restarts from the source rather than trying to patch a partial write. A non-empty target from an interrupted run is only truncated when the caller explicitly authorizes the reset. Verification mirrors the migration: it matches source top-level buckets to target rows and then compares their direct children batch by batch using the backend's batched verification API, validating the target without a second full per-key scan.
Add coverage for the bulk SQL KV migration path, focusing on the crash-recovery contract established by the BulkInProgress marker. The tests exercise recovery of an interrupted attempt, the guard that refuses to reset a non-empty target unless the reset was explicitly authorized, the check that a fresh migration requires an empty target, and that bulk verification detects a corrupted target.
Expose the bulk migration path through the migrate-db command via two new flags: --bulk-writes to opt into the fresh-only Postgres bulk path, and --reset-bulk-target to authorize recovery from an interrupted run. When --bulk-writes is set the destination is opened with its migration-only bulk capability (sqlbase.MigrationBackend), and the command dispatches to MigrateBulk/VerifyMigrationBulk instead of the default per-key migration. Validation keeps the flag combinations safe: --bulk-writes requires a postgres destination, --reset-bulk-target requires --bulk-writes, and --reset-bulk-target cannot be combined with --force-new-migration, since the two describe conflicting ways to restart. Adds unit and postgres-backed tests for the new wiring and validation.
Document the new --bulk-writes and --reset-bulk-target flags in the data-migration guide. Explain that the bulk path is a fresh-only, faster Postgres migration that is not resumable mid-write, how automatic recovery works when an interrupted run left the target empty, and that a non-empty target fails closed until the operator explicitly authorizes truncating and destroying the destination table with --reset-bulk-target. Also refresh the generated help output to include both new flags.
5d53429 to
0b93da4
Compare
|
tACK. Migration of our large test DB took 8m27s. |
Summary
This PR wires lndinit to the migration-only SQL KV bulk migration API from
lightningnetwork/lnd#10965and adds a Postgres-only bulk migration path.It keeps lndinit as orchestration code:
--bulk-writesfor fresh Postgres migrations--reset-bulk-targetfor explicitly authorized destructive recoverykvdb_postgresbuild constraintAn interrupted migration whose transaction left the target empty restarts
automatically. If the target contains rows, recovery refuses to modify it unless
the operator explicitly supplies
--reset-bulk-target, which truncates thetarget before restarting.
Dependency
This PR depends on
lightningnetwork/lnd#10965.The temporary
replacedirectives ingo.modshould be removed once that PRlands and lndinit can depend on upstream lnd module versions.
Verification