Skip to content

perf(fulltext2): reuse immutable bases across cache generations - #27462

Merged
XuPeng-SH merged 24 commits into
matrixorigin:mainfrom
VioletQwQ-0:codex/fulltext2-generation-reuse-pr
Aug 25, 2026
Merged

perf(fulltext2): reuse immutable bases across cache generations#27462
XuPeng-SH merged 24 commits into
matrixorigin:mainfrom
VioletQwQ-0:codex/fulltext2-generation-reuse-pr

Conversation

@VioletQwQ-0

@VioletQwQ-0 VioletQwQ-0 commented Aug 21, 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:

issue #27445

What this PR does / why we need it:

Reuses immutable FULLTEXT2 base segment mappings across cache generations and loads only new CDC tail/delete frames when the base is unchanged. This PR is stacked on codex/fulltext2-load-observability-pr and contains the second semantic commit for reusable bases.

The lifecycle repair:

  • records each local FULLTEXT2 invalidation once and uses reasonless cache removal after explicit reason registration;
  • preserves MERGE/REBUILD miss reasons while clearing reusable pools, with one invalidation-generation bump per event;
  • keeps DROP/restore cleanup reasonless so a later same-name load cannot inherit a misleading rebuild cause;
  • uses monotonic per-index invalidation/load generations so stale attempts cannot publish or consume over newer replacements;
  • scopes failed-load rollback to claims held by that attempt and preserves reusable state reused or committed by a newer load;
  • rejects stale tail-state installation while retaining lease-safe cleanup for active readers;
  • preserves the required MatrixOne cancellation classification and lifecycle-hook cleanup.

The PR does not change SQL/DDL, ranking, index format, CDC wire protocol, or cross-CN freshness semantics.

Validation

  • GOWORK=off .agents/skills/mo-dev/scripts/mo-cgo-test -mod=readonly -count=1 -timeout=240s ./pkg/fulltext2 — PASS.
  • GOWORK=off .agents/skills/mo-dev/scripts/mo-cgo-test -mod=readonly -race -count=1 -timeout=240s ./pkg/fulltext2 — PASS.
  • Full normal and race suites for pkg/fulltext2 and pkg/vectorindex/cache — PASS.
  • Focused reasonless-DROP regression passes normally and under -race -count=100; it holds active base/tail leases, invalidates through NewFulltext2Search, rejects stale publication, and verifies final pool cleanup.
  • Production-shaped publication-before-remove regressions for both VectorIndexCache.Search and SearchInto now exact-delete/destroy a superseded failed entry and transparently retry — normal PASS and exact -race -count=100 PASS.
  • Deterministic HouseKeeping concurrent-renewal regression restores the exact pre-claim expiry/staleness check — normal PASS and exact -race -count=100 PASS.
  • Merge-focused observer/cache lifecycle regressions and go list -mod=readonly — PASS.
  • git diff --check, self-review, and semantic preflight pass on exact clean head 700763d8207d17664bf5d8a5a980c3136a795871 with diff hash 722f5232ec11c4ae8e8f4ded94849b313bb75f13d8cc21084af14a5153a0ff07.
  • Direct go build and go vet remain blocked by the same reproducible clean-merge-base pkg/common/docfilter C declaration failure; no build/vet pass is claimed.
  • BVT: N/A for this internal FULLTEXT2 cache/load lifecycle repair because there is no SQL-visible behavior change; production-path regression coverage is present.

QA required: yes — this changes CDC invalidation, mmap/temp-file ownership, cancellation, and generation lifecycle. Production terminal: FULLTEXT2 MATCH across tail refresh, TTL, MERGE, REBUILD, and restart.

Current status: Ready for review; exact-head CI is running and XuPeng-SH/aptend re-review is pending on pushed head 700763d8207d17664bf5d8a5a980c3136a795871. QA remains pending.

Non-goals:

  • No persistent format or CDC protocol change.
  • No new public configuration, scan reuse, cross-query cache, or cross-CN SLA change.

Current-head follow-up

  • Addressed XuPeng-SH's current-head reason/generation finding in commit 86eab9e8bfa07d679c5f0f0ef7f100c0bbaa5b4d.
  • Reasonless DROP now invalidates the generation before reusable pool clearing; stale base/tail publication is rejected before retirement.
  • Merged the current upstream/main, integrated observer/cache lifecycle coverage, and preserved durable generation for empty-tail loads.
  • Added the deterministic active-lease reasonless-DROP regression and posted exact validation with no duplicate reviewer request.
  • Addressed the current-head generation-supersession and HouseKeeping renewal findings in commit 700763d8207d17664bf5d8a5a980c3136a795871; both public cache entrypoints now retry exact failed entries, and housekeeping revalidates expiry/staleness before eviction.

