fix: speed up migrations #1018
fix: speed up migrations #1018
Conversation
WalkthroughThis update modifies several SQL migration scripts to create permanent tables ( Changes
Sequence Diagram(s)sequenceDiagram
participant MigrationScript
participant Database
MigrationScript->>Database: Create permanent table (moves_view/txs_view)
MigrationScript->>Database: Add foreign key constraint to transactions(seq)
loop For each batch
MigrationScript->>Database: Perform update using optimized join
alt No rows found
MigrationScript->>Database: Drop permanent table
MigrationScript->>MigrationScript: Exit loop
end
end
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
internal/storage/bucket/migrations/27-fix-invalid-pcv/up.sql (1)
10-31: Same optimisation comments as migration 28All remarks about using
UNLOGGEDtables and avoiding redundant indexes/FKs from migration 28 apply here verbatim.internal/storage/bucket/migrations/19-transactions-fill-pcv/up.sql (1)
10-31: Same optimisation comments as migration 28The
UNLOGGEDtable suggestion and index/FK duplication concern equally apply to this migration.
🧹 Nitpick comments (4)
internal/storage/bucket/migrations/28-fix-pcv-missing-asset/up.sql (2)
10-10: Consider using UNLOGGED to avoid WAL overhead
moves_viewexists only for the duration of the migration and is later dropped. Declaring itUNLOGGEDcan cut write-ahead-log traffic and speed the migration with zero durability concerns.- create table moves_view as + create unlogged table moves_view as
29-31: Redundant index creation – let the FK reuse it
create index moves_view_idx on moves_view(transactions_seq);is immediately followed by a foreign-key constraint on the same column. Postgres will either:
- Reuse the existing index, or
- Create an identical one if it can’t reuse it (version-dependent).
Either way, you end up maintaining two indexes or one unnecessary DDL. Dropping the explicit index avoids ambiguity:
- create index moves_view_idx on moves_view(transactions_seq); -- speed up hash join when updating rows later alter table moves_view add foreign key(transactions_seq) references transactions(seq);internal/storage/bucket/migrations/31-fix-transaction-updated-at/up.sql (2)
10-10: UNLOGGED table can speed bulk updates
txs_viewis a scratch table dropped at the end. Marking itUNLOGGEDavoids WAL and often halves runtime on large ledgers.- create table txs_view as + create unlogged table txs_view as
18-19: Potential duplicate index from FK + explicit indexThe FK on
seqwill (re)use or recreate an index identical to the one already present ontransactions.seq. Unless you rely ontxs_view(seq)for other purposes, consider omitting the FK altogether and just keep the plain index to avoid catalog bloat and validation time.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
internal/storage/bucket/migrations/19-transactions-fill-pcv/up.sql(2 hunks)internal/storage/bucket/migrations/27-fix-invalid-pcv/up.sql(2 hunks)internal/storage/bucket/migrations/28-fix-pcv-missing-asset/up.sql(2 hunks)internal/storage/bucket/migrations/31-fix-transaction-updated-at/up.sql(2 hunks)
🧰 Additional context used
🧠 Learnings (4)
internal/storage/bucket/migrations/28-fix-pcv-missing-asset/up.sql (1)
Learnt from: gfyrag
PR: formancehq/ledger#935
File: internal/controller/system/state_tracker.go:0-0
Timestamp: 2025-05-20T13:48:07.455Z
Learning: In the Formance ledger codebase, sequence reset queries with `select setval` don't require COALESCE around max(id) even for brand new ledgers, as the system handles this case properly.
internal/storage/bucket/migrations/19-transactions-fill-pcv/up.sql (1)
Learnt from: gfyrag
PR: formancehq/ledger#935
File: internal/controller/system/state_tracker.go:0-0
Timestamp: 2025-05-20T13:48:07.455Z
Learning: In the Formance ledger codebase, sequence reset queries with `select setval` don't require COALESCE around max(id) even for brand new ledgers, as the system handles this case properly.
internal/storage/bucket/migrations/27-fix-invalid-pcv/up.sql (1)
Learnt from: gfyrag
PR: formancehq/ledger#935
File: internal/controller/system/state_tracker.go:0-0
Timestamp: 2025-05-20T13:48:07.455Z
Learning: In the Formance ledger codebase, sequence reset queries with `select setval` don't require COALESCE around max(id) even for brand new ledgers, as the system handles this case properly.
internal/storage/bucket/migrations/31-fix-transaction-updated-at/up.sql (3)
Learnt from: gfyrag
PR: formancehq/ledger#935
File: internal/controller/system/state_tracker.go:0-0
Timestamp: 2025-05-20T13:48:07.455Z
Learning: In the Formance ledger codebase, sequence reset queries with `select setval` don't require COALESCE around max(id) even for brand new ledgers, as the system handles this case properly.
Learnt from: gfyrag
PR: formancehq/ledger#935
File: internal/controller/system/state_tracker.go:50-55
Timestamp: 2025-05-20T13:07:54.504Z
Learning: In the ledger codebase's `handleState` method, when updating ledger state from `StateInitializing` to `StateInUse`, it's intentional to proceed silently when `rowsAffected == 0`. This indicates another parallel transaction has already updated the ledger state and configured the sequences, so no error needs to be returned and no sequence updating is required.
Learnt from: gfyrag
PR: formancehq/ledger#892
File: internal/controller/ledger/controller_default.go:196-196
Timestamp: 2025-04-29T11:24:28.923Z
Learning: In the ledger Import function, it's critical to maintain proper log ID tracking by updating lastLogID with the current log.ID after each processed log, rather than setting it to nil. This ensures the system can properly validate the ordering of logs and prevent duplicate or out-of-order processing, which is essential for maintaining data integrity in the ledger.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Cursor BugBot
- GitHub Check: Tests
🔇 Additional comments (2)
internal/storage/bucket/migrations/28-fix-pcv-missing-asset/up.sql (1)
13-13: Double-check type compatibility after removing the numeric cast
transactions_seqis now taken as-is frommoves.transactions_seq. Make sure this column’s type (likelybigint) exactly matchestransactions.seq; otherwise the FK added below will fail.internal/storage/bucket/migrations/31-fix-transaction-updated-at/up.sql (1)
34-34: Ledger-scoped uniqueness assumption – please confirmThe previous join filtered on both
seqandledger. Dropping theledgerpredicate assumesseqis unique within the current schema. If multiple ledgers can share a schema, this may update rows across ledgers inadvertently.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1018 +/- ##
==========================================
+ Coverage 80.51% 80.78% +0.26%
==========================================
Files 186 186
Lines 10199 10199
==========================================
+ Hits 8212 8239 +27
+ Misses 1565 1544 -21
+ Partials 422 416 -6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
internal/storage/bucket/migrations/34-fix-memento-format/up.sql (1)
90-90:COMMITinside aDOblock is invalid in PostgreSQL
DO $$ … $$;executes as a single anonymous code block inside its own transaction. IssuingCOMMIT(orROLLBACK) inside the block raises
ERROR: cannot commit while a subtransaction is active(PG ≥9.4).Unless you are using a non-standard pl/pgsql variant that permits commits, this line should be removed. If chunked commits are required, convert the code into an external script that opens/commits transactions between calls to a stored procedure, rather than from inside the procedure itself.
🧹 Nitpick comments (2)
internal/storage/bucket/migrations/34-fix-memento-format/up.sql (2)
4-4: Large default batch size may pressure memory / WALJumping from 1 000 to 10 000 rows per batch improves I/O efficiency but can spike memory usage and enlarge single-transaction WAL records, especially given the heavy
json_build_objectwork per row. Consider making_batch_sizeconfigurable (e.g., viaSETor a passed parameter) so operators can tune it for their environment.
93-93:DROP TABLE IF EXISTS txs_view;appears unrelated
txs_viewis never created in this migration. While harmless, keeping unrelated cleanup logic obscures intent and can confuse readers. Recommend removing the line unless the view is guaranteed to exist from prior steps.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
internal/storage/bucket/migrations/19-transactions-fill-pcv/up.sql(3 hunks)internal/storage/bucket/migrations/27-fix-invalid-pcv/up.sql(3 hunks)internal/storage/bucket/migrations/28-fix-pcv-missing-asset/up.sql(3 hunks)internal/storage/bucket/migrations/31-fix-transaction-updated-at/up.sql(2 hunks)internal/storage/bucket/migrations/34-fix-memento-format/up.sql(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/storage/bucket/migrations/19-transactions-fill-pcv/up.sql
- internal/storage/bucket/migrations/31-fix-transaction-updated-at/up.sql
- internal/storage/bucket/migrations/27-fix-invalid-pcv/up.sql
- internal/storage/bucket/migrations/28-fix-pcv-missing-asset/up.sql
🧰 Additional context used
🧠 Learnings (1)
internal/storage/bucket/migrations/34-fix-memento-format/up.sql (1)
Learnt from: gfyrag
PR: formancehq/ledger#935
File: internal/controller/system/state_tracker.go:0-0
Timestamp: 2025-05-20T13:48:07.455Z
Learning: In the Formance ledger codebase, sequence reset queries with `select setval` don't require COALESCE around max(id) even for brand new ledgers, as the system handles this case properly.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Dirty
- GitHub Check: Tests
- GitHub Check: Cursor BugBot
| with data as ( | ||
| select * | ||
| from logs | ||
| where seq >= _offset and seq < _offset + _batch_size | ||
| order by seq | ||
| offset _offset | ||
| limit _batch_size | ||
| ) |
There was a problem hiding this comment.
Range-based pagination risks skipping rows with sparse seq values
Switching from OFFSET/LIMIT to
where seq >= _offset and seq < _offset + _batch_sizeassumes that no gap in seq exceeds _batch_size.
If a gap larger than 10 000 exists (e.g., deleted rows or a sequence reset), the data CTE will return zero rows even though unprocessed rows still exist above the gap. Because exit when not found; triggers on the empty UPDATE, the loop terminates prematurely and leaves those rows un-migrated.
A safer pattern is to drive the loop by the last processed seq, not by a synthetic offset:
- _offset integer := 0;
+ _last_seq integer := -1;
...
- with data as (
- select *
- from logs
- where seq >= _offset and seq < _offset + _batch_size
- order by seq
- )
+ with data as (
+ select *
+ from logs
+ where seq > _last_seq
+ order by seq
+ limit _batch_size
+ )
...
- exit when not found;
-
- _offset = _offset + _batch_size;
+ exit when not found;
+
+ -- advance to the highest seq we just touched
+ _last_seq = (select max(seq) from data);This iterates until all rows are handled, independent of sparsity.
🤖 Prompt for AI Agents
In internal/storage/bucket/migrations/34-fix-memento-format/up.sql around lines
15 to 20, the current pagination uses a range condition on seq that assumes no
gaps larger than _batch_size, which can cause skipping rows if large gaps exist.
To fix this, refactor the loop to track the last processed seq value and use it
to fetch the next batch with a condition like seq > last_processed_seq ORDER BY
seq LIMIT _batch_size, ensuring all rows are processed regardless of gaps.
No description provided.