Cache the BM25 corpus figures per backend instead of rescanning - #59
Merged
Conversation
Up to standards ✅🟢 Issues
|
| Category | Results |
|---|---|
| Compatibility | 5 high |
🟢 Metrics 4 complexity · 0 duplication
Metric Results Complexity 4 Duplication 0
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds the 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/bm25.c (1)
323-355: 🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy liftBound the backend-local cache.
corpus_stats_cacheis allocated inTopMemoryContext. Expired entries are refreshed but never removed. A long-lived worker or pooled backend that accesses many chunk tables retains every entry until process exit.Add a maximum entry count with eviction, or periodically remove expired entries.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/bm25.c` around lines 323 - 355, Bound the backend-local corpus_stats_cache used by the surrounding corpus-stats lookup: add a finite entry limit with eviction, or periodically remove expired entries before or during insertion. Ensure refreshes still update existing entries while accesses to many chunk tables cannot retain every CorpusStatsEntry until process exit.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/bm25.c`:
- Line 232: Replace the name-based cache key in the chunk-table statistics path
with the resolved relation OID. Resolve chunk_table to its relation on every
call, use that OID consistently for cache lookup and storage, and ensure stale
entries cannot survive drop-and-recreate. Add regression coverage for same-named
tables in separate schemas and for dropping and recreating a chunk table within
one backend.
---
Nitpick comments:
In `@src/bm25.c`:
- Around line 323-355: Bound the backend-local corpus_stats_cache used by the
surrounding corpus-stats lookup: add a finite entry limit with eviction, or
periodically remove expired entries before or during insertion. Ensure refreshes
still update existing entries while accesses to many chunk tables cannot retain
every CorpusStatsEntry until process exit.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 529033c9-fad1-45ee-b087-292a8fa3fe35
⛔ Files ignored due to path filters (1)
test/expected/hybrid_test.outis excluded by!**/*.out
📒 Files selected for processing (5)
docs/index.mdsrc/bm25.csrc/guc.csrc/pgedge_vectorizer.htest/sql/hybrid_test.sql
dpage
added a commit
that referenced
this pull request
Aug 11, 2026
A chunk table is named unqualified and resolved through search_path, so one string can mean different relations at different moments. Keyed by name, a pooled backend that switched search_path between tenants was handed whichever tenant's corpus it had seen first, and scored the second tenant's queries against the first one's statistics. Dropping and recreating a chunk table under the same name went wrong the same way. Demonstrated before fixing, with two schemas holding identically named chunk tables whose documents differ only in length: the vectors came back byte for byte identical, so the second tenant was silently scored on the first tenant's mean document length. With the relation OID as the key they differ as they should. Resolving the name costs a syscache lookup, against the table scan the cache exists to avoid. A name resolving to nothing is left to the uncached read so that the caller still sees the "relation does not exist" it would have got anyway, rather than a different error invented here. Test 21 covers it. Raised by CodeRabbit on #59.
BM25 needs the corpus size and the mean document length, and getting them is an unindexable aggregate over the whole chunk table. It was paid on every call: once per search, and once per queue item in the worker, so a batch of ten scanned the same table ten times under a snapshot in which the answer could not have changed. Measured on a 300,000 chunk table, one call costs 34ms and two hundred cost 4.8 seconds. Both figures feed a ranking heuristic rather than an account that has to balance, and during ingest they are a moving target anyway, since every chunk written moves them. Holding them briefly therefore costs a little precision in a number that was never precise. Each backend now keeps them for pgedge_vectorizer.corpus_stats_cache_ttl seconds, sixty by default, which takes those two hundred calls from 4.8 seconds to 38ms: one scan, then two hundred lookups. A per-backend cache suits both callers. A worker is a long-lived process serving one database, and searches arrive over pooled connections that are also long lived. A short-lived backend running a single search pays the scan exactly as it did before, so nothing regresses. Setting the GUC to 0 restores the previous behaviour precisely, which is also how the new test compares the two paths. Deliberately not maintaining the figures incrementally. Chunk rows are inserted and deleted from seven places across the SQL, including the delete and truncate paths that have accounted for most of the recent bug history, and counters wrong in those paths would skew ranking silently and permanently. A cache that expires cannot drift for longer than its TTL, and recovers by itself. Closes #53
A chunk table is named unqualified and resolved through search_path, so one string can mean different relations at different moments. Keyed by name, a pooled backend that switched search_path between tenants was handed whichever tenant's corpus it had seen first, and scored the second tenant's queries against the first one's statistics. Dropping and recreating a chunk table under the same name went wrong the same way. Demonstrated before fixing, with two schemas holding identically named chunk tables whose documents differ only in length: the vectors came back byte for byte identical, so the second tenant was silently scored on the first tenant's mean document length. With the relation OID as the key they differ as they should. Resolving the name costs a syscache lookup, against the table scan the cache exists to avoid. A name resolving to nothing is left to the uncached read so that the caller still sees the "relation does not exist" it would have got anyway, rather than a different error invented here. Test 21 covers it. Raised by CodeRabbit on #59.
mason-sharp
force-pushed
the
fix/issue-53-corpus-stats
branch
from
August 11, 2026 23:40
e3c25ee to
21e756a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/sql/hybrid_test.sql (1)
496-500: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a cache-hit assertion.
The call at Line 497 populates an empty cache entry. It does not read a cached entry. A cache implementation that refreshes on every call still passes this test.
Call
bm25_query_vector()again before the TTL expires. Then change the corpus between calls and verify that the second cached result remains unchanged. Set the TTL to0and verify that the changed corpus produces a different result.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/sql/hybrid_test.sql` around lines 496 - 500, Extend the cache test around bm25_query_vector by making an initial call, modifying the corpus, and calling it again before the 60-second TTL expires; assert the second cached result matches the initial result despite the corpus change. Then set pgedge_vectorizer.corpus_stats_cache_ttl to 0, call bm25_query_vector again, and assert the refreshed result differs from the cached result.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/sql/hybrid_test.sql`:
- Line 550: Replace SET search_path = public with RESET search_path in the test
setup, preserving the configured role or database search path while leaving the
sparse-vector comparison unchanged.
---
Nitpick comments:
In `@test/sql/hybrid_test.sql`:
- Around line 496-500: Extend the cache test around bm25_query_vector by making
an initial call, modifying the corpus, and calling it again before the 60-second
TTL expires; assert the second cached result matches the initial result despite
the corpus change. Then set pgedge_vectorizer.corpus_stats_cache_ttl to 0, call
bm25_query_vector again, and assert the refreshed result differs from the cached
result.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 200696cb-9297-4677-926d-746e5c647a8d
⛔ Files ignored due to path filters (1)
test/expected/hybrid_test.outis excluded by!**/*.out
📒 Files selected for processing (3)
src/bm25.csrc/guc.ctest/sql/hybrid_test.sql
🚧 Files skipped from review as they are similar to previous changes (2)
- src/guc.c
- src/bm25.c
HASH_ENTER inserts an entry for the caller to fill, and the read that fills it goes to SPI, so a cancelled query or a statement timeout left an unfilled entry behind in TopMemoryContext for a later call to read as corpus figures. Measured: with SELECT revoked so the read throws at that exact point, the next call in the same backend returned a different vector from an uncached read of the same corpus. It agrees once the entry is only created after the read returns.
SET search_path = public does not put the session back as it was; it overwrites it for every test after this one, discarding any role- or database-level search_path. Verified: with the database set to 'myapp, public', SET leaves 'public' behind where RESET restores it. Raised by CodeRabbit on #59.
mason-sharp
approved these changes
Aug 12, 2026
mason-sharp
pushed a commit
that referenced
this pull request
Aug 12, 2026
A chunk table is named unqualified and resolved through search_path, so one string can mean different relations at different moments. Keyed by name, a pooled backend that switched search_path between tenants was handed whichever tenant's corpus it had seen first, and scored the second tenant's queries against the first one's statistics. Dropping and recreating a chunk table under the same name went wrong the same way. Demonstrated before fixing, with two schemas holding identically named chunk tables whose documents differ only in length: the vectors came back byte for byte identical, so the second tenant was silently scored on the first tenant's mean document length. With the relation OID as the key they differ as they should. Resolving the name costs a syscache lookup, against the table scan the cache exists to avoid. A name resolving to nothing is left to the uncached read so that the caller still sees the "relation does not exist" it would have got anyway, rather than a different error invented here. Test 21 covers it. Raised by CodeRabbit on #59.
4 tasks
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
2 participants
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.
Closes #53.
Measured
On a 300,000 chunk table, with the argument varied so nothing is folded away:
ttl = 0)ttl = 60s)Two hundred searches went from 4.8 seconds to one scan plus two hundred lookups. The gap widens with the corpus, because the thing removed is an unindexable aggregate over the whole chunk table.
Why a cache rather than maintained counters
The issue suggested keeping N and
sum(token_count)alongside the incrementally maintaineddoc_freq, and I started down that road before deciding against it. Chunk rows are inserted and deleted from seven places across the SQL, and they include the delete and truncate paths that have accounted for most of the recent bug history in this extension. Counters that go wrong in one of those paths would skew ranking silently and permanently, with no way for anyone to notice.These two figures feed a ranking heuristic, not an account that has to balance, and during ingest they are a moving target regardless, since every chunk written moves them. So a cache that expires is the better trade: it cannot drift for longer than its TTL, and it recovers by itself. If exact counters are ever wanted, this does not stand in the way.
A per-backend cache happens to suit both callers. A worker is a long-lived process serving one database, and searches arrive over pooled connections that are also long-lived. A short-lived backend running a single search pays the scan exactly as before, so nothing regresses.
pgedge_vectorizer.corpus_stats_cache_ttldefaults to 60 seconds; 0 reads afresh every time and restores the previous behaviour precisely.Test plan
hybrid_test.sqlcompares the two paths directly, asserting the vector produced withttl = 0is identical to the one produced with the cache on, so the optimisation is proven not to change results rather than merely proven fastbm25_avg_doc_len()still matches SQL's ownAVG(token_count)exactlyTwo notes on the benchmarking
My first two attempts measured nothing.
SELECT count(f(...)) FROM generate_series(...)let the planner elide the calls, and then I hadbm25_query_vector's arguments the wrong way round, which measured the error path for a non-existent relation. Both showed sub-millisecond times for 200 calls, which is what gave them away: the figures above are from a run where the stub-free single call takes the expected 34 ms and the counts line up.The wall-clock numbers are single-run on one machine, so treat the ratio as the shape of the result. The mechanism, one scan instead of two hundred, is the exact part.