Skip to content

fix(sql): admit view replacement before database locks - #30092

Merged
XuPeng-SH merged 4 commits into
matrixorigin:mainfrom
LeftHandCold:fix/30090-view-lifecycle-order-20261013
Oct 13, 2026
Merged

XuPeng-SH merged 4 commits into
matrixorigin:mainfrom
LeftHandCold:fix/30090-view-lifecycle-order-20261013

Conversation

@LeftHandCold

@LeftHandCold LeftHandCold commented Oct 13, 2026 •

Copy link
Copy Markdown
Collaborator

What type of PR is this?

  • BUG
  • Improvement
  • Test and CI

Which issue(s) this PR fixes:

Fixes #30090.

What this PR does / why we need it:

A fresh pessimistic RC CREATE OR REPLACE VIEW previously took the database lock before nested DROP lifecycle admission. That manufactured retained ownership and caused FastFail 20706 behind a SNAPSHOT gate instead of waiting. The replacement now enters the existing C/G/D admission before taking the ordinary database lock. Real retained-lock transactions still FastFail; nested DROP retains domain/identity revalidation and definition-change fences.

The replacement admission also preserves the existing no-default-database error: constant views without USE return MySQL 1046 before admission or engine/lock effects. The regression was reproduced on the previous PR implementation, where ordinary CREATE returned 1046 but OR REPLACE returned an internal lifecycle error.

Scope: 16 production lines, 215 test lines, no standalone documentation changes. Deleted the PR's redundant six-port test: the existing test already covers nine rendered listener addresses, allocator range and uniqueness. The integration test registers holder rollback immediately after BeginTx so assertion failures cannot bypass cleanup. No lock-service policy, retry, timeout, catalog format, or new admission state is introduced.

Validation

Rebased without conflicts onto main 8aaa8ae9e6; published head 76c1ab96c0. Local Go 1.27.1 / darwin-arm64; repository CGo wrapper and freshly built source-matched native artifacts. ONNX cache checksum matches the repository's required artifact.

  • Complete compile package: normal PASS 10.704s; race PASS 12.754s. New typed NoDB check forbids engine Database, Lock, and transaction Snapshot side effects.
  • Actual two-CN public regression: NoDB error/control, gate release, cancellation/server transaction drain before releasing the holder, unchanged old view on cancel, subsequent replacement, binary prepared-reader rebind, and retained-lock FastFail. Normal and race PASS; three same-process race repetitions PASS (30s budget derived from first 7.73s case).
  • Latest-main logtail read-barrier/reconnect dependency: six focused cases plus boundary subcases, normal PASS 1.407s and race PASS 2.123s.
  • Retained listener-range/uniqueness test PASS; removed test assertions are subsumed by that stronger existing oracle.
  • Final changed-package gofmt, go vet, configured molint and golangci-lint v2.14.0: PASS; zero new golangci diagnostics and git diff --check clean.
  • Whole issues package: normal PASS 265.380s and race PASS 644.282s; both modes have no failures or skips.
  • Required gpt-6.1-sol/xhigh amended design and final overall review: PASS, with no unresolved material blocker or validation requirement. Actual model/effort recorded from session 01a13720-7c29-77a2-a3ec-8d83274b3035.

Prior-head CI and author Shanghai50 evidence are historical, not new-head passes. New remote CI is not claimed passed before it executes.

Performance and safety

Additional work is bounded to pessimistic RC replacement DDL, including a registry/frontier admission pass. Plain CREATE VIEW and non-RC routing and SELECT/DML hot paths are unchanged. Replacement DDL is not claimed to have zero overhead or a measured throughput improvement.

