parser: support RETURNING clause syntax#65633
Merged
Merged
Conversation
ti-chi-bot
Bot
added
release-note
There was a problem hiding this comment.
Pull request overview
This PR adds support for the RETURNING clause in DML statements, specifically implementing full support for INSERT ... RETURNING while parsing (but not executing) UPDATE/DELETE ... RETURNING. The RETURNING clause allows INSERT statements to return the values of affected rows immediately after insertion, similar to MariaDB's implementation.
Changes:
- Added parser support for RETURNING clause syntax in INSERT, UPDATE, and DELETE statements
- Implemented planner logic to build RETURNING expressions, schemas, and column names for INSERT statements
- Added executor logic to capture and return inserted row data when RETURNING is specified
- UPDATE/DELETE RETURNING are parsed but return "not supported yet" errors
Reviewed changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pkg/parser/parser.y | Added grammar rules for RETURNING clause in INSERT, UPDATE, and DELETE statements with proper precedence |
| pkg/parser/misc.go | Added "RETURNING" keyword token mapping |
| pkg/parser/parser_test.go | Added parser test cases for RETURNING clause syntax validation |
| pkg/parser/ast/dml.go | Added Returning field to InsertStmt, UpdateStmt, and DeleteStmt AST nodes with Restore and Accept methods |
| pkg/planner/core/planbuilder.go | Implemented buildReturningClause method to convert RETURNING AST to expressions and schemas for INSERT |
| pkg/planner/core/logical_plan_builder.go | Added error handling to reject UPDATE/DELETE with RETURNING (not yet supported) |
| pkg/planner/core/point_get_plan.go | Disabled point get optimization when RETURNING clause is present for UPDATE/DELETE |
| pkg/planner/core/operator/physicalop/physical_common_plans.go | Added Returning, ReturningSchema, and ReturningNames fields to Insert, Update, and Delete structs |
| pkg/executor/builder.go | Modified buildInsert to set RETURNING schema on base executor and pass RETURNING expressions to InsertExec |
| pkg/executor/insert.go | Implemented RETURNING clause execution: appendReturningRow to capture rows, addRecord wrapper, and nextWithReturning to evaluate and return captured rows |
| tests/integrationtest/t/executor/returning.test | Added comprehensive integration tests for INSERT RETURNING covering various scenarios |
| tests/integrationtest/r/executor/returning.result | Expected results for RETURNING integration tests |
| e.appendReturningRow(row) | ||
| return nil | ||
| } | ||
|
|
There was a problem hiding this comment.
The addRecord wrapper only captures rows for RETURNING, but addRecordWithAutoIDHint is called directly at line 138 in the exec method, bypassing this wrapper. This means that for multi-row inserts, the first row of each shard batch (when i%sizeHintStep == 0) won't be captured in the RETURNING result, causing incomplete RETURNING output. You need to override addRecordWithAutoIDHint in InsertExec to also call appendReturningRow, similar to how addRecord is overridden.
Suggested change
| func (e *InsertExec) addRecordWithAutoIDHint(ctx context.Context, row []types.Datum, dupKeyCheck table.DupKeyCheckMode) error { | |
| if err := e.InsertValues.addRecordWithAutoIDHint(ctx, row, dupKeyCheck); err != nil { | |
| return err | |
| } | |
| e.appendReturningRow(row) | |
| return nil | |
| } |
bb7133
force-pushed
the
siddontang/returning-clause
branch
from
8772840 to
68d79a4
Compare
bb7133
force-pushed
the
siddontang/returning-clause
branch
from
68d79a4 to
68f1bb6
Compare
hawkingrei
force-pushed
the
siddontang/returning-clause
branch
from
fc78c71 to
ed4cca1
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #65633 +/- ##
================================================
- Coverage 77.7123% 76.8887% -0.8237%
================================================
Files 1991 2036 +45
Lines 552087 580288 +28201
================================================
+ Hits 429040 446176 +17136
- Misses 122127 131232 +9105
- Partials 920 2880 +1960
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
bb7133
force-pushed
the
siddontang/returning-clause
branch
from
ed4cca1 to
fb7a95e
Compare
bb7133
force-pushed
the
siddontang/returning-clause
branch
from
fb7a95e to
e412c24
Compare
Member
Author
|
/retest |
1 similar comment
Member
Author
|
/retest |
Contributor
|
@bb7133 Is it possible to break this PR into several? Like one parser only PR, one for INSERT RETURNING etc. If you need a reviewer, please assign me :) |
bb7133
force-pushed
the
siddontang/returning-clause
branch
from
e412c24 to
7421823
Compare
Member
Author
|
@mjonss Yes, splitting this into a parser-only precursor plus the INSERT RETURNING implementation makes sense. I have refreshed the branch against latest master first so the PR is no longer conflicting. If you prefer that review shape, I can extract the parser/AST/generated-parser pieces into a separate PR and keep this one focused on the executor/planner INSERT RETURNING work. I will also add you as a reviewer. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds SQL RETURNING clause parsing support to the TiDB parser for INSERT, UPDATE, and DELETE: registers the RETURNING keyword/token, adds Returning []*SelectField to DML AST nodes (restore and visitor traversal), updates grammar with ReturningClause and precedence fixes, and adds parser tests for RETURNING forms. ChangesSQL Parser RETURNING Clause Implementation
🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels: Suggested reviewers:
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/parser/parser.y (1)
6915-6920:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winScope the
RETURNINGprecedence hack toRETURNINGlists only.Line 6915 changes the shared
FieldAsNameOptempty arm to outrank theRETURNINGtoken, so ordinarySELECTfield lists can no longer use a bare alias namedreturning(for example,SELECT a returning FROM t). This should be isolated to a RETURNING-specific field/alias nonterminal instead of changing globalSELECTparsing behavior.Suggested direction
-FieldAsNameOpt: - /* EMPTY */ %prec higherThanReturning +FieldAsNameOpt: + /* EMPTY */ %prec empty { $$ = "" } | FieldAsName +ReturningFieldAsNameOpt: + /* EMPTY */ %prec higherThanReturning + { + $$ = "" + } +| FieldAsName + +ReturningField: + '*' %prec '*' + { + $$ = &ast.SelectField{WildCard: &ast.WildCardField{}} + } +| Identifier '.' '*' %prec '*' + { + wildCard := &ast.WildCardField{Table: ast.NewCIStr($1)} + $$ = &ast.SelectField{WildCard: wildCard} + } +| Identifier '.' Identifier '.' '*' %prec '*' + { + wildCard := &ast.WildCardField{Schema: ast.NewCIStr($1), Table: ast.NewCIStr($3)} + $$ = &ast.SelectField{WildCard: wildCard} + } +| Expression ReturningFieldAsNameOpt + { + $$ = &ast.SelectField{Expr: $1, AsName: ast.NewCIStr($2)} + } + +ReturningFieldList: + ReturningField + { + $$ = []*ast.SelectField{$1.(*ast.SelectField)} + } +| ReturningFieldList ',' ReturningField + { + $$ = append($1.([]*ast.SelectField), $3.(*ast.SelectField)) + } + ReturningClause: %prec empty { $$ = nil } -| "RETURNING" FieldList +| "RETURNING" ReturningFieldList { $$ = &ast.FieldList{Fields: $2.([]*ast.SelectField)} }You'd also need matching
%typeentries for the new nonterminals.Also applies to: 8053-8061
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/parser/parser.y` around lines 6915 - 6920, The change made FieldAsNameOpt's empty alternative to use %prec higherThanReturning, which unintentionally affects all SELECT field lists and prevents using an alias named "returning"; revert that global precedence change and instead create a RETURNING-scoped nonterminal (e.g., FieldAsNameOptReturning and related FieldAsNameReturning) whose empty arm uses %prec higherThanReturning, update the RETURNING-list productions to use these new nonterminals, and add matching %type declarations for the new nonterminals; apply the same scoping fix to the other occurrence referenced (the block around the second instance similar to 8053-8061).
🧹 Nitpick comments (2)
pkg/parser/parser_test.go (1)
920-931: ⚡ Quick winAdd parser/restore cases for the remaining INSERT RETURNING forms.
Lines 920-931 cover
VALUESandON DUPLICATE KEY UPDATE, but they don’t includeINSERT ... SELECT,INSERT ... SET, andINSERT IGNORE ... RETURNING, which are part of this PR’s scope. Adding them here would better lock the grammar/restore branches.🧪 Proposed test additions
{"INSERT INTO t (a) VALUES (1) RETURNING id, name", true, "INSERT INTO `t` (`a`) VALUES (1) RETURNING `id`, `name`"}, + {"INSERT IGNORE INTO t (a) VALUES (1) RETURNING id", true, "INSERT IGNORE INTO `t` (`a`) VALUES (1) RETURNING `id`"}, + {"INSERT INTO t SET a=1 RETURNING a", true, "INSERT INTO `t` SET `a`=1 RETURNING `a`"}, + {"INSERT INTO t (a) SELECT a FROM s RETURNING a", true, "INSERT INTO `t` (`a`) SELECT `a` FROM `s` RETURNING `a`"}, {"INSERT INTO t (a) VALUES (1) ON DUPLICATE KEY UPDATE a=2 RETURNING id", true, "INSERT INTO `t` (`a`) VALUES (1) ON DUPLICATE KEY UPDATE `a`=2 RETURNING `id`"},🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/parser/parser_test.go` around lines 920 - 931, The test table in parser_test.go is missing cases for INSERT ... SELECT, INSERT ... SET, and INSERT IGNORE ... RETURNING; add new entries to the existing test slice (the same block with other RETURNING cases) that assert correct restore strings for these forms — e.g. add a case for "INSERT INTO t (a) SELECT b FROM s RETURNING id" expecting "INSERT INTO `t` (`a`) SELECT `b` FROM `s` RETURNING `id`", a case for "INSERT INTO t SET a=1 RETURNING id" expecting "INSERT INTO `t` SET `a`=1 RETURNING `id`", and a case for "INSERT IGNORE INTO t (a) VALUES (1) RETURNING *" expecting "INSERT IGNORE INTO `t` (`a`) VALUES (1) RETURNING *"; place them alongside the existing RETURNING test rows so the parser/restore branches are exercised.pkg/planner/core/logical_plan_builder.go (1)
6227-6230: ⚡ Quick winFail fast for unsupported
RETURNINGbefore building/optimizing DML plans.Both guards are currently placed after expensive work (
BuildOnUpdateFKTriggers/DoOptimize). Since these paths always returnErrNotSupportedYet, reject earlier inbuildUpdate/buildDelete(right after AST-level validation) to avoid unnecessary planning overhead and side effects (extra warnings/stats-load interactions).💡 Suggested adjustment
func (b *PlanBuilder) buildUpdate(ctx context.Context, update *ast.UpdateStmt) (base.Plan, error) { + if update.Returning != nil { + return nil, plannererrors.ErrNotSupportedYet.GenWithStackByArgs("RETURNING clause") + } + b.pushSelectOffset(0) b.pushTableHints(update.TableHints, 0) ... - // Handle RETURNING clause - if update.Returning != nil { - return nil, plannererrors.ErrNotSupportedYet.GenWithStackByArgs("RETURNING clause") - } - return updt, nil } func (b *PlanBuilder) buildDelete(ctx context.Context, ds *ast.DeleteStmt) (base.Plan, error) { + if ds.Returning != nil && len(ds.Returning.Fields) > 0 { + return nil, plannererrors.ErrNotSupportedYet.GenWithStackByArgs("RETURNING clause") + } + b.pushSelectOffset(0) b.pushTableHints(ds.TableHints, 0) ... - // Handle RETURNING clause - if ds.Returning != nil && len(ds.Returning.Fields) > 0 { - return nil, plannererrors.ErrNotSupportedYet.GenWithStackByArgs("RETURNING clause") - } - return del, nil }Also applies to: 6703-6705
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/planner/core/logical_plan_builder.go` around lines 6227 - 6230, Reject unsupported RETURNING earlier: in buildUpdate and buildDelete, immediately after AST-level validation check for update.Returning != nil (or delete.Returning) and return plannererrors.ErrNotSupportedYet.GenWithStackByArgs("RETURNING clause") there, instead of waiting until later after BuildOnUpdateFKTriggers or DoOptimize; this avoids running BuildOnUpdateFKTriggers, DoOptimize and other expensive/side-effecting work when RETURNING is always unsupported.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/executor/insert.go`:
- Around line 57-61: The code currently accumulates all RETURNING rows in
returningRows and only emits after the entire INSERT finishes; change this to
stream RETURNING rows incrementally across Next calls by evaluating
returningExprs per affected row and producing chunks as you go, preserving
executor state between Next invocations. Modify the InsertExec state so instead
of populating returningRows you iterate the child/source rows inside Next (or a
helper like fetchChildRows) and for each affected row evaluate returningExprs
against returningSchema, append into the current output chunk, and return the
chunk when it reaches the executor chunk size or when the source is exhausted;
set returningDone only when no more rows remain and avoid retaining the full
result set in memory. Ensure you update/replace uses of
returningRows/returningDone in InsertExec, keep returningExprs and
returningSchema for per-row evaluation, and handle cancellation/errors exactly
as before while freeing any per-row temporary state as soon as it’s emitted.
In `@pkg/planner/core/operator/physicalop/physical_common_plans.go`:
- Around line 100-103: The MemoryUsage implementations for Insert, Update, and
Delete need to account for the newly added Returning, ReturningSchema, and
ReturningNames fields: update the MemoryUsage methods for the Insert, Update,
and Delete types to add the memory contributed by the Returning slice (include
slice header plus each expression's memory via its MemoryUsage or a conservative
per-element size), add the ReturningSchema reference memory (use
ReturningSchema.MemoryUsage() if available or include its structural size), and
add the ReturningNames slice memory (slice header plus names' bytes). Locate the
MemoryUsage methods named MemoryUsage on the Insert, Update, and Delete types
and add these three contributions using the same sizing approach/pattern used
elsewhere in the file for other slices/Schema/NameSlice to keep calculations
consistent.
In `@pkg/planner/core/planbuilder.go`:
- Around line 6462-6469: When handling field.WildCard in the RETURNING
expansion, first check whether the wildcard is qualified
(field.WildCard.Qualifier non-nil); if it is, only expand columns for the
current table when the qualifier matches this table's identifier or alias
(compare against the current table's name/alias/entry you use when building
tableNames), otherwise return an unknown-table error instead of expanding
tableSchema.Columns into exprs/cols/names. Ensure the unqualified case still
expands all visible columns as before.
- Around line 6474-6478: The rewritten plan returned from b.rewrite (np) is
being discarded for RETURNING expressions which breaks expressions that require
extra plan state (e.g., scalar subqueries); update the RETURNING handling in
planbuilder.go to check the returned np: if np != mockPlan then either
preserve/attach np to the surrounding plan node or explicitly reject the
expression by returning an error (e.g., "RETURNING does not support
non-row-local expressions"); specifically, change the code around the call expr,
np, err := b.rewrite(ctx, field.Expr, mockPlan, nil, true) to handle non-dual np
instead of unconditionally ignoring np so that expressions requiring subplans
are not lost.
- Around line 4199-4211: The INSERT branch that handles a RETURNING clause (in
buildInsert where buildReturningClause is called and
insertPlan.SetSchema/SetOutputNames are set) must also require SELECT privilege;
update the insert plan's required privileges so that when insert.Returning !=
nil and len(insert.Returning.Fields) > 0 you add the SELECT privilege (in
addition to existing INSERT/UPDATE/DELETE privileges) to insertPlan (e.g., via
the same mechanism used to record other privileges on the plan) before returning
the plan, ensuring SELECT is enforced when RETURNING is present.
---
Outside diff comments:
In `@pkg/parser/parser.y`:
- Around line 6915-6920: The change made FieldAsNameOpt's empty alternative to
use %prec higherThanReturning, which unintentionally affects all SELECT field
lists and prevents using an alias named "returning"; revert that global
precedence change and instead create a RETURNING-scoped nonterminal (e.g.,
FieldAsNameOptReturning and related FieldAsNameReturning) whose empty arm uses
%prec higherThanReturning, update the RETURNING-list productions to use these
new nonterminals, and add matching %type declarations for the new nonterminals;
apply the same scoping fix to the other occurrence referenced (the block around
the second instance similar to 8053-8061).
---
Nitpick comments:
In `@pkg/parser/parser_test.go`:
- Around line 920-931: The test table in parser_test.go is missing cases for
INSERT ... SELECT, INSERT ... SET, and INSERT IGNORE ... RETURNING; add new
entries to the existing test slice (the same block with other RETURNING cases)
that assert correct restore strings for these forms — e.g. add a case for
"INSERT INTO t (a) SELECT b FROM s RETURNING id" expecting "INSERT INTO `t`
(`a`) SELECT `b` FROM `s` RETURNING `id`", a case for "INSERT INTO t SET a=1
RETURNING id" expecting "INSERT INTO `t` SET `a`=1 RETURNING `id`", and a case
for "INSERT IGNORE INTO t (a) VALUES (1) RETURNING *" expecting "INSERT IGNORE
INTO `t` (`a`) VALUES (1) RETURNING *"; place them alongside the existing
RETURNING test rows so the parser/restore branches are exercised.
In `@pkg/planner/core/logical_plan_builder.go`:
- Around line 6227-6230: Reject unsupported RETURNING earlier: in buildUpdate
and buildDelete, immediately after AST-level validation check for
update.Returning != nil (or delete.Returning) and return
plannererrors.ErrNotSupportedYet.GenWithStackByArgs("RETURNING clause") there,
instead of waiting until later after BuildOnUpdateFKTriggers or DoOptimize; this
avoids running BuildOnUpdateFKTriggers, DoOptimize and other
expensive/side-effecting work when RETURNING is always unsupported.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c0522b19-8e61-4556-8407-ed69045c4205
📒 Files selected for processing (16)
pkg/executor/builder.gopkg/executor/insert.gopkg/parser/ast/dml.gopkg/parser/keywords.gopkg/parser/keywords_test.gopkg/parser/misc.gopkg/parser/parser.gopkg/parser/parser.ypkg/parser/parser_test.gopkg/planner/core/logical_plan_builder.gopkg/planner/core/operator/physicalop/physical_common_plans.gopkg/planner/core/operator/physicalop/plan_clone_generated.gopkg/planner/core/planbuilder.gopkg/planner/core/point_get_plan.gotests/integrationtest/r/executor/returning.resulttests/integrationtest/t/executor/returning.test
Comment on lines
+57
to
+61
| // For RETURNING clause support | ||
| returningExprs []expression.Expression | ||
| returningSchema *expression.Schema | ||
| returningRows [][]types.Datum | ||
| returningDone bool |
There was a problem hiding this comment.
Avoid full materialization of RETURNING rows before emission.
The current flow buffers every affected row in returningRows and only emits after the whole insert finishes. For large INSERT ... SELECT ... RETURNING, this can cause unbounded memory growth and high latency before any rows are returned.
Please switch to chunked/streamed RETURNING production (emit incrementally and keep executor state across Next calls) instead of collecting the full result set first.
Also applies to: 64-73, 422-466
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/executor/insert.go` around lines 57 - 61, The code currently accumulates
all RETURNING rows in returningRows and only emits after the entire INSERT
finishes; change this to stream RETURNING rows incrementally across Next calls
by evaluating returningExprs per affected row and producing chunks as you go,
preserving executor state between Next invocations. Modify the InsertExec state
so instead of populating returningRows you iterate the child/source rows inside
Next (or a helper like fetchChildRows) and for each affected row evaluate
returningExprs against returningSchema, append into the current output chunk,
and return the chunk when it reaches the executor chunk size or when the source
is exhausted; set returningDone only when no more rows remain and avoid
retaining the full result set in memory. Ensure you update/replace uses of
returningRows/returningDone in InsertExec, keep returningExprs and
returningSchema for per-row evaluation, and handle cancellation/errors exactly
as before while freeing any per-row temporary state as soon as it’s emitted.
Comment on lines
+100
to
+103
| // Returning stores the RETURNING clause expression list. | ||
| Returning []expression.Expression | ||
| ReturningSchema *expression.Schema `plan-cache-clone:"shallow"` | ||
| ReturningNames types.NameSlice `plan-cache-clone:"shallow"` |
There was a problem hiding this comment.
Update MemoryUsage() to include newly added RETURNING fields.
The new Returning* fields are added to Insert, Update, and Delete, but their corresponding MemoryUsage() methods still exclude those slices/schema/name memory. This under-accounts plan memory and can skew memory-control behavior.
💡 Suggested fix (pattern)
// Insert.MemoryUsage
- sum = ... + size.SizeOfSlice*7 + ...
+ sum = ... + size.SizeOfSlice*9 + ...
+ sum += int64(cap(p.Returning))*size.SizeOfInterface
+ sum += int64(cap(p.ReturningNames))*size.SizeOfPointer
+ if p.ReturningSchema != nil {
+ sum += p.ReturningSchema.MemoryUsage()
+ }
+ for _, expr := range p.Returning {
+ sum += expr.MemoryUsage()
+ }
+ for _, name := range p.ReturningNames {
+ sum += name.MemoryUsage()
+ }
// Update.MemoryUsage
- sum = ... + size.SizeOfSlice*3 + ...
+ sum = ... + size.SizeOfSlice*5 + ...
+ sum += int64(cap(p.Returning))*size.SizeOfInterface
+ sum += int64(cap(p.ReturningNames))*size.SizeOfPointer
+ if p.ReturningSchema != nil {
+ sum += p.ReturningSchema.MemoryUsage()
+ }
+ for _, expr := range p.Returning {
+ sum += expr.MemoryUsage()
+ }
+ for _, name := range p.ReturningNames {
+ sum += name.MemoryUsage()
+ }
// Delete.MemoryUsage
- sum = ... + size.SizeOfSlice
+ sum = ... + size.SizeOfSlice*3
+ sum += int64(cap(p.Returning))*size.SizeOfInterface
+ sum += int64(cap(p.ReturningNames))*size.SizeOfPointer
+ if p.ReturningSchema != nil {
+ sum += p.ReturningSchema.MemoryUsage()
+ }
+ for _, expr := range p.Returning {
+ sum += expr.MemoryUsage()
+ }
+ for _, name := range p.ReturningNames {
+ sum += name.MemoryUsage()
+ }Also applies to: 181-184, 236-239
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/planner/core/operator/physicalop/physical_common_plans.go` around lines
100 - 103, The MemoryUsage implementations for Insert, Update, and Delete need
to account for the newly added Returning, ReturningSchema, and ReturningNames
fields: update the MemoryUsage methods for the Insert, Update, and Delete types
to add the memory contributed by the Returning slice (include slice header plus
each expression's memory via its MemoryUsage or a conservative per-element
size), add the ReturningSchema reference memory (use
ReturningSchema.MemoryUsage() if available or include its structural size), and
add the ReturningNames slice memory (slice header plus names' bytes). Locate the
MemoryUsage methods named MemoryUsage on the Insert, Update, and Delete types
and add these three contributions using the same sizing approach/pattern used
elsewhere in the file for other slices/Schema/NameSlice to keep calculations
consistent.
Comment on lines
+4199
to
+4211
| // Handle RETURNING clause | ||
| if insert.Returning != nil && len(insert.Returning.Fields) > 0 { | ||
| returningExprs, returningSchema, returningNames, err := b.buildReturningClause( | ||
| ctx, insert.Returning, insertPlan.TableSchema, insertPlan.TableColNames, mockTablePlan) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| insertPlan.Returning = returningExprs | ||
| insertPlan.ReturningSchema = returningSchema | ||
| insertPlan.ReturningNames = returningNames | ||
| // Set the output schema for the insert plan to be the returning schema | ||
| insertPlan.SetSchema(returningSchema) | ||
| insertPlan.SetOutputNames(returningNames) |
There was a problem hiding this comment.
Require SELECT privilege when RETURNING is present.
This branch makes INSERT return row data, but buildInsert still only records INSERT/DELETE/UPDATE privileges. That lets statements like INSERT ... ON DUPLICATE KEY UPDATE ... RETURNING sensitive_col expose existing row values without any SELECT check.
Suggested fix
// Handle RETURNING clause
if insert.Returning != nil && len(insert.Returning.Fields) > 0 {
+ var selectErr error
+ if user != nil {
+ selectErr = plannererrors.ErrTableaccessDenied.FastGenByArgs("SELECT", user.AuthUsername, user.AuthHostname, tableInfo.Name.L)
+ }
+ b.visitInfo = appendVisitInfo(b.visitInfo, mysql.SelectPriv, tnW.DBInfo.Name.L, tableInfo.Name.L, "", selectErr)
+
returningExprs, returningSchema, returningNames, err := b.buildReturningClause(
ctx, insert.Returning, insertPlan.TableSchema, insertPlan.TableColNames, mockTablePlan)
if err != nil {
return nil, err
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Handle RETURNING clause | |
| if insert.Returning != nil && len(insert.Returning.Fields) > 0 { | |
| returningExprs, returningSchema, returningNames, err := b.buildReturningClause( | |
| ctx, insert.Returning, insertPlan.TableSchema, insertPlan.TableColNames, mockTablePlan) | |
| if err != nil { | |
| return nil, err | |
| } | |
| insertPlan.Returning = returningExprs | |
| insertPlan.ReturningSchema = returningSchema | |
| insertPlan.ReturningNames = returningNames | |
| // Set the output schema for the insert plan to be the returning schema | |
| insertPlan.SetSchema(returningSchema) | |
| insertPlan.SetOutputNames(returningNames) | |
| // Handle RETURNING clause | |
| if insert.Returning != nil && len(insert.Returning.Fields) > 0 { | |
| var selectErr error | |
| if user != nil { | |
| selectErr = plannererrors.ErrTableaccessDenied.FastGenByArgs("SELECT", user.AuthUsername, user.AuthHostname, tableInfo.Name.L) | |
| } | |
| b.visitInfo = appendVisitInfo(b.visitInfo, mysql.SelectPriv, tnW.DBInfo.Name.L, tableInfo.Name.L, "", selectErr) | |
| returningExprs, returningSchema, returningNames, err := b.buildReturningClause( | |
| ctx, insert.Returning, insertPlan.TableSchema, insertPlan.TableColNames, mockTablePlan) | |
| if err != nil { | |
| return nil, err | |
| } | |
| insertPlan.Returning = returningExprs | |
| insertPlan.ReturningSchema = returningSchema | |
| insertPlan.ReturningNames = returningNames | |
| // Set the output schema for the insert plan to be the returning schema | |
| insertPlan.SetSchema(returningSchema) | |
| insertPlan.SetOutputNames(returningNames) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/planner/core/planbuilder.go` around lines 4199 - 4211, The INSERT branch
that handles a RETURNING clause (in buildInsert where buildReturningClause is
called and insertPlan.SetSchema/SetOutputNames are set) must also require SELECT
privilege; update the insert plan's required privileges so that when
insert.Returning != nil and len(insert.Returning.Fields) > 0 you add the SELECT
privilege (in addition to existing INSERT/UPDATE/DELETE privileges) to
insertPlan (e.g., via the same mechanism used to record other privileges on the
plan) before returning the plan, ensuring SELECT is enforced when RETURNING is
present.
Comment on lines
+6462
to
+6469
| // Handle RETURNING * | ||
| if field.WildCard != nil { | ||
| // Expand * to all visible columns | ||
| for i, col := range tableSchema.Columns { | ||
| exprs = append(exprs, col) | ||
| cols = append(cols, col) | ||
| names = append(names, tableNames[i]) | ||
| } |
There was a problem hiding this comment.
Validate qualified wildcards before expanding them.
This treats every wildcard as bare *. RETURNING other_table.* or a mismatched schema-qualified wildcard would incorrectly expand the target table instead of raising an unknown-table error.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/planner/core/planbuilder.go` around lines 6462 - 6469, When handling
field.WildCard in the RETURNING expansion, first check whether the wildcard is
qualified (field.WildCard.Qualifier non-nil); if it is, only expand columns for
the current table when the qualifier matches this table's identifier or alias
(compare against the current table's name/alias/entry you use when building
tableNames), otherwise return an unknown-table error instead of expanding
tableSchema.Columns into exprs/cols/names. Ensure the unqualified case still
expands all visible columns as before.
Comment on lines
+6474
to
+6478
| expr, np, err := b.rewrite(ctx, field.Expr, mockPlan, nil, true) | ||
| if err != nil { | ||
| return nil, nil, nil, err | ||
| } | ||
| _ = np // We don't need the new plan for simple column references |
There was a problem hiding this comment.
Don't discard the rewritten plan for RETURNING expressions.
rewrite only returns a different np when the expression needs extra plan state, e.g. scalar subqueries. Ignoring it here means those expressions are accepted without preserving the subplan, so they'll fail or mis-evaluate later. If RETURNING is only meant to allow row-local expressions for now, reject non-dual np explicitly.
Suggested fix
expr, np, err := b.rewrite(ctx, field.Expr, mockPlan, nil, true)
if err != nil {
return nil, nil, nil, err
}
- _ = np // We don't need the new plan for simple column references
+ if _, ok := np.(*logicalop.LogicalTableDual); !ok {
+ return nil, nil, nil, errors.New("RETURNING doesn't support subqueries or other complex expressions yet")
+ }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/planner/core/planbuilder.go` around lines 6474 - 6478, The rewritten plan
returned from b.rewrite (np) is being discarded for RETURNING expressions which
breaks expressions that require extra plan state (e.g., scalar subqueries);
update the RETURNING handling in planbuilder.go to check the returned np: if np
!= mockPlan then either preserve/attach np to the surrounding plan node or
explicitly reject the expression by returning an error (e.g., "RETURNING does
not support non-row-local expressions"); specifically, change the code around
the call expr, np, err := b.rewrite(ctx, field.Expr, mockPlan, nil, true) to
handle non-dual np instead of unconditionally ignoring np so that expressions
requiring subplans are not lost.
7421823 to
fe20806
Compare
|
|
bb7133
changed the title
ti-chi-bot
Bot
added
do-not-merge/needs-tests-checked
release-note-none
Member
Author
|
@mjonss The split is done now.
|
bb7133
force-pushed
the
siddontang/returning-clause
branch
from
fe20806 to
5366eba
Compare
Member
Author
|
Updated both split PRs to align Changes:
Updated heads:
|
bb7133
force-pushed
the
siddontang/returning-clause
branch
2 times, most recently
from
9a4e0c1 to
bcd9b0a
Compare
tiancaiamao
approved these changes
May 14, 2026
ti-chi-bot
Bot
added
lgtm
and removed
needs-1-more-lgtm
[LGTM Timeline notifier]Timeline:
|
bb7133
force-pushed
the
siddontang/returning-clause
branch
from
bcd9b0a to
da245c5
Compare
yudongusa
approved these changes
May 18, 2026
yudongusa
left a comment
yudongusa
left a comment
There was a problem hiding this comment.
Please open a document PR and link to this
xhebox
approved these changes
May 18, 2026
Contributor
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: D3Hunter, mjonss, tiancaiamao, xhebox, yudongusa The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Member
Author
|
/retest |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Issue Number: ref #58939
Problem Summary:
This PR is the parser-only part of
RETURNINGsupport, split out after review feedback from @mjonss.The executor/planner implementation has been moved to the follow-up stacked PR: bb7133#49
What changed and how does it work?
This PR only teaches the parser/AST layer to recognize and restore DML
RETURNINGsyntax:RETURNINGgrammar for INSERT / UPDATE / DELETE statementsRETURNINGas an unreserved keywordparser.go/keywords.goRETURNINGsyntaxNo executor or planner behavior is included in this PR.
Check List
Tests
Validation run:
make parser_yacccd pkg/parser && go test . -run 'TestDMLStmt|TestKeywords' -count=1go test ./pkg/executor ./pkg/planner/core -run TestNonExistentReturningCompileOnly -count=1Side effects
This is parser-only syntax support. Runtime execution support is intentionally kept out of this PR and moved to the follow-up stacked PR.
Documentation
Release note
Summary by CodeRabbit
New Features
Parser
Tests