[XuPeng-SH]

What type of PR is this?

• BUG
• Improvement
• Test and CI

Which issue(s) this PR fixes:

Addresses #30185 on 4.2-dev. Backports the merged main fix #30212 (18d5335ee330bb7e3bd3a893bf81044120884021).

Base: e2c99334fd23b9260728695e29befd35ab26b454.

What this PR does / why we need it

Table privileges must not confer authority over CN files or configured server file services. A clean 4.2-dev public-role SQL probe successfully read a fixture-owned CN canary; the fixed version rejects that access.

• Enforce the current sys/moadmin identity at the existing SQL fileservice acquisition owner for local, named and dynamic-disk providers. Remote dynamic providers use explicit caller credentials, with no server credential discovery or local auxiliary files.
• Refresh the current statement identity and propagate it through synchronous child SQL and the existing process wire account field. Background execution does not reconstruct a foreground administrator grant.
• Admit only remote asynchronous Datalink sources, at CREATE/rebuild and final INSERT/UPDATE/REPLACE/LOAD values. Preserve NULL/inactive-row masking and rollback.
• Have native index owners persist their owned artifacts through bounded binary parameters in the existing transaction. Remove generated load_file INSERT paths rather than elevating internal SQL. Quote metadata values, including expression-looking IDs loaded from storage.
• Cover raw file functions, stages, OUTFILE, server LOAD, external/Hive readers, table dump/load, backup and indirect Datalink consumers. Client LOCAL/INLINE input remains supported.

4.2-specific adaptation

This is an adapted backport, not an unmodified cherry-pick:

• Keep the pinned AWS SDK/native dependencies and existing index metadata/on-disk formats. Do not import main-only provenance/build timestamps or GPU device-budget machinery.
• Keep 4.2 CAST and multi-column fulltext NULL behavior. Remove main's validation-only CAST helper because 4.2 has no production consumer for it.
• Preserve admission above the final row by recognizing its internal predicate during filter pushdown. Do not use IsEnd (it discards rows) or prepopulate a tagged filter's executable projection (the remapper owns that projection). No new protobuf field.
• Use the older UPDATE row-image/row-id owner. Duplicate-key paths conservatively validate final source values; unrelated updates do not resolve carried sources.
• Bring over the missing stage cycle/disabled-state/private-URL prerequisites at the existing resolver.
• Reuse the branch's shared three-CN SQL fixture, and exclude unrelated main-only tests and historical measurement documents.

Design and rollout contract: docs/design/sql-file-access.md.

Local validation

Go 1.26.4, Linux/amd64, source-matched native libraries, disk-backed temporary builds:

• make cgo and make build: passed (the normal build includes GOAMD64=v3, GOEXPERIMENT=simd).
• Owning/dependent package UT: fileservice, datalink, process, executor, sqlexec, HNSW, fulltext2, frontend, compile, planner/functions, external/externalwrite/table functions, fulltext compile plugin, stage/stageutil, backup and ISCP passed.
• Actual MySQL SQL, shared three-CN fixture: TestSQLFileAccessAuthorization, TestSQLFileAccessTenantStageCDC, TestSQLFileAccessPeerPersistence passed together (117.762 s). Covers role/prepared reuse, denial before side effects, LOCAL import, tenant remote stages, post-CREATE CDC changes and old-term removal, local-to-TEXT CDC, binary boundaries, rollback/retry, HNSW literal-ID CDC persistence, compaction/default/view authority and forced cross-CN role transport.
• -short -run '^TestLoadS3(WithIndex)?$' ./pkg/tests/partition: passed (73.903 s), retaining four blocks and unique-index maintenance.
• Focused -race tests passed across 11 owning packages, with an additional passing race run for the final HNSW/datalink/table-function adaptations.
• Incremental golangci-lint over 23 affected CPU packages: 0 issues. GPU-tag Go typechecking/incremental lint over CAGRA/IVFPQ/table functions: 0 issues.
• molint: no new diagnostics against the clean target base; the same 17 distinct pre-existing unsafe/recover messages (18 locations) occur on both versions. This is an incremental pass, not a claim that all existing findings are clean.
• Same 4 KiB named MemoryFS Datalink read, serial GOMAXPROCS=4, 3 × 300 ms: base median 2153 ns/op, 9008 B/op, 25 allocs/op; backport 578.5 ns/op, 480 B/op, 10 allocs/op. This measures the reader path only; it does not establish whole-workload QPS/P99.