@matrix-meow matrix-meow added the size/XL Denotes a PR that changes [1000, 1999] lines label Aug 21, 2026
@matrix-meow matrix-meow added size/XXL Denotes a PR that changes 2000+ lines and removed size/XL Denotes a PR that changes [1000, 1999] lines labels Aug 23, 2026

@XuPeng-SH XuPeng-SH 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.

Deep review of exact head a4000eb.

The latest waiting-owner, published-base, and lifecycle-hook panic fixes close the previously reported races. One blocking observability/generation defect remains:

MERGE and REBUILD immediately erase the miss reason they just recorded. invalidateLoadGeneration first calls rememberLoadReason, which increments the per-index invalidation and stores the reason under that generation. It then calls clearReusableLoadGeneration for merge/rebuild; that calls clearLoadGeneration, which deletes pendingLoadReasons[index] and increments invalidation a second time. The production CREATE/REBUILD, MERGE, and plugin paths all enter through this sequence. Consequently the actual cold reload is reported as process_start instead of merge/rebuild, and one logical invalidation advances the generation twice.

I reproduced this through invalidateLoadGeneration with the observer enabled: peekLoadReason returned empty for both merge and rebuild in the normal run and in 20/20 race runs. Existing coverage invokes these branches but never asserts that the reason survives.

Please separate reasonful invalidation from reasonless DROP cleanup: clear the reusable base/tail pools for MERGE/REBUILD while preserving the newly recorded pending reason and without a second generation bump. Add regressions proving each reason survives failed/canceled attempts and is consumed only by a successful current load.

Validation after removing the temporary counterexample: full pkg/fulltext2 and pkg/vectorindex/cache tests pass normally and under race; the four latest focused regressions pass under race for 100 repetitions; go list/build/vet, gofmt, git diff --check, and index-plugin guards pass.

@VioletQwQ-0

Copy link
Copy Markdown
Collaborator Author

Addressed XuPeng-SH's current-head observability/generation finding in commit 010f30d9d26f230a311a9093d58c613f6f102779.

  • MERGE and REBUILD now clear only the reusable base/tail pools after recording the reason. The pending reason is preserved and the invalidation generation advances once.
  • Reasonless cleanup remains the DROP/restore path. HandleDropIndex now uses that path, so DROP cannot leave a misleading REBUILD reason for a later same-name load.
  • Added production-path regressions for Merge and Rebuild reason retention, single generation bumps, failed/cancelled retries, successful current-load consumption, and reasonless cleanup.

Validation on the exact clean head:

  • Focused regressions: PASS, including -race -count=100.
  • Full pkg/fulltext2 and pkg/vectorindex/cache normal/race suites: PASS.
  • Parser coverage, go list, git diff --check, self-review, and semantic preflight: PASS.
  • Direct go build/go vet remain blocked by the same upstream-baseline pkg/common/docfilter C declaration failure; no build/vet pass is claimed.
  • BVT: N/A for this internal cache/load lifecycle repair; existing distributed FULLTEXT2 MERGE/REBUILD terminal coverage remains required.

Please re-review exact head 010f30d9d26f230a311a9093d58c613f6f102779.

@XuPeng-SH XuPeng-SH 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.

Deep re-review of exact head 010f30d.

The previous MERGE/REBUILD reason bug is fixed: reasonful invalidation now advances the generation once, preserves the pending reason across failed attempts, and clears only the reusable pools. One blocking DROP generation-order race remains.

clearReusableLoadGeneration currently clears the base and tail pools first, then calls clearLoadGeneration. An already-active load is therefore still current during the interval after the final pool clear. It can publish a base or tail state in that interval; the later generation bump makes the load obsolete but does not clear what it just published. HandleDropIndex now routes production DROP through exactly this reasonless path, and the following reasonless Cache.Remove deliberately does not invoke the invalidation hook again. DROP can consequently return with old-generation mappings/state retained in the reusable pools, up to their multi-GiB bounds, until a later lifecycle sweep. This also leaves stale material eligible if the key/generation metadata is reused.

I reproduced the ordering deterministically without sleeps using the production generation and tail-pool operations: hold pendingLoadReasons so DROP completes its pool clear and pauses immediately before clearLoadGeneration; publish through installAndAcquireIfCurrent from the still-current active generation; release DROP; the newly published tail remains in loadedTailPool. The assertion failed normally and in 20/20 race runs. The same ordering exists for base commit/acquire.

Please invalidate/clear the load generation before performing the final reusable-pool clear, while preserving the existing lock order by releasing the generation locks before taking pool locks. Then old loads cannot pass their current-generation publication checks, and the final clear removes any state created before invalidation. Add the deterministic active-load-vs-reasonless-DROP regression for both publication ownership and final pool emptiness.

