fix: too many loops while running migrations #618
Conversation
WalkthroughThe pull request involves modifications to three SQL migration scripts in the Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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
CodeRabbit Configuration File (
|
![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
🧹 Nitpick comments (2)
internal/storage/bucket/migrations/20-accounts-volumes-fill-history/up.sql (1)
34-34: LGTM! Loop condition fix prevents unnecessary iterationThe change from
0.._countto0.._count-1correctly aligns with SQL's 0-based OFFSET, preventing an unnecessary extra iteration that could cause out-of-bounds errors.Consider adding a progress log showing the percentage complete:
- perform pg_notify('migrations-{{ .Schema }}', 'continue: ' || _batch_size); + perform pg_notify('migrations-{{ .Schema }}', 'progress: ' || round((i::float/_count * 100))::text || '% (' || _batch_size || ' records)');internal/storage/bucket/migrations/19-transactions-fill-pcv/up.sql (1)
27-32: LGTM! Early termination optimization and improved progress trackingThe addition of the early termination check is a good optimization that prevents unnecessary processing when there's no data to migrate. Moving the notification after the check ensures accurate progress reporting.
Consider adding a count to the notification message to help with monitoring:
- perform pg_notify('migrations-{{ .Schema }}', 'init: ' || (select count(*) from moves_view)); + with stats as (select count(*) as total, min(transactions_id) as min_id, max(transactions_id) as max_id from moves_view) + perform pg_notify('migrations-{{ .Schema }}', 'init: ' || total || ' records (ID range: ' || min_id || '-' || max_id || ')') from stats;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
internal/storage/bucket/migrations/18-transactions-fill-inserted-at/up.sql(1 hunks)internal/storage/bucket/migrations/19-transactions-fill-pcv/up.sql(1 hunks)internal/storage/bucket/migrations/20-accounts-volumes-fill-history/up.sql(1 hunks)
🔇 Additional comments (1)
internal/storage/bucket/migrations/18-transactions-fill-inserted-at/up.sql (1)
27-27: LGTM! Loop condition fix and proper trigger handling
The change to 0.._count-1 prevents unnecessary iteration, and the migration properly handles triggers during the bulk update operation.
Let's verify that all relevant triggers are properly handled:
✅ Verification successful
LGTM! Loop condition and trigger handling are correctly implemented
The migration properly handles all triggers by setting session_replication_role = replica before the bulk update and resetting it to default afterward. This ensures that the five existing triggers on the transactions table (metadata history, addresses) are temporarily disabled during the migration, preventing unnecessary trigger executions during the bulk update operation. The loop condition fix to 0.._count-1 is also correct.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any other triggers on the transactions table that might need special handling
ast-grep --pattern 'create trigger $_ on transactions'
Length of output: 56
Script:
#!/bin/bash
# Let's try with ripgrep to search for trigger definitions
rg -i "create\s+trigger.*transactions" -g "*.sql"
Length of output: 1308
Script:
#!/bin/bash
# Let's verify if the migration properly handles session_replication_role
rg "session_replication_role" internal/storage/bucket/migrations/18-transactions-fill-inserted-at/up.sql
Length of output: 190
gfyrag
force-pushed
the
fix/migration-too-many-loops
branch
from
40b22c5 to
da5b319
Compare
No description provided.