Compatibility and limits

• Local/named server resources are intentionally restricted to the current sys/moadmin role. Existing async local-Datalink indexes need remote sources or authorized foreground materialization into TEXT followed by rebuilding.
• Upgrade all SQL-serving CNs before treating the boundary as enforced; older CNs do not enforce it. Rollback restores the previous policy. No new catalog/disk schema or process wire field.
• GPU native linking/runtime, the complete distributed BVT corpus and end-to-end workload benchmarks were not run. GPU-tag static validation is reported separately from runtime validation.

Change-size review

Implementation +2201/-983, tests +4817/-468, design documentation +326/-0. The test increment covers distinct acquisition, identity, lifecycle and public SQL contracts; it is not exempt from review. Removed obsolete SQL artifact-reader generators, unused production test hooks/helpers and unrelated proxy test changes. No new privilege cache, global policy hook, duplicate storage format or background authority state machine.

Follow-up corrections

• Legacy UPDATE admits only assigned, generated or automatic indexed Datalink sources on distinct physical targets. Repeated writable aliases retain conservative source admission. The predicate runs after target dedup/NULL-row removal and before the delete/reinsert fan-out; unchanged sources are not resolved. Reuse the existing completed-remap marker to preserve the executable row projection only in the legacy phase.
• Match FULLTEXT2 SUM queries to their int64 consumers with explicit CAST(... AS SIGNED), including tail byte budgets and base row counts. Compare the tail byte bound before multiplication so large totals cannot wrap into an admitted load.
• Reuse the three-CN SQL fixture to verify disabled-stage unrelated updates and admitted remote assignments, with both target values, preserved sources, cardinality and rollback assertions. Add a typed REPLACE/FK child admission control for the before-remap path.

Final correction validation uses Go 1.26.4, Linux/amd64, matching worktree native libraries and disk-backed, non-hidden temporary paths:

• Planner focused race, including existing UPDATE/FK/REPLACE controls: passed (1.789 s). Planner complete normal UT: passed (4.112 s).
• Compile complete race UT: passed (5.059 s). FULLTEXT2 complete normal and race UT: passed (1.966 s and 7.573 s).
• Incremental go vet and configured golangci-lint over all 23 affected CPU packages: passed, 0 issues. The final planner test correction also passed a fresh focused static check. Earlier GPU-tag typechecking/static evidence applies to unchanged GPU sources; GPU native runtime remains untested.
• Three-CN SQL race: passed (121.272 s).
• Solution/design and independent final review: gpt-6.1-sol / xhigh, revision-2 bounded correction design. Final assessment: PASS, no unresolved in-scope blockers, frozen correction fingerprint 60b39a7e… (commit bf0988190befdae412217f47d98bb56352e2d637).

Known independent baseline failures are not hidden by changed expectations or retries:

• The four Proxy BVT protected_database.sql case-variant CREATE/DROP mismatches were reproduced on clean 4.2-dev base e2c99334fd with separate CN processes. The same-process control denies them correctly. Existing peer sessions keep stale per-process global configuration after another CN changes protected_databases.
• Admitted repeated writable aliases already have a separate legacy UPDATE row-layout defect on that clean base (two-row public SQL fails with a delete projection col3/len3). A broader delete-owner experiment was rejected because its overlap control duplicated rows; none of those execution-owner changes is included here. This PR preserves that baseline execution path and fixes its new source-admission boundary.
• The original quota-test failure occurred while acquiring a connection ID from HAKeeper, before its initial SET ROLE could execute. That exact test passed a fresh isolated race run (17.136 s); no quota/file-policy code was changed for the connection failure.
• Resource-contaminated runs are excluded from passing evidence. Parquet fan-out tests require ordinary disk temporary paths; hidden .cache paths reproduce failures on clean base as well.

Remote CI has not been declared green or awaited.

Correction size: production +75/-27 (net +48), tests +95/-1 (net +94), tracked documentation +0/-0. No new persistent state, workers, caches or execution ownership path.

CSV cleanup test isolation

The latest review found an order-dependent fixture in TestDataBranchCSVFinalizationCleanup: it asserted that another test had already registered the service runtime. The test now supplies a neutral default when no previous runtime exists and registers restoration/batch cleanup before installing its mock executor. An existing runtime is restored unchanged. All ten real CSV success, finalization, cancellation and authorization cases keep their original assertions. Production code is unchanged; this follow-up is test-only, +4/-2 lines.