Validation after removing the temporary counterexample: the three new reason regressions pass independently under race for 100 repetitions; full pkg/fulltext2, pkg/fulltext2/plugin/compile, and pkg/vectorindex/cache tests pass normally and under race; all five changed owning packages pass go list/build/vet; gofmt, git diff --check, and index-plugin guards pass.

@VioletQwQ-0

Copy link
Copy Markdown
Collaborator Author

Addressed the current-head generation/lifecycle finding and pushed commit 86eab9e8bfa07d679c5f0f0ef7f100c0bbaa5b4d.

  • Reasonless DROP now invalidates the load generation before clearing reusable base/tail pools, so stale base publication and tail installation are rejected before pool retirement.
  • Merged the current upstream/main and integrated its observer/cache lifecycle coverage.
  • Added a deterministic production-shaped regression that holds active base/tail leases, triggers reasonless DROP through NewFulltext2Search, observes generation removal, rejects stale publication, and verifies both pools are empty.
  • Preserved the durable generation from the same load snapshot for empty-tail loads so observer state remains coherent.

Validation on the exact clean head:

  • Focused normal and race tests pass; the focused race regression also passes with -race -count=100.
  • Full normal and race suites pass for pkg/fulltext2, pkg/fulltext2/plugin/compile, and pkg/vectorindex/cache; merge-focused observer/cache tests and go list -mod=readonly pass.
  • Semantic preflight passes for exact head 86eab9e8bfa07d679c5f0f0ef7f100c0bbaa5b4d with diff hash ae85d1ddcd6a11d1b54233d841729d80d0b07a4f4fd077a365f927ce3d41cf97; self-review and git diff --check pass.
  • Direct go build and go vet remain blocked by the reproducible clean-merge-base pkg/common/docfilter C declaration failure; no build/vet pass is claimed.
  • BVT: N/A. This is an internal FULLTEXT2 cache/load lifecycle repair with no SQL-visible behavior change; production-path regression coverage is present.

Please re-review the exact pushed head. No duplicate reviewer request was added; the existing XuPeng-SH request remains active.

@XuPeng-SH XuPeng-SH 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.

Deep re-review of exact head 86eab9e8bfa07d679c5f0f0ef7f100c0bbaa5b4d.

The previous DROP-ordering blocker is fixed: clearReusableLoadGeneration now invalidates the load generation before clearing the reusable pools, and the new regression test closes the stale-publish window. However, two independent blockers remain:

  1. [P1 correctness] Generation supersession is exposed to SQL as an internal error instead of being retried by the cache.

    The production mutation sequence publishes OnCacheInvalidated(reason) before Cache.Remove(key). If an in-flight miss observes the new generation in that interval, Fulltext2Search.Load returns errLoadGenerationSuperseded. At that moment algo.evicting is still false, and VectorIndexCache.Search/SearchInto only retry an evicting entry or ErrInvalidState, so the user receives:

    internal error: fulltext2 load superseded by a newer generation

    I reproduced this through the real local VectorIndexCache and Fulltext2Search: a SQL mock publishes LoadMissCDCFlush during the first load, before any remove claim. The expected transparent retry instead failed with the error above, both normally and 20/20 times under -race -count=20.

    Please make generation supersession an exact-entry cache retry: exact-delete/destroy the failed mapped entry and retry. Merely changing the sentinel to ErrInvalidState is not sufficient with the current branch, because it continues without removing the non-evicting failed entry. Please add production-path coverage for the publication-before-remove window for both Search and SearchInto unless the handling is centralized.

  2. [P1 performance] HouseKeeping evicts an exact entry even if a concurrent successful search renewed its sliding TTL after the snapshot.

    This PR preserves exact identity in the snapshot, but drops the pre-claim Expired()/stale recheck that existed in the base implementation. A deterministic two-entry test demonstrates the race: housekeeping snapshots both expired entries, the first invalidation callback blocks, a successful search renews the second entry, and housekeeping still evicts that same second entry after it resumes. The test failed 20/20 times under -race -count=20.

    This forces an unnecessary full cold load (the expensive path this PR is meant to avoid) and publishes a phantom TTL invalidation reason. Please restore an exact pre-claim condition such as:

    if !entry.algo.Expired() && !entry.algo.stale.Load() { continue }

    immediately before computing the reason / calling evictEntry, and add the concurrent-renewal regression.

Validation performed on the locked head:

  • full normal and race suites passed for pkg/fulltext2, pkg/fulltext2/plugin/compile, and pkg/vectorindex/cache;
  • the DROP-generation regression and adjacent generation tests passed 100/100 under race;
  • go list -mod=readonly, build, vet, gofmt/diff checks, and index-plugin architecture guards passed;
  • lifecycle audit found no additional confirmed leak, hang, or unbounded-growth defect.

