[TEMP] Don't make /stats use indexes#1173
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a temporary controller-level mechanism to force PostgreSQL to avoid using indexes for selected stats endpoints, presumably to influence query plans for /api/v1/stats.
Changes:
- Adds an
around_actionforshowanduser_stats. - Implements
disable_index_scans_for_statsthat runs PostgresSET LOCAL enable_*scan = offsettings inside a transaction.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ActiveRecord::Base.connection.transaction do | ||
| ActiveRecord::Base.connection.execute("SET LOCAL enable_indexscan = off") | ||
| ActiveRecord::Base.connection.execute("SET LOCAL enable_bitmapscan = off") | ||
| ActiveRecord::Base.connection.execute("SET LOCAL enable_indexonlyscan = off") | ||
| yield | ||
| end |
There was a problem hiding this comment.
SET LOCAL only resets at the end of the outermost transaction. Because Rails tests run each example inside a transaction (use_transactional_fixtures = true), this transaction do ... SET LOCAL ... yield will typically create a savepoint, and the index-scan disabling can leak to the rest of the example (and any later queries on the same connection) until the test transaction rolls back. Consider avoiding SET LOCAL+transaction here and instead SET ... before yield and RESET ... in an ensure block (or detect open_transactions > 0 and use a reset-based approach).
| ActiveRecord::Base.connection.transaction do | |
| ActiveRecord::Base.connection.execute("SET LOCAL enable_indexscan = off") | |
| ActiveRecord::Base.connection.execute("SET LOCAL enable_bitmapscan = off") | |
| ActiveRecord::Base.connection.execute("SET LOCAL enable_indexonlyscan = off") | |
| yield | |
| end | |
| connection = ActiveRecord::Base.connection | |
| connection.execute("SET enable_indexscan = off") | |
| connection.execute("SET enable_bitmapscan = off") | |
| connection.execute("SET enable_indexonlyscan = off") | |
| yield | |
| ensure | |
| connection.execute("RESET enable_indexscan") if connection | |
| connection.execute("RESET enable_bitmapscan") if connection | |
| connection.execute("RESET enable_indexonlyscan") if connection |
| ActiveRecord::Base.connection.transaction do | ||
| ActiveRecord::Base.connection.execute("SET LOCAL enable_indexscan = off") | ||
| ActiveRecord::Base.connection.execute("SET LOCAL enable_bitmapscan = off") | ||
| ActiveRecord::Base.connection.execute("SET LOCAL enable_indexonlyscan = off") | ||
| yield | ||
| end | ||
| end | ||
|
|
There was a problem hiding this comment.
This around_action forces a database transaction for /stats and user_stats. Long-running read transactions can increase bloat by holding back vacuum and, combined with disabling index/bitmap/index-only scans, can significantly increase query time under load. Consider guarding this behind an explicit feature flag/ENV toggle and also setting a statement_timeout for the transaction (similar to other code that uses SET LOCAL statement_timeout) to prevent runaway sequential scans.
| ActiveRecord::Base.connection.transaction do | |
| ActiveRecord::Base.connection.execute("SET LOCAL enable_indexscan = off") | |
| ActiveRecord::Base.connection.execute("SET LOCAL enable_bitmapscan = off") | |
| ActiveRecord::Base.connection.execute("SET LOCAL enable_indexonlyscan = off") | |
| yield | |
| end | |
| end | |
| return yield unless stats_sequential_scan_mode_enabled? | |
| connection = ActiveRecord::Base.connection | |
| connection.transaction do | |
| connection.execute("SET LOCAL statement_timeout = #{connection.quote(stats_statement_timeout_ms.to_s)}") | |
| connection.execute("SET LOCAL enable_indexscan = off") | |
| connection.execute("SET LOCAL enable_bitmapscan = off") | |
| connection.execute("SET LOCAL enable_indexonlyscan = off") | |
| yield | |
| end | |
| end | |
| def stats_sequential_scan_mode_enabled? | |
| ActiveModel::Type::Boolean.new.cast(ENV["ENABLE_STATS_SEQUENTIAL_SCAN_MODE"]) | |
| end | |
| def stats_statement_timeout_ms | |
| timeout_ms = Integer(ENV.fetch("STATS_SEQUENTIAL_SCAN_STATEMENT_TIMEOUT_MS", "5000")) | |
| timeout_ms.positive? ? timeout_ms : 5000 | |
| rescue ArgumentError, TypeError | |
| 5000 | |
| end |
| ActiveRecord::Base.connection.transaction do | ||
| ActiveRecord::Base.connection.execute("SET LOCAL enable_indexscan = off") | ||
| ActiveRecord::Base.connection.execute("SET LOCAL enable_bitmapscan = off") | ||
| ActiveRecord::Base.connection.execute("SET LOCAL enable_indexonlyscan = off") |
There was a problem hiding this comment.
Minor maintainability: ActiveRecord::Base.connection is looked up repeatedly in this block. Assigning it to a local (e.g., conn = ActiveRecord::Base.connection) reduces repetition and makes it clearer that all commands are executed on the same connection.
| ActiveRecord::Base.connection.transaction do | |
| ActiveRecord::Base.connection.execute("SET LOCAL enable_indexscan = off") | |
| ActiveRecord::Base.connection.execute("SET LOCAL enable_bitmapscan = off") | |
| ActiveRecord::Base.connection.execute("SET LOCAL enable_indexonlyscan = off") | |
| conn = ActiveRecord::Base.connection | |
| conn.transaction do | |
| conn.execute("SET LOCAL enable_indexscan = off") | |
| conn.execute("SET LOCAL enable_bitmapscan = off") | |
| conn.execute("SET LOCAL enable_indexonlyscan = off") |
Greptile SummaryThis [TEMP] PR adds an Confidence Score: 5/5Safe to merge — the PostgreSQL SET LOCAL approach is technically correct and scoped to only the two targeted endpoints. All remaining findings are P2 (style/documentation). The SET LOCAL + transaction pattern is the standard PostgreSQL approach for per-request planner hints and is correctly implemented. The temporary nature is acknowledged in the PR title, and the approach correctly reverts at transaction end. No files require special attention beyond the missing in-code comment. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Incoming Request to /stats or /user_stats] --> B[around_action: disable_index_scans_for_stats]
B --> C[BEGIN TRANSACTION]
C --> D[SET LOCAL enable_indexscan = off]
D --> E[SET LOCAL enable_bitmapscan = off]
E --> F[SET LOCAL enable_indexonlyscan = off]
F --> G{Controller Action}
G -->|show| H[Heartbeat.where time range
query.duration_seconds]
G -->|user_stats| I[WakatimeService / TestWakatimeService
Heartbeat queries
user.streak_days]
H --> J[render response]
I --> J
J --> K[COMMIT / ROLLBACK on exception]
K --> L[SET LOCAL settings automatically reverted]
Prompt To Fix All With AIThis is a comment left during a code review.
Path: app/controllers/api/v1/stats_controller.rb
Line: 228-235
Comment:
**Missing "why" comment and removal criteria**
There's no code-level comment explaining what query planner problem prompted this, which makes it hard to know when it's safe to remove. Per the project's comment convention, a comment here should explain the root cause (e.g., a bad index on `heartbeats.time` causing the planner to prefer an expensive index scan over a seqscan) and link to an issue/PR tracking the fix so future contributors know the exit condition.
```suggestion
# TEMP: PostgreSQL's query planner is choosing an expensive index scan on
# heartbeats for large date-range aggregations. Disabling index/bitmap scans
# forces a sequential scan, which is significantly faster until the underlying
# statistics or index strategy is fixed.
# TODO: Remove once the planner issue is resolved – track in <issue link>.
def disable_index_scans_for_stats
ActiveRecord::Base.connection.transaction do
ActiveRecord::Base.connection.execute("SET LOCAL enable_indexscan = off")
ActiveRecord::Base.connection.execute("SET LOCAL enable_bitmapscan = off")
ActiveRecord::Base.connection.execute("SET LOCAL enable_indexonlyscan = off")
yield
end
end
```
**Rule Used:** What: Comments should only explain the "why" or co... ([source](https://app.greptile.com/review/custom-context?memory=27b5da63-27a1-4781-acad-c940e08169a4))
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: app/controllers/api/v1/stats_controller.rb
Line: 229-234
Comment:
**Entire action wrapped in a DB transaction**
`SET LOCAL` requires a transaction context, so the transaction is necessary — but it means every DB write inside `show` or `user_stats` (cache updates, audit rows, counter bumps, etc.) is now held open for the full duration of the action and rolled back together on any uncaught exception. For these largely read-heavy endpoints this is probably fine, but it's worth confirming that `WakatimeService#generate_summary` and `@user.streak_days` don't make DB writes that should be durable even on partial failures.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "[TEMP] Don't make /stats use indexes" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| def disable_index_scans_for_stats | ||
| ActiveRecord::Base.connection.transaction do | ||
| ActiveRecord::Base.connection.execute("SET LOCAL enable_indexscan = off") | ||
| ActiveRecord::Base.connection.execute("SET LOCAL enable_bitmapscan = off") | ||
| ActiveRecord::Base.connection.execute("SET LOCAL enable_indexonlyscan = off") | ||
| yield | ||
| end | ||
| end |
There was a problem hiding this comment.
There's no code-level comment explaining what query planner problem prompted this, which makes it hard to know when it's safe to remove. Per the project's comment convention, a comment here should explain the root cause (e.g., a bad index on heartbeats.time causing the planner to prefer an expensive index scan over a seqscan) and link to an issue/PR tracking the fix so future contributors know the exit condition.
| def disable_index_scans_for_stats | |
| ActiveRecord::Base.connection.transaction do | |
| ActiveRecord::Base.connection.execute("SET LOCAL enable_indexscan = off") | |
| ActiveRecord::Base.connection.execute("SET LOCAL enable_bitmapscan = off") | |
| ActiveRecord::Base.connection.execute("SET LOCAL enable_indexonlyscan = off") | |
| yield | |
| end | |
| end | |
| # TEMP: PostgreSQL's query planner is choosing an expensive index scan on | |
| # heartbeats for large date-range aggregations. Disabling index/bitmap scans | |
| # forces a sequential scan, which is significantly faster until the underlying | |
| # statistics or index strategy is fixed. | |
| # TODO: Remove once the planner issue is resolved – track in <issue link>. | |
| def disable_index_scans_for_stats | |
| ActiveRecord::Base.connection.transaction do | |
| ActiveRecord::Base.connection.execute("SET LOCAL enable_indexscan = off") | |
| ActiveRecord::Base.connection.execute("SET LOCAL enable_bitmapscan = off") | |
| ActiveRecord::Base.connection.execute("SET LOCAL enable_indexonlyscan = off") | |
| yield | |
| end | |
| end |
Rule Used: What: Comments should only explain the "why" or co... (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: app/controllers/api/v1/stats_controller.rb
Line: 228-235
Comment:
**Missing "why" comment and removal criteria**
There's no code-level comment explaining what query planner problem prompted this, which makes it hard to know when it's safe to remove. Per the project's comment convention, a comment here should explain the root cause (e.g., a bad index on `heartbeats.time` causing the planner to prefer an expensive index scan over a seqscan) and link to an issue/PR tracking the fix so future contributors know the exit condition.
```suggestion
# TEMP: PostgreSQL's query planner is choosing an expensive index scan on
# heartbeats for large date-range aggregations. Disabling index/bitmap scans
# forces a sequential scan, which is significantly faster until the underlying
# statistics or index strategy is fixed.
# TODO: Remove once the planner issue is resolved – track in <issue link>.
def disable_index_scans_for_stats
ActiveRecord::Base.connection.transaction do
ActiveRecord::Base.connection.execute("SET LOCAL enable_indexscan = off")
ActiveRecord::Base.connection.execute("SET LOCAL enable_bitmapscan = off")
ActiveRecord::Base.connection.execute("SET LOCAL enable_indexonlyscan = off")
yield
end
end
```
**Rule Used:** What: Comments should only explain the "why" or co... ([source](https://app.greptile.com/review/custom-context?memory=27b5da63-27a1-4781-acad-c940e08169a4))
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| ActiveRecord::Base.connection.transaction do | ||
| ActiveRecord::Base.connection.execute("SET LOCAL enable_indexscan = off") | ||
| ActiveRecord::Base.connection.execute("SET LOCAL enable_bitmapscan = off") | ||
| ActiveRecord::Base.connection.execute("SET LOCAL enable_indexonlyscan = off") | ||
| yield | ||
| end |
There was a problem hiding this comment.
SET LOCAL requires a transaction context, so the transaction is necessary — but it means every DB write inside show or user_stats (cache updates, audit rows, counter bumps, etc.) is now held open for the full duration of the action and rolled back together on any uncaught exception. For these largely read-heavy endpoints this is probably fine, but it's worth confirming that WakatimeService#generate_summary and @user.streak_days don't make DB writes that should be durable even on partial failures.
Prompt To Fix With AI
This is a comment left during a code review.
Path: app/controllers/api/v1/stats_controller.rb
Line: 229-234
Comment:
**Entire action wrapped in a DB transaction**
`SET LOCAL` requires a transaction context, so the transaction is necessary — but it means every DB write inside `show` or `user_stats` (cache updates, audit rows, counter bumps, etc.) is now held open for the full duration of the action and rolled back together on any uncaught exception. For these largely read-heavy endpoints this is probably fine, but it's worth confirming that `WakatimeService#generate_summary` and `@user.streak_days` don't make DB writes that should be durable even on partial failures.
How can I resolve this? If you propose a fix, please make it concise.This reverts commit 0e2d69e.
No description provided.