feat: support PERFORM SELECT statements - #26601
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? |
iamlinjunhong
left a comment
There was a problem hiding this comment.
Findings
[P1] 失败的 PERFORM 会污染下一条 saved query
PERFORM 把 batch 写入 query-result saver,但只有 runner.Run() 成功后才显式 finalize;runner 一旦报错会直接返回。status_stmt.go L44-L53
saveBatch 在真正创建/写入对象前就增加 queryRowCount 和 blockIdx;writer 创建、写 batch 或 WriteEnd 均可能失败。query_result.go L111-L145 失败时 pipeline 的 Output.Reset 不调用 terminal callback,output/types.go L131-L134,而 blockIdx、大小和行数只在 saveMeta 的 defer 中重置。query_result.go L160-L187
因此,下一个成功查询看到 blockIdx != 0 后跳过初始化,从 block 2 开始写,并为新的 statement ID 生成包含不存在的 block 1 的 metadata。query_result.go L63-L76 result_scan/meta_scan 随后可能读到错误路径、旧计数和旧过期时间。这正好破坏 issue #13060 的核心使用场景。
建议为 query-result generation 增加明确的 abort/reset 操作,并在 runner、saver、cancel、kill 的所有失败出口调用;长期应把这些 session 字段封装成每语句独立 generation。
[P2] 括号查询绕过 PERFORM ... INTO OUTFILE 拒绝
select_stmt 遇到括号查询时会新建一层 tree.Select,mysql_sql.y L6292-L6297,而 PERFORM 只给这层外部节点设置 IsPerform。mysql_sql.y L14442-L14446
Plan 校验只检查外层 stmt.Ep。build.go L436-L443 所以:
PERFORM (SELECT 1 INTO OUTFILE 'result.csv');
内层节点持有 Ep、外层 Ep == nil,不会命中显式拒绝,也不会初始化 frontend export config,最后会执行查询并返回 OK,而不是报 not supported。mysql_cmd_executor.go L4048-L4063
同一语法还通过 select_with_parens 接受 PERFORM (VALUES ROW(1)) 和 PERFORM (TABLE t),mysql_sql.y L6614-L6630,与 PR 声明的“non-SELECT statements 为 non-goal”不一致。
建议增加递归 AST 校验,或为 v1 使用只接受目标 SELECT 形态的专用 grammar production。
[P2] SIDECAR 路径完全绕过 PERFORM 的响应契约
COM_QUERY 在正常解析前拦截 /*+ SIDECAR */。mysql_cmd_executor.go L4938-L4960 Sidecar handler 会接受带 IsPerform 的 tree.Select,随后格式化 AST;formatter 又会把 perform 前缀写回 SQL。sidecar_offload.go L242-L279、select.go L42-L45
结果是配置 sidecar 后,/+ SIDECAR */ PERFORM SELECT ... 要么被下游 SQL 方言拒绝,要么成功后仍由 ExecRequest 返回 ResultResponse 和结果行,而不是 affected-rows=0 的单个 OK。
建议检测 sel.IsPerform 后回退到 CN pipeline;若确实需要 offload,则发送内部 SELECT,但在 MO 侧继续执行保存结果、抑制行和返回 OK 的完整契约。
[P2] 新的 "Perform" statement type 漏掉 trace 聚合
PR 将 statement type 从 "Select" 改成 "Perform"。select.go L77-L83 但 StatementInfoFilter 只聚合 "Select" 等旧类型,没有 "Perform"。report_statement.go L163-L175
因此符合聚合条件的短 PERFORM 会逐条写入原始 statement telemetry,而同等 SELECT 会合并;高频调用会显著增加 trace/ETL 存储量。建议加入 "Perform",或基于稳定的 DQL 分类判断,并补充过滤器用例。
4ee6b0a to
5964bac
Compare
|
Addressed all four findings on exact head
Validation: full |
gouhongshen
left a comment
There was a problem hiding this comment.
Codex automated review
The four previously raised issues are addressed on the current head, but one blocking grammar/planner gap remains. Focused parser tests and diff validation passed; CGo-backed tests could not run locally because libmo.dylib is absent. The worktree is clean.
P2 - Reject INTO OUTFILE in parenthesized UNION branches (pkg/sql/plan/build.go:438)
This checks only the outer Select.Ep. The new perform_select delegates to simple_select, whose UNION productions accept parenthesized branches. Both PERFORM (SELECT 1 INTO OUTFILE 'result.csv') UNION SELECT 2 and the symmetric right-branch form parse with outer IsPerform=true/Ep=nil and inner Ep!=nil. getUnionSelects then passes that inner Select directly to bindSelect, and no recursive planner check reads its Ep, so the statement executes and returns OK instead of the promised not-supported error. Validate export parameters throughout the PERFORM query or restrict these grammar branches, with left/right UNION regression tests.
|
Addressed the new exact-head finding by recursively rejecting INTO OUTFILE anywhere in the PERFORM query tree, including either side of a UNION. Added parser-to-planner regressions for top-level, left-branch, and right-branch exports. Focused and full owning-package tests, go vet, git diff --check, self-review, and exact-head preflight pass on 5782d90. GitHub CI is pending. |
gouhongshen
left a comment
There was a problem hiding this comment.
Codex automated review
Saved-result cleanup, unsupported parenthesized forms, SIDECAR handling, trace aggregation, and the prior left/right UNION blocker are fixed. However, the claimed rejection “anywhere” still misses CTE bodies and expression subqueries, leaving one blocking correctness gap. git diff --check passes; focused tests could not start because cgo/libmo.dylib is absent.
P2 - Reject INTO OUTFILE inside PERFORM CTEs and subqueries (pkg/sql/plan/build.go:435)
Author response: The author says this head recursively rejects INTO OUTFILE anywhere in the PERFORM query tree and added top-level and both UNION-side tests. Why this remains blocking: selectHasExportParam follows only Select.Select, ParenSelect, and UnionClause; it returns false at SelectClause and never inspects Select.With. The grammar stores a CTE body in With.CTEs[i].Stmt, so perform with c as (select 1 into outfile 'result.csv') select * from c has IsPerform=true and Ep=nil outside but an inner CTE Select with Ep!=nil. This check therefore passes, while the frontend also examines only the outer st.Ep, executes the query through the PERFORM status path, and returns OK instead of the promised not-supported error. This differs from the fixed UNION finding because the export is below With, not on the SelectStatement/UNION spine. Traverse CTE bodies and subqueries reachable from SelectClause, or enforce the exclusion in the grammar, with parser-to-planner regressions for those shapes.
|
Addressed the current-head review finding in The PERFORM guard now traverses the complete parser tree, so Validation:
GitHub CI for this exact head is now running. |
gouhongshen
left a comment
There was a problem hiding this comment.
Codex automated review
All prior blockers are resolved. Saved-result failure cleanup, parenthesized-form restrictions, SIDECAR routing, trace aggregation, UNION export rejection, and the CTE/expression-subquery export gap are fixed. The latest traversal reaches those nested AST forms and is covered by rejection and valid-control tests, so the previous blocker is withdrawn. No remaining finding identified; git diff --check passes. Local package execution was unavailable because cgo/libmo.dylib is absent.
gouhongshen
left a comment
There was a problem hiding this comment.
Codex automated review
Revalidated the complete change at the unchanged previously approved head; the follow-up delta is empty. Saved-result failure cleanup, parenthesized-form restrictions, SIDECAR routing, trace aggregation, UNION export rejection, and the CTE/expression-subquery export blocker all remain fixed. The latter blocker remains withdrawn. Focused parser tests and diff validation pass; no new finding identified.
Merge Queue Status
This pull request spent 47 minutes 59 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 ↓. |
…erform # Conflicts: # pkg/sql/parsers/dialect/mysql/mysql_sql.go
Merge Queue Status
This pull request spent 22 minutes 22 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. Tick the box to put this pull request back in the merge queue (same as
|
What type of PR is this?
Which issue(s) this PR fixes:
issue #13060
What this PR does / why we need it:
Adds v1 support for
PERFORM SELECT ...as a non-reserved SQL prefix.last_query_id(),result_scan, andmeta_scancan inspect saved PERFORM output, while failed or cancelled statements finalize saved-query state as failed.PERFORM SELECT ... INTO OUTFILE, including when the export is nested in a UNION branch, CTE body, or expression subquery, withnot supported: PERFORM SELECT INTO OUTFILE.performnon-reserved for existing table, column, and alias identifiers.Non-goals: PostgreSQL expression-form PERFORM/FOUND semantics, the
PERFROMtypo alias, non-SELECT statements, and changes to MySQLDO.QA Decision
Required.
Reason: this changes user-visible SQL syntax, MySQL response shape, permission behavior, prepared-statement metadata, stored-procedure output, and query-result behavior.
Production entrypoint:
COM_QUERY / COM_STMT_PREPARE / COM_STMT_EXECUTE -> parser -> SELECT planner -> CN pipeline -> query-result saver -> OK responseExact-head evidence
298dc63bbc6707289dcf877563e3a3fc11113ba958b741a2aecd9cddce4a1c5faf54ad99aed56b3bPASS review=PASS validation=PENDING7a46804454fd3f69ebd296678bf8f185337ce0636d8a1aa959e3fea277af844bLocal validation on this exact head:
.agents/skills/mo-dev/scripts/mo-cgo-test ./pkg/sql/plan -count=1: PASSgit diff --check: PASSEarlier validation on prior heads remains supporting historical evidence and was not rerun after the latest nested-export fix:
make buildperform.sql47/47,do.sql2/2,query_result.sql196/196GitHub CI for the current exact head is pending and remains an independent gate.
QA scenarios
save_query_result,last_query_id(),result_scan, andmeta_scan, including failure/cancellation cleanup.ROW_COUNT() = 0; failed PERFORM recordsROW_COUNT() = -1.SERVER_MORE_RESULTS_EXISTS.SELECT ... INTO OUTFILEare rejected before execution.Keep issue #13060 open after merge. Transfer it to a confirmed tester with
phase/testing; close only after terminal QA PASS evidence records the tested version/SHA, environment, scenarios, and regression result.