@aptend aptend 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.

Deep re-review of exact head 86eab9e8bfa07d679c5f0f0ef7f100c0bbaa5b4d. I read all prior reviews, inline threads, resolutions, author replies, and issue comments; compared the incremental changes since my previous reviewed head d6b7b627bd447a422e3ef34e8ae2b8730f931ed2; and audited the complete base diff, including cache/generation ownership, cleanup, failure, cancellation, reuse, and shutdown paths.

The previous reason-retention and reasonless-DROP generation-ordering issues are closed. Two blocking races remain, detailed inline: a normal mutation can expose the internal generation-supersession sentinel to SQL instead of retrying, and housekeeping can evict an exact entry after a successful search renewed its TTL.

I reproduced both with production-shaped deterministic tests on the locked head, normally and under -race; the temporary tests were removed. After removal, full normal and race suites passed for pkg/fulltext2, pkg/fulltext2/plugin/compile, and pkg/vectorindex/cache; go vet and diff checks passed. Current GitHub required checks pass.

Comment thread pkg/vectorindex/cache/cache.go Outdated
Comment thread pkg/vectorindex/cache/cache.go

@aunjgr aunjgr 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.

Deep re-review of exact head 86eab9e8bfa07d679c5f0f0ef7f100c0bbaa5b4d against merge base 6dccc954a1a32c12a2114328c752f15cf97ef2ba.

The reasonless-DROP ordering and stale reusable-pool publication paths are closed, but two independent blockers remain.

[P1 correctness] Treat generation supersession as an exact-entry cache retry, not a SQL error

FULLTEXT2 mutation paths publish OnCacheInvalidated(reason) before calling Cache.Remove (for example pkg/iscp/fulltext2_consumer.go:123-132, with the same order in compact/rebuild). In the valid interval after the generation bump but before removal claims the cache entry, an in-flight Fulltext2Search.Load observes !loadGenerationCurrent and returns errLoadGenerationSuperseded (pkg/fulltext2/search_cache.go:161-168). The containing cache entry is not yet evicting, and the sentinel is an Internal error.

VectorIndexCache.Search therefore takes its ordinary load-error branch (pkg/vectorindex/cache/cache.go:535-543): it exact-deletes/destroys that failed entry and returns the internal error to the SQL statement. SearchInto has the same behavior at lines 570-577. A normal CDC/MERGE/REBUILD race is thus visible as internal error: fulltext2 load superseded by a newer generation instead of transparently retrying the replacement generation.

Make supersession a dedicated retryable-load outcome: remove/destroy only the exact failed mapped entry, then retry. Simply mapping it to ErrInvalidState is insufficient with the current branch because lines 537-539/571-573 continue without removing the non-evicting failed entry. Add production-cache regressions for the publish-before-remove interval through both Search and SearchInto (or one centralized handler exercised by both).

[P1 performance] Recheck expiry/staleness immediately before claiming the exact entry

HouseKeeping snapshots entries that are expired/stale at lines 443-449, but lines 451-456 call evictEntry without revalidating that condition. A successful search on the snapshotted entry can run in between and renew its sliding TTL through extend(false) (cache.go:265-271,308-309). Housekeeping still sets evicting, publishes a phantom ttl_expired reason, removes the exact renewed entry, and destroys it.

Concrete deterministic ordering: snapshot expired entries A and B; A's invalidation hook blocks the sweep; a search on B succeeds and renews B; release A; the sweep still evicts B. This forces the next query to remap/reload the potentially multi-GiB base—the expensive path this PR exists to avoid—and records a false miss reason.

Restore an exact pre-claim condition recheck immediately before reason selection/evictEntry, while retaining stale entries as unconditionally reclaimable. Add a barrier-synchronized two-entry regression proving a renewed TTL entry survives and no TTL invalidation is published.

Q1 lease/mmap retirement, Q2 waiter/cancellation cleanup, Q3 pool/registry bounds, diff cleanliness, and index-plugin architecture guards otherwise look closed.

@aunjgr aunjgr 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.

Re-review of exact head 700763d8207d17664bf5d8a5a980c3136a795871.

The superseded-load path is now internally retryable and exact-entry cleanup is correct. The new generation tests exercise the publication-before-remove race through both Search and SearchInto. However, the TTL renewal blocker is narrowed, not closed.

[P1] Make expiry validation atomic with the eviction claim

