fix(CONN-107): added missing indices #492
Conversation
WalkthroughA new project guideline document was introduced, outlining the structure, development, and deployment practices for the Formance Payments project. Additionally, a new database migration was added to create several indices on foreign key columns across multiple tables, aiming to improve query performance. The migration is registered and executed in the migration system. Changes
Sequence Diagram(s)sequenceDiagram
participant MigrationRunner
participant DB
MigrationRunner->>DB: Start migration "foreign key indices creation"
MigrationRunner->>DB: Call AddForeignKeyIndices
loop For each index
DB->>DB: Execute CREATE INDEX CONCURRENTLY
DB-->>MigrationRunner: Return error if any
end
MigrationRunner->>DB: Log completion
Assessment against linked issues
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
✨ 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
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/migrations/17-add-foreign-key-indices.go (2)
9-19: Address the TODO comments for comprehensive foreign key supportThe TODO comments indicate several missing foreign key relationships that should be addressed. Consider creating follow-up tasks to implement these missing foreign key constraints and indices.
Would you like me to create a follow-up issue to track these missing foreign key implementations?
118-119: Consider shortening the index name to avoid PostgreSQL limitsThe index name
payment_initiation_reversal_adjustments_pir_idis quite long and might approach PostgreSQL's identifier length limit (63 characters). Consider using a shorter name likepir_adjustments_pir_idfor consistency.- CREATE INDEX CONCURRENTLY payment_initiation_reversal_adjustments_pir_id - ON payment_initiation_reversal_adjustments (payment_initiation_reversal_id); + CREATE INDEX CONCURRENTLY pir_adjustments_pir_id + ON payment_initiation_reversal_adjustments (payment_initiation_reversal_id);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.junie/guidelines.md(1 hunks)internal/storage/migrations/17-add-foreign-key-indices.go(1 hunks)internal/storage/migrations/migrations.go(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: laouji
PR: formancehq/payments#392
File: docs/api/README.md:910-960
Timestamp: 2025-03-31T13:59:49.115Z
Learning: Files under the `docs/` directory in the formancehq/payments repository are auto-generated and don't require code review.
Learnt from: paul-nicolas
PR: formancehq/payments#245
File: internal/storage/migrations/v2_migrations_test.sql:69-81
Timestamp: 2025-01-08T10:10:24.758Z
Learning: The SQL file `internal/storage/migrations/v2_migrations_test.sql` is used for testing migrations from v2 to v3 payments and should replicate the exact v2 schema without modifications.
.junie/guidelines.md (1)
Learnt from: laouji
PR: formancehq/payments#392
File: docs/api/README.md:910-960
Timestamp: 2025-03-31T13:59:49.115Z
Learning: Files under the `docs/` directory in the formancehq/payments repository are auto-generated and don't require code review.
🧬 Code Graph Analysis (1)
internal/storage/migrations/migrations.go (1)
internal/storage/migrations/17-add-foreign-key-indices.go (1)
AddForeignKeyIndices(10-238)
🔇 Additional comments (3)
internal/storage/migrations/migrations.go (1)
230-238: LGTM - Migration registration follows established patternThe migration registration correctly follows the established logging and error handling pattern. The notable difference from other migrations is that it doesn't use
db.RunInTx(), which is appropriate sinceCREATE INDEX CONCURRENTLYstatements cannot be executed within transactions in PostgreSQL.internal/storage/migrations/17-add-foreign-key-indices.go (1)
21-238: LGTM - Well-structured concurrent index creationThe implementation correctly uses
CREATE INDEX CONCURRENTLYwhich is appropriate for production environments as it doesn't block table access during index creation. The error handling is appropriate, returning immediately on any failure..junie/guidelines.md (1)
1-79: Documentation looks comprehensive but unrelated to PR objectivesThis appears to be a well-structured project guidelines document that would be helpful for developers. However, it seems unrelated to the main PR objective of adding missing indices (PMNT-107).
Is this documentation intentionally included in this PR, or should it be part of a separate documentation update?
fguery
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #492 +/- ##
==========================================
- Coverage 69.65% 69.56% -0.09%
==========================================
Files 627 628 +1
Lines 32208 32401 +193
==========================================
+ Hits 22433 22539 +106
- Misses 8560 8618 +58
- Partials 1215 1244 +29 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| @@ -0,0 +1,78 @@ | |||
| # Formance Payments Project Guidelines | |||
|
FYI I tried creating a migration test for this, but the test fails -- I think as we have "concurrently" in the query we'd have to wait/sleep, which I"m not super keen to do in a test. Testing manually I did get the indices created though. Also do we have some dashboards ready in prod to check the impact? I imagine it won't be massive as 99% of the time spent in Payment is probably waiting for PSP calls, but still. |
|
|
||
| // Accounts | ||
| _, err := db.ExecContext(ctx, ` | ||
| CREATE INDEX CONCURRENTLY accounts_connector_id ON accounts (connector_id); |
There was a problem hiding this comment.
We must add a IF NOT EXISTS if possible
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
internal/storage/migrations/17-add-foreign-key-indices.go (5)
23-28: Add IF NOT EXISTS clause to prevent migration failures.Following the past review comment suggestion, add
IF NOT EXISTSto prevent the migration from failing if indices already exist.- _, err = db.ExecContext(ctx, ` - CREATE INDEX CONCURRENTLY balances_account_id ON balances (account_id); - `) + _, err = db.ExecContext(ctx, ` + CREATE INDEX CONCURRENTLY IF NOT EXISTS balances_account_id ON balances (account_id); + `)
29-34: Add IF NOT EXISTS clause to prevent migration failures.- _, err = db.ExecContext(ctx, ` - CREATE INDEX CONCURRENTLY balances_connector_id ON balances (connector_id); - `) + _, err = db.ExecContext(ctx, ` + CREATE INDEX CONCURRENTLY IF NOT EXISTS balances_connector_id ON balances (connector_id); + `)
37-42: Add IF NOT EXISTS clause to prevent migration failures.- _, err = db.ExecContext(ctx, ` - CREATE INDEX CONCURRENTLY bank_accounts_psu_id ON bank_accounts (psu_id); - `) + _, err = db.ExecContext(ctx, ` + CREATE INDEX CONCURRENTLY IF NOT EXISTS bank_accounts_psu_id ON bank_accounts (psu_id); + `)
45-62: Add IF NOT EXISTS clause to prevent migration failures.- _, err = db.ExecContext(ctx, ` - CREATE INDEX CONCURRENTLY bank_accounts_related_accounts_bank_account_id ON bank_accounts_related_accounts (bank_account_id); - `) + _, err = db.ExecContext(ctx, ` + CREATE INDEX CONCURRENTLY IF NOT EXISTS bank_accounts_related_accounts_bank_account_id ON bank_accounts_related_accounts (bank_account_id); + `)Apply similar changes to the other two index creation statements in this block (lines 51-56 and 57-62).
65-230: Apply IF NOT EXISTS clause to all remaining index creation statements.All remaining
CREATE INDEX CONCURRENTLYstatements should includeIF NOT EXISTSto prevent migration failures when indices already exist.This pattern should be applied consistently to all remaining index creation statements from lines 65-230. Each statement should follow the format:
CREATE INDEX CONCURRENTLY IF NOT EXISTS index_name ON table_name (column_name);
🧹 Nitpick comments (1)
internal/storage/migrations/17-add-foreign-key-indices.go (1)
9-13: Consider documenting the performance impact of concurrent index creation.Given the PR author's concern about monitoring performance impact and the use of
CREATE INDEX CONCURRENTLY, consider adding a comment about the expected behavior and any potential performance implications.// AddForeignKeyIndices is adding a lot of missing indices; note that some indices created here are not mapping 1:1 to "real" foreign key constrains +// This migration uses CREATE INDEX CONCURRENTLY to minimize table locking during index creation. +// Note: CONCURRENTLY cannot be used within a transaction block. func AddForeignKeyIndices(ctx context.Context, db bun.IDB) error {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/storage/migrations/17-add-foreign-key-indices.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: laouji
PR: formancehq/payments#392
File: docs/api/README.md:910-960
Timestamp: 2025-03-31T13:59:49.115Z
Learning: Files under the `docs/` directory in the formancehq/payments repository are auto-generated and don't require code review.
Learnt from: paul-nicolas
PR: formancehq/payments#245
File: internal/storage/migrations/v2_migrations_test.sql:69-81
Timestamp: 2025-01-08T10:10:24.758Z
Learning: The SQL file `internal/storage/migrations/v2_migrations_test.sql` is used for testing migrations from v2 to v3 payments and should replicate the exact v2 schema without modifications.
⏰ 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). (1)
- GitHub Check: Dirty
| _, err := db.ExecContext(ctx, ` | ||
| DROP INDEX accounts_connector_id;CREATE INDEX CONCURRENTLYON accounts (connector_id); | ||
| `) | ||
| if err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
Fix critical syntax error in index creation.
There's a missing space between CONCURRENTLY and ON in the SQL statement, which will cause the migration to fail.
- _, err := db.ExecContext(ctx, `
- DROP INDEX accounts_connector_id;CREATE INDEX CONCURRENTLYON accounts (connector_id);
- `)
+ _, err := db.ExecContext(ctx, `
+ DROP INDEX IF EXISTS accounts_connector_id;
+ CREATE INDEX CONCURRENTLY accounts_connector_id ON accounts (connector_id);
+ `)Also addresses the missing IF EXISTS clause for the DROP INDEX operation to prevent errors if the index doesn't exist.
📝 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.
| _, err := db.ExecContext(ctx, ` | |
| DROP INDEX accounts_connector_id;CREATE INDEX CONCURRENTLYON accounts (connector_id); | |
| `) | |
| if err != nil { | |
| return err | |
| } | |
| _, err := db.ExecContext(ctx, ` | |
| DROP INDEX IF EXISTS accounts_connector_id; | |
| CREATE INDEX CONCURRENTLY accounts_connector_id ON accounts (connector_id); | |
| `) | |
| if err != nil { | |
| return err | |
| } |
🤖 Prompt for AI Agents
In internal/storage/migrations/17-add-foreign-key-indices.go around lines 15 to
20, fix the SQL syntax by adding a space between CONCURRENTLY and ON in the
CREATE INDEX statement. Also, modify the DROP INDEX statement to include IF
EXISTS to avoid errors if the index does not exist. Ensure the SQL commands are
properly separated and formatted for correct execution.
dcac92f to
659807a
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Bug: Concurrent Index Creation Fails on Re-runs
The migration uses CREATE INDEX CONCURRENTLY statements without ensuring idempotency. Since IF NOT EXISTS is not supported with CONCURRENTLY in PostgreSQL, these statements will cause the migration to fail if executed when the indices already exist (e.g., on re-runs or after partial failures). This lack of idempotency was explicitly raised as a concern during the PR discussion.
internal/storage/migrations/17-add-foreign-key-indices.go#L14-L232
payments/internal/storage/migrations/17-add-foreign-key-indices.go
Lines 14 to 232 in 659807a
Was this report helpful? Give feedback by reacting with 👍 or 👎
* fix(PMNT-107): added missing indices * fix(CONN-107): create index only if they don't exist * doc: remove TODO, created ticket CONN-108 * fix: remove 'if not exists' to prevent silent failures
No description provided.