perf(backend): mirror ROM filter values into a narrow trigger-maintained table#3885
Open
gantoine wants to merge 1 commit into
Open
perf(backend): mirror ROM filter values into a narrow trigger-maintained table#3885gantoine wants to merge 1 commit into
gantoine wants to merge 1 commit into
Conversation
…ned table Building the filter dropdowns aggregated the facet columns straight off `roms`. Those columns are cheap to compute since #3826, but they sit inline with the raw provider-metadata blobs, so the aggregation read every row of a multi-gigabyte table: seconds of pure disk reading on any database whose buffer pool is smaller than the table (the stock 128-256 MB defaults always are on a large library). `roms_facets` holds only the filter values and is kept in sync by AFTER INSERT/UPDATE triggers on `roms`, so every write path stays consistent without an application hook. Deletes ride the ON DELETE CASCADE foreign key. Fixes #3882 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
Greptile SummaryThis PR moves ROM filter aggregation to a narrow database-maintained mirror table. The main changes are:
Confidence Score: 4/5The migration-time synchronization gap needs to be fixed before merging.
backend/alembic/versions/0100_roms_facets_table.py Important Files Changed
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
backend/alembic/versions/0100_roms_facets_table.py:124
**Writes Can Escape Initial Sync**
A ROM inserted or updated after this backfill but before the triggers are installed is not captured by either operation. Its facet row stays missing or stale until another update occurs, so the new filter queries silently omit its values; trigger installation and the backfill need an ordering that cannot lose concurrent writes.
Reviews (1): Last reviewed commit: "perf(backend): mirror ROM filter values ..." | Re-trigger Greptile |
| ) | ||
| op.create_index("idx_roms_facets_platform_id", "roms_facets", ["platform_id"]) | ||
|
|
||
| op.execute(_BACKFILL_SQL) |
Contributor
There was a problem hiding this comment.
Writes Can Escape Initial Sync
A ROM inserted or updated after this backfill but before the triggers are installed is not captured by either operation. Its facet row stays missing or stale until another update occurs, so the new filter queries silently omit its values; trigger installation and the backfill need an ordering that cannot lose concurrent writes.
Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/alembic/versions/0100_roms_facets_table.py
Line: 124
Comment:
**Writes Can Escape Initial Sync**
A ROM inserted or updated after this backfill but before the triggers are installed is not captured by either operation. Its facet row stays missing or stale until another update occurs, so the new filter queries silently omit its values; trigger installation and the backfill need an ordering that cannot lose concurrent writes.
How can I resolve this? If you propose a fix, please make it concise.
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
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.
Description
Fixes #3882.
Building the search/gallery filter dropdowns aggregated the facet columns straight off
roms. Those columns are cheap to compute since #3826, but they live inline with the raw provider-metadata blobs, so the aggregation still had to read every row of a multi-gigabyte table. On the reporter's ~92k-game library that is ~5.3s, identical cold and warm, i.e. purely disk-read bound: the stockinnodb_buffer_pool_sizedefaults (128 MB on the officialmariadbimage, 256 MB on LinuxServer.io) are far below the ~1 GBromstable, and it recurs after every scan because the cached values are invalidated.This adds
roms_facets, a narrow table holding only the per-ROM filter values (a few MB for the same library):0100creates the table (rom_idPK →roms.idON DELETE CASCADE,platform_id+ the 10 facet columns, index onplatform_id), backfills it with a singleINSERT ... SELECT, and installs the sync triggers:roms_facets_after_insert/roms_facets_after_updateon MySQL/MariaDB (upsert viaON DUPLICATE KEY UPDATE) andromm_sync_rom_facets()+roms_facets_syncon PostgreSQL (ON CONFLICT DO UPDATE). Deletes ride the FK cascade.downgrade()drops the triggers, function, and table.update(), scan) stays consistent without a hook to remember. Stored generated columns are confirmed readable fromNEW.*in anAFTERtrigger on both engines.with_filter_values/get_rom_filtersnow share one select overRomFacets;romsno longer appears in the facet query at all, only in the id subquery.Verified on MariaDB 11.3 and PostgreSQL 16: insert/update/delete sync, and a downgrade → re-upgrade round trip backfills pre-existing rows.
EXPLAINon the new query reachesromsonly through a covering secondary index (Using index) for the id list, theneq_refPK lookups into the narrow table, so the wide rows are never scanned.Two caveats: I could not reproduce the reporter's ~90k-game timing locally, so the win is argued from the query plan and table width rather than a measured 5.3s → sub-second on real data. And the separate full ordered rom-id scan that virtual scrolling needs (~5.1s in the report) is untouched, as the issue scopes it out.
Checklist
Screenshots (if applicable)
n/a - backend only, no user-visible UI change.
AI assistance disclosure: this change was written with AI assistance (Claude Code). The approach was chosen by a human; the model wrote the migration, model, handler change, and tests, and ran the verification described above. All of it has been reviewed by a human before submission.