feat(db): add indexes to FK and timestamp columns across all models - #1140
feat(db): add indexes to FK and timestamp columns across all models#1140Ananya-CM wants to merge 2 commits into
Conversation
|
@Ananya-CM is attempting to deploy a commit to the rishabhmishra0510-5147's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds indexes to selected foreign-key and timestamp columns across nine SQLAlchemy models and introduces an Alembic migration that creates the corresponding indexes concurrently and removes them during downgrade. ChangesDatabase Indexing
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
backend/alembic/versions/039e8649beb5_add_indexes_to_fk_and_timestamp_columns.py (2)
21-59: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftIndex creation will lock affected tables for writes during migration.
op.create_indexwithoutpostgresql_concurrently=Truetakes a table-level lock (blocking writes) for the duration of index build on PostgreSQL. The PR's own rationale is that these tables are expected to grow, so on larger production tables this migration could cause a noticeable write-blocking window at deploy time. If any of these tables (e.g.generated_scripts,ai_audit_log) are non-trivial in size in production, considerCREATE INDEX CONCURRENTLYviaop.executewithcontext.autocommit_block(), since concurrent index creation cannot run inside a transaction.Example pattern using autocommit_block for concurrent index creation
def upgrade() -> None: with op.get_context().autocommit_block(): op.create_index( "ix_ai_sessions_diagnostic_id", "ai_sessions", ["diagnostic_id"], postgresql_concurrently=True, ) ...🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/alembic/versions/039e8649beb5_add_indexes_to_fk_and_timestamp_columns.py` around lines 21 - 59, The index creation in upgrade() uses op.create_index without concurrent index building, which can block writes on large PostgreSQL tables. Update the migration to build these indexes concurrently by using the Alembic context’s autocommit_block() in combination with postgresql_concurrently=True for the create_index calls in this migration. Keep the same index names and target tables (for example ai_audit_log, generated_scripts, and ai_sessions) but switch to the non-locking creation pattern.
23-59: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffConsider composite indexes for common FK + timestamp filter/sort combos.
Several tables get separate single-column indexes on an FK column and
created_at(e.g.ai_audit_log.session_id/created_at,verification_results.profile_id/created_at). If typical queries filter by the FK and order bycreated_attogether, a single composite index (e.g.(session_id, created_at)) would serve both needs more efficiently than two separate indexes and reduce index-maintenance overhead on writes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/alembic/versions/039e8649beb5_add_indexes_to_fk_and_timestamp_columns.py` around lines 23 - 59, Replace the separate single-column FK and created_at indexes in the Alembic migration with composite indexes for the common filter/sort pairs where queries use both together. Update the index creation in the migration that adds indexes for ai_sessions, ai_suggestions, ai_audit_log, script_generation_jobs, generated_scripts, verification_results, verification_checks, uninstall_feedbacks, and profile_packages so each relevant table uses a combined foreign-key-plus-created_at index instead of two independent ones, especially in the migration’s create_index calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@backend/alembic/versions/039e8649beb5_add_indexes_to_fk_and_timestamp_columns.py`:
- Around line 21-59: The index creation in upgrade() uses op.create_index
without concurrent index building, which can block writes on large PostgreSQL
tables. Update the migration to build these indexes concurrently by using the
Alembic context’s autocommit_block() in combination with
postgresql_concurrently=True for the create_index calls in this migration. Keep
the same index names and target tables (for example ai_audit_log,
generated_scripts, and ai_sessions) but switch to the non-locking creation
pattern.
- Around line 23-59: Replace the separate single-column FK and created_at
indexes in the Alembic migration with composite indexes for the common
filter/sort pairs where queries use both together. Update the index creation in
the migration that adds indexes for ai_sessions, ai_suggestions, ai_audit_log,
script_generation_jobs, generated_scripts, verification_results,
verification_checks, uninstall_feedbacks, and profile_packages so each relevant
table uses a combined foreign-key-plus-created_at index instead of two
independent ones, especially in the migration’s create_index calls.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7aa3d0e1-fb7e-4850-9b95-ede5208793fa
📒 Files selected for processing (6)
backend/alembic/versions/039e8649beb5_add_indexes_to_fk_and_timestamp_columns.pybackend/app/models/ai_session.pybackend/app/models/diagnostic.pybackend/app/models/feedback.pybackend/app/models/profile.pybackend/app/models/script_job.py
rishabh0510rishabh
left a comment
There was a problem hiding this comment.
@Ananya-CM Apologies for the delay! Please address the CodeRabbit findings before we can merge this PR.
|
@rishabh0510rishabh Regarding the composite indexes suggestion — kept separate single-column Migration verified locally: downgrade and upgrade both run cleanly. |
Description
Adds
index=Trueto frequently queried foreign key and timestamp columnsacross the model files, and generates the corresponding Alembic migration.
Without these indexes, queries that filter or join on FK columns (e.g.
WHERE session_id = ?) or sort bycreated_atperform full table scansas the tables grow.
Related Issues
Closes #1057
Changes Made
Model files —
index=Trueadded to FK and timestamp columns:AISessiondiagnostic_id,verification_id,profile_id,created_atAISuggestionsession_id,created_atAIAuditLogsession_id,created_atScriptGenerationJobprofile_id,created_at,completed_atGeneratedScriptjob_id,created_atVerificationResultreport_id,profile_id,created_atVerificationCheckresult_id,created_atUninstallFeedbackcreated_atProfilePackageprofile_id,created_atNote on
webhookstable: Thewebhookstable indexes are excludedfrom this migration because the
compress_generated_scriptsmigration(which creates that table) has a pre-existing type casting bug that
prevents it from running locally. The
index=Trueflags are removed fromwebhook.pyto keep the model in sync with what can be migrated. Flaggingthis for the maintainer to resolve separately.
Alembic migration:
alembic/versions/039e8649beb5_add_indexes_to_fk_and_timestamp_columns.py21fe8cc61865Verification
pytest tests/successfullySafetyFilterMigration applied successfully. All 801 backend tests pass:
801 passed, 679 warnings in 53.45s
Documentation
docs/FEATURES.mdCHANGELOG.mdScreenshots
Summary by CodeRabbit
Performance
Database