Repository navigation
Conversation
Episode catalog entries are refreshed in place by triggers on episodes, media files, memberships and series. The refresh rewrote all 30 stored columns on every call, even when the values matched, and each rewrite adds a row version and an entry in most of the table's 30 indexes. Two sources of those no-op rewrites: - The episodes trigger counted still_path and still_thumbhash as catalog changes, but entries store no still: catalog reads join the still from episodes. Every still cache write refreshed an entry. - Changes to watched file or series columns that the entry doesn't surface (a re-probed video track, a series runtime masked by the episode's own) still rewrote the whole entry. The refresh UPDATE now only writes when a stored value differs, or a search document is missing. When it finds nothing to change, the loop exits if the entry already holds the refreshed values instead of falling through to the INSERT. The episodes trigger no longer watches the still columns. Down restores the previous definitions.
Silo Kody — review completeReview finished. Check the inline comments for findings and verify each suggestion against the code and tests. Reviewing changes in Silo
Review settingsReview OptionsThe following review options are enabled or disabled:
|
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 13 minutes. View limit detailsLimit details: You’ve used all 4 included reviews currently available. Review configuration: ⚙️ Run configuration
📒 Files selected for processing (4)
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 |
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.
Problem
Related issue: N/A
Validation tasks: none affected. Catalog results don't change, only how often their rows are rewritten.
Episode catalog entries get rewritten even when nothing in them changed, and on my server those rewrites are about a quarter of all WAL.
episode_catalog_entriesis refreshed in place by triggers on episodes, media files, memberships and series. The refresh UPDATE assigned all 30 stored columns on every call with no change check. Each rewrite adds a row version plus entries in most of the table's 30 indexes (14% of its updates are HOT).Two things make many of those rewrites no-ops:
still_pathandstill_thumbhashas catalog changes, but entries store no still. Catalog reads join the still fromepisodes(episode_catalog_source.go). Every still cache write therefore refreshes the entry for nothing.video_trackswith the same HDR and codec facts, or a series runtime that an episode's own runtime masks.This makes the refresh write only when a stored value differs, and stops still edits from triggering it.
Approach
One migration, which replaces the definitions from
20260930130212:refresh_episode_catalog_entry's in-place UPDATE now hasROW(<30 stored columns>) IS DISTINCT FROM ROW(<src values>)in itsWHERE. It still rewrites an entry whose search documents are NULL, so the BEFORE UPDATE trigger keeps repairing them. When the UPDATE finds nothing to change, the loop exits if the entry already holds the refreshed values, instead of falling through to theINSERT ... ON CONFLICT DO NOTHING, which would otherwise spin. A missing entry still gets inserted. An entry another session inserted with different values is still retried and updated.episode_catalog_entries_episodes_triggerandtrg_episode_catalog_entries_episodesdropstill_pathandstill_thumbhashfrom the change check and fromUPDATE OF.updated_aton an entry now only moves when the entry changes. Nothing in the server reads it.Validation
TestSkipUnchangedEpisodeCatalogRefreshMigrationPostgresruns the real functions in an isolated schema. It applies20260930130212, then this migration's Up, Down and Up again, and counts refresh calls and entry writes with transaction-local statistics. With the previous definitions (the predecessor and Down stages) a still edit refreshes and rewrites the entry, and so do an unchanged refresh and a masked series runtime. After Up they write nothing. A file edit, a series genre change, a deleted entry and a missing search document still write once each, and the entry matches its sources at every stage. Added toscripts/ci/db-pins.txt.TestEpisodeSearchRefreshWorkPostgres,TestEpisodeSearchRefreshMigrationPostgresand the other episode catalog and search tests in./internal/catalogpass against a database migrated with this branch.make migrate-validatepasses.gofmtandgo vet ./migrations/are clean. golangci-lint with gocritic on./migrations/, filtered to changed lines, reports 0 issues. (make lint-changedlints the whole tree whenever a.sqlfile undermigrations/changes, which CI's Go lint job covers.)Benchmarks
Baseline
mainat ca186fe, changed this branch. PostgreSQL 18.6 with pgvector 0.8.7 on an Apple Silicon Mac,shared_buffers512 MB, C locale. I built one database migrated tomainand one to this branch, each seeded with the same library: 20 series of 500 episodes, one file each, so 10,000 entries. Each workload ran as one transaction. WAL ispg_current_wal_insert_lsn()before and after, entry updates arepg_stat_xact_user_tables.n_tup_updforepisode_catalog_entries, and time is wall time for the statement. There was aCHECKPOINTbetween runs, and each workload ran three times in the order shown.main: entry updates / WAL / timevideo_tracksrewritten, same facts)The WAL left on this branch is the
episodesandmedia_filesrows themselves and, for stills, the artwork GC queue. After those three workloads the entries table was 125 MB onmainand 45 MB on this branch.A change that does reach the entries costs the same. I ran it first, on freshly seeded databases, so neither table was bloated:
mainOn my server (read-only
pg_stat_statements, about three days since a stats reset): the entry refresh UPDATE ran 176,249 times and wrote 3.1 GB of WAL, about a quarter of the cluster's 12 GB in that window. The still cache'sUPDATE episodes SET still_path ...ran 26,513 times, and each one refreshed an entry. 527 series-level refreshes rewrote 125,723 entries. I can't tell from the statistics how many of those rewrites changed nothing.Limitations: synthetic data on one machine. The production figures are the before side only, because measuring after would need a deploy.
Evidence
No user-visible change: catalog and search results are the same, and the tests check entries against their sources at every stage. The raw output behind Benchmarks and Validation:
My server, read-only statistics
Local benchmark, raw rows
Tests
Risks
20260930130212exactly.episode_catalog_entries.updated_atto move on every refresh would see it move less often. I found no reader of that column outside one test, which sets it directly.Checklist
AI Disclosure
updated_atand the still columns, an exact Down, and whether the test fails on the old behaviour. It found no defects. On its suggestions I added a test stage for a refresh repairing a missing search document (checked by removing that clause, which makes the test fail), a statement timeout so a broken retry loop fails the test instead of hanging it, a comment tying the compared columns to theSETlist, and a sentence indocs/architecture/postgres-search.md.AI-assisted with Claude Opus. I directed the task and designed the work.