[Ledger] Fix N+1 rendering and a pathological query plan on the ledger page#14357
Open
garyhtou wants to merge 5 commits into
Open
[Ledger] Fix N+1 rendering and a pathological query plan on the ledger page#14357garyhtou wants to merge 5 commits into
garyhtou wants to merge 5 commits into
Conversation
…n ledger pages Every render of the ledger item list (app/views/ledger/_item.html.erb, shared by the event ledger page, card grant ledger pages, and LedgersController#show) N+1'd two things per row: - item.author: belongs_to, never preloaded. - item.icon, for Disbursement/Donation/CardCharge rows: touches linked_object's own associations (card_grant, recurring_donation, raw_stripe_transactions, raw_pending_stripe_transaction) that the existing `.preload(:linked_object)` only preloads the polymorphic association itself, not those nested ones. Ledger::Query#execute is the single method all four of those call sites funnel through, so the preload is added there once rather than duplicated per controller. Rails preloads each named association only on the linked_object records whose class actually has it, so listing all four together is safe even though no single row has all of them. Benchmarked rendering a 25-item page (real card-charge-heavy pages, before -> after): - Hack Club HQ: 668ms / 115 queries -> 32ms / 0 queries - HCB Operations: 229ms / 109 queries -> 18ms / 0 queries - Org 5516: 185ms / 114 queries -> 25ms / 0 queries Added a query-count regression test in query_spec.rb; verified it fails against the pre-fix preload (2 extra queries) and passes with this fix (0). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… list set_ledger_filters built @users (the ledger page's author-filter dropdown) as User.where(id: subquery_a).or(User.where(id: subquery_b)). On at least one large ledger, Postgres chose to evaluate both subqueries as a hashed SubPlan filter under a full sequential scan of the entire users table (73,978 rows) and a full sequential scan of ledger_items (607,801 rows, across every event) to build the filter, rather than using the ledger_mappings index to narrow down first - observed at ~360ms in the worst case, confirmed via EXPLAIN. Replaced with two plain id lookups (Ledger::Item...distinct.pluck and event.users.pluck) merged in Ruby, then a single indexed User.where(id: [...]). This is a robustness fix more than a guaranteed speedup: on this dataset the wall-clock difference across several orgs is mostly modest and sometimes negligible (a query planner already choosing a reasonable plan doesn't get much faster from being asked more simply), but it removes the .or()-with-two- subqueries shape that produced the pathological full-table-scan plan in the first place, so it can't regress that way again as ledgers grow. Verified identical result sets (same user ids) before and after on several ledgers. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
garyhtou
marked this pull request as ready for review
July 19, 2026 00:16
…lper The N+1 regression test defined its own query-counting helper, which duplicated the query_count gem already in the Gemfile (QueryCount::Counter, already attached to ActiveRecord notifications via its railtie and already used elsewhere in the app for the debug footer's query count). Switched to QueryCount::Counter.reset_counter/.counter instead. Same behavior (it also excludes SCHEMA queries, plus EXPLAIN); verified the test still fails against the pre-fix preload and passes with it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
query_count was a dev-widget for the app's debug footer (already
removed from the Gemfile on main), not a test tool - it happened to
share a name with the helper I'd written, which is what led me to it
in the first place. A Rails-testing-focused review flagged this: the
N+1 regression test should use a real RSpec query-count matcher.
Added db-query-matchers (test group only) and a spec/support file
configuring it to exclude schema-introspection queries, matching
Rails' own assert_queries_count default. Rewrote the preload
regression test as `expect { ... }.not_to make_database_queries`,
which also gives a more useful failure message (prints the actual
queries). Verified it still fails against the pre-fix preload and
passes with it.
Also documented the test's actual coverage boundary: it exercises the
CardCharge branch of the preload (a has_many-through plus a
belongs_to); Disbursement's card_grant and Donation's
recurring_donation share the same preload directive but aren't
separately exercised (Disbursement's Outgoing/Incoming split isn't
plain STI, and building that fixture wasn't worth it for coverage the
CardCharge case already substantiates for the underlying mechanism).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
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
1 participant
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.
Note
This PR was written by Claude Code. Gary directed the investigation and reviewed the changes, but the diagnosis, code, and benchmarks below were produced by Claude.
Summary of the problem
Two separate issues found auditing the Ledger page for the "every Ledger page under 4 seconds" goal.
1. N+1 on every ledger item row.
app/views/ledger/_item.html.erb— the partial shared by the event ledger page, card grant ledger pages, andLedgersController#show— callsitem.author(never preloaded) anditem.icon, which forDisbursement/Donation/CardChargerows toucheslinked_object's own associations (card_grant,recurring_donation,raw_stripe_transactions,raw_pending_stripe_transaction). The existing.preload(:linked_object)inLedger::Query#executeonly preloads the polymorphic association itself, not those nested ones.2. A pathological query plan for the author-filter dropdown.
set_ledger_filtersbuilt@usersasUser.where(id: subquery_a).or(User.where(id: subquery_b)). On a large ledger, Postgres chose to evaluate this as a hashed SubPlan filter under a full sequential scan of the entireuserstable and a full sequential scan ofledger_itemsacross every event, rather than using theledger_mappingsindex to narrow down first.Describe your changes
Commit 1. Added the missing associations to
Ledger::Query#execute's preload —.preload(:author, linked_object: [:card_grant, :recurring_donation, :raw_stripe_transactions, :raw_pending_stripe_transaction]). Fixed centrally here since all four Ledger-rendering call sites funnel through this one method. Rails only preloads a named association on the linked_object records whose class actually has it, so this is safe even though no single row has all four associations.Commit 2. Replaced the
.or()-of-two-subqueries pattern with two plain id lookups merged in Ruby, then one indexedUser.where(id: [...]). Framing this honestly: this is a robustness fix more than a guaranteed speedup — verified identical result sets before/after, but the wall-clock difference across several orgs on this dataset was mostly modest and sometimes negligible. What it does fix is the query shape that produced a full-table-scan plan in the first place, so it can't regress that way again as ledgers grow, regardless of what the planner decides to do with the old pattern on any given day.Benchmarks
Rendering a 25-item ledger page (real card-charge-heavy pages), before → after:
org_lbu5qR@usersquery, before → after (identical result sets confirmed on every org tested):org_lbu5qRTests
Added a query-count regression test to
spec/models/ledger/query_spec.rbfor the preload fix, using db-query-matchers (added to the test group):expect { ... }.not_to make_database_queries, covering the CardCharge branch of the preload (a has_many-through plus a belongs_to). Verified it fails against the pre-fix code (2 extra queries) and passes with this fix (0). Disbursement'scard_grantand Donation'srecurring_donationshare the same preload directive but aren't separately exercised.The
@usersfix is a pure query-shape change with an identical result set, verified directly rather than via a new spec (existing coverage inevents_controller_spec.rb/card_grants_controller_spec.rbalready exercises the code path this touches).