Repository navigation
feat(fulltext2): add default-off immutable Base file reuse experiment - #29701
VioletQwQ-0 wants to merge 7 commits into
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
This adds a third cache layer on top of the existing disk cache and in-memory index cache. The PR's own design doc notes net performance "remains unaccepted," so the benefit is unproven. The build tag only gates installation — ~900 lines of pool/owner code ( Even when enabled the benefit is narrow: a READY hit still re-checksums and re-decodes, so it only saves the SQL-stream + local re-materialize of the unchanged base, and that saving is unmeasured. Given two cache layers already exist and the gain is both unproven and narrow, I don't think the added complexity/risk is justified. Suggest closing, and if the base-reload cost is real, measuring it first — and exploring whether keying the existing index cache so the immutable base survives a generation flip gets most of the benefit without a new mmap-pool lifecycle. |
cpegeric
left a comment
There was a problem hiding this comment.
- Unproven. The PR's own doc says "net performance remains unaccepted." A 5.8k-line, third-cache-layer change with no
demonstrated net win is a close-by-default. - It isn't contained. The build tag only gates installation. ~900 lines of pool/owner compile into every binary, and it
refactors two shared hot paths in place — the base-load path (storage.go) and CN shutdown (server.go) — so experiment-off
users carry the risk anyway (e.g. the Segment.Free() munmap-error path now leaks the spill file for ordinary loads). - Narrow benefit even when on. A READY hit still re-checksums and re-decodes; it only saves the SQL-stream + materialize of
the unchanged base — and that saving is unquantified. - Cost/benefit. Two cache layers already exist; a third with its own mmap/pin/debt/quarantine lifecycle is exactly the
class that caused the prior fulltext2 cache bugs (#28005, #28837). The burden is on the PR to prove the win, and it
hasn't.
|
A new cache layer is only justified when it caches something the existing layers structurally cannot, and can't be extended The two current layers and what each owns:
The gap the PR found is real: on a generation flip (CDC/MERGE/stale-TTL) the immutable base gets re-streamed from SQL → But that gap lives inside the memory cache's own domain — it's caching the decoded index, and the base is just the immutable
So the PR's new layer is strictly worse than extending the existing one: more code, a brand-new mmap/pin/debt/quarantine The one honest caveat so you're not caught out: the gap is real, so the answer isn't "caching is already perfect, do |
What type of PR is this?
Which issue(s) this PR fixes:
Related to #27445 (partial immutable-file experiment) and #28005. This does not close either issue, fix the reported timeout, or restore incremental mutation of a warm Search's Tail cache.
What this PR does / why we need it:
Add default-off, CN-lifecycle-local reuse of immutable FULLTEXT2 Base files. The ordinary build retains the ordinary loader; only
fulltext2_base_file_reuseenables this experiment. Repeated Search replacement can reuse immutable files while decoding private Segment/mmap/Index state and loading Tail/Delete separately. Metadata, request/transaction identity and permission checks precede acquisition.Resource and lifecycle contract
Revision and dependencies
Reviewed candidate:
dd6ebd7811aeb9fa3d6a62b0df95409b47471b03; frozen target base:ffcce7ecbab771d78edf8415db32880b133cce71onmain, not a claim of tracking the newest main.Delivered design and failure boundaries. Current engineering scope acceptance is under this repair authorization; no historical human approval or rollout approval is inferred.
Prerequisites are already merged: #29227, merge
ca71f51e2f962c0fbe0a92d9d01bb071bb1bb9c9, and #29291, merge5e965dece9ea15fa5d73c9e9615c59fbde651aaa. Merge order is those prerequisites before this experiment. No remaining implementation predecessor is required; deployment resources and rollout acceptance are separate.Developer validation and independent review
Local evidence: Go 1.27.1, darwin/arm64, CGo enabled with task-owned rebuilt native libraries. Actual embedded test-binary dependency resolution was observed and matched owned libmo/usearch plus the complete observed non-system dependency closure. Precommit build metadata is retained; delivered production bytes are bound to the final Git objects.
TestSQLWarmSearchCNCloseRestartpassed: fixed PK fixture, warm immutable Tail versus fresh replacement Tail, unchanged Base identity on reload, MERGE/rebuild, real close/restart, same-name recreation, snapshot/restore and second-account sessions. SQL does not assert a Base-file hit. Actual file-hit/fallback identity, source-stream counts and ordinary-loader PK/score-bit equivalence are proven by the real materializer component regression; this is not whole-ranking acceptance.BVT: the extended canonical
fulltext2_snapshot.sql/.resultwas run in normal comparison mode twice per build mode at historical24b3548864d63c840d8fa0452d746f4b25fb27d1: each run 43/43 with successful teardown and service exit. Exact dd6e standalone canonical BVT is NOT_RUN. SQL/result and representation are unchanged; final-byte real SQL/CN and consumer/race evidence renew the changed ownership paths. BVT N/A for deterministic OS cleanup faults and current-check/drain/retirement interleavings: SQL cannot select these internal barriers; real cache/materializer/registry fault regressions provide equivalent terminal coverage. Historical BVT is not relabeled as a final-head execution.Earlier failed/aborted environment or fixture runs remain failures in the evidence, with later terminal corrected executions separately bound; no zero-test, skip-only or hash-only success is counted.
Historical large-Base evidence — not new-head performance acceptance
Two separate Linux D→R pairs on earlier
986ca0a7plus test-only observers used 2,303,643 rows / 86 immutable Base segments, identical 1K INSERT and natural stale reload. Those observation patches are not enabled in this delivery.ft2b-largebase-20260928-040355ft2b-fixed-pair-20260929-094606Each reload had 64 hits / 22 capacity fallbacks and avoided 2,079,538,688 bytes each of Base streaming and successful materialization writes. The latter run directly recorded ~1.937 GiB retained, 64 pool FDs/mappings/leases, and zero owner accounting after successful shutdown. These are pooled resources, not complete CN RSS or fallback mappings.
Fixed PK/score-bit digests matched (19 queries / 8 nonempty earlier, 40 / 22 later). Both D and R had zero C10 timeouts and clean shutdown: this does not demonstrate repair of the historical 30-second timeout.
Keep
PERF_INCONCLUSIVE_HOST_BUSY. The historical pair is not pair 2/3 of the fixed series; reversed-order repeatability, comparable CPU, fallback/warm timing, normal-lifecycle frequency/net payback and final-main performance remain unestablished. Third-party initialization blocked the historicalcgocheck2attempt before target tests; it is not a safety pass.QA and review decision
QA required: yes. Automated developer coverage reaches public query load, cache replacement, failure/pending/retry resource cleanup, service-scoped generation retirement and real CN restart. Local semantic pre-review opens maintainer review only. Exact-head Linux/tagged CI, deployment QA, broader permission/multi-CN scenarios, final performance/net payback, whole-ranking acceptance and default enablement remain NOT_RUN or unaccepted. No QA handoff message, issue closure, merge or deployment is performed by this delivery.
The experiment remains default-off. Ready does not signify CI, QA, performance, merge or production acceptance. Rollback uses an ordinary build without the tag and clean service shutdown.