Skip to content

[TEMP] Don't make /stats use indexes #1173

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

Merged
skyfallwastaken merged 1 commit into from
Apr 15, 2026
Merged

[TEMP] Don't make /stats use indexes #1173

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
10 changes: 10 additions & 0 deletions app/controllers/api/v1/stats_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
class Api::V1::StatsController < ApplicationController
before_action :ensure_authenticated!, only: [ :show ], unless: -> { Rails.env.development? }
before_action :set_user, only: [ :user_stats, :user_spans, :user_projects, :user_project, :user_projects_details ]
around_action :disable_index_scans_for_stats, only: [ :show, :user_stats ]

def show
# take either user_id with a start date & end date
Expand Down Expand Up @@ -224,6 +225,15 @@ def user_projects_details

private

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")
Comment on lines +229 to +232
Copy link

Copilot AI Apr 15, 2026

Choose a reason for hiding this comment

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

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.

Suggested change
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")

Copilot uses AI. Check for mistakes.
yield
end
Comment on lines +229 to +234
Copy link

Copilot AI Apr 15, 2026

Choose a reason for hiding this comment

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

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).

Suggested change
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

Copilot uses AI. Check for mistakes.
Comment on lines +229 to +234
Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps bot Apr 15, 2026

Choose a reason for hiding this comment

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

P2 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.

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.

end
Comment on lines +228 to +235
Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps bot Apr 15, 2026

Choose a reason for hiding this comment

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

P2 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.

Suggested change
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!


Comment on lines +229 to +236
Copy link

Copilot AI Apr 15, 2026

Choose a reason for hiding this comment

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

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.

Suggested change
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

Copilot uses AI. Check for mistakes.
def set_user
token = request.headers["Authorization"]&.split(" ")&.last
identifier = params[:username] || params[:username_or_id] || params[:user_id]
Expand Down
Loading