Follow on to corpus stats cache - #60
Conversation
A wall-clock TTL bounds staleness in time, but the harm is proportional: a thousand chunks added to a million barely move the weights, the same thousand added to two hundred change them several fold. The figures are also written into stored vectors by the worker rather than only used for a query, so a stale reading does not simply correct itself. corpus_stats_cache_max_growth re-reads once uses of a cached entry reach that percentage of N, alongside the existing TTL, whichever comes first. It scales itself: a million-row corpus tolerates fifty thousand uses, a two-hundred-row one ten, and re-reading a small table is cheap because it is small. Counting uses is a proxy rather than a measurement, since the worker does not insert chunk rows itself and a search does not grow the corpus at all; the comment says so. Test 24 covers both directions: a use inside the budget is served from cache, the use that spends it re-reads and matches an uncached read, and with the bound off the same sequence stays stale.
A term cannot appear in more documents than exist, but total_docs may be a cached reading while doc_freq is read fresh, so doc_freq can outrun it. Left alone ln((N+1)/(df+0.5)) goes negative, and bm25_compute_sparse_str keeps only scores above zero, so the term is dropped from the vector rather than merely underweighted. Measured with doc_freq deliberately set past the corpus: unclamped the query vector came back empty, clamped it carries a near-zero weight, which is what a term appearing in every document should score. Taking the larger of the two changes nothing whenever total_docs is at least doc_freq, which is every case a fresh reading can produce, so the expression stays bit-identical to the SQL it replaced for all previously reachable inputs.
The regression tests exercise it through bm25_query_vector, whose result is discarded at the end of the statement. The worker runs the same lookup per queue item and writes the outcome into sparse_embedding, which is the case the bound exists for. Every chunk holds the same term and token_count, so avg_doc_len cannot vary and doc_freq is fixed by hand; the corpus size is the only input that differs, and the weights are checked against the ratio it predicts rather than merely for being different. With the bound disabled the first four assertions still pass and the last two fail.
Up to standards ✅🟢 Issues
|
| Category | Results |
|---|---|
| Compatibility | 10 high (10 false positives) |
🟢 Metrics 13 complexity · 0 duplication
Metric Results Complexity 13 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.
📝 WalkthroughWalkthroughThe change adds the 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![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 (2)
docs/index.md (1)
177-177: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider stating that the bound counts cache uses, not measured growth.
The implementation in
src/bm25.ccounts each use of a cached entry as one assumed added chunk. A read-only search workload therefore also spends the budget and triggers a re-read, even when the corpus does not grow. The current wording implies the extension measures corpus growth. One added clause would set the expectation for users who tune this value against query volume.🤖 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 `@docs/index.md` at line 177, Update the description of pgedge_vectorizer.corpus_stats_cache_max_growth to clarify that the percentage bound is consumed per cached-entry use/read, not by measured corpus growth, so read-only searches can exhaust the budget and trigger a re-read.test/sql/hybrid_test.sql (1)
595-605: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: make the phase-2 starting state explicit.
Line 588 sets
corpus_stats_cache_ttl = 0, so thegtruthcall takes the uncached path inbm25_corpus_statsand writes no cache entry. The entry left byg3therefore survives withuses_since_read = 0. When line 596 restores the TTL,h1is served from that surviving entry rather than from a fresh read. The assertionh1 = h3still holds, because the growth bound is off and all three calls hit the same entry.The comment on line 595 reads as though
h1establishes a fresh baseline. Add aSET pgedge_vectorizer.corpus_stats_cache_max_growth = 0;before an explicit warm-up call, or note the carried-over entry, so the intent survives future edits to the ordering.🤖 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 595 - 605, Make the phase-2 baseline explicit around the growth_chunks sequence by adding the cache growth-bound setting before an explicit warm-up call, or documenting that h1 uses the cache entry carried over from g3. Preserve the assertion that h1 and h3 remain equal with growth disabled, and anchor the change to the bm25_query_vector calls producing h1, h2, and h3.
🤖 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/t/006_corpus_stats_growth.pl`:
- Around line 115-137: Update the queue setup in the test around await_sparse so
item 2 is inserted and awaited before inserting item 3. Keep the existing
assertions and metadata unchanged, ensuring the worker claims item 2 before item
3 deterministically.
---
Nitpick comments:
In `@docs/index.md`:
- Line 177: Update the description of
pgedge_vectorizer.corpus_stats_cache_max_growth to clarify that the percentage
bound is consumed per cached-entry use/read, not by measured corpus growth, so
read-only searches can exhaust the budget and trigger a re-read.
In `@test/sql/hybrid_test.sql`:
- Around line 595-605: Make the phase-2 baseline explicit around the
growth_chunks sequence by adding the cache growth-bound setting before an
explicit warm-up call, or documenting that h1 uses the cache entry carried over
from g3. Preserve the assertion that h1 and h3 remain equal with growth
disabled, and anchor the change to the bm25_query_vector calls producing h1, h2,
and h3.
🪄 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 Plus
Run ID: 85727514-7d1c-4d1b-828d-9171589fa673
⛔ Files ignored due to path filters (1)
test/expected/hybrid_test.outis excluded by!**/*.out
📒 Files selected for processing (6)
docs/index.mdsrc/bm25.csrc/guc.csrc/pgedge_vectorizer.htest/sql/hybrid_test.sqltest/t/006_corpus_stats_growth.pl
Inserted together they shared a created_at, and the claim orders by that column alone with no tie-breaker, so which one the worker took first was unspecified. Taken in the other order the cached use and the re-read swap places and both assertions invert, failing against working code. Raised by CodeRabbit on #62.
dpage
left a comment
dpage
left a comment
There was a problem hiding this comment.
Review summary
Both mechanisms are individually sound and the C is careful. I checked the parts that usually go wrong and found them right: the budget arithmetic cannot overflow (int64 * <= 100, and N would have to exceed 9e16), there is no division by zero (the divisor is the literal 100, bm25_corpus_stats_uncached() clamps N to at least 1, and Max(budget, 1) covers small N), the threshold is recomputed from entry->total_docs on every call so it tracks the refreshed reading, the counter resets on re-read, and the cache remains a per-backend HTAB in TopMemoryContext with no shared state to race over. PGC_USERSET with no unit flag matches the sibling corpus_stats_cache_ttl and is right for a percentage. The two bounds compose coherently in both directions: max_growth = 0 returns false and leaves the TTL alone, and ttl = 0 short-circuits to the uncached path before the growth bound is consulted, so max_growth simply has no effect there, consistent with what the docs say about ttl = 0. Regression tests 24 and 25 both genuinely discriminate; I verified that unclamped, test 25's term scores -6.12, is dropped by the score > 0.0 filter, and the vector comes back as {}/65536.
My concerns are not with the implementation but with its evidence and its labelling. There is one blocker in the new TAP test, where a fixture schema mismatch means two assertions pass for the wrong reason, and the GUC's name and documentation promise a measurement that the code deliberately does not make.
🔴 Must Fix
1. TAP 006's fixture omits updated_at, so every bm25_update_idf_stats() call fails silently and two assertions pass for the wrong reason
test/t/006_corpus_stats_growth.pl:65-67 creates chunks_idf_stats (term, doc_freq), but the extension creates that table with a third column, updated_at (sql/pgedge_vectorizer--1.1.sql:316-321), and upsert_term_idf() emits ... ON CONFLICT (term) DO UPDATE SET doc_freq = ..., updated_at = now() (src/bm25.c:870-876). Against this fixture that statement fails at parse-analysis with column "updated_at" of relation "chunks_idf_stats" does not exist, and bm25_update_idf_stats() swallows the failure wholesale in its own PG_CATCH and reduces it to a WARNING (src/bm25.c:913-924). I confirmed this on a scratch cluster: the statement errors and doc_freq stays at 5.
The consequence is that doc_freq is frozen for the whole test, which is exactly the premise stated at line 18, that "the corpus size is then the only input that differs". In a real deployment it is not: the worker increments doc_freq for every first-processed chunk (src/worker.c:1769-1772) after bm25_load_idf_stats() has already read it, so items 1, 2 and 3 see df of 5, 6 and 7 respectively. Working the arithmetic through (tf = 1 and token_count = 10 = avg_doc_len, so the tf factor cancels and the stored weight is exactly the IDF), with a schema-correct fixture:
- line 140,
is($cached, 't'), fails: w1 = 1.33977 at df 5 and w2 = 1.17272 at df 6, both at the cached N = 20. - line 165,
is($ratio, 't'), fails: the real ratio is 2.5252 (N 220 / df 7 over N 20 / df 5) against the 2.7567 the test hardcodes fromln(221/5.5)/ln(21/5.5), which is the df-frozen figure. The tolerance is 0.01 and the error is 0.23.
So the "with the bound disabled, assertions 1-4 still pass and 5-6 fail" check in the description is true only in a configuration that cannot occur in production, and as written the test is a green light sitting on top of a per-item error.
Suggested fix: add updated_at TIMESTAMPTZ DEFAULT now() so the fixture matches what the extension builds, then remove the dependence on df being constant by giving each chunk its own single term (alpha, beta, gamma), each seeded at doc_freq = 5. The three terms then have independent df, only N differs between them, the existing weight-extraction SQL still works, and the hardcoded ratio stays correct. Failing that, drop the cross-item equality and assert each weight against the (N, df) pair actually in force at that point.
🟡 Should Fix
2. The GUC name and documentation claim to bound growth; the code bounds uses of the cache entry
src/guc.c:334-341 and docs/index.md:177 both describe "Percent the corpus may grow before cached figures are re-read", but corpus_stats_stale_by_growth() (src/bm25.c:267-279) never looks at the corpus at all: it compares uses_since_read against N * pct / 100. The code comment at lines 259-265 is admirably honest that this is a proxy, and even notes that a search does not grow the corpus, yet none of that honesty reaches the operator who has to tune the setting. A read-only search workload against a completely static corpus will re-read every N/20 calls, which is close to the opposite of what the parameter appears to promise, and this is a PGC_USERSET knob that is meant to be tuned.
Suggested fix: either carry the proxy into the GUC long description and the docs row (something along the lines of "counted as uses of the cached figures, taken as a proxy for chunks added, since the figures are read once per queue item and once per search"), or rename to something that does not overclaim, for instance corpus_stats_cache_max_uses_pct. Renaming now is cheap; after a release it is not.
3. doc_freq > N is unambiguous evidence that the cached N is stale, and the clamp hides that evidence rather than acting on it
src/bm25.c:606 substitutes Max(total_docs, doc_freq) for the affected term only. The maths is right, and I agree it prevents the term being dropped by the score > 0.0 filter at src/bm25.c:791. But the clamp leaves the stale entry in place, so every subsequent document processed within the remaining TTL keeps getting the substituted weight of log(1 + 0.5/(df+0.5)), which is effectively zero, and those weights are persisted into sparse_embedding rather than recomputed on the next call. That persistence is this PR's own stated reason for caring. Since the cache is per-backend and in-process, the fix is a couple of lines: on observing doc_freq > total_docs, force a re-read or stamp the entry stale and recompute, which repairs this document and every one after it.
A smaller point in the same area: the clamp is applied per row, so in a pathological case two terms of the same document are weighted against different effective corpus sizes, which BM25 does not contemplate. Taking the maximum df across the result set and clamping once would at least keep the model internally consistent.
4. No changelog entry, and the README GUC table is now two parameters behind
docs/changelog.md keeps a detailed [Unreleased] section, and the most recent comparable addition, pgedge_vectorizer.worker_service_quantum, got an ### Added entry (docs/changelog.md:46-51), as did a GUC context change under ### Security. Neither corpus_stats_cache_ttl (missed by #59) nor corpus_stats_cache_max_growth appears anywhere in the changelog. Separately, the "Hybrid Search GUC Parameters" table at README.md:211-215 lists enable_hybrid, bm25_k1 and bm25_b, and is now missing both cache parameters. docs/index.md was updated correctly.
Suggested fix: add an ### Added entry covering both parameters, since this PR is the natural place to catch up #59's omission, and add both rows to the README table.
🟢 Suggestions
5. Remove the dead ignored CTE column in test/t/006_corpus_stats_growth.pl:156
(sparse_embedding::text)::jsonb on a sparsevec is not valid JSON; I confirmed that ('{1234:1.34}/65536')::jsonb raises "invalid input syntax for type json". The query runs today only because w is referenced once, gets inlined, and the planner then prunes the unreferenced target entry. Anything that causes the CTE to be materialised instead, whether a second reference, a MATERIALIZED keyword or a planner change, turns a green test into a hard safe_psql failure. The column contributes nothing to the assertion, so it may as well go.
6. The first use after a refresh is free, so the effective allowance is budget + 1
src/bm25.c:398 increments only on the hit path and src/bm25.c:416 resets to zero on refresh, but the refresh itself is a use, so at N = 20 with 5% the entry serves two lookups from the read rather than one. Harmless, and test 24 is written around it, but the documented semantics would read more truthfully with the counter starting at 1 after a read.
7. Two small test tidies
test/sql/hybrid_test.sql:602 computes h2 and never asserts on it, and since the growth bound is disabled in that block it is not advancing anything meaningful either. Test 25 at test/sql/hybrid_test.sql:631 asserts only that the vector is non-empty, whilst its own comment claims "the weight below is near zero, which is what a term appearing in every document should score"; bounding that weight above by something small would test the claim rather than restate it.
ℹ️ Notes
8. The clamp's rescue has a ceiling, imposed by %f
build_sparsevec_string() formats scores with %f (src/bm25.c:741), so anything below 5e-7 prints as 0.000000, and pgvector strips zero elements on input (I confirmed '{1234:0.000000}/65536'::sparsevec equals '{}/65536'). The clamped weight is roughly 0.5/df times the tf factor, so past a df of a couple of million the term is dropped again regardless of the clamp. That is an unlikely corner, since the growth bound would normally have fired long before, and the six-decimal truncation is pre-existing rather than introduced here, but it is worth knowing the clamp is not an unconditional guarantee.
9. Read-heavy cost is self-limiting
The bound does mean a static corpus under query traffic re-reads periodically for no benefit, but because the budget scales with N the amortised cost falls as the scan gets more expensive, roughly 33ms per 15,000 queries at 300k chunks, and a 200-row scan is free. I do not think this needs changing. It is worth stating only because "the cache is now invalidated by reads as well as writes" is a behaviour change from #59 that an operator might not expect.
10. CodeRabbit's thread was legitimate and correctly resolved
It objected that items 2 and 3, inserted in one VALUES, share a created_at whilst the claim query orders by created_at alone with no unique tie-breaker (src/worker.c:1406), leaving their order unspecified. I confirmed the ordering, and ce70b6e fixes it properly by inserting and awaiting each item separately rather than papering over it with a sleep. The absence of a tie-breaker in the claim itself remains a latent fragility, but it is pre-existing and out of scope here.
Checklist
- Correctness — the growth bound and the clamp are both arithmetically sound; no overflow, divide-by-zero or race
- Security — nothing introduced; no new user input reaches SQL, and the GUC context matches its sibling
- Error handling — failures are handled, though finding 1 turns an inherited swallow-and-warn path into a hidden test failure
- Tests — the regression tests discriminate, but TAP 006 passes for the wrong reason (finding 1)
- Documentation —
docs/index.mdupdated, but the description overclaims (finding 2) and the changelog and README are missing entries (finding 4)
Back to you @mason-sharp. Finding 1 is the one that has to change before this goes in; finding 2 is a judgement call and the name is yours to pick, but I would rather we settled it before the parameter ships. The rest is small.
The fixture omitted updated_at, which upsert_term_idf() writes, so every doc_freq update failed and bm25_update_idf_stats() reduced it to a WARNING: the test passed while logging three per-item errors, and it did so because doc_freq was frozen at its seeded value. With the column present doc_freq moves as it does in production, which would have broken two assertions. Each chunk now carries its own term so the terms have independent doc_freq, restoring the stated premise that the corpus size is the only input that differs. The weights are compared directly, since vectors over different terms occupy different dimensions, and a new assertion on doc_freq catches the fixture drifting again. Raised by dpage on #60.
The parameter was called max_growth and documented as bounding how far the corpus may grow, but nothing measures the corpus: the code compares uses of the cached entry against a percentage of N. A search spends the budget without adding anything, so a static corpus under query traffic re-reads periodically, which is close to the opposite of what the name promised. Renamed to corpus_stats_cache_max_uses_pct, with the GUC description and the docs row now saying that uses stand in for chunks added, that this is a proxy rather than a measurement, and that reads spend it too. The internal helper follows the same naming. Raised by dpage on #60.
doc_freq above N proves the cached corpus size is stale, since doc_freq is read fresh on every call. Clamping the weight left the stale entry in place, so every later chunk in the same window was scored against the same wrong corpus and had that persisted into sparse_embedding. Drop the entry and read again, which repairs this document and the ones after it. If the fresh reading is still below the largest doc_freq the statistics are inconsistent rather than stale, and re-reading cannot help, so the entry adopts that value: measured, twenty further calls then cause no additional scans, where an unguarded recheck would rescan on every one. The maximum doc_freq is taken across the whole result set before any weight is computed, so one corpus size covers the whole document rather than each term being scored against its own. Raised by dpage on #60.
Neither cache parameter appeared in either: corpus_stats_cache_ttl was missed when the cache landed, and max_uses_pct followed it. The changelog entry covers both, since this is the natural place to catch up the first. Raised by dpage on #60.
h2 was computed and never used, so the block checked only that the ends matched; it now checks all three, which is what "no number of uses moves the figures" actually means. Test 25 claimed in a comment that the clamped weight is near zero and asserted only that the vector was non-empty; it now bounds the weight. The counter's semantics are documented rather than changed: counting the reading call would leave no cached use at all wherever the budget works out to one, which is every corpus below 20 rows at the default 5%. Raised by dpage on #60.
The re-read had no automated test: it was verified by hand, which is the same gap that let the fixture mismatch through earlier on this branch. Test 26 checks that a doc_freq above the cached size produces the weights of a fresh read rather than a clamped one, and that the entry stays repaired once doc_freq drops back below it. Both assertions return false against a clamp-only build, so they discriminate on the re-read itself. Also note why the re-read is not skipped when the reading was already fresh: it costs one scan, only for inconsistent statistics, and only until max_df is adopted, after which nothing contradicts the entry.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
docs/changelog.md (1)
65-76: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider documenting the third re-read trigger.
The entry states that two settings bound staleness and that whichever is reached first triggers a re-read.
src/bm25.cadds a third trigger in this PR: a freshdoc_freqabove the cached corpus size drops the entry and reads the figures again, even when both bounds are still satisfied. An operator who sees an unexpected scan cannot map it to either documented bound.📝 Proposed addition
Each use of a cached reading counts as one chunk that may have been added since — a proxy rather than a measurement, so searches spend the budget too. Setting it to 0 bounds by time alone. + + A cached reading is also dropped and read again when a term's document + frequency exceeds the cached corpus size, since a term cannot appear in more + documents than exist. If the fresh reading still contradicts the data, the + cached corpus size adopts the largest observed document frequency so that + weights stay non-negative.🤖 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 `@docs/changelog.md` around lines 65 - 76, Update the changelog entry to document the third corpus-stats cache re-read trigger: when a fresh doc_freq exceeds the cached corpus size, the cache entry is invalidated and the figures are reread even if both configured bounds remain satisfied. Clarify that this can cause an unexpected scan independently of corpus_stats_cache_ttl and corpus_stats_cache_max_uses_pct.
🤖 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.
Nitpick comments:
In `@docs/changelog.md`:
- Around line 65-76: Update the changelog entry to document the third
corpus-stats cache re-read trigger: when a fresh doc_freq exceeds the cached
corpus size, the cache entry is invalidated and the figures are reread even if
both configured bounds remain satisfied. Clarify that this can cause an
unexpected scan independently of corpus_stats_cache_ttl and
corpus_stats_cache_max_uses_pct.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3078acad-59c1-4177-ae5a-38d7e3deb6d6
⛔ Files ignored due to path filters (1)
test/expected/hybrid_test.outis excluded by!**/*.out
📒 Files selected for processing (8)
README.mddocs/changelog.mddocs/index.mdsrc/bm25.csrc/guc.csrc/pgedge_vectorizer.htest/sql/hybrid_test.sqltest/t/006_corpus_stats_growth.pl
2 participants
Follow on to PR #59
Two bounds on how stale a cached reading may get.
Growth bound. The existing TTL bounds staleness in time, but the harm is proportional: a thousand chunks added to a million barely move the weights, while the same thousand added to two hundred change them several fold. pgedge_vectorizer.corpus_stats_cache_max_growth (percent, default 5) re-reads once uses of a cached entry reach that fraction of N, applied alongside the TTL so whichever is reached first wins. It scales itself — a million-row corpus tolerates fifty thousand uses, a two-hundred-row one ten, and re-reading a small table is cheap because it is small. Counting uses is a proxy rather than a measurement, since the worker does not insert chunk rows itself; that is stated in the code.
This matters more than a query-side cache would suggest. The worker runs the same lookup per queue item and writes the result into sparse_embedding, so a stale corpus size is persisted rather than recomputed on the next call.
Clamp. doc_freq is read fresh while N may be cached, so df can outrun it. ln((N+1)/(df+0.5)) then goes negative, and only scores above zero are kept, so the term is dropped from the stored vector rather than merely underweighted. Taking max(N, df) changes nothing whenever N >= df, which is every case a fresh reading can produce.
Test plan
006_corpus_stats_growth.pldrives the real worker and checks the storedsparse_embedding. With the bound disabled, assertions 1–4 still pass and5–6 fail, so it discriminates on the feature rather than on setup.