HouseKeeping now rechecks Expired() at pkg/vectorindex/cache/cache.go:458, but there is still a TOCTOU window before evictEntry calls beginEviction() at line 465. A concurrent Search can acquire the shared mutex, observe evicting == false, and execute extend(false) in that window. Housekeeping then successfully claims and removes the entry even though its TTL has just been renewed.

The new two-entry test only renews the second entry before the line-458 recheck, so it cannot expose this remaining interleaving:

  1. housekeeping's line-458 recheck observes the old expiry;
  2. search enters under Mutex.RLock and stores a fresh ExpireAt;
  3. housekeeping sets evicting and removes the entry.

The expiry decision and eviction claim must be serialized against Search.extend, for example by taking the entry write lock, rechecking expiry/staleness there, and claiming eviction before releasing it (with destruction structured so the lock is not recursively acquired). A generation/token CAS is another option. Please add a deterministic hook/barrier test at this exact recheck-to-claim boundary and assert that a successful renewal prevents TTL eviction. Stale-marked entries should remain unconditionally reclaimable.

I found no other blocker in the incremental fix.

@aptend aptend 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.

Deep re-review of exact head 700763d8207d17664bf5d8a5a980c3136a795871. I read the complete review/thread/reply history, compared the delta from my prior reviewed head 86eab9e8bfa07d679c5f0f0ef7f100c0bbaa5b4d, and rechecked the complete PR diff. The exact-delete/retry path fixes the initiating loader, and the housekeeping recheck fixes renewal that happens before that recheck, but two blocking concurrency windows remain: waiters can still observe the superseded load as an internal error, and a renewal between the expiry recheck and eviction claim can still be discarded.

Validation: full pkg/fulltext2, pkg/fulltext2/plugin/compile, and pkg/vectorindex/cache tests passed normally and under -race; the new focused regressions passed under -race -count=100; go vet passed. Two temporary deterministic counterexamples reproduced the remaining windows and were removed. The worktree is clean.

Comment thread pkg/vectorindex/cache/cache.go Outdated
Comment thread pkg/vectorindex/cache/cache.go

@XuPeng-SH XuPeng-SH 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.

Deep re-reviewed exact head 700763d8207d17664bf5d8a5a980c3136a795871, including the incremental fix, the full cache/load lifecycle, and prior review history.

The initiating superseded load is now removed and retried, and the added focused tests pass. Two blocking concurrency windows remain:

  1. [P1 correctness] Make every waiter retry a superseded shared load.

    VectorIndexSearch.Load stores STATUS_ERROR and releases/broadcasts before the initiating goroutine calls discardFailedLoad. A query that already found this map entry and is waiting on its read lock can acquire it in that interval. VectorIndexSearch.Search / SearchInto see STATUS_ERROR and return NewInternalError("Load index error"); the outer cache loop retries only ErrInvalidState, so this normal generation race still leaks an internal error to SQL for waiters.

    The new retry tests exercise only the goroutine that owns Load; the production-shaped FULLTEXT2 test is also single-caller. Please make the superseded outcome retryable for all callers sharing the failed entry, with exact-entry cleanup/destruction remaining single-owner, and add a barrier test with one loader plus at least one already-blocked waiter for both cache APIs (or their centralized path).

  2. [P1 performance/lifecycle] Make TTL validation atomic with the eviction claim.

    HouseKeeping rechecks Expired(), then calls evictEntry; beginEviction() is still later and does not serialize with the entry mutex. Between those operations, Search can acquire the shared entry lock, observe evicting == false, successfully search, and refresh ExpireAt via extend(false). Housekeeping then claims and destroys that just-renewed entry, publishes a false ttl_expired reason, and forces an avoidable cold reload.

    The new two-entry test renews before the added recheck, so it cannot exercise the recheck-to-claim interleaving. Serialize expiry/staleness validation and the eviction claim against search renewal, and add a deterministic barrier at that exact boundary. Stale-marked entries should remain reclaimable regardless of TTL renewal.

Validation on the locked head: the new retry and concurrent-renewal focused tests pass with the deterministic CGo wrapper; they do not cover the two windows above.

@aptend aptend 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.

Deep re-review of exact head 90219ad7ce2e03ee5c56e4cc637502810e755777 against base 6dccc954a1a32c12a2114328c752f15cf97ef2ba.

I read the complete review/thread/reply history, compared the incremental diff from my prior reviewed head 700763d8207d17664bf5d8a5a980c3136a795871, and rechecked the complete cache/load lifecycle diff.

Both prior blockers are closed. Retryable ErrInvalidState loads publish STATUS_DESTROYED before broadcasting, so already-blocked Search/SearchInto waiters return the retryable outcome instead of Load index error; exact-entry deletion/destruction remains owned by the cache retry path. Sliding TTL renewal and the final ttl_expired eviction decision/claim now share ttlMu, while stale/generation-changed eviction remains unconditional. Lock ordering keeps resource destruction outside the TTL gate and search read lock.