Locks stay transaction-owned through existing commit/rollback/cancellation. Tests observe a real queued waiter and cancelled server ownership disappearance, rather than using sleeps to establish phases. No worker, cache, queue, retry loop or unbounded retained state is added. Full TKE nightly acceptance is not claimed. This is separate from temporary DELETE (#30089/#30091) and prepared pruning (#29661).

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

@LeftHandCold

Copy link
Copy Markdown
Collaborator Author

已修复这轮 CI 的共同编译阻断,保留本 PR 原有 SQL 修复与全部测试门禁。

根因不是本 PR 的 SQL 专项失败:Main 的 #30029 引入 Gossip fixture 调用后,#29978 将 randomPorts.allocate 改为 (first, limit, probe),但遗漏了该调用点。SCA、UT 和 UT Coverage 都报 gossip_fixture_test.go:130: not enough arguments;Coverage 汇总因缺少 UT profile 随后失败。干净 Main 21f19ae063 和原 PR head 上已复现同一编译错误。

最小修复仅改一个测试文件:先获取现有 testPortRange(),错误直接返回,再把合法范围传给分配器。新增两节点配置检查,确认六个端口不重复且在允许范围内。不改生产服务端口、原有锁逻辑、重试上限或 CI 门禁。

本地验证(50,Linux/amd64,Go 1.27.1):

  • Gossip fixture 全部定向测试通过,含真实端口冲突、失败清理和重建。
  • Logservice 全包 -short -tags=matrixone_test 通过(196.285s)。
  • 新端口检查 race 100 次通过;六节点 Gossip 收敛/重启 race 6 次通过。
  • 完整仓库 molint 成功、golangci-lint v2.14.0 零问题;另核对另一 PR 的不同包也零问题。
  • 临时 DELETE planner 专项通过;视图替换真实双 CN 专项再次通过。原远端 UT 中两个 PR 各自的 SQL 集成专项、构建和 BVT 本来也已通过。

同一修复已同步 #30091 和 #30092。推送会触发完整 CI;本地验证不冒充新的远端 ARM64 SCA/完整覆盖率已绿,最终以新一轮 CI 结果为准。

@LeftHandCold LeftHandCold left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

深度 review 结论:没有发现本 PR 引入的阻断级问题,代码层面建议 APPROVE。当前认证账号与 PR 作者相同,GitHub 不允许 self-approve,因此以 COMMENT 记录结论,不冒充正式审批。

范围:main / merge-base 21f19ae063 → 最新 HEAD 7a6b891311,完整审查 3 个文件;审查中发现 HEAD 更新后,已补看端口 fixture 修复。

设计与正确性

  • #30090 的修复落在正确的入口:RC CREATE OR REPLACE VIEW 在拿数据库锁前进入已有 C → G → D 准入。没有放宽 FastFail、新增重试或另造生命周期状态。
  • 真实保留前序数据锁的事务仍由现有准入逻辑选择 FastFail;新鲜事务可以等待 SNAPSHOT gate,而不是被本语句提前取得的数据库锁误判。
  • 保留后面的 lockMoDatabase 和嵌套 DROP 不是应当直接删除的重复代码:前者保留普通锁的定义变更检查,后者仍负责 DROP 的对象身份/域复核。提前取得准入不能替代这些检查。
  • 普通 CREATE VIEW、非 RC 路径未改。额外锁请求、catalog 校验和 snapshot barrier 限于 RC 的 CREATE OR REPLACE VIEW 语句(包括目标尚不存在的情况),不进入 SELECT/DML 热路径;不声称该 DDL 零开销。
  • 最新 Gossip 增量正确复用 testPortRange() 和现有有界 allocator,范围解析失败在配置创建前返回;不改变产品端口、重试预算或 CI 门禁。
审计 结论
Q1 所有权/清理 准入锁继续归同一事务,沿已有 commit/rollback/cancel 释放;取消测试在仍持有 blocker 时确认服务端事务消失,再验证旧视图未变。
Q2 等待/退出 恢复 C → G → D 顺序,保留 retained-lock FastFail;锁等待继续使用现有 context/lock-wait budget。测试采用真实入队观察点,不靠 sleep 判定阶段。
Q3 增长/资源 无新增常驻 goroutine、缓存或重试队列;新增端口测试只创建两节点配置,复用已有清理。

验证与边界

  • 已核对 旧 HEAD CI run 38346948671:实际 checkout c45ff22405;Linux/amd64 race 模式下 TestIssue30090ReplaceViewLifecycleOrder 及 release/cancel/prepared-reader/retained-lock 四个子场景均 PASS,顶层用例 2.33s;pkg/sql/compile PASS 18.193s,issues 四个执行批次均 exit 0。两个已启用 BVT job 也通过。
  • c45ff22405 → 7a6b891311 仅改变 Logservice 的 _test.go;上述 SQL 生产代码、测试、依赖和构建配置均未变,该 SQL/race 证据可复用。
  • 已读取 50 上此次修复的原始日志和验证记录:Logservice 全包 PASS 196.285s、新端口检查 race 100 次 PASS、六节点 Gossip consumer race 6 次 PASS;最新 head 双 CN 视图专项复跑 PASS 8.656s。也核对了最新源码 hash、Go 1.27.1 gofmt 和 diff-check。
  • 我额外发起的单任务 race 复跑限制为 2 核 / 6 GiB / 5 分钟;到上限时未产生测试执行结果,任务已停止回收,不把这次复跑计作 PASS,也不把它当成产品测试失败。没有运行全套本地 UT。
  • 旧 CI 的 randomPorts.allocate 参数不匹配属于独立的测试编译问题,最新增量已修正;最新完整 CI 38361983307 尚未完成,本结论不代表所有合并门禁已绿,也不替代完整 nightly 验收。

当前没有待处理的 inline review thread。没有需要以 speculative blocker、代码风格或额外抽象为由要求修改的项。

@XuPeng-SH

Copy link
Copy Markdown
Contributor

Updated in 76c1ab96c0, rebased onto main 8aaa8ae9e6 with an explicit old-head lease.

  • Addressed the review P3: removed TestGossipFixtureConfigPortRange. Existing TestGossipFixtureConfigPortsUseListenerRange retains all range/uniqueness oracles and checks nine rendered listener addresses.
  • Fixed an additional confirmed compatibility regression: no USE + constant CREATE VIEW returns 1046, but the prior replacement patch returned an internal lifecycle error. A three-line guard restores 1046 before engine/lock/admission effects, with typed unit and real two-CN controls.
  • Registered holder rollback immediately after BeginTx to cover assertion-failure cleanup; retained existing cancel/unblock/join and server-drain checks. No new lock state or retry path.

Final source validation: whole compile normal/race PASS (10.704/12.754s), whole issues normal/race PASS (265.380/644.282s), no failures/skips in either issues mode. Five real public subcases, three same-process race repetitions, updated barrier/reconnect normal/race, retained listener test and configured incremental SCA all passed. Source fingerprints are unchanged by committing. Required actual gpt-6.1-sol/xhigh design and final overall gates PASS.

New remote CI has been triggered by the push; its completion is not claimed. No merge or review dismissal/approval was performed. Added admission work remains bounded to pessimistic RC replacement DDL; ordinary SELECT/DML paths are unchanged. PR body now records the final scope, validation and limitations.

@XuPeng-SH

XuPeng-SH commented Oct 13, 2026 •

Copy link
Copy Markdown
Contributor

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

Commit: 76c1ab96c0bd1c570fe61a8f6885c372edb7b8cd

Review completed. Read the assessment.

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

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

Commit: 76c1ab96c0bd1c570fe61a8f6885c372edb7b8cd

Assessment: approve

Reviewed the complete three-file change and lifecycle ownership chain. Early admission fixes fresh view replacement while preserving retained-lock FastFail, cancellation cleanup, nested DROP validation and error 1046. The previous duplicate-test concern is resolved; no actionable findings remain.

Model: gpt-6-astra / xhigh

Session: 01a12d8f-8e48-7622-ad17-b605f08450ce · slot 1 · generation 0

Validation:

  • Verified clean frozen HEAD and matching patch; the supplied patch only omits its final newline.

  • Built native prerequisites from matching exported inputs through make cgo; retained input/artifact hashes and build receipt. Isolation preflight passed.

  • Normal TestScope_CreateView and the two-CN TestIssue30090ReplaceViewLifecycleOrder passed, covering missing database, gate release/cancellation, subsequent replacement, prepared-reader rebinding and retained-lock rejection. Receipt: checks/1791929695133278154-2.json; log: focused-tests.log.

  • Race regression passed (10.73s), followed by two same-process repetitions; complete pkg/sql/compile passed under race (11.107s). No failed or skipped tests. Receipt: checks/1791930212238638345-2.json; logs: focused-race.log, focused-race-stress.log, compile-package-race.log.

  • Commands, source fingerprints, terminal receipts and log hashes reconciled in /home/xupeng/.local/state/mo-bot/jobs/a862e6697d35ecbcdde54466/attempt-1/review-evidence-audit.json; final reconciliation receipt: checks/1791930250595323085-2.json.

Limitations:

  • No full-repository or deployed-cluster suite was run. Author-reported broader checks lacked retained execution artifacts and were not counted as independent passes.

  • No quantitative latency benchmark was performed; added admission work is confined to pessimistic RC view replacement.

@mergify

mergify Bot commented Oct 13, 2026 •

Copy link
Copy Markdown
Contributor

Merge Queue Status

  • ✅ Entered queue — 2026-10-13 23:27 UTC · Rule: main · triggered by rule Automatic queue on approval for main
  • 🟠 Checks running · in-place
  • 🚫 Left the queue — 2026-10-13 23:37 UTC · at ac624dc92cf7edb2982b9a13f2601e1e7ce87300

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

Reason

Pull request #30092 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.

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.

Waiting for any of
  • check-neutral = CI Required
  • check-skipped = CI Required
  • check-success = CI Required
All conditions

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

  • Requeue this pull request

This branch had an error being deployed

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

Labels

dequeued kind/bug Something isn't working kind/enhancement kind/test-ci size/M Denotes a PR that changes [100,499] lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: CREATE OR REPLACE VIEW takes database locks before lifecycle admission and spuriously FastFails

3 participants