Skip to content

fix/temporary tables #1257

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
gfyrag wants to merge 2 commits into
base: main
Choose a base branch
from
+29 −2
Open

fix/temporary tables #1257

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3c59e0a
feat: add replica identity on tables without primary key
gfyrag Jan 23, 2026
2ee74d7
fix: potentially orphan temporary tables
gfyrag Feb 6, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion internal/storage/bucket/migrations/11-make-stateless/up.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -629,7 +629,8 @@ select aggregate_objects(volumes_to_jsonb(volumes_with_asset))
from get_all_account_volumes(_ledger, _account_address, _before := _before) volumes_with_asset
$$ set search_path from current;

create temporary table tmp_volumes as
drop table if exists tmp_volumes;
create temporary table tmp_volumes on commit drop as
select
ledger,
accounts_address,
Expand Down
3 changes: 3 additions & 0 deletions internal/storage/bucket/migrations/17-moves-fill-transaction-id/up.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ do $$
begin
set search_path = '{{.Schema}}';

drop table if exists transactions_ids;
create temporary table transactions_ids as
select row_number() over (order by transactions.seq) as row_number,
moves.seq as moves_seq, transactions.id, transactions.seq as transactions_seq
Expand Down Expand Up @@ -37,6 +38,8 @@ do $$

perform pg_notify('migrations-{{ .Schema }}', 'continue: ' || _batch_size);
end loop;

drop table transactions_ids;
end
$$
language plpgsql;
1 change: 1 addition & 0 deletions internal/storage/bucket/migrations/18-transactions-fill-inserted-at/up.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ do $$
execute _vsql;
end loop;

drop table if exists logs_transactions;
create temporary table logs_transactions as
select row_number() over (order by ledger, id) as row_number, ledger, date, (data->'transaction'->>'id')::bigint as transaction_id
from logs
Expand Down
3 changes: 2 additions & 1 deletion internal/storage/bucket/migrations/20-accounts-volumes-fill-history/up.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,8 @@ do $$
begin
set search_path = '{{.Schema}}';

create temporary table tmp_volumes as
drop table if exists tmp_volumes;
create temporary table tmp_volumes on commit drop as
Comment on lines +8 to +9
Copy link
Contributor

@coderabbitai coderabbitai bot Feb 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Critical: ON COMMIT DROP will destroy tmp_volumes after the first batch COMMIT at Line 47, breaking all subsequent loop iterations.

Since the DO block issues intermediate COMMIT statements (Line 47) for batch processing, each commit ends the current transaction and starts a new one. The ON COMMIT DROP clause causes PostgreSQL to drop the temp table at each commit boundary. The second batch iteration will fail with a "relation tmp_volumes does not exist" error.

Remove ON COMMIT DROP to match the pattern used in migration 17 (plain temp table + explicit drop after the loop):

Proposed fix 
 		drop table if exists tmp_volumes;
-		create temporary table tmp_volumes on commit drop as
+		create temporary table tmp_volumes as
 		select distinct on (ledger, accounts_address, asset)
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
drop table if exists tmp_volumes;
create temporary table tmp_volumes on commit drop as
drop table if exists tmp_volumes;
create temporary table tmp_volumes as
select distinct on (ledger, accounts_address, asset)
🤖 Prompt for AI Agents 
In `@internal/storage/bucket/migrations/20-accounts-volumes-fill-history/up.sql`
around lines 8 - 9, Remove the ON COMMIT DROP clause from the temporary table
creation so tmp_volumes survives intermediate COMMITs inside the DO block;
locate the CREATE TEMPORARY TABLE tmp_volumes ON COMMIT DROP AS statement and
change it to a plain temporary table creation (matching migration 17's pattern),
and ensure an explicit DROP TABLE IF EXISTS tmp_volumes is executed after the
loop/DO block to clean up.

select distinct on (ledger, accounts_address, asset)
ledger,
accounts_address,
Expand Down
1 change: 1 addition & 0 deletions internal/storage/bucket/migrations/45-add-replica-identity/notes.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
name: Add replica identity on tables without primary key
20 changes: 20 additions & 0 deletions internal/storage/bucket/migrations/45-add-replica-identity/up.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
do $$
declare
ledger record;
vsql varchar;
begin
set search_path = '{{ .Schema }}';

for ledger in select * from _system.ledgers where bucket = current_schema loop
vsql = 'ALTER TABLE transactions REPLICA IDENTITY USING INDEX transactions_ledger';
execute vsql;

vsql = 'ALTER TABLE accounts REPLICA IDENTITY USING INDEX accounts_ledger';
execute vsql;

vsql = 'ALTER TABLE logs REPLICA IDENTITY USING INDEX logs_ledger';
execute vsql;
end loop;
Comment on lines +8 to +17
Copy link
Contributor

@coderabbitai coderabbitai bot Feb 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# First, read the complete migration file to see full context
cat internal/storage/bucket/migrations/45-add-replica-identity/up.sql

Repository: formancehq/ledger

Length of output: 546

🏁 Script executed:

# Search for where these indexes are created to understand if they're per-ledger
rg -n "CREATE.*INDEX.*(transactions_ledger|accounts_ledger|logs_ledger)" --type sql -B2 -A2

Repository: formancehq/ledger

Length of output: 43

🏁 Script executed:

# Look for other migrations to understand table and ledger structure
ls -la internal/storage/bucket/migrations/ | head -20

Repository: formancehq/ledger

Length of output: 1491

🏁 Script executed:

# Search for how ledger is used in similar migration patterns
rg -n "for ledger in select" internal/storage/bucket/migrations/ -A10

Repository: formancehq/ledger

Length of output: 9897

🏁 Script executed:

# Search for the index creation for these specific indexes
rg -n "CREATE.*INDEX.*transactions_ledger|accounts_ledger|logs_ledger" internal/storage/bucket/migrations/ --type sql

Repository: formancehq/ledger

Length of output: 1472

🏁 Script executed:

# Also check the initial schema to see if these indexes are defined there
cat internal/storage/bucket/migrations/0-init-schema/up.sql | grep -n "transactions_ledger\|accounts_ledger\|logs_ledger"

Repository: formancehq/ledger

Length of output: 276

The ledger loop variable is never used — the loop redundantly executes identical ALTER TABLE statements once per ledger when they should execute once per schema.

The indexes transactions_ledger, accounts_ledger, and logs_ledger are schema-global (created in migration 0-init-schema as unique indexes on the shared tables, with the ledger column as part of the composite key). They do not differ per ledger, so the loop is unnecessary overhead.

Remove the loop and execute the three ALTER TABLE statements once. (Note: migration 45-fix-missing-primary-keys already uses these same indexes without looping, confirming this is the correct approach.)

🤖 Prompt for AI Agents 
In `@internal/storage/bucket/migrations/45-add-replica-identity/up.sql` around
lines 8 - 17, Remove the unnecessary loop "for ledger in select * from
_system.ledgers loop" (and its loop-end) and the per-iteration use of the vsql
variable so the three ALTER TABLE statements run exactly once for the current
schema; specifically keep/emit the three EXECUTE statements that run "ALTER
TABLE transactions REPLICA IDENTITY USING INDEX transactions_ledger", "ALTER
TABLE accounts REPLICA IDENTITY USING INDEX accounts_ledger", and "ALTER TABLE
logs REPLICA IDENTITY USING INDEX logs_ledger" outside of any loop (remove the
loop and the unused loop variable `ledger` and leave the EXECUTE calls once).

end
$$;

Loading