Validation on this exact head: full normal and race suites passed for pkg/vectorindex/cache, pkg/fulltext2, and pkg/fulltext2/plugin/compile; the two new waiter/TTL regressions passed under -race -count=100; go vet and git diff --check passed. The worktree remained clean.

@XuPeng-SH XuPeng-SH 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.

Deep re-review of exact head 90219ad7ce2e03ee5c56e4cc637502810e755777 against merge base 6dccc954a1a32c12a2114328c752f15cf97ef2ba.

The two previously requested fixes are closed:

  • a superseded load now publishes STATUS_DESTROYED before broadcasting, so initiating callers and already-blocked Search/SearchInto waiters all take the retry path; exact-entry destruction remains single-owner;
  • sliding-TTL renewal and the final TTL eviction check/claim are serialized by ttlMu, while stale/generation eviction remains unconditional. The lock graph has no reverse held-lock edge because eviction releases ttlMu before entering Destroy.

One independent blocker remains:

[P1 correctness/liveness] Do not retry every Load error whose MO code is ErrInvalidState

VectorIndexSearch.Load maps every ErrInvalidState from every algorithm to STATUS_DESTROYED at pkg/vectorindex/cache/cache.go:246-255. Both generic cache APIs then exact-delete the entry and unconditionally retry forever at lines 580-588 and 619-625. That error code is not exclusive to FULLTEXT2 generation supersession.

There is a concrete production path in IVF-FLAT:

  1. planReader installs its real relation scanner and calls cache.Cache.Search (pkg/vectorindex/ivfflat/plan_reader.go:253-255, 369-370, 419);
  2. IvfflatSearch.Load calls LoadCentroids;
  3. the scanner returns moerr.NewInvalidStateNoCtxf("ivfflat hidden relation ... has no table definition") when GetTableDef is nil (plan_reader.go:541-552);
  4. the cache treats that persistent catalog/storage state as “load superseded”, destroys the exact entry, and immediately repeats the same load with the same transaction/state.

The query therefore does not receive the real error. It loops doing relation scans and entry construction/destruction. LoadCentroids also logs IVFFLAT START/END: Load Centroids at INFO on every attempt (search.go:71-74), so this is also a deterministic log storm.

I confirmed the cache behavior with a temporary terminating probe: an algorithm returned a permanent ErrInvalidState; VectorIndexCache.Search exceeded 100 loads without returning, and stopped only when the probe switched to a non-retryable error for cleanup. This reproduced 20/20 normally and 20/20 under -race. The temporary test was removed.

Please use a dedicated retryable-load outcome/marker for generation supersession instead of the shared MO error code, and preserve the destroyed-before-broadcast waiter behavior only for that outcome. Add controls proving:

  • the dedicated superseded outcome retries for both initiating loaders and blocked waiters through Search and SearchInto;
  • an ordinary/permanent ErrInvalidState load is destroyed exactly once and returned to the caller without retry.

Validation on the locked head:

  • all five focused supersession/TTL regressions passed under -race -count=100;
  • an additional public-cache loader+waiter probe for both APIs passed under -race -count=100;
  • full normal and race suites passed for pkg/vectorindex/cache, pkg/fulltext2, and pkg/fulltext2/plugin/compile;
  • list/build/vet, gofmt, diff checks, and index-plugin guards passed;
  • parallel no-op cache-hit benchmark: previous head ~166 ns/op vs current ~190 ns/op, 0 allocs (about +24 ns fixed cost; bounded and not a blocker).

@aunjgr aunjgr 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.

Re-reviewed exact head 90219ad7ce2e03ee5c56e4cc637502810e755777 against merge base 6dccc954a1a32c12a2114328c752f15cf97ef2ba.

Both prior blockers are closed. ttlMu now makes the final expiry check and eviction claim atomic with Search/SearchInto renewal; the lock ordering releases that gate before destruction waits on the entry RWMutex, so the search, load, remove, stale, and shutdown paths do not form a cycle. Retryable superseded loads publish STATUS_DESTROYED before broadcasting, allowing existing waiters as well as the initiating caller to re-enter through a replacement entry. Exact-entry cleanup and generation ownership remain intact. The full lifecycle and plugin-dispatch guard found no blocking issue.

@aptend aptend 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.

Deep re-review of exact head 90219ad against base 6dccc95.

The head is unchanged since my previous approval. For this new request I re-read all 37 historical reviews, 32 inline comments and replies, all 16 thread states, and the issue comments; rechecked the complete diff and the zero delta from my prior reviewed head.

The prior waiter and TTL blockers remain closed. However, one independent P1 correctness/liveness blocker is confirmed: the generic cache treats every algorithm Load error with MO code ErrInvalidState as the FULLTEXT2 generation-supersession signal. A permanent IVF-FLAT catalog error is therefore exact-deleted and retried forever instead of being returned.

I independently reproduced this through both public cache APIs. The probe returned permanent ErrInvalidState four times and a distinct terminating error on the fifth call; Search and SearchInto each performed all four reload/destroy cycles and returned only the artificial fifth error. A truly permanent error never terminates. The temporary probe was removed.

Validation on the locked head after removal:

  • full normal tests passed for pkg/vectorindex/cache, pkg/fulltext2, and pkg/fulltext2/plugin/compile
  • focused supersession/waiter/TTL race tests passed for 20 repetitions
  • go vet and git diff --check passed
  • worktree is clean

Comment thread pkg/vectorindex/cache/cache.go Outdated
@VioletQwQ-0

Copy link
Copy Markdown
Collaborator Author

Addressed the current-head load-error classification finding in 4da2f788ad2.

  • Added a cache-internal retryable-load marker. Only FULLTEXT2 generation supersession uses it.
  • Ordinary algorithm Load errors, including moerr.ErrInvalidState, now publish a terminal error; Search and SearchInto remove and destroy the exact failed entry once, then return the original error.
  • Added the public Search/SearchInto regression for a permanent ErrInvalidState load, plus controls for superseded-load retry and waiter behavior.

Validation on the repaired local head:

  • Focused and full normal CGo-backed tests for ./pkg/vectorindex/cache ./pkg/fulltext2 ./pkg/fulltext2/plugin/compile: PASS.
  • Focused -race -count=100 repetitions and package race tests: PASS.
  • git diff --check: PASS.
  • Candidate build/vet remain blocked by the same four existing pkg/common/docfilter C declaration errors reproduced on the clean reviewed merge base; this is a baseline/toolchain blocker, not a new error from this change.

@VioletQwQ-0

Copy link
Copy Markdown
Collaborator Author

Additional gate note: BVT: N/A. This repair changes an internal vector-index cache load/error-classification contract with no SQL-visible syntax, result, or persisted-format change. The production VectorIndexCache.Search and SearchInto entry points are covered directly by deterministic normal and race tests, including ordinary permanent ErrInvalidState and marked supersession paths.

@aptend aptend 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.

Deep re-review of exact head 4da2f78 against base 6dccc95 and my prior reviewed head 90219ad. I re-read the complete review, inline-thread, resolution, reply, and issue-comment history; checked the incremental fix and re-audited the complete cache/generation/lease lifecycle diff. The previous blocker is closed: only the dedicated cache-internal retryable-load marker now publishes STATUS_DESTROYED and retries, while an ordinary ErrInvalidState remains terminal, is exact-deleted/destroyed once, and is returned to the caller. Existing loader/waiter coverage passes, and an independent public-cache loader-plus-waiter counterexample passed for both Search and SearchInto under race for 100 repetitions. Full normal and race suites passed for pkg/vectorindex/cache, pkg/fulltext2, and pkg/fulltext2/plugin/compile; go vet and diff checks passed. No blocking issues found.

@XuPeng-SH XuPeng-SH 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.

Deep re-review on 4da2f78. The prior correctness/liveness blocker is closed.

The generic cache now distinguishes FULLTEXT2 generation supersession with a dedicated retryable-load marker. Only that marker publishes STATUS_DESTROYED before broadcasting and enters exact-entry delete/destroy/retry; an ordinary ErrInvalidState remains terminal, is destroyed once, and is returned unchanged. I rechecked initiating-loader and blocked-waiter behavior for Search and SearchInto, TTL renewal versus eviction claim, exact-entry replacement cleanup, load-generation ownership, base/tail lease retirement, bounded registries, cancellation, and shutdown hooks. No new correctness, liveness, resource, performance, or log-storm blocker was found.

Validation on this exact head:

  • full normal and race suites passed for pkg/fulltext2, pkg/vectorindex/cache, and pkg/fulltext2/plugin/compile;
  • focused retry/permanent-error tests passed under race for 100 repetitions;
  • focused generation/rollback/TTL/reasonless-drop tests passed under race for 20 repetitions;
  • go vet, gofmt, and git diff --check passed.

@mergify

mergify Bot commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Merge Queue Status

  • Entered queue2026-08-25 10:27 UTC · Rule: main · triggered by rule Automatic queue on approval for main
  • 🟠 Checks running · in-place
  • 🚫 Left the queue2026-08-25 10:45 UTC · at 147b76e17c8e3edf50be18cd5fad4a65d3d65423

This pull request spent 17 minutes 45 seconds in the queue, with no time running CI.

Waiting for
  • any of: [🛡 GitHub branch protection]
    • check-neutral = Matrixone CI / UT Test on Ubuntu/x86
    • check-skipped = Matrixone CI / UT Test on Ubuntu/x86
    • check-success = Matrixone CI / UT Test on Ubuntu/x86
  • any of: [🛡 GitHub branch protection]
    • check-neutral = Matrixone CI / SCA Test on Linux/arm64
    • check-skipped = Matrixone CI / SCA Test on Linux/arm64
    • check-success = Matrixone CI / SCA Test on Linux/arm64
  • any of: [🛡 GitHub branch protection]
    • check-neutral = Matrixone Compose CI / multi cn e2e bvt test docker compose(PROXY)
    • check-skipped = Matrixone Compose CI / multi cn e2e bvt test docker compose(PROXY)
    • check-success = Matrixone Compose CI / multi cn e2e bvt test docker compose(PROXY)
  • any of: [🛡 GitHub branch protection]
    • check-neutral = Matrixone Utils CI / Coverage
    • check-skipped = Matrixone Utils CI / Coverage
    • check-success = Matrixone Utils CI / Coverage
  • any of: [🛡 GitHub branch protection]
    • check-neutral = Matrixone UT Coverage / UT Coverage on Ubuntu/x86
    • check-skipped = Matrixone UT Coverage / UT Coverage on Ubuntu/x86
    • check-success = Matrixone UT Coverage / UT Coverage on Ubuntu/x86
  • any of: [🛡 GitHub branch protection]
    • check-neutral = Matrixone Standlone CI / multi CN e2e BVT Test on Linux/x64(COMPOSE, PESSIMISTIC)
    • check-skipped = Matrixone Standlone CI / multi CN e2e BVT Test on Linux/x64(COMPOSE, PESSIMISTIC)
    • check-success = Matrixone Standlone CI / multi CN e2e BVT Test on Linux/x64(COMPOSE, PESSIMISTIC)
All conditions
  • any of [🛡 GitHub branch protection]:
    • check-neutral = Matrixone CI / UT Test on Ubuntu/x86
    • check-skipped = Matrixone CI / UT Test on Ubuntu/x86
    • check-success = Matrixone CI / UT Test on Ubuntu/x86
  • any of [🛡 GitHub branch protection]:
    • check-neutral = Matrixone CI / SCA Test on Linux/arm64
    • check-skipped = Matrixone CI / SCA Test on Linux/arm64
    • check-success = Matrixone CI / SCA Test on Linux/arm64
  • any of [🛡 GitHub branch protection]:
    • check-neutral = Matrixone Compose CI / multi cn e2e bvt test docker compose(PROXY)
    • check-skipped = Matrixone Compose CI / multi cn e2e bvt test docker compose(PROXY)
    • check-success = Matrixone Compose CI / multi cn e2e bvt test docker compose(PROXY)
  • any of [🛡 GitHub branch protection]:
    • check-neutral = Matrixone Utils CI / Coverage
    • check-skipped = Matrixone Utils CI / Coverage
    • check-success = Matrixone Utils CI / Coverage
  • any of [🛡 GitHub branch protection]:
    • check-neutral = Matrixone UT Coverage / UT Coverage on Ubuntu/x86
    • check-skipped = Matrixone UT Coverage / UT Coverage on Ubuntu/x86
    • check-success = Matrixone UT Coverage / UT Coverage on Ubuntu/x86
  • any of [🛡 GitHub branch protection]:
    • check-neutral = Matrixone Standlone CI / multi CN e2e BVT Test on Linux/x64(COMPOSE, PESSIMISTIC)
    • check-skipped = Matrixone Standlone CI / multi CN e2e BVT Test on Linux/x64(COMPOSE, PESSIMISTIC)
    • check-success = Matrixone Standlone CI / multi CN e2e BVT Test on Linux/x64(COMPOSE, PESSIMISTIC)
  • #review-threads-unresolved = 0 [🛡 GitHub branch protection]
  • github-review-approved [🛡 GitHub branch protection]

Reason

Pull request #27462 has been dequeued

Pull request from fork cannot be queued. This pull request comes from a fork, and Mergify needs the author's permission to update its branch.

The author needs to enable "Allow edits from maintainers" on this pull request.

Failing checks:

Hint

You should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it.
If you do update this pull request, it will automatically be requeued once the queue conditions match again.
If you think this was a flaky issue instead, you can requeue the pull request, without updating it, by posting a @mergifyio queue comment.

Tick the box to put this pull request back in the merge queue (same as @mergifyio queue).

  • Requeue this pull request

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants