fix: wrong encoding on migration #651
Conversation
WalkthroughThis pull request introduces a comprehensive update to the SQL migration script, focusing on database schema modifications. The changes involve creating new functions and triggers for the Changes
Sequence DiagramsequenceDiagram
participant DB as Database
participant Trigger as Constraint Triggers
participant Func as Custom Functions
DB->>Trigger: Insert/Update Transaction
Trigger->>Func: Enforce Reference Uniqueness
Trigger->>Func: Set Transaction Metadata
Trigger->>Func: Update Address Arrays
Func-->>DB: Validate and Modify Data
Possibly related PRs
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: 0
🔭 Outside diff range comments (2)
internal/storage/bucket/migrations/11-make-stateless/up.sql (2)
Line range hint
523-587: Add error handling in DO blockThe DO block creates multiple triggers and sequences without proper error handling. Consider adding exception handling to ensure atomic execution.
DO $do$ declare ledger record; vsql text; BEGIN + -- Add transaction to ensure atomic execution + BEGIN for ledger in select * from _system.ledgers where bucket = current_schema loop -- Wrap each ledger's operations in a sub-transaction + BEGIN vsql = 'create sequence "transaction_id_' || ledger.id || '" owned by transactions.id'; execute vsql; -- ... (rest of the operations) + EXCEPTION WHEN OTHERS THEN + RAISE NOTICE 'Failed to process ledger %: %', ledger.name, SQLERRM; + RAISE; + END; end loop; + EXCEPTION WHEN OTHERS THEN + RAISE NOTICE 'Migration failed: %', SQLERRM; + RAISE; + END; END $do$;
Line range hint
589-617: Consider using a unique index instead of advisory locksThe current implementation uses advisory locks to enforce reference uniqueness, which could cause unnecessary blocking. A unique partial index would be more efficient.
-create or replace function enforce_reference_uniqueness() returns trigger - security definer - language plpgsql -as -$$ -begin - perform pg_advisory_xact_lock(hashtext('reference-check' || current_schema)); - - if exists( - select 1 - from transactions - where reference = new.reference - and ledger = new.ledger - and id != new.id - ) then - raise exception 'duplicate reference'; - end if; - - return new; -end -$$ set search_path from current; - -create constraint trigger enforce_reference_uniqueness -after insert on transactions -deferrable initially deferred -for each row -when ( new.reference is not null ) -execute procedure enforce_reference_uniqueness(); +-- Create a unique partial index instead +CREATE UNIQUE INDEX transactions_reference_unique_idx +ON transactions (ledger, reference) +WHERE reference IS NOT NULL;
🧹 Nitpick comments (2)
internal/storage/bucket/migrations/11-make-stateless/up.sql (2)
Line range hint
3-22: Consider using a more robust approach for transaction_date()The current implementation using a temporary table could cause issues with concurrent transactions. Consider using a simpler approach:
-create or replace function transaction_date() returns timestamp as $$ - declare - ret timestamp without time zone; - begin - create temporary table if not exists transaction_date on commit delete rows as - select statement_timestamp(); - - select * - from transaction_date - limit 1 - into ret; - - if not found then - ret = statement_timestamp(); - - insert into transaction_date - select ret at time zone 'utc'; - end if; - - return ret at time zone 'utc'; - end +create or replace function transaction_date() returns timestamp as $$ + begin + return statement_timestamp() at time zone 'utc'; + end $$ language plpgsql;
Line range hint
523-587: Consider optimizing trigger operationsThe current implementation creates multiple triggers per ledger, which could impact insert performance. Consider:
- Combining related operations into single triggers where possible
- Moving non-critical operations to the application layer
For example,
set_transaction_addressesandset_transaction_addresses_segmentscould be combined:create or replace function set_transaction_addresses() returns trigger security definer language plpgsql as $$ begin new.sources = ( select to_jsonb(array_agg(v->>'source')) as value from jsonb_array_elements(new.postings::jsonb) v ); new.destinations = ( select to_jsonb(array_agg(v->>'destination')) as value from jsonb_array_elements(new.postings::jsonb) v ); + new.sources_arrays = ( + select to_jsonb(array_agg(explode_address(v ->> 'source'))) as value + from jsonb_array_elements(new.postings::jsonb) v + ); + new.destinations_arrays = ( + select to_jsonb(array_agg(explode_address(v ->> 'destination'))) as value + from jsonb_array_elements(new.postings::jsonb) v + ); return new; end $$ set search_path from current;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/storage/bucket/migrations/11-make-stateless/up.sql(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Tests
🔇 Additional comments (1)
internal/storage/bucket/migrations/11-make-stateless/up.sql (1)
464-464:⚠️ Potential issueVerify hash consistency after encoding change
The change from implicit LATIN1 to explicit UTF-8 encoding is good for handling international characters, but it will affect how new log hashes are generated. This could create inconsistencies between old and new log entries.
Run this script to check for potential hash inconsistencies:
✅ Verification successful
Encoding change is safe and well-implemented
The change from LATIN1 to UTF-8 encoding is safe because:
- Previous hashes are preserved in their base64-encoded form
- The UTF-8 encoding is only applied to new JSON data
- Hash calculation remains atomic within transactions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential hash inconsistencies in logs table # Look for any logs with non-ASCII characters that might be affected by the encoding change # Check for logs with non-ASCII characters rg -l '[^\x00-\x7F]' | grep -i 'log' # Check the logs table structure \d logs # Sample a few log entries to verify hash consistency SELECT id, encode(hash, 'hex') as hash, type FROM logs WHERE date < NOW() ORDER BY id DESC LIMIT 5;Length of output: 368
Script:
#!/bin/bash # Check the complete migration file content cat internal/storage/bucket/migrations/11-make-stateless/up.sql # Look for other encoding-related changes rg -i "convert_to|encode|::bytea|latin1|utf-8|utf8" internal/storage/bucket/migrations/11-make-stateless/ # Find related test files fd -e go -e sql test | grep -i "log\|hash\|encoding"Length of output: 18594
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #651 +/- ##
==========================================
+ Coverage 81.62% 81.64% +0.01%
==========================================
Files 131 131
Lines 7059 7059
==========================================
+ Hits 5762 5763 +1
Misses 994 994
+ Partials 303 302 -1 ☔ View full report in Codecov by Sentry. |
No description provided.