Repository navigation
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
LeftHandCold
left a comment
LeftHandCold
left a comment
There was a problem hiding this comment.
代码 review 结论：PASS，未发现本次修改引入的阻断问题。注意这是代码审查结论，不是 CI 全绿或允许绕过合并门禁。当前审查账号与 PR 作者同为 LeftHandCold，GitHub 不允许自我 APPROVE，因此以 COMMENT 记录。
范围：HEAD 2f897d7；获取的 main / base / merge-base 为 21f19ae。完整审查 4 个文件的全部改动，并结合 #30089 的复现、两套 planner、临时表解析、索引维护和 runtime TRUNCATE 的入口核对。属于聚焦 bug fix，不需要新状态机或重大设计文档。
设计与正确性：
- 责任放在 DELETE→TRUNCATE 的既有准入判断处是合适的：modern planner 拒绝临时表重写；legacy 的主表和索引两处同时排除临时表，避免只删主表却跳过隐藏索引。
- 保留 isUnrestrictedDelete 的语义分类；只是禁止临时表物理替换，没有误把全删当成带过滤条件的 DELETE，也未改动永久表的 eligibility、显式 TRUNCATE 或 retained-lock FastFail。
- TxnCompilerContext.Resolve 为会话别名/已拥有的物理临时表设置 IsTemporary；索引通过现有 ResolveIndexTableByRef 解析。modern 路径维护各索引的 UpdateCtx；legacy 路径保留已有隐藏索引全扫描及 row-delete，最终 CanTruncate=false，不能再进入 Scope.TruncateTable 的持久生命周期门禁。
- 新测试复用已有两 CN fixture，用另一个真实事务持有 SNAPSHOT shared gate，DELETE 必须在 holder 释放前成功；检查 rollback 的固定行值、提交后唯一键复用、同名永久表不受影响。没有依赖 sleep 或概率调度。
Unhappy-path / 性能审计：
| 维度 | 结论 |
|---|---|
| Q1 归属/清理 | 生产代码复用原有行 DML 的事务/索引所有权；测试在获取连接、事务后安排 Close/Rollback，临时表和数据库有清理，未新增后台资源。 |
| Q2 等待 | 未修改锁协议或添加等待/重试；DELETE 不再错误申请全局生命周期独占锁。测试有 2 分钟总上下文、10 秒 DELETE 和 20 秒清理上下文。 |
| Q3 增长/性能 | 没有新缓存、队列或状态机；但大临时表无条件 DELETE 由物理替换变为数据量相关的扫描、行删除和索引维护，会增加时间及事务资源成本。接受 PR 已披露的这项正确性取舍，不将其表述为性能无损；永久表不受新增条件影响。 |
证据（复用并核验当前精确 HEAD，未在本地重复启动 MO 集群）：
- Ubuntu/x86 UT job：确认 checkout 为上述 HEAD，Go 1.27.1 / linux amd64。下载该 run 的 ut-diagnostics artifact，核对 JSON 终态和 checkpoint：pkg/sql/plan 8/8 race 分片 status=0；TestTemporaryDeleteDoesNotRewriteToTruncate 两个子场景均 PASS，0.01s。
- 同一报告中 TestIssue30089TemporaryDeleteAvoidsPersistentLifecycle PASS，1.77s；indexed_false / indexed_true 均 PASS；pkg/tests/issues 分片终态均 PASS。报告文件有裁剪标记，上述结论只引用仍在 artifact 中的明确终态，不以裁剪内容推断其他测试。
- 两项现有真实 BVT checks（Compose PROXY、PESSIMISTIC）为 SUCCESS；新增场景的 public-path 证据来自上面的真实 SQL 集成测试，而非单靠 mock/EXPLAIN。
- 本地 gofmt 检查四个变更文件和 git diff --check 均通过。没有既有人工 review thread 待闭环。
CI 限制：整体仍 FAILURE。UT 与 coverage 日志均指向 pkg/logservice/gossip_fixture_test.go:130 的 randomPorts.allocate(probeTestPort)，实际签名需要 (int, int, func(int) error)；SCA 日志也报告这一编译错误。已核对调用/定义文件在本 PR HEAD 与 base 的 blob 完全一致，这是源码层面的归属证据；没有额外重跑完整 clean-base CI，也不宣称 SCA 其他输出已经全部分类。应单独修复该编译阻塞并重新验证 CI，不能靠本次代码 PASS 将红色门禁当作通过。
|
已修复这轮 CI 的共同编译阻断，保留本 PR 原有 SQL 修复与全部测试门禁。 根因不是本 PR 的 SQL 专项失败：Main 的 #30029 引入 Gossip fixture 调用后，#29978 将 最小修复仅改一个测试文件：先获取现有 本地验证（50，Linux/amd64，Go 1.27.1）：
同一修复已同步 #30091 和 #30092。推送会触发完整 CI；本地验证不冒充新的远端 ARM64 SCA/完整覆盖率已绿，最终以新一轮 CI 结果为准。 |
Merge Queue Status
This pull request spent 10 seconds in the queue, with no time running CI. ReasonThe pull request can't be updated
HintYou should update or rebase your pull request manually. If you do, this pull request will automatically be requeued once the queue conditions match again. Requeued — the merge queue status continues in this comment ↓. |
## What type of PR is this? - [ ] API-change - [ ] BUG - [ ] Improvement - [x] Documentation - [ ] Feature - [ ] Test and CI - [ ] Code Refactoring ## Which issue(s) this PR fixes: Related to #25267. ## What this PR does / why we need it: PR and issue bodies submitted through CLI/API can miss repository template fields or use headings and line endings incompatible with description consumers. Add a shared GitHub delivery reference to `mo-dev` and `mo-self-review` that checks current templates and body encoding before publication, then verifies the stored body afterward. The guidance requires a real, relevant issue reference, preserves author content and intentional code escapes, covers required issue-form fields, and distinguishes a rendering failure from an unverified CRLF hypothesis. Both skills link to the same contract and keep publication checks in the appropriate phase. Validation: - Both skills pass `skill-creator` validation; shared-reference links resolve. - Executed the documented normalization example against BOM/mixed line endings, Unicode, literal code escapes, LF idempotence, empty input, and invalid UTF-8. - Complete diff review and `git diff --check` pass. This is agent guidance; the CRLF root-cause hypothesis for #30091 remains unverified.
aptend
left a comment
aptend
left a comment
There was a problem hiding this comment.
Deep review of exact head 95bf977a132e8db3f5be460f8807251c4b50bd40, base/merge-base 0a8a28f7582ab530c9672994c6d13cf4d976c0be: APPROVE. No actionable blocker found.
Reviewed every hunk in all five changed files, both historical reviews, all conversation comments, the complete #30089 reproduction, and the empty inline/thread history. This is a focused eligibility repair, not a major design-gated refactor. I independently checked both planner paths, compiler dispatch, session temporary/physical-name resolution, hidden-index resolution and maintenance, and existing TRUNCATE/lifecycle admission.
The modern eligibility guard and both legacy guards coherently exclude temporary targets. Legacy index deletion can no longer be skipped while the base table uses row deletion. Permanent-table eligibility, explicit TRUNCATE, and retained-lock FastFail are unchanged; the compiler selects existing row DML rather than persistent replacement. No new production ownership, goroutine, retry, cache or wait edge is introduced. The port-range addition is test-only, uses the existing bounded allocator and closes runtime registrations, including partial-configuration failure.
Independent validation in a detached exact-head worktree, CGO enabled with the canonical wrapper's verified matching primary native artifacts:
- Full planner normal package: PASS, 16.564s; full logservice normal package with -short: PASS, 195.744s.
- Full planner -race: PASS, 58.631s; twelve Gossip fixture roots / 43 named results under -race: PASS, 96.035s. Planner has two existing skips (cosine_similarity and resetInterval); they are not counted as passes.
- New planner eligibility and Gossip port-range roots each separately passed -race -count=100; measured T=0, B=30s, hence N=100.
- Committed authenticated two-CN temporary DELETE regression passed two normal rounds. Its exact -race measurement passed at T=7.43s; adaptive N=floor(30/7.43)=4 repetitions all passed (package 15.373s). Indexed/unindexed rollback, committed key reuse and same-name permanent isolation remained asserted.
- An additional temporary native probe passed twice on one physical session using real prepared/binary DELETE, a unique index plus a secondary index, duplicate NULL keys, UPDATE followed by DELETE behind a held SNAPSHOT reader, rollback restoration, explicit-transaction commit/key reuse, and public temporary TRUNCATE. The permanent same-name row remained 99. Probe was removed after validation; tracked source and diff whitespace are clean.
Temporary unrestricted DELETE now costs row scanning/deletion and index maintenance; this correctness tradeoff is accepted, not described as performance-neutral. No large-table benchmark or full nightly run was performed.
Exact-head build, Ubuntu UT, ARM64 SCA, coverage, PROXY and PESSIMISTIC BVT checks are successful. The Mergify queue rule failure is separately explained by its workflow-write permission error; it is not a product-test failure. This approval does not bypass merge protections or authorize permission changes.
Merge Queue Status
This pull request spent 1 hour 47 minutes 2 seconds in the queue, with no time running CI. ReasonPull request #30091 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.
HintYou 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. Waiting for any of
All conditions
Tick the box to put this pull request back in the merge queue (same as
|
This branch was successfully deployed
4 participants
What type of PR is this?
Which issue(s) this PR fixes:
issue #30089
What this PR does / why we need it:
Fixes #30089. Keep unrestricted DELETE on a session temporary table as transactional row DML instead of lowering it to persistent-table TRUNCATE lifecycle admission.