fix(frontend): allow binary prepare for single SET assignment - #26694
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
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
Deep review complete on exact head 223255882c8cbad48c661967f067ec3071a59d52; no blocking finding.
The grammar change closes only the binary COM_STMT_PREPARE entry gap by admitting one SET var_assignment. Multi-assignment remains rejected before planning, so this does not expose the separate #26685 panic/partial-update path. The real ExecRequest(COM_STMT_PREPARE) regression proves the retained tree.SetVar, one parameter slot, and rejection of two assignments.
The generated parser is reproducible from mysql_sql.y (only goyacc's output-path header differs when generated into a temp file) and reports zero conflicts. Exact-head validation: owning packages list/build/vet pass; full parser and frontend tests pass; focused test passes 10x and race passes 3x; git diff --check is clean.
Q1: the test-owned session and prepared statement are closed by the existing session lifecycle; no production resource owner changes. Q2: no new wait edge. Q3: a failed multi-assignment prepare is not installed in the prepared-statement registry, so no new accumulation path.
iamlinjunhong
left a comment
iamlinjunhong
left a comment
There was a problem hiding this comment.
Reviewed the complete diff on exact head 223255882c8cbad48c661967f067ec3071a59d52. No P0, P1, P2, or P3 findings.
The new grammar branch admits exactly one SET var_assignment for the internal binary COM_STMT_PREPARE wrapper and constructs the same tree.SetVar used by the existing prepared-SET planner/executor path. A second top-level assignment remains a syntax error before planning, so this change does not expose the separate multi-assignment panic/partial-update path tracked by #26685. I also traced prepare registration, parameter metadata, execution-time expression binding, authorization, and session cleanup; the patch does not add a resource owner, wait edge, or hot-path work outside parsing.
Validation on this head: goyacc regeneration completed with zero conflicts and matched the committed parser body; git diff --check is clean; the MySQL parser package build, vet, and full tests passed locally. All 26 GitHub checks are complete with no failing or pending check, including the frontend/owning-package CI coverage.
|
@Mergifyio refresh |
✅ Pull request refreshed |
Merge Queue Status
This pull request spent 1 hour 9 minutes 51 seconds in the queue, with no time running CI. Waiting for any of
All conditions
ReasonThe merge conditions cannot be satisfied due to failing checks Failing checks: HintYou may have to fix your CI before adding the pull request to the queue again. Requeued — the merge queue status continues in this comment ↓. |
Merge Queue Status
This pull request spent 45 minutes 12 seconds in the queue, including 41 minutes 59 seconds running CI. Required conditions to merge
|
…set-prepare # Conflicts: # pkg/sql/parsers/dialect/mysql/mysql_sql.go
4 participants
What type of PR is this?
Which issue(s) this PR fixes:
issue #25996
What this PR does / why we need it:
Binary
COM_STMT_PREPAREwraps the client payload asPREPARE <name> FROM <statement>. The prepareable grammar admitted SELECT/DML statements but notSET, soPrepareContext("SET @binary_value = ?")failed with ERROR 1064 before parameter binding even though #26096 already supports planning and executing prepared SET expressions.This patch admits only
SETwith one variable assignment in that grammar and regenerates the MySQL parser. The regression goes through the realExecRequest(COM_STMT_PREPARE)path, verifies the retainedtree.SetVarplan and parameter metadata, and confirms binary multi-assignment remains rejected.The single-assignment boundary is intentional: prepared multi-assignment currently has a separate panic and partial-update atomicity defect tracked by #26685. This PR does not expose that known path or change SET execution semantics.
Tested with:
go test -mod=readonly ./pkg/sql/parsers/dialect/mysql -count=1go test -mod=readonly ./pkg/frontend -count=1 -timeout=10mgo vet -mod=readonly ./pkg/sql/parsers/dialect/mysql ./pkg/frontendgo test -mod=readonly -race ./pkg/frontend -run "^TestExecRequestStmtPrepareAcceptsSetVariable$" -count=100go test -mod=readonly -race ./pkg/frontend -run "^TestPreparedSetBinaryProtocolReportsAndReplacesParameters$" -count=100go test -mod=readonly -race ./pkg/sql/parsers/dialect/mysql ./pkg/frontend -count=1The parser generator completed with zero conflicts. CGo-transitive frontend commands used the repository third-party include/library and
libmolinker configuration.