Repository navigation
fix(database): fix non-UTC log partitions and cut index and JIT overhead - #2157
Conversation
On a server whose TimeZone is not UTC, migration 028 cut the first operational_logs and activity_log partitions at local midnight while partman extends them at UTC midnight, so the next partition overlapped or left a gap and log writes landed in the default partition. Only fresh installs run 028. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The CTE body kept PostgreSQL from inlining the function, so normalize_search_text planned a subquery for every token of every title write and search. The single CASE body returns the same output, so stored columns and indexes stay valid. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… builds Six migrations ran CREATE INDEX CONCURRENTLY IF NOT EXISTS without first dropping an invalid copy, so an interrupted build left an index the planner never uses. A Go migration rebuilds whichever of them is invalid or missing with DROP and CREATE INDEX CONCURRENTLY, so serving replicas keep reading and writing those tables. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Ten indexes duplicate a primary key or unique constraint or are a leading prefix of another index on the same table; three are never read. Each one only costs writes on tables that scans, metadata refreshes and progress reports rewrite constantly. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Silo's short catalog queries often cross jit_above_cost and take longer to compile than to run. Each new connection turns JIT off unless the server configuration, the database or role, or DATABASE_URL already set it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
PostgreSQL cannot prune partitions on the (timestamp, id) row comparison, so every cursor page scanned every daily partition. A redundant bare bound on timestamp lets it skip the ones newer than the cursor. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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 19 minutes. View limit detailsLimit details: You’ve used all 4 included reviews currently available. Review configuration: ⚙️ Run configuration
📒 Files selected for processing (17)
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 |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Quick104
added
priority: P2
Cursor
|
Note Grok commenting on Quick's behalf. Ranking: Scope: this bundles six separable database changes (partition timezone fix, 13 index drops, invalid-index rebuild, search-function inlining, JIT off by default, ops-log pruning), and the "one concern" box is unchecked. Consider splitting the partition fix out from the riskier index-drop and JIT-default changes so each can merge on its own. |
![silo-kody[bot]](https://avatars.githubusercontent.com/u/287058192?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d41bdfaf16
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Splitting the branch into commits appended the earlier pin blocks again in each later commit, and the pin runner rejects duplicate entries. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 67b9048702
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
1 participant
Problem
Related issue: N/A
Validation tasks: none
A review of Silo's PostgreSQL use against Microsoft's
postgresql-best-practices guidance found one bug that
breaks log partitioning on some fresh installs, and several costs every deployment pays:
028 creates the first
operational_logsandactivity_logpartitions at the server's localmidnight, and
internal/partmanlater adds partitions at UTC midnight. The next one overlaps(SQLSTATE 42P17) or leaves a gap,
EnsureFuturePartitionsfails at startup and on every cleanuprun, and log rows pile into the default partition until retention removes the partitions 028 made,
about a week later. The Docker image runs PostgreSQL in UTC; distro packages usually use the host's
timezone.
constraint, or are a leading prefix of another index on the same table. Three are never read: the
episodesfull-text indexes left behind when episode search moved toepisode_catalog_entries,and an HNSW index on
user_taste_clusters, which is only read by profile.CREATE INDEX CONCURRENTLY IF NOT EXISTSmigrations can leave an index unusable forgood. If a build was interrupted, the retry skipped the invalid leftover and goose recorded the
migration as applied.
normalize_search_number_tokencannot be inlined because its body is a CTE, sonormalize_search_textplans a subquery for every token of every title write and search.jit_above_costand spendlonger compiling than running; two call sites already turn it off per transaction.
the
(timestamp, id) < (...)row comparison.Approach
TimeZoneto UTC for its own transaction. Only fresh installs run it;editing an applied legacy migration for fresh-install behavior follows fix(playback): align default transcode directory #573.
idx_user_watch_progress_profileandidx_media_files_folderstay even though a superset couldserve them: deduplication keeps them several times smaller, so profile-wide progress reads and
per-folder file counts touch far fewer pages.
DROP INDEX CONCURRENTLYandCREATE INDEX CONCURRENTLY, so serving replicas keep reading andwriting those tables; valid indexes are left alone. It is Go because SQL cannot choose per index
outside a transaction, and a plain
DROP INDEXwould take anACCESS EXCLUSIVElock.normalize_search_number_tokenbecomes a singleCASEexpression with the same output, sostored columns and indexes stay valid without a rebuild.
ALTER DATABASEorALTER ROLE, orDATABASE_URLalready setjit; it readspg_settings.sourcerather thanparsing the URL. The opt-in tuner recommends the same value.
timestamp <= $cursorbound so PostgreSQL prunes newerpartitions.
DEVELOPMENT.mddocuments the migration lock-safety rules these changes follow, and.env.examplenotes thatPOSTGRES_TUNE_CONNECTIONSmust cover every Silo process's pool in acluster.
The review also found that
hnsw.ef_searchcan exceed pgvector's limit of 1000; #2095 fixes that,so this PR leaves it out. Commits are split by concern.
Validation
make lint-changed: pass. Every commit builds on its own.make test-go: pass, exceptinternal/mediasampleTestRunStatsWithRealFFmpeg, which runs thehost's ffmpeg build and depends on no changed package.
verify-*targets: pass.Each new test fails on the code it replaces.
recomputing every stored normalized title with the new function gave 0 mismatches, and the access
paths that lose an index ran equal or faster. That deployment ran an earlier revision of these
migrations; its schema was then brought in line with this one.
explicit
ORDER BY, and dropping an index does not change results, so their passed cases areunaffected. No other validated feature reaches this change.
Measurements
standard_conforming_stringson and offnormalize_search_textover 50,000 titlesepisodesfull-text indexesidx_user_watch_progress_profiledroppedEvidence
Evidence: none, no user-visible change
Risks
idx_scanfor these indexes on production before merge would confirm none is still hot. Downrecreates them concurrently.
jit=oninDATABASE_URLor the server configuration restores it.concurrently. That takes time on a large table but does not block other sessions.
siloserver.org. They are added to the open manual issue that covers those pages,
docs: add the Docker deployment details the server guide is dropping siloserver.org#38, and the site update waits for this PR to merge.
Checklist
AI Disclosure
and a scratch PostgreSQL 18.6 cluster, covering migration parsing and locking, index redundancy,
function equivalence, JIT precedence, and the tests. They found a regex backreference that broke
under
standard_conforming_strings=off, a repair that tookACCESS EXCLUSIVElocks and later onethat skipped an index after an interrupted drop, an index worth keeping, and URL-based JIT
detection that misread
options. Each is fixed with a test that fails on the earlier code; JITdetection now reads
pg_settings.source. A finding that the Go migration starves on aone-connection pool was dismissed: migrations always run on the 20-connection bootstrap pool.
🤖 Generated with Claude Code