Validation on Go 1.26.4 / Linux amd64 with matching native libraries:

• Before the fix, isolated execution failed all ten cases at the missing-runtime assertion (0.038 s).

• Isolated normal and race execution: passed (0.911 s / 2.852 s).

• Existing data-branch, output validation and streaming tests under race, -shuffle=30433 -count=3: passed (22.962 s).

• Frontend go vet and configured incremental golangci-lint against merge base e2c99334fd: passed, 0 issues. Prior full-delta evidence remains applicable to unchanged packages.

• Solution/design and independent final review: gpt-6.1-sol / xhigh, PASS for correction 0d1637598769ac85c029ede64665906c253925b7 (diff c4fc5ac8…). The existing baseline limitations above remain unchanged; remote CI is pending.

[qodo-code-review]

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 →

[XuPeng-SH]

mo-bot review · mo-bot-local · done

Commit: 981e88a63d8727fdf857d3eeead2d0f235714ca7

Review completed. Read the assessment.

[XuPeng-SH]

mo-bot review · mo-bot-local · review

Commit: 981e88a63d8727fdf857d3eeead2d0f235714ca7

Assessment: request_changes

Factory-owned authorization and native binary persistence are coherent designs, but the legacy UPDATE planner violates the documented unchanged-source contract. A public SQL reproduction confirms the regression.

Model: gpt-6-astra / xhigh

Session: 01a13dcf-1586-7a01-9000-d45e20995cf8 · slot 1 · generation 1

• P2 pkg/sql/plan/build_dml_util.go:341: Create an async FULLTEXT2 index on docs.path, insert a remote stage Datalink, then disable that stage. Execute UPDATE docs d JOIN peer p ON d.id=p.id SET d.status=2,p.status=2, targeting two distinct tables without assigning path.
  Impact: The statement unnecessarily resolves the unchanged Datalink and fails, preventing unrelated updates after source invalidation. Apply the modern planner's assigned-source filtering to the legacy path, preserving full admission where alias merging requires it, and add this SQL regression case.
  Evidence: dml_context.go routes multiple targets to the legacy planner, where this new code admits every indexed Datalink column. bind_update.go:419 instead selects assigned sources. The diagnostic TestReviewDistinctTargetAsyncDatalinkUpdate confirmed that single-table UPDATE docs SET status=1 succeeds, but the multi-table statement returns Error 20300: stage review_async_stage is disabled. Retained evidence: review-legacy-update-corrected.log and checks/1792172027946994263-2.json; production sources were unchanged.

Validation:

• Preflight passed. Built native prerequisites from the frozen revision using repository Makefiles; verified the native input ledger and clean source fingerprint.

• Passed 22 focused tests covering provider authorization, explicit credentials, cancellation, resource ownership, binary chunk boundaries and executor parameters.

• Passed TestSQLFileAccessAuthorization, TestSQLFileAccessTenantStageCDC and TestSQLFileAccessPeerPersistence together in 118.914 seconds.

• The corrected diagnostic SQL test reproduced the finding. Its initial run stopped at diagnostic SQL syntax and was excluded as product evidence.

• Commands, source fingerprints, exit receipts, log outcomes and artifact hashes were reconciled in /home/xupeng/.local/state/mo-bot/jobs/22f26e414dfefb7922002b66/attempt-1/review-validation-audit.json.

Limitations:

• Local tests used Go 1.27.1 on Linux/amd64. Full distributed BVT, race coverage, GPU runtime and comparative end-to-end performance remain outside executed validation.

• Binary transport bounds batches but adds parameter copies; no whole-workload performance conclusion is established.

• Frozen local_validation was empty and no separate linked-issue body was supplied. Author-only validation claims beyond the reproduced checks were not treated as verified execution.

[XuPeng-SH]

Updated this PR in bf0988190befdae412217f47d98bb56352e2d637.

Addressed the disabled-stage unrelated-UPDATE finding: the legacy planner now checks final assigned/generated/automatic sources before delete/reinsert, preserves its executable row projection, and leaves carried sources alone on distinct physical targets. SQL regressions check both target values, preserved sources and row cardinality. Added a REPLACE/FK child control for the pre-remap path.

Also corrected the FULLTEXT2 SUM/int64 contract and made tail-budget comparison overflow-safe.

