fix: migration postgres azure #681
Conversation
gfyrag
commented
Feb 13, 2025
gfyrag
commented
- fix: migration using azure postgres with no superuser provileges
- fix: next migration
WalkthroughThe changes update two migration scripts related to the transactions table. They remove session-level disabling and enabling of triggers, replacing it with ledger-specific trigger creation. The script in migration 18 introduces new variables (_ledger record and _vsql text) and sets up a trigger for updating transaction metadata upon changes. The migration 19 script similarly removes the global session trigger modifications and retains batch processing for updating transactions and enforcing constraints. Changes
Sequence Diagram(s)sequenceDiagram
participant MS as Migration Script
participant LS as Ledgers Table
participant TT as Temporary Table (logs_transactions)
participant TX as Transactions Table
participant UTM as update_transaction_metadata_history Function
loop For each ledger in LS
MS->>LS: Fetch ledger record
MS->>MS: Create trigger for metadata update
MS->>UTM: Link trigger to update_transaction_metadata_history
end
MS->>TT: Create and populate temporary logs
MS->>TX: Update transactions based on TT
MS->>TX: Alter inserted_at column default
MS->>TT: Drop temporary table
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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: 1
🔭 Outside diff range comments (1)
internal/storage/bucket/migrations/18-transactions-fill-inserted-at/up.sql (1)
20-22: 💡 Verification agent🧩 Analysis chain
Avoid hard-coding migration version IDs.
The query uses a hard-coded
version_id = 12which could cause issues if migration numbering changes.Consider using a more robust way to identify the migration, such as by name or timestamp. Run this script to check other migrations for similar patterns:
🏁 Script executed:
#!/bin/bash # Description: Check for hard-coded version IDs in migrations # Expected: This should be a unique case or follow a consistent pattern rg -g "*.sql" "version_id\s*=\s*\d+" internal/storage/bucket/migrations/Length of output: 303
Hard-coded migration version IDs and missing error handling
- The migration in
internal/storage/bucket/migrations/18-transactions-fill-inserted-at/up.sqluses a hard-codedversion_id = 12, and the same pattern is also present ininternal/storage/bucket/migrations/19-transactions-fill-pcv/up.sql. This could lead to issues if migration numbering changes.- Consider adopting a more robust mechanism to reference migration versions—such as using migration names, timestamps, or a dedicated version management strategy.
- Additionally, the migration script lacks explicit error handling for failed batches. Please add appropriate error handling to address potential failures during migration.
🧹 Nitpick comments (2)
internal/storage/bucket/migrations/18-transactions-fill-inserted-at/up.sql (2)
11-13: Great architectural improvement!The change from session-level trigger management to per-ledger triggers is a solid improvement that:
- Fixes Azure PostgreSQL compatibility
- Optimizes performance by only triggering on metadata updates
36-52: Consider adding error handling for batch processing.The batch processing loop lacks error handling for failed updates. A failure in one batch could leave the data in an inconsistent state.
Consider adding error handling:
for i in 0.._count-1 by _batch_size loop begin with _rows as ( select * from logs_transactions order by ledger, transaction_id offset i limit _batch_size ) update transactions set inserted_at = _rows.date from _rows where transactions.ledger = _rows.ledger and transactions.id = _rows.transaction_id; commit; perform pg_notify('migrations-{{ .Schema }}', 'continue: ' || _batch_size); exception when others then perform pg_notify('migrations-{{ .Schema }}', 'error: ' || sqlerrm); raise; end; end loop;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/storage/bucket/migrations/18-transactions-fill-inserted-at/up.sql(1 hunks)internal/storage/bucket/migrations/19-transactions-fill-pcv/up.sql(0 hunks)
💤 Files with no reviewable changes (1)
- internal/storage/bucket/migrations/19-transactions-fill-pcv/up.sql
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Tests
🔇 Additional comments (1)
internal/storage/bucket/migrations/18-transactions-fill-inserted-at/up.sql (1)
3-4: LGTM! Well-structured variable declarations.The new variables
_ledgerand_vsqlare appropriately typed for their intended use in dynamic trigger creation.
| for _ledger in select * from _system.ledgers where bucket = current_schema loop | ||
| _vsql = 'create or replace trigger "update_transaction_metadata_history_' || _ledger.id || '" after update of metadata on "transactions" for each row when (new.ledger = ''' || _ledger.name || ''') execute procedure update_transaction_metadata_history()'; | ||
| execute _vsql; | ||
| end loop; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Review SQL injection risk in dynamic trigger creation.
The dynamic SQL construction using string concatenation with _ledger.id and _ledger.name could be vulnerable to SQL injection if these values contain malicious content.
Consider using proper quoting functions. Run this script to check if PostgreSQL's quote_ident and quote_literal functions are available in your environment:
If available, consider this safer approach:
_vsql = format(
'create or replace trigger %s after update of metadata on transactions for each row when (new.ledger = %s) execute procedure update_transaction_metadata_history()',
quote_ident('update_transaction_metadata_history_' || _ledger.id),
quote_literal(_ledger.name)
);🏁 Script executed:
#!/bin/bash
# Description: Check if proper quoting functions are available
# Expected: Functions should be available in PostgreSQL
curl -s https://www.postgresql.org/docs/current/functions-string.html | grep -A 2 "quote_ident"Length of output: 1275
SQL Injection Risk: Use PostgreSQL Quoting Functions for Safe Dynamic SQL Generation
-
File:
internal/storage/bucket/migrations/18-transactions-fill-inserted-at/up.sql(Lines 14-17) -
Issue: The dynamic SQL construction concatenates
_ledger.idand_ledger.namedirectly, which may lead to SQL injection if these values ever contain unexpected or malicious content. -
Verification: A shell script confirmed that PostgreSQL’s
quote_identfunction is available (and by extension,quote_literalis available), so safe quoting functions can be used. -
Recommendation: Replace the current string concatenation with a safer version using the
formatfunction combined withquote_identandquote_literal. For example:_vsql = format( 'create or replace trigger %s after update of metadata on transactions for each row when (new.ledger = %s) execute procedure update_transaction_metadata_history()', quote_ident('update_transaction_metadata_history_' || _ledger.id), quote_literal(_ledger.name) );
Please update the trigger creation code to use this format, ensuring that all identifiers and literals are safely quoted to mitigate any SQL injection risks.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #681 +/- ##
==========================================
- Coverage 81.63% 81.58% -0.05%
==========================================
Files 131 131
Lines 7061 7061
==========================================
- Hits 5764 5761 -3
- Misses 994 996 +2
- Partials 303 304 +1 ☔ View full report in Codecov by Sentry. |
