Skip to content

feat(fulltext2): add default-off immutable Base file reuse experiment - #29701

Closed
VioletQwQ-0 wants to merge 7 commits into
matrixorigin:mainfrom
VioletQwQ-0:codex/fulltext2-base-file-reuse-draft
Closed

VioletQwQ-0 wants to merge 7 commits into
matrixorigin:mainfrom
VioletQwQ-0:codex/fulltext2-base-file-reuse-draft

Conversation

@VioletQwQ-0

@VioletQwQ-0 VioletQwQ-0 commented Oct 8, 2026 •

Copy link
Copy Markdown
Collaborator

What type of PR is this?

  • API-change
  • BUG
  • Improvement
  • Documentation
  • Feature
  • Test and CI
  • Code Refactoring

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_reuse enables 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

  • The pool retains at most 4 GiB / 64 files. Capacity/FD/pinned/validation-byte refusal and specified recoverable cache failures use one ordinary fallback; source errors remain errors. Typed pending Segment or linked-path cleanup instead quarantines same-owner admission, without fallback.
  • Each pooled load has private decoded state. Live ordinary fallback mappings retain an owner lifetime pin through successful cleanup, including the cache-map-removal/pre-Free window. Failed unmap or unlink retains a reachable retry owner and its debt; a closed FD is not counted or closed again. Deferred debt is owner-retained and blocks same-owner admission, rather than continuous generic governor residency after cache eviction. This is not a cross-algorithm strict governor cap.
  • Cancel complete Preload/Load/fallback operations before shutdown waits. Current-entry checking, service cache drain, owner close and matching registry retirement share the entry's close lock. Dedicated/global protocol tests prove queued old closers cannot drain a replacement generation; this does not claim arbitrary concurrent global Cache.Destroy is supported.
  • No new SQL syntax, durable encoding, protocol, background refresh or cross-restart pool. CDC and compaction do not prewarm or mutate the pool. First fill still validates and decodes twice; READY hits still checksum and decode. Net performance remains unaccepted.

Revision and dependencies

Reviewed candidate: dd6ebd7811aeb9fa3d6a62b0df95409b47471b03; frozen target base: ffcce7ecbab771d78edf8415db32880b133cce71 on main, 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, merge 5e965dece9ea15fa5d73c9e9615c59fbde651aaa. 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.

  • Full FULLTEXT2 owning normal/race in ordinary and tagged modes passed on final production bytes. Six tag-only skips in ordinary mode are intentional, not evidence for the experiment. Vector cache/sqlexec relevant normal/race evidence is retained with unchanged-content binding.
  • Selected real CN/table-function/ISCP consumer normal/race: 78 cases each passed; these are selected consumers, not full suites.
  • Deterministic negative overlays on the preceding revisions exposed detached fallback cleanup, linked-file EACCES debt and dedicated/global generation-finalization defects. Corrected controls, rescue/join and fixed terminal assertions passed. New generation exact selector race100 passed (one top-level case and two subcases per repetition); prior fault/cancel regressions retain their own identities and results.
  • Rebuilt real embedded SQL/CN TestSQLWarmSearchCNCloseRestart passed: 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.
  • Formatting, lint, vet and complete diff checks passed. Independent Astra whole-diff A was frozen before B opened implementation/old reports/raw evidence. Final A→B accepted this limited default-off semantic scope with no unresolved in-scope code blocker or mandatory local evidence gap.

BVT: the extended canonical fulltext2_snapshot.sql/.result was run in normal comparison mode twice per build mode at historical 24b3548864d63c840d8fa0452d746f4b25fb27d1: 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 986ca0a7 plus 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.

Run Ordinary reload Reuse reload Saved First-load overhead
ft2b-largebase-20260928-040355 17.936 s 11.017 s 38.6% 0.505 s / 2.8%
ft2b-fixed-pair-20260929-094606 19.082 s 11.744 s 38.5% 0.451 s / 2.5%

Each 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 historical cgocheck2 attempt 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.

@qodo-code-review

Copy link
Copy Markdown

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

@cpegeric

Copy link
Copy Markdown
Contributor

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 (base_file_pool.go, base_file_owner.go) compile into every binary, and the change refactors the shared base-load path (storage.go) and CN-shutdown path (server.go) in place, so experiment-off builds still carry the risk (e.g. Segment.Free() now returns early on a munmap error and leaks the local spill file for ordinary loads too).

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 cpegeric left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. 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.
  2. 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).
  3. 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.
  4. 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.

@VioletQwQ-0
VioletQwQ-0 marked this pull request as draft October 10, 2026 10:27
@cpegeric

Copy link
Copy Markdown
Contributor

A new cache layer is only justified when it caches something the existing layers structurally cannot, and can't be extended
to. That test fails here.

The two current layers and what each owns:

  • Disk cache (fileservice) — owns raw object-store bytes.
  • Memory cache (VectorIndexCache) — owns the decoded Index, keyed by generation/checksum.

The gap the PR found is real: on a generation flip (CDC/MERGE/stale-TTL) the immutable base gets re-streamed from SQL →
re-materialized to a local spill file → re-decoded, even though its bytes never changed. Neither layer catches it: the disk
cache can't, because the base isn't read as a file (it's SQL chunk rows, not an object read); the memory cache can't,
because it's keyed by whole generation, so a new generation is a fresh key and the old base is thrown away.

But that gap lives inside the memory cache's own domain — it's caching the decoded index, and the base is just the immutable
part of that index. So the fix is to extend that layer, not add one beside it: split the entry so the decoded base is keyed
by base identity (segment id + checksum) and survives a generation flip, while tail/delete reload per generation. That:

  • reuses the cache's existing eviction, governor accounting, and lifecycle (all already proven),
  • and saves the decode too — which the PR's file pool doesn't (a READY hit still re-decodes; it only saves the stream +
    materialize).

So the PR's new layer is strictly worse than extending the existing one: more code, a brand-new mmap/pin/debt/quarantine
failure surface, and it captures less of the cost. That's the core of why a third layer is the wrong shape.

The one honest caveat so you're not caught out: the gap is real, so the answer isn't "caching is already perfect, do
nothing." It's "prove the base-reload cost is actually material first; if it is, close it by making the memory cache
base-aware — not by adding a third cache." That's the position I'd stand on, and it's consistent with what I posted.

This branch had an error being deployed

1 failed deployment
ci — dd6ebd78 Deployed Oct 10, 2026 by VioletQwQ-0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/enhancement size/XXL Denotes a PR that changes 2000+ lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants