Skip to content

fix(plan): make fulltext2 json_extract index probe reachable on current reads (#27926) - #28025

Open
cpegeric wants to merge 41 commits into
matrixorigin:mainfrom
cpegeric:bug_27926
Open

fix(plan): make fulltext2 json_extract index probe reachable on current reads (#27926)#28025
cpegeric wants to merge 41 commits into
matrixorigin:mainfrom
cpegeric:bug_27926

Conversation

@cpegeric

@cpegeric cpegeric commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

What type of PR is this?

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

Which issue(s) this PR fixes:

issue #27926

What this PR does / why we need it:

Makes a json_extract_string|json_extract_float64(col,'$.path') <op> const predicate
use a fulltext2 json-tuple index as a mandatory filter, correctly and without
dropping rows while the async (ISCP-maintained) index lags the base table.

The predicate is rewritten to:

base scan of t ──INNER JOIN on pk── candidate pks (from the probe)
(re-checks json_extract(...) on (fulltext2_search TVF, self-completing)
the row's CURRENT value at snapshot S)

The probe only needs to return a superset of matching pks — the base scan re-checks
the real predicate, so an extra candidate is dropped and (crucially) no matching row is
ever missed.

The problem

fulltext2 is always-async: an ISCP consumer tails the base table and appends index
segments, so the index trails the base by some lag. A naive index probe ANDed into a
predicate would silently drop rows committed inside that lag.

Design — self-completing probe with runtime tail binding

The fulltext2_search operator self-completes: after searching the bulk index it
runs the freshness-gap query itself and unions the results, so there is no plan-time
UNION ALL arm
.

  • bulk — rows the searched generation reflects (<= build_ts), matched through the index.
  • tailtable_changes(db, t, searched, S] AS c WHERE c.change_type='insert' AND <json predicate>,
    projected to the pk, emitted as (doc_id=pk, score=0). The gap rows aren't in the index
    (that's the point), so the json predicate is evaluated directly on the changed rows —
    no index
    . S is the read point (current txn snapshot, or the {snapshot=...} TS).

A group-by dedup + INNER JOIN above narrow the union (bulk∩tail overlap on updates;
per-term repeats). Deletes need no handling — the tail is insert-only and the base fetch
is at S, so MVCC drops a pk deleted after build_ts.

Runtime tail binding (the correctness core)

The tail's lower bound is the generation the bulk search actually ran on, captured
atomically under the cache entry's read lock via the new RuntimeConfig.SearchedBuildTS
out-param — never read afterward. A post-search GetBuildTS would be a TOCTOU: the entry
lock releases when Search returns, so a concurrent evict+reload could publish a newer
generation and bind the tail above what was searched, dropping the rows in between.
Binding at search time closes that race with no plan-time generation pin.

Constraints honored

  • table_changes runs inside the TVF (pkg/sql/colexec/table_function), never in
    pkg/fulltext2.
  • The tail is streamed via RunStreamingSql, so a large gap streams in bounded batches
    rather than materializing every gap pk (OOM guard).
  • The reconstructed tail SQL is surfaced on the scan node's Stats.Sql, so
    EXPLAIN (VERBOSE) shows it (same mechanism as classic fulltext) — the internally-run
    tail is visible, not a black box. It is shown only when the tail is expected to run
    (index behind as of planning); a caught-up probe shows none.

Coverage gate

The plan decides probe vs full scan (not covered-vs-behind, since self-completion
makes them equivalent). It probes when the index is usable — built (build_ts > 0),
table_changes can serve the table (ordinary, explicit non-hidden pk), and there are no
transaction-local writes
to the source (those aren't visible to the index or the tail →
full scan). Fails closed on any uncertainty.

Current and {snapshot=...} reads share one path: a snapshot read binds the
snapshot-keyed generation, the snapshot TS as the tail's upper bound, and the snapshot's
owning account.

Key files

File Change
pkg/sql/plan/apply_indices_fulltext_json.go decideJSONProbe (probe/skip); recordJSONProbeTail +
jsonComparisonSQL/jsonLiteralToSQL (rebuild the predicate as SQL over the source column); gated display SQL
pkg/sql/plan/apply_indices_fulltext.go splice: single self-completing node (no UNION), publish Stats.Sql
pkg/sql/plan/apply_indices_fulltext2.go buildFulltext2SearchCfg emits probe_tail/src/pkey/probe_tail_where
pkg/sql/colexec/table_function/fulltext2_search.go streaming tail
(startProbeTail/emitProbeTail/appendTailResult/closeProbeTail), SearchedBuildTS binding, aliased table_changes
pkg/vectorindex/types.go, pkg/vectorindex/cache/cache.go RuntimeConfig.SearchedBuildTS out-param, populated under the
entry lock
pkg/fulltext2/{storage.go,search_cache.go,plugin/coverage/coverage.go} TableConfig.ProbeTail/ProbeTailWhere; removed
the maxTs pin; searchedBuildTS warm-or-cold
docs/design/json_tuple_wordbreaker.md §10 aligned to the implemented semantics

Testing

  • Unit (pkg/sql/plan, pkg/sql/colexec/table_function, pkg/fulltext2/...,
    pkg/vectorindex/cache): probe decision matrix, tail-SQL reconstruction, streaming
    tail emit/close, empty-gap skip. -race clean.
  • BVT (pessimistic_transaction/fulltext2, clean instance, -g -n): json-probe
    154/154 + datalink 49/49 = 203/203, 100%
    — covering current/snapshot, covered/partial,
    fallback (txn-local write → full scan), composite-pk decline, ALTER, prepared, multi-col.

…nt reads (matrixorigin#27926)

The json_extract index probe fengttt added in matrixorigin#27821 never fired on an ordinary
current read: the coverage gate required watermark >= txn.SnapshotTS() (≈ now),
but fulltext2 is always-async so the ISCP watermark chases the wall clock with a
built-in lag (~10s scheduler tick + ~5s index-job flush, and it advances even on
an idle table). It therefore always trails "now", "now" never stays covered, and
the probe was permanently unreachable for current reads -- the query silently
fell back to Table Scan + Filter.

Demanding watermark >= now is also pointless: the probe does not read the durable
index directly, it reads a per-CN VectorIndexCache copy that is itself served up
to its cross-CN freshness window stale. Requiring coverage fresher than the cache
can ever deliver only disables the probe with no correctness benefit -- and an
exact gate is anyway unachievable multi-CN, where the watermark, the base-table
state, and the read snapshot are all independently eventually-consistent.

Fix (contained to the planner; the generic coverage.CoversSnapshot exact contract
is unchanged): indexCoversSnapshot now asks about a snapshot lowered by
asyncCoverageStaleness = 2*VectorIndexCacheTTL (the cache's own ~10-min cross-CN
freshness bound). So a healthy-but-lagging async index counts as covering a
current read, and ONLY a watermark further behind than that window (a genuinely
stuck/broken maintenance job) still declines. The probe becomes eventually
consistent within a window MO already tolerates for every other async index,
rather than dead for all current reads. It stays safe: the retained json_extract
predicate re-checks every candidate, so a returned row is never wrong; the probe
may only omit a row written inside the window -- exactly what the cached read path
can already do. All other fail-closed guards (no live job, pending/error/canceled
state, unreadable watermark, lookup error) are untouched.

Unit test TestAsyncCoverageBar pins the bar: snapshot lowered by exactly
2*VectorIndexCacheTTL, a ~15s-lagging watermark counts as covering, the window
boundary flips covered/fallback, and an underflowing tiny TS clamps to the strict
check.

Follow-up: fulltext2_json_probe.sql only asserts row-equivalence (the probe never
fired, so equivalence held trivially); it needs an EXPLAIN assertion that the plan
now contains fulltext2_search, regenerated via mo-tester genrs on a cluster.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@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 →

…p context (matrixorigin#27926)

Live end-to-end verification of the matrixorigin#27926 fix surfaced a SECOND blocker beyond the
watermark tolerance: indexCoversSnapshot passed proc.Ctx to the coverage hook, but
during planning proc.Ctx is already canceled, so the nested mo_iscp_log lookup
failed with "context canceled". CoversSnapshot then fell closed on the error and
the json_extract probe NEVER fired on a current read -- independent of the
watermark. This was invisible to matrixorigin#27821 because its unit tests mock execWithResult
and its BVT only asserts row-equivalence (which holds trivially when the probe is
absent).

Fix: run the lookup under proc.GetTopContext() -- the same live context the
compiler itself uses for catalog access (compilerContext.GetContext /
GetAccountId both use GetTopContext), which is not canceled during planning and
carries the account id.

Verified on a live single-node cluster: with both this and the tolerance fix, the
plan for
  select id from t where json_extract_string(j,'$.foo')='needle'
now contains `Table Function on fulltext2_search` (the probe), with the original
json_extract predicate retained as a Filter Cond above the join, and results are
identical to the Table-Scan fallback. Before the fix the same query planned as a
plain Table Scan + Filter.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…t read (matrixorigin#27926)

Adds pessimistic_transaction/fulltext2/fulltext2_json_probe_plan.sql, the
regression the issue asked for: the existing cases/fulltext2/fulltext2_json_probe.sql
only checks row-equivalence (which held trivially while the probe never fired), so
it never caught that the probe was unreachable on current reads.

This case reproduces the issue's exact shape (create index first, then insert so
the writes travel the ISCP CDC tail), polls the CDC tail via @wait_expect until the
watermark is live, and then asserts with @regex that EXPLAIN contains
`Table Function on fulltext2_search` for both an equality and a numeric-range
json_extract predicate -- with the original predicate retained and results exact.

Generated via mo-tester genrs and validated in run mode (-g -n): 17/17, 100%.
cpegeric and others added 14 commits September 10, 2026 16:12
…d_ts

Rework the fulltext2 coverage gate to decide on the build_ts of the index
generation a probe would actually search, instead of the mo_iscp_log watermark.
This binds coverage to what execution reads, so a stale warm cache can no longer
over-report (P1-2).

- Add VectorIndexCache.GetBuildTS(key): the loaded generation's build_ts, read
  from an entry atomic published by captureSize under the entry lock (never from
  the algo, which a concurrent eviction may be tearing down).
- CoversSnapshot now checks two conditions: liveness (mo_iscp_log job_state/drop_at)
  and coverage (build_ts >= SourceCommitTS). build_ts is the cached generation's
  when warm, else MAX(build_ts) from the index metadata (what a fresh load sees) --
  a cold cache must check the durable coverage, not probe blind.
- Snapshot-consistent: a {snapshot=...}/AS OF read targets the snapshot-bound
  generation for BOTH sources -- cache key index_table@snapshot and a metadata read
  on a txn cloned at the snapshot -- derived from one choice so the two cannot
  disagree. The planner resolves the hidden tables and the effective snapshot TS
  and passes them on coverage.Request.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A {snapshot=...}/AS OF read sees a fixed past state, so the coverage bar is the
snapshot TS itself (build_ts >= snapshot ⇒ every source commit up to that point is
indexed), not SourceCommitTS. A snapshot read is binary -- the snapshot-bound index
generation covers the snapshot data (probe) or the read falls back to a table scan
AS OF the snapshot -- never partial.

- CoversSnapshot: bar = ScanSnapshotTS for a historical read, else SourceCommitTS.
  The cache key and the durable-metadata-read txn are derived from one snapshot
  choice so the warm and cold paths can't disagree about which generation they
  measure.
- indexCoversSnapshot: skip SourceCommitTS (and the GetRelationById/partition-state
  scan) entirely for a historical read -- neither needed nor meaningful there, and
  the at-now scan was over-strict.

Add a BVT (fulltext2_json_probe_snapshot) asserting a snapshot json_extract read
returns the historical rows only and a current read returns all -- results, not the
plan, since whether a historical probe fires is timing-dependent.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…s gate

Section 10 still described the removed watermark>=snapshot-delay design. Rewrite it
to the shipped gate: coverage is build_ts (the searched generation's
MAX(metadata.build_ts) over base+cdc_tail, warm from the cache or cold from
metadata) vs a bar that is SourceCommitTS for a current read and the snapshot TS
for a historical one. Add 10.3 for the planned partial plan (bulk probe UNION
table_changes gap tail, json_extract only, snapshots binary) and the freshness/
partial test matrix in section 9.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
When a json_extract fulltext2 index is behind on a CURRENT read, complete
the probe with a table_changes tail over (build_ts, snapshot] instead of
declining to a full scan of the large-content base table.

- indexplugin: IndexBuildTS hook + dispatch exposing the searched
  generation's build_ts (the tail's exclusive lower bound); fulltext2 impl
  factored out of the coverage check. Returns 0 (no partial) for algos
  without a real build_ts.
- plan: decideJSONProbe replaces indexCoversSnapshot with a 3-way decision
  (covered probe / partial / full-scan skip). No cost gate on the partial
  path -- the base carries large per-row content, so a full scan is the
  expensive outcome to avoid; the tail reconstructs only gap rows.
- plan: buildJSONProbeTail builds table_changes(build_ts, snapshot] filtered
  to change_type='insert' AND the json predicate (rebased onto the tail
  columns by name), projected to (pk, score); unionFtWithTail UNION ALLs it
  with the bulk arm, then the existing group-by dedup collapses shared pks.
- Correctness rests on the base scan keeping every WHERE conjunct: the stale
  index arm is candidate-pk-only, re-checked on current values, so an
  updated row cannot leak. Snapshots stay binary; json_extract only.

Not yet live-validated; BVT (P5) follows.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- apply_indices_fulltext.go: if the table_changes tail cannot be built for a
  behind-index json probe, fail closed instead of falling through to a
  bulk-only plan that would silently drop rows committed after build_ts.
  Eligibility is already checked in decideJSONProbe, so this guards an
  unexpected internal failure rather than a reachable path.
- BVT fulltext2_json_probe_partial: current-read json_extract against a behind
  index -- insert-gap, update match->nomatch (stale index arm dropped by the
  base re-check), update nomatch->match (tail includes), delete-in-gap (MVCC),
  and a multi-column filter (non-indexed conjunct enforced on the base scan).
  Asserts RESULTS, deterministic regardless of which plan timing selects.

Live-validated: EXPLAIN shows table_changes + UNION ALL + group-by dedup with
the full WHERE retained on the base scan; all matrix cases return correct rows;
DDL-in-gap returns correct rows (ALTER resets the index generation).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A json_extract fulltext2 probe's covered/partial/skip decision is derived from
the async index's freshness at plan-build time -- state no table schema version
represents. Cross-transaction prepared-plan reuse (protocol >= v32) therefore
froze that decision: a covered probe cached while the index was current, reused
after the index fell behind, dropped rows committed in the gap (the mandatory
index filter excluded them). Demonstrated live: EXECUTE returned 1,3 instead of
1,3,4 after a gap insert. This affected the covered probe already; the partial
plan would likewise freeze its table_changes (build_ts, snapshot] window.

Fix: PreparedPlanDependsOnIndexCoverage flags a plan that carries an injected
fulltext2_search probe (identified by the JSONProbeMode arg, so a user MATCH --
freshness-tolerant -- is excluded), and shouldRebuildPreparePlan forces such a
plan to rebuild on every EXECUTE, mirroring FK/subscription-metadata plans. The
coverage decision and the tail window then re-derive from the current snapshot
each execution. Live-verified: reused EXECUTE now returns 1,3,4 then 1,3,4,5.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… probe

Unit tests raise apply_indices_fulltext_json.go statement coverage to ~76%
(UT alone, over the 0.75 gate; BVT lifts decideJSONProbe/dedup further), and
cover the partial-plan builders the BVT exercises only at runtime -- which the
coverage gate cannot rely on when the fulltext2 BVT suite is truncated in CI:
- TestPreparedPlanDependsOnIndexCoverage: nil / plain-scan / user-MATCH (not
  flagged) / injected-probe (flagged) / mixed.
- TestRebaseToTableChanges: remap by name, ColPos-name fallback, unpushable
  (absent column -> nil), foreign binding left untouched.
- TestRecordJSONPartialProbe: keyed record + deep-copied predicate.
- TestUnionFtWithTail: UNION ALL shape, left-tag projection, pk reference.
- TestBuildJSONProbeTail: full tail (table_changes -> FILTER insert+json ->
  PROJECT pk,score) against a resolvable ordinary table.

BVT fulltext2_json_probe_prepared: bake -> prepare -> EXECUTE (covered) -> gap
insert -> re-EXECUTE must still return the gap rows. Deterministic with the
rebuild-every-execute fix; a regression that reused the stale covered plan
would drop them. Results 1,3 -> 1,3,4 -> 1,3,4,5.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- fulltext2_json_probe_partial: add a cross-arm dedup case -- update a baked
  needle to another needle in the gap so the row is in BOTH the index (bulk)
  arm and the table_changes tail; UNION ALL + the group-by dedup must return
  it exactly once (a dedup regression would duplicate the id via the INNER
  join). Also covers that a delete-in-gap row is dropped by the base scan at
  the read snapshot even though the stale index arm still nominates its pk.
- fulltext2_json_probe_multicol: a json probe ANDed with a non-indexed column
  filter. Asserts the PLAN once coverage holds -- the probe fires (fulltext2_
  search) AND the base Table Scan retains the status predicate -- then asserts
  results for a covered read (1) and a behind/partial read (1,4), proving the
  non-indexed conjunct is enforced on the base scan, not the index/tail.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Diff coverage is measured against bug_27941, so the whole json-probe surface
counts as new; lift the under-covered new functions by unit test:

- apply_indices_fulltext_json.go: indirect the two runtime coverage lookups
  (CoversSnapshot, IndexBuildTS) behind package vars so TestDecideJSONProbeMatrix
  can drive the covered / partial / skip decision -- including the current-read
  source-relation path via a fake engine/relation answering SourceCommitTS.
  decideJSONProbe 39.6% -> 83.3%; the file 66.6% -> 82.6% (UT alone).
- indexplugin: TestIndexBuildTS covers the dispatch (unregistered / no-capability
  -> zero TS; capable algo passed through) -> 100%.
- fulltext2 coverage: TestIndexBuildTS covers the hook wrapper (incomplete
  request -> zero; cold path -> MAX(build_ts)) -> 100%.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Code review found decideJSONProbe promised a partial plan for tables the
table_changes tail cannot build, turning a former correct full scan into a hard
runtime error. Guard the partial decision at plan time so it declines to a full
scan instead:

- Composite / hidden primary key: table_changes emits only non-hidden source
  columns, so a hidden composite pk (__mo_cpkey_col) cannot anchor the tail's pk
  projection. Previously this hard-errored at the fail-closed splice for any
  composite-PK table with a behind json index on a current read. (blocking)
- Empty/inverted window: when build_ts has already reached the read snapshot
  (possible when the SourceCommitTS bar exceeds it), (build_ts, snapshot] is
  empty and table_changes rejects from >= to; the index already covers the read,
  so full-scan.

Also: remove a duplicated maxDurableBuildTS doc block; correct the
dedupFulltextDocIDs comment (partial passes the UNION id, the scan id is marked
at the splice).

Unit tests: TestDecideJSONProbeMatrix gains composite-PK and empty-window cases.
BVT fulltext2_json_probe_compositepk: behind composite-PK index returns rows via
full scan (EXPLAIN shows a plain Table Scan), not an error. Live-verified; all 7
fulltext2 json-probe BVTs pass.

Known remaining (decision-logged, not fixed): a schema-version change committed
inside the gap can still make the tail error at execution (rare race, error not
corruption, ALTER usually rebuilds the generation past the DDL); the partial
path re-reads MAX(build_ts) once more on a cold cache (minor plan-time perf).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…sion BVT

Two review follow-ups:

- Remove the double build_ts read on the partial path. CoversSnapshot now
  returns (covered, buildTS, err): it computes the searched generation's
  build_ts once and returns it, so decideJSONProbe reuses it as the
  table_changes lower bound instead of a second IndexBuildTS lookup (a second
  MAX(build_ts) metadata query on a cold cache). The now-redundant IndexBuildTS
  hook/dispatch is removed. build_ts is returned even when the index is not live
  (liveness gates only the covered verdict, not the gap bound), preserving the
  partial decision exactly.

- fulltext2_json_probe_alter BVT: an ALTER copy-table rebuild creates a new base
  table AND a new index table whose build_ts reflects the rebuild snapshot
  (>= the ALTER = the new schema version's start), so a table_changes tail over
  (build_ts, snapshot] never spans the ALTER boundary. The BVT bakes the rebuilt
  index, then a gap insert forces the partial path to fire post-ALTER; it returns
  1,3,4 with no schema-window error. This closes the "ALTER in the gap" limitation
  (a false positive: the tail's lower bound is always within the current schema
  version). Live-verified; all 8 fulltext2 json-probe BVTs pass 100%.

Tests: TestCoversSnapshotReturnsBuildTS (dispatch + fulltext2 impl, incl. the
not-live path returning build_ts); TestDecideJSONProbeMatrix updated for the
single coverage lookup.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A {snapshot=...} json_extract read no longer declines to a full scan when the
snapshot-bound index generation is behind. It takes the same covered/partial/skip
decision as a current read, measured as of the snapshot:

- decideJSONProbe computes the coverage bar (source last commit) AS OF THE READ --
  the current txn, or a txn cloned at the snapshot -- via GetRelationById +
  SourceCommitTS, and reads build_ts at that same read point. Covered => probe;
  behind => complete with a table_changes tail over (build_ts, S]; decline on any
  uncertainty (empty/inverted window, hidden/composite pk, table_changes-ineligible
  source).
- CoversSnapshot's bar is SourceCommitTS on both paths (the planner supplies the
  as-of-S value); the snapshot TS now only selects which generation's build_ts is
  measured, not the bar.
- indexCoversSnapshot moves to the test file (production has no caller).

BVT: new fulltext2_json_probe_snapshot_partial (snapshot taken while behind a
committed gap row; tail recovers it; EXPLAIN-verified partial union) and updated
snapshot header. Full json-probe suite 154/154, unit tests green.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Two fixes to the json_extract coverage gate:

#2 cross-account snapshot: decideJSONProbe ran its freshness reads (source
relation, ISCP log, index metadata) under the READER's account, so a
{snapshot=...} read of another account's data declined to a full scan (wrong
account -> table-not-found / empty). It now binds the snapshot's owning tenant
(from ApplyScanSnapshot's resolved account) onto the context for those reads,
mirroring what the base-table scan already does. No-op for an ordinary
current/same-account read.

#3 bounded SourceCommitTS: the coverage bar is only ever compared against the
index build_ts, so the exact source-commit maximum is unnecessary. SourceCommitTS
now takes a mustExceed hint (the build_ts) and returns as soon as the terms
gathered so far exceed it -- checked before each object's disk I/O, so a recent
in-memory row or the retention-boundary floor settles "behind" with zero object
reads. An empty mustExceed disables the short-circuit (exact maximum) for any
other caller. The bar is also computed lazily via a Request callback and only
when build_ts is a live, non-empty value it could gate, so a not-built or
not-live index skips the scan entirely.

Tests: partition-state short-circuit skips an un-loadable object (proving no
I/O) + retention-floor early-out; cross-account binding asserts the snapshot
tenant reaches GetRelationById; coverage-gate callback contract; all existing
source-commit-ts/coverage/decide-matrix suites updated and green. Full
json-probe BVT suite 154/154.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… gate

Adversarial-review find: the lazy bar callback introduced in the previous commit
was consulted only AFTER the liveness gate, so a NOT-live-but-built index skipped
it. That index still takes the partial plan (a table_changes tail completes the
frozen generation), and the tail cannot see uncommitted workspace rows -- so a
transaction-local write to the source (INSERT then SELECT in one txn on a table
whose ISCP job was dropped) would be silently dropped by the probe.

CoversSnapshot now computes the bar (whose provider fails closed on
transaction-local writes) BEFORE the liveness check, so the guard holds for a
not-live index too: the error surfaces and the planner full-scans. A live index
was already safe (the callback ran on the live path); only the not-live+built
case regressed, which no BVT reaches (making an index not-live needs a dropped
job), so it is covered by a unit test asserting the bar guard is consulted before
the liveness query.

Full json-probe BVT suite 154/154; coverage/plan/source-commit suites green.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@cpegeric

Copy link
Copy Markdown
Contributor Author

bug fixed.

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

Re-reviewed exact head e3db4a7 against main 9a5dd0c. REQUEST_CHANGES: the previous execution-generation completeness requirement is improved but not closed.

[P1] A planning-time GetBuildTS observation does not bind the generation used by the mandatory probe.

Concrete interleaving:

  1. On CN B, an earlier query at snapshot S0 starts loading G0 under the ordinary index-table cache key; pause that loader before STATUS_LOADED.
  2. On CN A, commit a matching JSON row at T > S0 and let ISCP persist G1/build_ts W1 >= T. CN B observes the source commit and plans a new read S1.
  3. GetBuildTS returns found=false for B's in-flight entry. searchedBuildTS consequently reads durable W1 and declares the new query covered, emitting no gap tail.
  4. Resume B's loader. The new query's Cache.Search uses LoadOrStore on the same key, waits for the existing entry, and searches G0 loaded with the first caller's snapshot. There is no required-build-ts/snapshot validation at search time. G0 omits the new PK, and the mandatory join drops a row visible at S1.

This is not merely an atomic-read race: the atomic protects the field, not the lifetime/identity of the chosen generation. Even a loaded entry can be replaced between planning and execution. A generation refreshed after a current query's snapshot can also delete postings that its base snapshot still needs; a lower-bound watermark alone cannot establish snapshot compatibility.

Please pin/bind a compatible execution generation or make the exact JSON-probe execution path independently enforce the coverage/snapshot requirement with a correctness-preserving fallback/tail. Treating in-flight entries as genuinely absent is immediately unsafe; changing that case alone does not solve replacement after the check. The partial tail also needs its lower bound tied to the generation actually searched. Ordinary MATCH's eventual-consistency contract can remain unchanged. Add a barrier-driven loader/planner/search regression for this interleaving and a refresh-after-read-snapshot deletion case.

The previous fixed-delay issue has been removed. The new TN-object commit-zonemap handling, transaction-local/partitioned fail-closed guards, partial UNION ALL + PK dedup with the original base predicates retained, and prepared-plan rebuild are substantive improvements; this review does not repeat the removed delay arithmetic finding.

The design record still needs alignment before approval: section 10.2 says historical reads compare directly with S without SourceCommitTS; section 10.3 says snapshots never use partial plans and promises a small-gap cost gate/DDL fallback. This head explicitly uses historical SourceCommitTS and historical partial plans with no cost gate. Update the versioned contract together with execution-generation binding; the PR body and several comments also disagree with that implementation.

Validation: inspected the current production change map, prior review findings, coverage/cache/search ownership paths, planner/tail/prepare changes, source-commit persistence transitions, and focused test/BVT oracles. The new GetBuildTS unit test explicitly expects an in-flight entry to return not-found, but does not compose that result with durable fallback and the waiting Search consumer. Author-reported UT/BVT/GPU results were considered as reported; I did not run a fresh native suite, GPU workload or live two-CN reproducer and did not wait for CI. No repository files were modified; head/base were rechecked before posting.

Comment thread pkg/fulltext2/plugin/coverage/coverage.go Outdated

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

Reviewed the readiness calculation in the latest commits.

The async fulltext2 probe is now gated by:

watermark >= SourceCommitTS

instead of comparing the watermark directly with the wall-clock SnapshotTS.

SourceCommitTS is evaluated at the transaction snapshot and covers:

  • in-memory source DML commit timestamps with commit_ts <= snapshot;

  • appendable-object commit timestamps with commit_ts <= snapshot;

  • CN-created objects through NewObjectsIter(snapshot, onlyVisible=true), which applies CreateTime <=
    snapshot and the corresponding delete visibility check;

  • the partition-state retention boundary.

The coverage hook uses wm.LT(req.SourceCommitTS) as the fail-closed condition, which is equivalent
to requiring watermark >= SourceCommitTS.

I do not see a correctness issue in the current readiness calculation.

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

Re-review of exact head e3db4a7 against base 9a5dd0c. I read the complete review/comment/thread history, checked the delta since my prior reviewed head b9644e0, and re-audited the full 59-file diff. The earlier fixed-delay and flushed-object coverage gaps are materially improved by BuildTS, SourceCommitTS, the partial tail, and prepared-plan rebuilding. Two blockers remain:

  1. The planning decision still does not bind the index generation later searched. A loading cache entry is treated as a cache miss here, so planning may use a newer durable build_ts while execution waits for and searches that older in-flight entry. The resulting INNER JOIN can drop committed matching rows; the partial plan can also begin its tail after a gap that the executed generation does not cover.
  2. The mandatory design record is not exact for the implementation under review: it says historical reads use the snapshot as the bar and never use partial coverage, and requires a small-gap/DDL cost gate, while the current code/tests use SourceCommitTS plus partial plans for snapshots and emit partial plans without that gate. This high-risk planner/cache/MVCC change needs the exact implemented contract reviewed and approved before merge.

Validation: exact-head CI is green and git diff --check passes. I did not rerun native/GPU/cluster suites locally because the generation race is source-provable and the design gate already fails.

Comment thread pkg/fulltext2/plugin/coverage/coverage.go Outdated
Comment thread docs/design/json_tuple_wordbreaker.md Outdated

@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 e3db4a7 against base 9a5dd0c. Two blockers remain.

[P1] Coverage must bind the generation execution actually searches, including in-flight loads.

pkg/vectorindex/cache/cache.go:1136-1148 returns found=false for an existing entry that is not STATUS_LOADED. searchedBuildTS (pkg/fulltext2/plugin/coverage/coverage.go:183-198) treats that as a cold miss and uses MAX(metadata.build_ts). But Cache.Search/SearchInto uses LoadOrStore on the same key and waits for that existing entry, rather than necessarily loading the generation planning measured.

Concrete schedule: query A begins loading generation W0; a matching row commits at T>W0 and ISCP publishes W1>=T; query B plans while A's entry is still loading. B's gate reads durable W1 and accepts coverage. A finishes W0, and B executes against that older entry. The mandatory join drops the matching row. In the partial variant, starting table_changes after W1 also leaves the missing interval (W0,W1] uncovered. Retaining the base predicate cannot recover a missing candidate.

Bind/retain a snapshot-compatible generation or enforce its coverage at execution with a safe fallback. Distinguishing absent from loading is necessary but does not by itself solve replacement between planning and search or execution on another CN. Add deterministic held-load and plan-to-search generation tests.

Design gate: the expanded partial-probe feature crosses planner, MVCC, cache generations and table_changes. docs/design/json_tuple_wordbreaker.md:535-565 says historical reads use snapshot TS, never partial coverage, and partial plans require a small-gap/DDL gate. The implementation uses SourceCommitTS for historical reads and permits historical partial plans; decideJSONProbe:426-435 explicitly has no cost gate. The PR body also describes a different historical contract. Align the exact versioned design with the chosen behavior and obtain review of that revision.

The earlier warm-loaded-cache coverage problem is materially improved, but this loading-generation path remains. I independently traced the source path, confirming the existing exact-head review. This is a design-gated incremental review, not a claim of a fresh full 59-file implementation approval. Reused green exact-head CI; no new native/GPU/cluster run.

cpegeric and others added 9 commits September 11, 2026 10:49
§10.2/§10.3 described the superseded contract (historical bar = snapshot ts,
snapshots binary, small-gap cost gate). Align them with the shipped behavior:
historical reads use SourceCommitTS-as-of-S, snapshot reads take the same partial
(bulk + table_changes tail) path as current reads, and there is no cost gate
(the base table's per-row content makes a full scan never cheaper than the tail).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A current-read mandatory json_extract probe could have its plan-time coverage
verdict (covered/partial + tail bound) computed against one index generation while
execution searched a different one -- e.g. coverage read durable W1 while a
concurrent in-flight load bound W0 -- silently dropping rows (a covered probe has no
tail; a partial tail bounded above the searched generation leaves a gap).

Tie plan and execution to one value, GetMaxTS(indexTable) in pkg/vectorindex/cache:
  - tier 1 warm: the loaded entry's build_ts (what execution reuses);
  - tier 2 memo: a process-shared, per-key value so a planner inherits an in-flight
    loader's generation rather than a newer durable one;
  - tier 3 SQL MAX(build_ts): the cold first caller, computed once under a per-key
    mutex (singleflight); tiers 2/3 cleared once the load publishes.
GetMaxTS returns (ts, indexFound, memoFound) so a caller can see the source.

The planner records that build_ts for covered AND partial (recordJSONProbeMaxTs) and
buildFulltext2SearchCfg carries it in the search config as TableConfig.MaxTs. The
fulltext2_search TVF reads MaxTs -- it does NOT re-derive it (a fresh read could
diverge under a concurrent load/eviction) -- and tags the loaded entry with it, so a
stale/in-flight generation is never substituted for the one the plan approved.
MaxTs==0 is ordinary MATCH (whole current index). {snapshot=...} probes are exempt
(SnapshotKey already binds the immutable as-of-snapshot generation).

Design §10.4 documents the binding and the one accepted limitation (a cold-load race
between concurrent probes at slightly different snapshots, same class as the
documented cross-CN eventual-consistency won't-fix).

Tests: GetMaxTS tiers/singleflight/error-self-clean (-race); buildFulltext2SearchCfg
emits max_ts from the stash; coverage suites clear the shared memo; json-probe BVT
suite 154/154.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…pshot)

The shared cold-window memo could hand a query a build_ts seeded by a concurrent
NEWER-snapshot query -- a generation the inheriting query's own snapshot cannot
load. It would then mark itself covered (or bound its tail) at that ts, search the
older generation it can actually load, and drop rows with no tail below it.

GetMaxTS cold path now returns min(memo, own): it computes the caller's own durable
MAX(build_ts) -- the newest generation its snapshot can load -- and clamps the shared
memo to it. A smaller memo (a concurrent loader building an older generation) is still
inherited so the tail is bounded at what execution reuses; a larger memo is clamped
away. compute() therefore runs on every COLD call (the memo is a cap-from-below, not a
compute-skip), so the per-key singleflight is dropped and the entry is write-once
(no mutex). This is plan-time only and cold-only: a warm index answers from GetBuildTS
in memory with no SQL; the extra MAX is bounded by concurrency during the first-load
window and is a cheap metadata aggregate.

Residual (documented, §10.4): the clamp bounds MaxTs at own, but the tail bound is
still plan-time-fixed, so reusing a shared entry OLDER than that bound (loaded by an
even-older-snapshot query in the cold window) can still bound a tail above what was
searched. Fully closing it needs runtime tail binding, scoped separately.

Tests: TestGetMaxTSClampToOwn (inherit-smaller + clamp-to-own), error-propagates,
concurrent-clamp-no-race (-race), distinct-keys; coverage suites still green.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Self-review find. A freshly-created json index (no generation built yet) makes the
coverage compute return build_ts=0. GetMaxTS memoized that 0; the probe then declined
to a full scan (build_ts=0), so it never executed and never removed the memo. Every
later query for that index read min(0, own)=0 and kept declining -- the probe was
permanently disabled for the index even after it built (until process restart), plus
the 0-entry leaked.

Return unknown (0) without touching the shared memo, so a later caller with a real
build_ts computes and memoizes its own. Not a data-loss bug (a decline is a correct
full scan), but it silently defeats the probe optimization for the index.

Regression: TestGetMaxTSNeverMemoizesUnknown.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Replace the plan-time generation pin (GetMaxTS/maxTs) with a self-completing
fulltext2_search operator: after the bulk search it runs table_changes(searched, S]
itself and unions the gap pks, binding the tail's lower bound to the generation the
search ACTUALLY ran on -- captured atomically under the cache entry lock via
RuntimeConfig.SearchedBuildTS (never a post-search GetBuildTS, which a concurrent
evict+reload could advance past the searched generation and drop the gap rows).

- removes the plan-time UNION ALL arm; one self-completing node
- table_changes runs INSIDE the TVF (pkg/sql/colexec/table_function), never pkg/fulltext2
- streamed via RunStreamingSql, so a large gap streams in bounded batches (OOM guard)
- tail predicate rebuilt over the source columns and pushed into table_changes
- reconstructed tail SQL on Stats.Sql for EXPLAIN (VERBOSE), shown only when the tail
  is expected to run (index behind as of planning)
- coverage gate collapses to probe-vs-fullscan; the covered verdict is advisory
- deletes the whole maxts machinery (cache/maxts.go, SetMaxTs/pinnedMaxTs, TableConfig.MaxTs)
- unit tests for the streaming tail + generation capture; BVT 203/203 (json-probe + datalink)

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Runtime tail binding removed the freshness reasons the comment cited (no frozen
table_changes window, no covered-filters-gap). The prepared-plan rebuild is still
required, but for the plan-build-time probe-vs-full-scan decision -- chiefly the
transaction-local-write guard: a probe plan built in a clean txn and reused in a txn
with uncommitted writes to the source would drop those rows. Comment-only.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Removing the RemoveMaxTSMemo calls left testIndexStorageTable unused, failing
golangci-lint (unused) in static-check-analysis. Point gateReq's IndexStorageTable
at the const (its documented purpose) so it is referenced again. Test-only.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…n span)

A caught-up json probe was still self-completing with a table_changes(build_ts, S]
tail. On a CREATE-INDEX-on-existing-data index (the common case) build_ts sits at the
pre-create schema version while the read snapshot is post-create, so table_changes
spans a schema-version change and errors ("single source schema version across after,
until, and the query snapshot") -- failing every json_extract probe over such an index
(cases/fulltext2, json_variant, ... deterministically).

decideJSONProbe now uses the coverage verdict again: covered (build_ts >= bar) -> probe
with NO tail; behind -> the runtime-bound self-completing tail (both endpoints then live
in the same post-create schema version). The runtime-binding fix for the behind case is
unchanged.

BVT: cases/fulltext2 701/701, cases/json_variant 35/35 (both were failing),
cases/pessimistic_transaction/fulltext2 643/643 (no regression on the partial path).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…pan a schema change

The behind-branch tail runs table_changes(build_ts, readTS], which table_changes refuses
to span across a schema-version change (its after/until/query-snapshot must share one
TableDef.Version). So in the BEHIND branch only -- after the covered short-circuit, so a
caught-up index still uses the probe and never full-scans -- compare the source table's
schema Version at build_ts vs the read point; if they differ (a DDL sits in the gap), or
the build_ts relation cannot be resolved, return jsonProbeSkip (full scan) instead of a
tail that would error at runtime. Reuses table_changes' own version-equality rule
(GetRelationById at the two timestamps + TableDef.Version compare), applied at plan time
to DECIDE rather than ERROR. Matches the design's "a DDL in the gap forces a full scan".

BVT (span-check build): cases/fulltext2 701/701, cases/json_variant 35/35,
cases/pessimistic_transaction/fulltext2 643/643 -- covered path still uses the probe
(verified via EXPLAIN), behind path still tails when versions match.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@cpegeric

Copy link
Copy Markdown
Contributor Author

fixed and add schema version check

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

Labels

kind/bug Something isn't working size/XL Denotes a PR that changes [1000, 1999] lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants