Repository navigation
[4.2-dev] fix: make account restore repeatable and rebind grants - #29431
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? |
|
@XuPeng-SH CI rollout blocker on
Run 36650587281 used trusted workflow SHA #28547 removed the old aggregate names and documented migrating required contexts to the GitHub Actions Correction to the earlier status: the aggregate job passed, not all required merge checks. The merge gate remains blocked by the missing contexts and required review. |
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
Deep review: REQUEST_CHANGES
审阅 head 37fbfd16dca22168ae5fd9218af6a396845702bb，base/merge-base 28f1f02a89339def1b2572dbe07e98144a203167。31 个文件的实现、测试、SQL/golden 均已审阅；未开启 subagent。
统一 grants 恢复入口、复用恢复事务、按 account/database/name/kind 重绑对象 ID、复用 DATA BRANCH 授权的方向合理。两个公开 SQL 反例说明授权/ownership 的边界尚未闭环，详见两条 inline comments：
- P1：跨账号恢复后，旧目标连接继承另一个身份的 SELECT 权限。 恢复前拒绝，恢复后同一物理连接读到 42；旧用户的新连接却因用户已不存在被拒绝。清 grants cache 没有使旧 UserID/DefaultRoleID 失效。
- P2：合法 TABLE RESTORE 被未重建数据库的历史创建者阻断。 表创建者仍有效；删除数据库创建者前恢复成功，删除后对同一快照恢复报错，数据仍保留但恢复无法完成。
验证与工程要求
- 使用该 head 的 CI 共享构建产物（BUILD_INFO SHA 匹配，
-cover），在独立端口、UUID、gossip seed 和 MinIO 前缀的实例复现；GOMAXPROCS=2、GOMEMLIMIT=1GiB、各配置 memory cache 8MB，表只有一两行。两项反例及正向对照均有断言，夹具/实例已清理。 - 复用 exact-head CI 的 UT、SCA、PROXY/PESSIMISTIC 多 CN BVT、Coverage 和 CI Required 成功证据。它们未覆盖上述反例，不能据此认定授权修复完整。
- 优先扩展现有 authenticated restore/PITR fixture，覆盖同 ID 不同身份的旧连接、跨 CN、prepared EXECUTE、失败恢复，以及已有数据库/实际重建数据库的区别。保留真实表 owner 缺失时的拒绝和回滚，不增加重复集群/状态机。
性能与评论闭环
每语句清权限缓存取消了跨语句 warm hit，让普通查询/prepared EXECUTE 重新进入 background executor、授权事务和角色/目录查询。PR 已说明未 benchmark；本次覆盖率构建和共享机器反例也不是匹配 base/head 的性能基准，性能尚不能给 PASS。优化应保留跨连接/CN 的立即 REVOKE/RESTORE 生效，由已有权限目录与会话生命周期统一提供有效性依据。
作者提出的 required-context 名称迁移 仍未闭环：本次只读 branch protection 仍要求五个旧状态名，实际成功 jobs 使用 execution 名称，PR 为 BLOCKED/REVIEW_REQUIRED。CI Required 成功不等于所有保护要求成功。
Iceberg E2E 日志显示 MinIO 镜像拉取 unauthorized、createbuckets 中断，尚未进入 SQL 测试；没有证据将其归因于本 PR，也不能当作 Iceberg 功能通过。
|
|
||
| // Cache grants only within one statement. A session-local cache cannot | ||
| // observe REVOKE or RESTORE committed by another connection or another CN. | ||
| if cache := ses.GetPrivilegeCache(); cache != nil { |
There was a problem hiding this comment.
[P1] 清 grants cache 没有使恢复前的已认证身份失效，旧连接可以继承另一个角色的权限。
已在该 head 的实际构建产物用公开 SQL 复现，权限检查开启：
- 新建 source/target 两个账号，各有
app.secret(id int)和一行 42。source 的第一普通角色/user 为secret_reader/source_user，role_id=3、user_id=3，授予 CONNECT 和 SELECT。target 的第一普通角色/user 为guest/target_user，也是 role_id=3、user_id=3，只授予 CONNECT 和 INSERT。 - 保留
target#target_user#guest的物理连接。SELECT * FROM app.secret被拒绝。 - SYS 执行源账号快照和恢复：
CREATE SNAPSHOT s FOR ACCOUNT source; RESTORE ACCOUNT source {snapshot='s'} TO ACCOUNT target; - 同一个旧 target 连接的 SELECT 返回 42，
current_user(),current_role()仍为target_user@localhost,guest。此时用旧用户名/角色重新登录却报there is no user target_user。恢复后的target#source_user#secret_reader新连接能读 42，作为恢复成功的正向对照。
这里的 cache.invalidate() 只让 grants 重新查目录；登录时 TenantInfo 保存的 UserID/DefaultRoleID 没有被刷新或失效。authenticate.go 的权限搜索 直接使用旧 DefaultRoleID。恢复后数字 3 属于另一个角色，且 grants 已正确重绑到新表，于是旧身份获得新角色的 SELECT。数字 ID 相同不能证明身份相同。
请将身份目录替换与已有账号/会话生命周期衔接，复用账号版本/AccountRoutineManager/跨 CN 会话失效职责：成功恢复后旧身份在下一次授权前失效或重新验证；失败回滚不提前使有效连接失效。仅提交后异步 kill 仍有生效前窗口。普通查询、text/binary EXECUTE 和跨 CN 应共享这个边界，并在现有认证 fixture 中保留同 ID 不同名称的旧连接进行断言。
There was a problem hiding this comment.
The fix in ac0f58d, confirmed by your12:18UTC APPROVED review, is retained in current59f36461. Authorization now proves the current account/user ID plus authenticated username and role relations against the statement catalog snapshot; numeric-ID coincidence alone cannot authorize an old connection. Existing cache validity, replay and session/account lifecycle owners supply that boundary for ordinary, text/binary prepared and protected native commands. The shared authenticated regressions cover cross-CN retained identities, positive replacement-login controls and failed-restore retained access. This heartbeat's latest delta is limited to license headers and behavior-neutral analyzer cleanup; the exact59f source tree has fresh frontend-race PASS23.825s and full frontend configuredlint0issues. Latest remote CI remains unfinished; prior review/proofs are not called current-head CI passes. Please confirm the thread closure together with the final follow-up; no self-resolution.
There was a problem hiding this comment.
CI follow-up in 377869d preserves this identity boundary. The actual 59f BVT failures exposed an internal-executor mismatch: DefinerOpts supplied the creator and numeric IDs, but applyOverride changed only the protocol username, leaving TenantInfo.User as internal/empty. The shared override now parses the existing canonical identity once, propagates malformed-name errors before SQL and closes allocated sessions on failure; ID/name authorization is not relaxed. Tests cover real taskservice.DefinerOpts, qualified names, explicit IDs, root ID zero and malformed base/command overrides. The snapshot fixture now requires the retained acc02:admin connection to be denied and checks 50/11 rows through a fresh restored acc02:test_user login. The protected-database fixture reuses its real tenant connection, not a cached physical SYS connection. Fresh local full frontend/taskservice race and frontend configured lint pass; strict task BVT is 170/170 and snapshot BVT 215/215. Protected-database strict metadata is 160/164, with the same four header differences on pristine 59f; its separate nometa semantic comparison is 164/164, not a strict pass. Latest remote CI is unfinished. Please review this final identity/fixture follow-up; the original retained-session rejection remains enforced.
| } | ||
| } | ||
| owners := make(map[restoreObjectName]restoreOwner, len(dbs)+len(tables)) | ||
| for _, row := range append(dbs, tables...) { |
There was a problem hiding this comment.
[P2] TABLE RESTORE 将未重建数据库的历史 creator/owner 也作为必需身份，错误拒绝合法表恢复。
已在该 head 复现：普通用户 db_builder 创建 app 数据库；账号管理员创建 app.t(id int) 并插入 1，做账号快照。目录里数据库 creator=3（db_builder），表 creator=2（仍存在的 admin）。
INSERT INTO app.t VALUES (2);
RESTORE TABLE app.t {snapshot='s'}; -- 成功，结果恢复为 1，正向对照
INSERT INTO app.t VALUES (2);
DROP USER db_builder;
RESTORE TABLE app.t {snapshot='s'};
-- 20101: cannot restore app.: creator no longer exists失败后表仍为 1、2：没有破坏数据，但合法恢复无法完成。表自身的 creator/owner 仍有效，目标 app 数据库也未被重建。
table 模式仍读取历史数据库 ownership，这个循环将 dbs/tables 一起做 principal 映射并要求数据库 creator 存在。snapshot.go 对已有数据库只是 CREATE DATABASE IF NOT EXISTS，无需安装历史数据库身份。
请复用实际恢复目标/对象是否重建的决定，按实际将重建的对象验证 ownership；已有数据库的表恢复应保留当前数据库身份。不要用 SYS fallback 放宽实际表 owner 缺失的拒绝。同一 helper 的 PITR 表级路径也要同步核对。最小回归应覆盖数据库创建者缺失但表身份有效时成功、实际表 creator/owner 缺失时失败、数据库真正重建时仍检查历史数据库身份，以及 snapshot/PITR 的对应边界。
There was a problem hiding this comment.
The actual recreation-set fix in ac0f58d, confirmed by your latest APPROVED review, is preserved in current59f36461. Snapshot/PITR table restore retains the existing parent DB identity and does not require its historical creator/owner; a truly recreated database/table still requires the applicable historical principals before destructive DDL. The existing shared authenticated fixture covers retained DB success and missing actual-table-owner rejection/rollback, without SYS fallback. Current follow-up removes only an unreachable SQL helper/constant and aligns the prepared receiver name; full frontend-race PASS23.825s and configured frontendlint0issues on the identical current source tree. No ownership behavior change, new restore branch or weakened rejection. Please confirm the discussion can close; remote checks/protection are still separate.
Rebind restored grants to current logical objects while preserving surviving principals and parent owners. Validate cached privileges against statement snapshots and storage-owned catalog revisions, with role provenance and failure-safe cache sealing. Reuse immutable prepared requirements, unify executable EXPLAIN checks, and remove redundant SQL record parsing from trusted binary execute envelopes. Cover retained identities, cross-CN revocation, publication ordering, rollback, cancellation, prepared reuse and role boundaries.
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
Deep review: APPROVE
审阅 head ac0f58d322729557896418cce3c8abdb0d4d32b6，base/merge-base 28f1f02a89339def1b2572dbe07e98144a203167。本轮直接审查，未开 subagent。未发现新的阻塞性代码问题。
之前的代码评论
- P1：旧连接身份串权，已解决。 授权验证当前账号、用户 ID/已认证用户名及角色关系；覆盖跨 CN、普通查询、text/binary prepared、旧管理员原生入口，以及失败恢复后保留合法访问。对应 discussion_r4162050879。
- P2：表恢复误检查父数据库历史创建者，已解决。 表恢复复用现存数据库时保留当前父库身份；实际重建对象仍检查 creator/owner，缺失身份继续拒绝。Snapshot/PITR 共用该边界。对应 discussion_r4162050881。
设计、失败路径与工程质量
- grants 重绑仍由原恢复事务负责；授权复用现有 privilege cache/evaluator，目录版本由 logtail/replay 负责，prepared requirements 绑定现有 AST/plan 生命周期。没有新增并行权限存储、后台同步任务或重复恢复事务。
- 删除 Session.priv、可变 matched-role 字段、重复权限范围查询及失效注释路径。恢复失败回滚，授权失败清除未证实的部分缓存；取消/关闭能终止快照等待，缓存受 1024 scopes/256 KiB 名称限制。
- 测试覆盖公开 SQL、跨 CN、实际 KILL 回滚、同一快照的独立 SQL 对照、prepared 重用和拒绝写入后的数据断言。实现净增 1,429 行，测试/预期净增 3,837 行，共享夹具净增 5 行；规模较大，但对应不同正确性与生命周期边界，未发现阻塞性的重复框架。
- 授权契约是语句快照下的目录视图。单次缓存校验依赖 TN 现有时间戳分配顺序，该依赖已有说明及真实存储回归保护。
验证与性能范围
本轮核对并复用同一代码树 db83b2d966c790a6a0c0280165ad7dd43febd001 的已有验证，没有重复运行测试：frontend -race、增量 SCA、vet、完整构建，以及分布式 SQL/有界压力验证通过。
72 个读性能进程、16 个配对块的四项零回退门槛通过；COUNT/SELECT 中位延迟分别改善约 3.79/2.69 µs，p95 也改善。六轮较长并发 UPDATE 未支持明显退化结论；基线波动较大，不能据此保证所有写负载性能不变。
尚未完成的仓库配置项
issuecomment-5907964607 的 required-context 名称迁移仍未完成：最新读取的 4.2-dev 保护配置仍要求五个旧名称。这是独立的合并配置问题；本次代码 approve 不表示保护条件已满足，也不表示最新 CI 已通过。未等待或 rerun CI，未修改仓库保护配置。
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
Deep review: REQUEST_CHANGES
审阅 head 377869d30673d2d48aaec0ba421acdb9c2222f47，base/merge-base 28f1f02a89339def1b2572dbe07e98144a203167。本轮未开 subagent。
已确认的阻塞问题
P2：SQL task 的 accountadmin 身份只绑定了用户名，没有完整绑定租户/角色名称，导致合法管理员语句被拒绝。 详见 inline comment。
真实公开 SQL 验证使用现有三 CN authenticated fixture，一个账号、一张两行表、一个 reader role，无授权/catalog stub：
-- qa_task_admin#admin#accountadmin 连接；app.t 和 reader 已存在
revoke select on table app.t from reader; -- 成功：直接执行的正向对照
grant select on table app.t to reader;
create task qa_task_insert as begin insert into app.t values(2); end;
execute task qa_task_insert; -- 成功：表 count=2
create task qa_task_revoke as begin revoke select on table app.t from reader; end;
execute task qa_task_revoke; -- 20101: do not have privilege
-- reader 的 SELECT grant 仍有 1 条，未完成 REVOKE黑盒测试 16.921s，确定性失败。另一个白盒挑战使用真实 DefinerOpts、newCmdSession 和 authenticateUserCanExecuteStatement：SYS admin 与完整命名的同一数字身份通过，实际非 SYS task principal 被拒绝。既有 IE focused suite 通过（0.044s），说明当前字段/格式断言没有覆盖该行为。测试串行，GOMAXPROCS=2、GOMEMLIMIT=2GiB；创建账号已清理，测试进程已退出。
修复应归属现有 internal session 身份绑定入口，保证数字 IDs 与所有现有 policy 使用的身份信息一致，保留真实 membership 校验。不能简单复制 Creator 中的角色：SET ROLE 后 Creator 登录名与 CreatorRoleID 可能不同。建议扩展已有 fixture，覆盖合法 tenant-admin REVOKE 的实际 grant 变化、普通角色拒绝、SET ROLE 和 bare username/显式 ID 边界；不要另加权限缓存或绕过管理员检查。
整体设计、旧评论和工程质量
- 完整变更图为 79 文件：实现 31 文件 +2063/-620，测试/夹具 39 文件 +3967/-550，结果 9 文件 +522/-19，无 tracked 文档增量。复用 此前完整评审 的未变化部分，并重新审阅相对 ac0f58d 的全部 9 文件 +130/-38 及共享身份/授权闭包；没有把另一份未交付 staged 实现当作当前证据。
- 原恢复事务仍负责 grants 重绑与回滚，现有 privilege cache/evaluator、storage replay/subscription 版本、PrepareStmt 生命周期各自持有职责。整体方向合理，没有发现另建事务、存储或后台状态机的必要。
- cache 有 1024 scopes/256KiB 名称上限；失败/取消清理未证实缓存；internal override 解析失败关闭新 session。dead helper/constant 已清理。F1 暴露的是身份生产者与现有 policy 消费者不一致，应统一关闭这个边界。
- discussion_r4162050879 旧连接身份串权、discussion_r4162050881 表恢复误查未重建父库 creator：代码问题已解决，当前 delta 未破坏原修复；两条 GitHub threads 尚未 resolved。
- required-context 名称迁移 仍未闭环：最新读取的 4.2-dev 保护要求旧名称，实际 producer 使用 execution 名称。这是管理员配置项，独立于 F1。
验证与性能范围
当前 head CI run 37023454288 的 SCA 和 UT Coverage 已成功；评审抓取时实际 UT、PROXY/PESSIMISTIC BVT 尚在运行，未等待 CI，也未将 pending 计为通过。Iceberg 镜像 unauthorized 在进入 SQL 前失败，不能计为功能通过或归因为本 PR。
普通/text/binary prepared 未变化路径复用先前 72 个读性能进程、16 个配对块的证据；本轮没有重新跑 benchmark。读结论只覆盖已测 warm COUNT/SELECT，六轮 UPDATE 诊断不能保证所有写负载无回退。既有 task/procedure BVT 通过也没有覆盖本轮复现的管理员 task 形状。
本轮只有一条已确认代码 blocker；不把尚未证明公开可达的理论越权路径作为发现。
| return err | ||
| } | ||
| if acc := sess.GetTenantInfo(); acc != nil { | ||
| acc.SetUser(principal.GetUser()) |
There was a problem hiding this comment.
[P2] 完整绑定 SQL-task definer 的身份，避免合法 accountadmin REVOKE 被拒绝
这里仅 SetUser，非 SYS 的 newCmdSession 原本只从 context 填数字 IDs，Tenant/DefaultRole 名称仍为空。SQL-task 的实际 producer 使用 AttachAccount(AccountID, CreatorUserID, CreatorRoleID)，因此即使 ID 对应 accountadmin，IsAccountAdminRole/IsAdminRole 的名称判断仍为 false。我已用真实三 CN 公开 SQL 复现：同一管理员直接 REVOKE 成功、INSERT task 成功，但 REVOKE task 报 20101 do not have privilege，SELECT grant 仍保留。完整命名的同一数字身份可通过实际授权函数的正向对照。
请在现有身份绑定入口统一保证 principal 与 name-based policy 一致，并保留当前 membership 校验；同时覆盖 SET ROLE 后 Creator 登录字符串与实际 CreatorRoleID 不同的场景，不能直接把登录字符串中的角色当作实际选中角色。
There was a problem hiding this comment.
Fixed in 465d94d. I reproduced the actual grant-changing accountadmin task failure on377869d3 (direct REVOKE/INSERT task positive controls), plus SET ROLE and bare-name/explicit-ID failures. The existing internal-session boundary now resolves tenant and selected-role names by final numeric IDs after both overrides, reusing the role lookup; username/ID and current-membership checks remain enforced. SET ROLE also exposed fresh native-only task metadata taking account0 from a plan-populated field. Task creation and every task/run filter now use the authenticated TenantInfo IDs consistently; Creator retains its original reader login while CreatorRoleID selects accountadmin. Final public authenticated regression asserts actual grant removal, selected-role provenance, bare-ID positive, ordinary/selected-reader/mismatched-user-ID denial, and revoked-membership denial; the complete TestIssue29399 family passes22.993s. Full frontend/taskservice race passes24.760/44.518s, final IE/task-handler10x race2.763s, frontend lint0issues, binary build and independent local four-file review APPROVE. No preparatory SELECT, fallback, role/membership bypass, workflow/tester/golden change or cache framework. Current-head CI is newly triggered, not passed; old377 ALL CI success is historical. Please re-review this delivered fix; I have not self-resolved or approved the thread.
SCA follow-up in 8f27fa7: the separate Makefile err-check rejects the fmt.Errorf role-error wrapper (job110928686599). Returning the existing lookup error directly also preserves *moerr.Error, which IsMoErrCode does not unwrap. Both missing-account/role Exec and Query now assert the original error code; the old wrapper fails this strengthened control. Fresh make err-check PASS, IE/task-handler 10x race PASS2.949s, complete authenticated issue family PASS25.966s, configured frontend lint0issues and independent two-file local review APPROVE. Selected-role binding, membership rejection and cleanup are unchanged; no workflow/tester/policy suppression. Latest-head CI remains unfinished; please include this error-contract follow-up in the re-review.
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
Deep review: APPROVE
审阅 head 465d94dcb1db8aced132a0ea9b60bf361b2cb6ae，base 28f1f02a89339def1b2572dbe07e98144a203167。本轮直接评审和独立 QA 挑战，未开 subagent。未确认新的 P1/P2 代码问题。
评论闭环
- 4162050879：恢复后旧连接身份串权，代码已解决。 当前账号/用户 ID、已认证用户名及角色校验仍覆盖普通查询、text/binary prepared 和受保护 native 入口；新 task 改动保留该边界。
- 4162050881：表恢复误检查已有父数据库的历史 creator，代码已解决。 Snapshot/PITR 共用实际重建对象的 ownership 边界；已有父库保留当前身份，实际重建表的缺失 creator/owner 仍拒绝。
- 4167312881：SQL-task definer 身份不完整，代码已解决。 在两层 override 后按最终数字 IDs 绑定 tenant/selected-role 名称，复用现有用户名解析和角色查询。SET ROLE 后不从原始登录字符串恢复旧角色。Task 创建、管理和 run filters 统一使用 authenticated TenantInfo。真实 SQL 验证 accountadmin REVOKE、SET ROLE、bare-name/explicit-ID 正向控制及拒绝路径通过。
- 5907964607：required CI context 名称迁移，仍未完成。 最新读取的 4.2-dev protection 仍要求五个历史名称，属于独立管理员配置事项。三个代码 review threads 目前也仍未 resolve；代码 closure 与 GitHub discussion 状态应区分。
设计、失败路径和工程质量
Grants 重绑仍由现有恢复事务统一负责，授权复用现有 evaluator/cache，目录版本归 replay/subscription owner，prepared requirements 归 PrepareStmt replacement/Close。未发现阻塞性的重复状态机、并行权限存储或额外恢复事务。旧 Session.priv、matched-role 状态、重复范围查询及无用 SQL helper 已清理。
| 审查维度 | 结论 |
|---|---|
| 资源归属 | Binder 的 background executor 由 defer 关闭；绑定失败关闭已分配 session。恢复回滚和 prepared 清理保留原 owner。 |
| 等待终止 | Catalog SQL 使用现有 caller/session timeout；无新增后台重试或等待循环。ReadSnapshot 在等待前释放 client lock，并保留 caller cancel/client close 边界。 |
| 有界增长 | Privilege cache 保持 1024 scopes/256 KiB 名称限制；certificate 只保留标量版本，prepared 状态跟随 plan 生命周期。 |
当前 head 本地验证
Linux amd64 / Go 1.26.4，使用仓库 CGo wrapper：
- 定向 internal-executor/SQL-task-handler -race：PASS，1.989 s。
- 真实 authenticated
TestIssue29399SQLTaskDefinerAuthorization-race：PASS，19.381 s。断言实际 grant 删除、SET ROLE provenance、普通/选中 reader/mismatched-user-ID 拒绝及撤销普通角色 membership 后拒绝。 - 独立外置 Go overlay probe 12 场景 -race：PASS，1.224 s：Exec/Query × account/role lookup × ordinary/canceled/deadline error。错误链保留，后续读取及用户 SQL 停止，background Close 恰好一次，session proc/tenant 清理。Probe 不随 PR 交付；该证据验证注入错误返回的传播和清理，不声称证明任意阻塞 transport 的实时取消或 durable rollback。
- 当前增量 configured SCA（frontend/issues，自 ac0f58d）：0 issues；完整 PR diff whitespace 检查通过。
未变化的 restore/replay/snapshot/prepared/SQL-record 路径复用先前精确源码验证；未重复运行或宣称当前 head 全量测试/构建通过。
性能和增量
Public warm binary COUNT/SELECT 热路径未被最新提交改变；原有 72-process/16-block 测量四项零回退上界均通过，中位延迟变化分别约 −3.792/−2.694 µs。六轮较长 UPDATE 不构成统计上的写负载不回退保证，也不证明所有 workload 无下降。
具名 internal session 每条语句增加一次 selected-role query，非 SYS 再增加一次 account-name query。此 correctness 成本没有 task-path 对比测量，不能保证 SQL task 性能不下降。 未具名 bootstrap 和 public warm 路径不承担这两次名称查询。
完整当前 diff 分别审查：
| 类别 | 文件数 | 新增 | 删除 | 净增 |
|---|---|---|---|---|
| 生产代码 | 32 | 2115 | 627 | +1488 |
| 测试/结果 | 46 | 4634 | 569 | +4065 |
| 共享夹具 | 2 | 5 | 0 | +5 |
| 仓库文档 | 0 | 0 | 0 | 0 |
增量较大；新增 task 回归复用现有 authenticated cluster，验证真实数据/目录效果并包含拒绝对照；mapping/error mocks 与 public SQL、真实 snapshot/timestamp、KILL rollback 各证明不同边界。未发现阻塞性的重复测试框架，未因测试占比高而免审。
此 APPROVE 是代码评审结论，不表示配置评论已完成、当前 CI 已通过或 merge gate 已满足。本轮未等待/rerun CI，未更新源码 commit 或 PR body。
This branch was successfully deployed
3 participants
What type of PR is this?
Which issue(s) this PR fixes:
Fixes #29399 on
4.2-dev.What this PR does / why we need it:
Repeated account restore misinterprets stored CLONE provenance as CREATE TABLE metadata. Copying historical object IDs also leaves restored grants attached to the wrong objects, while retained connections can reuse obsolete user and role identities.
PrepareStmt, tied to its existing plan replacement/Close lifecycle. Remove the mutable session requirement slot and matched-role field. Ordinary, binary/text prepared and executable EXPLAIN wrappers use the shared evaluators. Preserve EXPLAIN output handling across compile reuse.Visibility follows the existing statement RC snapshot and logtail frontier. The TN serial pre-prepare/prepare ordering establishes the inclusive snapshot boundary; a real-store regression and a fixed-clock SQL/publication oracle protect this invariant.
Validation
Rebased the original PR branch onto
4.2-devat28f1f02a89339def1b2572dbe07e98144a203167.Current local checks use Linux amd64 / Go 1.26.4 and the repository CGo wrapper:
go vet: passed. The final cleanup regression passes-race(1.888 s) and 46 repeated same-process runs (32.724 s). The TN ordering regression passes 100 repetitions (1.740 s); its full owning package race passes (2.256 s). The completemo-servicebinary builds with matching prebuilt native libraries. The final binary-envelope delta also passes frontend incremental SCA (0 issues) andgo vet.Performance
The final warm binary COUNT/SELECT experiment passes all four predeclared zero-margin nonregression gates. Sixteen balanced ABBA/BAAB blocks plus four A/A control pairs completed 72/72 successful processes. Each variant in each block ran both workload orders; each workload had 2,000 warm-up and 30,000 measured queries.
Candidate-minus-base latency (negative is faster):
Each endpoint uses a one-sided 98.75% t bound across 16 block contrasts (simultaneous 95% coverage for four endpoints, assuming independent block errors). Acceptance required every upper bound ≤ 0; no positive tolerance was allowed. Matched native inputs, frozen binaries, CPU affinity, GOMAXPROCS and raw per-query samples were retained. An earlier candidate failed this gate; profiling identified redundant binary record parsing, which the final candidate removes.
A longer four-connection prepared UPDATE check used 200 warm-up and 10,000 measured updates per worker (40,000 per process), with disjoint primary keys and affected-row assertions. All six AA+ABBA runs passed:
This longer write diagnostic reports all observed values; six processes do not establish a statistical nonregression bound for UPDATE. No tolerance was introduced from the A/A variability.
These measurements support the stated warm workloads, not universal workload parity. Catalog changes and other uncertified cache states still require SQL validation. Earlier ordinary/admin/public, text prepared and JOIN probes provide supporting coverage but are not a statistical whole-system throughput guarantee.
Change size
The following original implementation counts exclude the final license-header, behavior-neutral analyzer-cleanup and internal-executor/BVT follow-ups.
Most new implementation is restore grant rebinding and a bounded certificate over the existing privilege cache. Storage revisions stay with replay state; authorization stays with the existing evaluator. Prepared requirements reuse PrepareStmt lifecycle. Tests cover distinct public SQL, distributed publication, restore rollback, and ownership boundaries; benchmark probes and measurement artifacts are not shipped.
Review / CI
The requested Astra xhigh design and independent gpt-6.1-sol xhigh review are recorded with exact source and test artifacts. Delivered implementation commit:
ac0f58d322729557896418cce3c8abdb0d4d32b6; current head:8f27fa7ff0fcee217feef013e476be1495ced552(license/analyzer cleanup followed by scoped internal-executor and BVT identity fixes). Original implementation independent gpt-6.1-sol / xhigh review: APPROVE for source treedb83b2d966c790a6a0c0280165ad7dd43febd001, with no unresolved material blocker. The reviewer independently checked all 72 read-process raw samples and the six UPDATE runs, and reconciled prior source, ownership, failure-path and test-quality reviews. UPDATE variability and unmeasured-workload limits remain explicit above.Implementation-head SCA job 110829631877 failed the configured license header check, identifying
authorization_cache_test.go,prepared_authorization.go, andexplain_authorization_test.go. Commit6dee99854aed55b661db220767c4c68703d8de51completes their standard Apache headers only; all non-header bytes in that delta are unchanged. Local license-eye v0.4.0 checks pass for the three files and all tracked configured source extensions (4,614 checked; 0 invalid). Whitespace checks and independent local header-delta review pass. The earlier implementation/runtime and benchmark evidence above is unchanged-source evidence, not a fresh whole-suite run for that comment-only commit.The next SCA job 110843254669 passes header/dependency checks, then reports three analyzer findings: an unreachable user-role SQL helper/constant and inconsistent receiver naming on
PrepareStmt. Follow-up head59f364619cf30816a7be4629db15a4070a6a1137removes only that dead helper/constant and aligns the receiver name. A concurrent local candidate has the identical complete source-tree hash8c7a50c7db5305733cb365b74f7a4846f5a7015f; the remote commit is preserved, with no duplicate push or force push. On this exact source tree, full frontend configured lint reports 0 issues, full frontend -race passes (23.825 s), and independent read-only local cleanup review approves. No authorization/cache/prepared-plan semantics, tests, workflows or linter policy are changed.Current CI-regression follow-up
Completed prior-head run 37014090595 passes Ubuntu UT, arm64 SCA, shared build and UT Coverage, but does not pass BVT or its aggregates. PROXY 110863242559 has six failures: two reads through the obsolete cross-account target-admin connection and four protected-database operations through a reused session ID that still belongs to a deleted SYS user. PESSIMISTIC 110863242558 has 58 failures in the two SQL-task files because the definer username was not propagated into the internal authorization principal. Downstream Coverage and CI Required fail; no changed-code coverage percentage is claimed for that run.
Commit
377869d30673d2d48aaec0ba421acdb9c2222f47fixes the existing internal-executor identity boundary and the two stale BVT session assumptions. The SQL-task golden results and authorization checks stay unchanged; the old-admin denial is retained alongside the legitimate restored-login positive control. No workflow, tester, metadata suppression, protection policy, native build input or generated transfer file is changed.Fresh monitor-local proof on macOS ARM64 / Go 1.26.4 / Java 20:
@@protected_databasesbut return@@global.protected_databases. Pristine prior-head control is 156/164, with the same four labels plus the four identity failures addressed here. Protected row/error semantic validation is 164/164 using the existing localnometaoption; full strict metadata validation is not passed. No label expectations or tester configuration are committed. This local prior-head control does not establish a clean-upstream/platform root cause.Prior-head ALL CI 37023454288 for
377869d3subsequently completes successfully: actual Ubuntu UT, SCA, UT Coverage, shared build, PROXY/PESSIMISTIC BVT, Coverage and CI Required succeed. Mongo E2E succeeds. Iceberg 37023457426 / 110892062786 fails at 15:02:48 UTC while pulling MinIO (unauthorized), before integration SQL; the persistent setup blocker is not blindly rerun. None of these prior-head successes replaces the new-head checks below. The previously confirmed4.2-devprotection rule requires five historical context names while producers use execution names; the existing administrator request is separate from code fixes.SQL-task principal follow-up (prior head
465d94dcb1)SQL-task follow-up head:
465d94dcb1db8aced132a0ea9b60bf361b2cb6ae. XuPeng's new P2 4167312881 identifies a valid in-scope producer/consumer mismatch: non-SYS task sessions have user/role IDs but lack tenant/role policy names, so legitimate accountadmin REVOKE is rejected. A fresh authenticated three-CN reproduction on377869d3passes direct administrator REVOKE and INSERT task, but fails actual REVOKE task, SET ROLE task and bare-name/explicit-ID positive controls (18.020 s).Monitor-local macOS ARM64 / Go 1.26.4 proof on the
465d94dcb1source:TestIssue29399*family, including the final public task controls plus retained identity, snapshot/PITR and failed-restore authorization, passes (22.993 s). Test-only atomic[no statements]is not production statement coverage. The earlier intermediate candidate passed administrator/bare controls but still failed SET ROLE because metadata stored account 0; the final canonical-account correction closes that failure without a preparatory SELECT.The remote CI and local proofs for this prior
465d94dcb1head are historical; see the exact-current-head CI status below. Earlier warm-workload measurements remain unchanged-path evidence, not a universal performance or merge-protection guarantee.Current SCA Error-Contract Follow-up
Current head:
8f27fa7ff0fcee217feef013e476be1495ced552. Prior-head SCA 110928686599 fails the repository Makefileerr-checkat 16:42:08 UTC: the new role-errorfmt.Errorfwrapper is prohibited. This is introduced by the SQL-task follow-up, not an infrastructure failure. The previous golangci-lint success did not exercise this separate policy.*moerr.Errorand error code. Tenant/user/selected-role binding, fail-before-SQL, membership rejection and background/session cleanup are unchanged. No substitute exception, new error wrapper, fallback, linter suppression, workflow or tester change.ExecandQuerycontrols assertmoerr.ErrInternalin addition to their messages. The original role wrapper deterministically fails this strengthened contract (1.524 s); the repository'sIsMoErrCodeintentionally requires a direct*moerr.Error.Fresh local proof of this two-file follow-up (macOS ARM64 / Go 1.26.4 with real absolute CGo inputs/rpaths):
make err-check: PASS, after a recorded pre-fix rejection.TestIssue29399*family: PASS (25.966 s), including actual accountadmin REVOKE, fresh SET ROLE, bare IDs, ordinary-role and revoked-membership denial plus retained restore/PITR/identity controls. Test-only atomic[no statements]is not production coverage.Normal push follows a fresh awaited/read OPEN/head/body guard. XuPeng-SH subsequently submitted an actual APPROVED review on this exact
8f27fa7fhead at 2026-10-02 17:17:31 UTC; no redundant review request or self-resolution is made.Current-head ALL CI run 37036619505, attempt 1, completes SUCCESS at 17:40 UTC: actual Ubuntu UT, arm64 SCA, shared build, UT Coverage, PROXY/PESSIMISTIC multi-CN BVT, Coverage and CI Required all succeed. The CI Required log confirms
fullscope, generation37036619505-1, all five dependency groups successful andcoverage_ready=true/coverage_status=passed. The Coverage log selects UT plus complementary BVT profiles from that same generation and publishes the merged result. The current SCA log confirms the prohibited-error check, license headers (4,642 files; 0 invalid) and configured analyzer result (0 issues) pass. Mongo E2E also succeeds. Skipped checks are not tests passed.The actual current-head Iceberg job remains FAILURE before SQL: MinIO image pull is unauthorized. Its workflow-level SUCCESS comes from the existing
continue-on-error, not a successful integration test. No additional rerun is requested without recovery evidence. The PR remains OPEN / Ready / MERGEABLE / BLOCKED / APPROVED. The five historical Required contexts were last confirmed still expected by the reviewer's read-only protection inspection at 16:49 UTC; this monitor cannot revalidate that configuration through its unreadable protection API or unsigned-in browser. Completed execution jobs and CI Required do not establish that all merge protections are satisfied. Three code discussion threads remain unresolved; their code fixes and current-head approval are separate from discussion status. No protection, Ready/base, approval or merge policy is changed.