Validation (Go 1.26.4, Linux/amd64): three-CN file-access/CDC/persistence race tests PASS 121.272 s; planner focused race PASS 1.789 s and full normal UT PASS 4.112 s; compile full race PASS 5.059 s; FULLTEXT2 full normal/race PASS; complete PR incremental vet/lint across 23 CPU packages 0 issues. The exact quota test that failed obtaining a HAKeeper connection ID also passed isolated race, 17.136 s. Separate gpt-6.1-sol/xhigh design and independent final review: PASS for this scoped correction.

Remaining baseline limits are explicit in the PR body. Clean target base e2c99334fd reproduces all four protected-database Proxy BVT mismatches with separate CN processes, and separately reproduces the admitted repeated-writable-alias legacy row-width failure. No golden expectations were changed, no broader delete/insert ownership rewrite is included, and remote CI is not claimed green.

[XuPeng-SH]

mo-bot review · mo-bot-local · done

Commit: bf0988190befdae412217f47d98bb56352e2d637

Review completed. Read the assessment.

[XuPeng-SH]

Fixed the latest review P2 in 0d1637598769ac85c029ede64665906c253925b7.

TestDataBranchCSVFinalizationCleanup now handles an absent runtime itself and registers restoration/batch cleanup before its helper installs the mock executor. Existing runtimes are restored unchanged; an initially absent runtime is restored to a neutral default. All ten CSV behavior/cleanup assertions remain intact. Only the test changed (+4/-2), with no production hot-path cost.

Validation (Go 1.26.4, Linux/amd64): isolated pre-fix run reproduced all ten failures; fixed isolated normal/race passed (0.911 s / 2.852 s); existing data-branch/output/streaming tests passed under race with shuffled order for three rounds (22.962 s). Frontend vet and incremental configured lint against the PR merge base passed, 0 issues. Unchanged-package evidence is reused. Separate gpt-6.1-sol/xhigh design and final overall review both PASS for this correction.

The independent baseline defects previously documented are not claimed fixed. No manual CI rerun or CI wait.

[XuPeng-SH]

mo-bot review · mo-bot-local · done

Commit: 0d1637598769ac85c029ede64665906c253925b7

Review completed. Read the assessment.

[XuPeng-SH]

mo-bot review · mo-bot-local · review

Commit: 0d1637598769ac85c029ede64665906c253925b7

Assessment: approve

Reviewed the complete 162-file delta, surrounding owners/callers and supplied review concerns. Factory-owned authorization, current-identity propagation and bounded binary persistence form a coherent design that preserves existing transaction ownership. Both previous findings are resolved. No actionable defects remain.

Model: gpt-6-astra / xhigh

Session: 01a13dcf-1517-7f90-b3f9-c5f9f9e7031b · slot 0 · generation 1

Validation:

• Verified frozen HEAD, clean source and supplied patch correspondence. Native recovery verified 170 matching inputs and retained build/output hashes.

• Fresh Go 1.26.4 runs passed TestDataBranchCSVFinalizationCleanup normally and under race, exercising all ten cases. A shuffled race run passed 103 frontend tests for three rounds.

• Independently reconciled 20 retained checks against commands, source fingerprints, exits and log contents. Their production and non-frontend test inputs match this head; the only subsequent change is the CSV test correction.

• Retained public SQL authorization, tenant CDC and peer-persistence tests passed normally and under race, covering unrelated UPDATEs after stage invalidation, binary boundaries, rollback and cross-CN identity.

• Verified passing owner-package and focused race results, plus the fileservice race suite and 100 lifecycle repetitions. Inspected fixture-only controls for parquet and HNSW directory failures; unsuccessful aggregate runs remain classified separately.

• Controlled 64 MiB transport measurements showed approximately 9.4% more allocated bytes with binary parameters and median elapsed time of 1.04 s versus 0.90 s, with substantial variation. The bounded copying cost supports the security design but establishes no general performance gain.

• Receipt/log reconciliation and detailed scope are retained in /home/xupeng/.local/state/mo-bot/jobs/c72b2bf205913a97fd4cbb2d/attempt-1/review-validation-audit.json and review-notes.json.

Limitations:

• GPU execution, native ONNX runtime tests and the complete distributed BVT corpus were not established. Live-provider tests included skips; controlled provider and acquisition tests supplied the relevant authorization coverage.

• The transport comparison uses the base SQL shape against the current engine, not separate base binaries or production throughput/P99 measurements.

• Frozen local_validation was empty and no separate linked-issue body was supplied. Pending CI and unreceipted author claims were not counted as verified passes.