fix: add composite index for pr_creators DISTINCT ON query timeout#69
Open
rnetser wants to merge 8 commits intomyk-org:mainfrom
Open
fix: add composite index for pr_creators DISTINCT ON query timeout#69rnetser wants to merge 8 commits intomyk-org:mainfrom
rnetser wants to merge 8 commits intomyk-org:mainfrom
Conversation
The contributors endpoint timed out (>60s) due to missing index for DISTINCT ON (repository, pr_number) ORDER BY created_at pattern. Adds partial composite index on webhooks(repository, pr_number, created_at) WHERE pr_number IS NOT NULL. Closes myk-org#68
Collaborator
|
Report bugs in Issues Welcome! 🎉This pull request will be automatically processed with the following features: 🔄 Automatic Actions
📋 Available CommandsPR Status Management
Review & Approval
Testing & Validation
Container Operations
Cherry-pick Operations
Label Management
✅ Merge RequirementsThis PR will be automatically approved when the following conditions are met:
📊 Review ProcessApprovers and ReviewersApprovers: Reviewers: Available Labels
💡 Tips
For more information, please refer to the project documentation or contact the maintainers. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a partial composite index on Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/migrations/versions/20260219_0001_add_pr_creators_index.py`:
- Around line 44-46: The downgrade currently calls
op.drop_index("ix_webhooks_repo_pr_number_created_at", table_name="webhooks")
which issues a normal DROP INDEX inside a transaction; change downgrade() to run
a DROP INDEX CONCURRENTLY for the same index name and do it outside a
transaction (autocommit). Concretely, replace op.drop_index(...) in downgrade
with executing the raw SQL "DROP INDEX CONCURRENTLY
ix_webhooks_repo_pr_number_created_at" using the connection returned by
op.get_bind() with execution/autocommit enabled (e.g., via
conn.execution_options(isolation_level="AUTOCOMMIT") or using a direct
connection.execute) so the index is dropped concurrently like the upgrade
created it.
In `@tests/test_models.py`:
- Around line 28-54: Both tests duplicate the same loop to find an Index in
Webhook.__table_args__; extract a shared helper (e.g., a static method named
_find_index) that takes a name: str and returns the matching Index or
raises/asserts if not found, then replace the loop in
test_composite_index_columns and test_composite_index_has_partial_where_clause
with calls to _find_index(INDEX_NAME) and use the returned target_index for
assertions; reference Webhook.__table_args__, Index, INDEX_NAME, and the test
functions when making the change.
- Around line 125-147: Both tests currently assert against mock_bind.execute but
the migration uses an autocommit connection returned by
mock_bind.execution_options(...); update test_upgrade_targets_webhooks_table and
test_upgrade_index_column_order to grab the autocommit connection (e.g.,
autocommit_conn = mock_bind.execution_options.return_value) after patching
migration.op.get_bind and assert on autocommit_conn.execute.call_args
(extracting .text and .strip() the same way) instead of mock_bind.execute so the
assertions target the actual executed SQL.
- Around line 108-123: The test test_upgrade_creates_correct_index is asserting
execute() on the original mock_bind instead of the autocommit connection
returned by mock_bind.execution_options(), which hides the real migration bug;
change the assertion to capture the autocommit mock (the return value of
mock_bind.execution_options) and assert that its execute() was called and
inspect its call_args to get the executed SQL (replace references to
mock_bind.execute with the autocommit_conn.execute call and validate sql_text
from that call), while still asserting
mock_bind.execution_options.assert_called_once_with(isolation_level="AUTOCOMMIT").
backend/migrations/versions/20260219_0001_add_pr_creators_index.py
Outdated
Show resolved
Hide resolved
- Use DROP INDEX CONCURRENTLY in migration downgrade for consistency - Extract _find_index helper to reduce test duplication - Update downgrade test for CONCURRENTLY pattern
Contributor
Author
|
/build-and-push-container |
Collaborator
|
New container for ghcr.io/myk-org/github-metrics:pr-69 published |
…Y DDL Alembic's env.py wraps migrations in context.begin_transaction(). SQLAlchemy 2.0 rejects changing isolation_level on a connection with an active transaction, causing InvalidRequestError in production. Fix by committing Alembic's active transaction before running CREATE/DROP INDEX CONCURRENTLY, which cannot run inside a transaction.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/test_models.py`:
- Around line 103-145: Extract the repeated migration execution + SQL extraction
into a single helper (e.g., _run_migration_and_get_executed_sql) that accepts a
migration instance and optionally the execute index/expected call count; have it
create the MagicMock, patch migration.op.get_bind, call migration.upgrade(),
assert mock_bind.execute.call_count == expected (default 2), and return
str(mock_bind.execute.call_args_list[desired_index][0][0].text).strip(); then
update test_upgrade_creates_correct_index, test_upgrade_targets_webhooks_table,
and test_upgrade_index_column_order to call this helper and use its returned
executed_sql instead of duplicating the setup and extraction logic.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
backend/migrations/versions/20260219_0001_add_pr_creators_index.pytests/test_models.py
Extract _run_upgrade_and_get_sql() and _run_downgrade_and_get_sql() helpers to eliminate duplicated mock setup, COMMIT verification, and DDL extraction across four migration tests.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/test_models.py`:
- Line 28: The AssertionError message in tests/test_models.py is too long per
TRY003; update the raise in the check that verifies index presence (the line
raising AssertionError(f"Index {name} not found in __table_args__")) to use a
much shorter message such as "Index not found" (the variable name "name" is
already the argument and provides context), i.e., replace the long formatted
string with a concise literal while leaving the assertion logic and the symbol
name unchanged.
Contributor
Author
|
/build-and-push-container |
Collaborator
|
New container for ghcr.io/myk-org/github-metrics:pr-69 published |
Contributor
Author
|
/verified |
myakove-bot
added
lgtm-coderabbitai[bot]
commented-coderabbitai[bot]
and removed
changes-requested-coderabbitai[bot]
labels
- Split test file into per-module files (test_models.py + test_migration_add_pr_creators_index.py) - Remove brittle index count test - Unify duplicate upgrade/downgrade test helpers - Add DROP INDEX before CREATE to handle invalid indexes from failed CONCURRENTLY builds - Add BEGIN after CONCURRENTLY DDL to restore Alembic transaction context - Use Literal type for migration action parameter - Use more robust alembic.op.get_bind patching
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/test_migration_add_pr_creators_index.py`:
- Around line 85-101: The test suite doesn't verify the defensive DROP executed
before creating the index; add an assertion in the tests (e.g., in
test_upgrade_creates_correct_index or a new test) that the executed SQL returned
by _run_upgrade_and_get_sql contains the DROP statement for INDEX_NAME with
"DROP INDEX CONCURRENTLY IF EXISTS" and that this DROP appears before the CREATE
(compare positions of the "DROP INDEX CONCURRENTLY IF EXISTS {INDEX_NAME}"
substring and the "CREATE INDEX" substring in executed_sql) to ensure the
defensive cleanup remains present and ordered correctly.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
backend/migrations/versions/20260219_0001_add_pr_creators_index.pytests/test_migration_add_pr_creators_index.pytests/test_models.py
Contributor
Author
|
build-and-push-container |
Contributor
Author
|
/build-and-push-container: |
Contributor
Author
|
/build-and-push-container |
Collaborator
|
New container for ghcr.io/myk-org/github-metrics:pr-69 published |
Contributor
Author
|
/verified |
Contributor
Author
|
all requested changes were addressed; approve the pr |
|
✅ Migration & Index: Clean Alembic migration with Approving the PR now! 🧠 Learnings usedIf you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! ✅ Actions performedComments resolved and changes approved. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ix_webhooks_repo_pr_number_created_atonwebhooks(repository, pr_number, created_at ASC) WHERE pr_number IS NOT NULLTimeoutErroron/api/metrics/contributorsendpoint caused by full table sort forDISTINCT ONqueriesCREATE INDEX CONCURRENTLY IF NOT EXISTSfor production safety (no table locks, idempotent)Test plan
tests/test_models.pycovering migration revision chain, index definition, and model consistencycnv-tests-covnamespace)Closes #68
Summary by CodeRabbit
Chores
Tests