Support conditional ON CONFLICT updates - #3248
Open
fulghum wants to merge 2 commits into
Open
Conversation
Contributor
|
|
SummaryCoverage spans normal value binding, conditional updates, false and NULL conditions, mixed multi-row changes, affected-row reporting, atomic error handling, and safe rejection of unsupported syntax. The basic binding and several safety behaviors work, but core conditional-upsert flows involving incoming-row values and mixed changes are not healthy. Not safe to merge yet — PR-attributable failures break the core conditional-upsert path for mixed inputs and prevent expected updates, inserts, and affected-row reporting, making this a merge blocker despite other passing behaviors. Separate pre-existing compatibility gaps also affect related scenarios, but they are caveats rather than the primary reason for this verdict. Tests run by Ito
Additional Findings DetailsThese findings are unrelated to the current changes but were observed during testing. 🟠 Mixed order updates cannot run
Evidence Package🟠 Mixed upsert stops before updating rows
Evidence Package🟡 Conditional upsert rejects valid conflict updates
Evidence Package🟡 Conditional update rejects proposed values
Evidence Package🟡 NULL predicate rejects conditional update
Evidence PackageTip Reply with @itoqa to send us feedback on this test run. |
![itoqa[bot]](https://avatars.githubusercontent.com/in/2622614?s=60&v=4){kind=small}
Contributor
|
@fulghum DOLT
|
fulghum
force-pushed
the
fulghum/doltgres-3235
branch
from
September 1, 2026 00:15
3f02494 to
57c45dd
Compare
Contributor
Author
|
Follow-up work remains for exact PostgreSQL affected-row counts on unconditional |
Commit: SummaryThe run covers database write behavior across normal inserts and updates, stale-write protection, parameter handling, concurrent conflicts, row-count reporting, and multi-row operations. It also exercises edge cases involving incoming conflict values, type casts, returned rows, constraint handling, and atomicity; basic paths are healthy, but several important conditional-write variations remain broken. Not safe to merge yet — several attributable medium-severity failures affect core conditional write scenarios, including incoming values, returned rows, casted values, and multi-row behavior, making supported upserts unreliable. An additional unrelated limitation is a flag for later, but is not a driver of the merge decision. Tests run by Ito
Additional Findings DetailsThese findings are unrelated to the current changes but were observed during testing. 🟡 EXCLUDED values fail in conditional upserts
Evidence PackageTip Reply with @itoqa to send us feedback on this test run. |
fulghum
force-pushed
the
fulghum/doltgres-3235
branch
from
September 1, 2026 18:35
57c45dd to
3bb4987
Compare
Commit: SummaryCoverage spans insert-or-update behavior across normal updates, conditional and bound-value decisions, null and ordering edge cases, affected-row counts, returned data, type conversion, and rollback after constraint errors. The broad behavior is healthy, but a conditional insert-from-query scenario still fails to update and return the existing record. Merge with caution — a PR-attributable medium-severity failure remains in a supported conditional insert-or-update path, preventing expected updates and returned results in that scenario. The other exercised behaviors pass, so this is a focused functional risk rather than a broad data-integrity failure. Tests run by Ito
Tip Reply with @itoqa to send us feedback on this test run. |
There was a problem hiding this comment.
Conditional insert fails to update a row
What failed: The conditional upsert shows an error instead of updating the existing row and returning its new values.
Impact · Steps · Stub / mock · Analysis · Why this is likely a bug
- Severity: Medium
- Impact: Users of conditional upsert queries cannot update an existing row or receive the changed row in the result. The existing data stays unchanged, so the operation must be corrected or retried through another supported path.
- Steps to Reproduce:
- Create a table with an integer primary key, a bigint amount column, and a text note column.
- Insert one row with key 1, amount 10, and note 'initial'.
- Run an INSERT ... SELECT for key 1 with a larger amount, use EXCLUDED.amount with an explicit bigint cast, compare the old amount with EXCLUDED.amount in the conflict predicate, and add RETURNING key, amount, and note.
- Observe the error about the excluded table and query the row again; it still contains amount 10 and note 'initial'.
- Stub / mock content: No stubs, mocks, or bypasses were applied for this test in the recorded run.
- Code Analysis: server/ast/insert.go now converts the action-level WHERE expression at lines 58-63 and attaches it as OnDupWhere, while setting OnDupValuesAlias to excluded at lines 116-129. For statements requiring insert cast analysis, server/analyzer/assign_insert_casts.go:117-121 computes replacement update expressions. The PR then builds exprs at lines 123-127 by appending checks, OnDupWhere, and Returning, and calls insertInto.WithExpressions(ctx, exprs...) at line 129. The recorded go-mysql-server InsertInto implementation used by this checkout exposes and reconstructs only the duplicate-update expressions, checks, and Returning expressions; it does not include OnDupWhere in either Expressions or WithExpressions. Consequently, the analyzer rebuild loses the conflict predicate's proposed-row scope while rebuilding the casted insert, leaving EXCLUDED references unresolved and producing
table not found: excluded. The smallest practical fix is to update the InsertInto expression reconstruction contract used by this PR so OnDupWhere is included and restored in the same position, or to use a reconstruction path that explicitly preserves OnDupWhere before calling WithExpressions; then rerun the cast-plus-RETURNING INSERT ... SELECT test. - Why this is likely a bug: The SQL is valid PostgreSQL syntax and the table setup succeeds, but the update statement fails before changing data. The same error occurs with uppercase and lowercase EXCLUDED references, which points to lost proposed-row scope rather than identifier casing. The failure affects the feature this PR adds: action-level conflict predicates, proposed-row references, casted assignments, and RETURNING are all expected to work together; a targeted preservation of OnDupWhere during cast reconstruction fixes that path without changing unrelated unconditional upsert behavior.
Relevant code
server/analyzer/assign_insert_casts.go:117-135
if insertInto.OnDupExprs.HasUpdates() {
newDupExprs, err := assignUpdateFieldCasts(ctx, insertInto.OnDupExprs.AllExpressions())
...
exprs := append(newDupExprs, insertInto.Checks().ToExpressions()...)
if insertInto.OnDupWhere != nil {
exprs = append(exprs, insertInto.OnDupWhere)
}
exprs = append(exprs, insertInto.Returning...)
newInsertInto, err := insertInto.WithExpressions(ctx, exprs...)
}server/ast/insert.go:46-63
if node.OnConflict != nil {
...
if node.OnConflict.Where != nil {
onDuplicateWhere, err = nodeExpr(ctx, node.OnConflict.Where.Expr)
}
}server/ast/insert.go:116-129
return &vitess.Insert{
...
OnDupValuesAlias: "excluded",
OnDupWhere: onDuplicateWhere,
CountOnDuplicateUpdateAsOneRow: node.OnConflict != nil && node.OnConflict.Where != nil,
}Evidence Package
Copy prompt for an agent
Ito QA identified the following failure during automated PR testing. Please investigate and propose a fix.
**Medium severity — Conditional insert fails to update a row**
**What failed:** The conditional upsert shows an error instead of updating the existing row and returning its new values.
- **Impact:** Users of conditional upsert queries cannot update an existing row or receive the changed row in the result. The existing data stays unchanged, so the operation must be corrected or retried through another supported path.
- **Steps to reproduce:**
1. Create a table with an integer primary key, a bigint amount column, and a text note column.
2. Insert one row with key 1, amount 10, and note 'initial'.
3. Run an INSERT ... SELECT for key 1 with a larger amount, use EXCLUDED.amount with an explicit bigint cast, compare the old amount with EXCLUDED.amount in the conflict predicate, and add RETURNING key, amount, and note.
4. Observe the error about the excluded table and query the row again; it still contains amount 10 and note 'initial'.
- **Stub / mock content:** No stubs, mocks, or bypasses were applied for this test in the recorded run.
- **Code analysis:** server/ast/insert.go now converts the action-level WHERE expression at lines 58-63 and attaches it as OnDupWhere, while setting OnDupValuesAlias to excluded at lines 116-129. For statements requiring insert cast analysis, server/analyzer/assign_insert_casts.go:117-121 computes replacement update expressions. The PR then builds exprs at lines 123-127 by appending checks, OnDupWhere, and Returning, and calls insertInto.WithExpressions(ctx, exprs...) at line 129. The recorded go-mysql-server InsertInto implementation used by this checkout exposes and reconstructs only the duplicate-update expressions, checks, and Returning expressions; it does not include OnDupWhere in either Expressions or WithExpressions. Consequently, the analyzer rebuild loses the conflict predicate's proposed-row scope while rebuilding the casted insert, leaving EXCLUDED references unresolved and producing `table not found: excluded`. The smallest practical fix is to update the InsertInto expression reconstruction contract used by this PR so OnDupWhere is included and restored in the same position, or to use a reconstruction path that explicitly preserves OnDupWhere before calling WithExpressions; then rerun the cast-plus-RETURNING INSERT ... SELECT test.
- **Why this is likely a bug:** The SQL is valid PostgreSQL syntax and the table setup succeeds, but the update statement fails before changing data. The same error occurs with uppercase and lowercase EXCLUDED references, which points to lost proposed-row scope rather than identifier casing. The failure affects the feature this PR adds: action-level conflict predicates, proposed-row references, casted assignments, and RETURNING are all expected to work together; a targeted preservation of OnDupWhere during cast reconstruction fixes that path without changing unrelated unconditional upsert behavior.
**Relevant code:**
`server/analyzer/assign_insert_casts.go:117-135`
~~~go
if insertInto.OnDupExprs.HasUpdates() {
newDupExprs, err := assignUpdateFieldCasts(ctx, insertInto.OnDupExprs.AllExpressions())
...
exprs := append(newDupExprs, insertInto.Checks().ToExpressions()...)
if insertInto.OnDupWhere != nil {
exprs = append(exprs, insertInto.OnDupWhere)
}
exprs = append(exprs, insertInto.Returning...)
newInsertInto, err := insertInto.WithExpressions(ctx, exprs...)
}
~~~
`server/ast/insert.go:46-63`
~~~go
if node.OnConflict != nil {
...
if node.OnConflict.Where != nil {
onDuplicateWhere, err = nodeExpr(ctx, node.OnConflict.Where.Expr)
}
}
~~~
`server/ast/insert.go:116-129`
~~~go
return &vitess.Insert{
...
OnDupValuesAlias: "excluded",
OnDupWhere: onDuplicateWhere,
CountOnDuplicateUpdateAsOneRow: node.OnConflict != nil && node.OnConflict.Where != nil,
}
~~~
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
3 participants
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.
Support the action-level
WHEREclause onON CONFLICT DO UPDATE. The predicate is evaluated against the existing and proposed rows before assignments, with false and NULL results leaving the conflicting row unchanged.Conditional upserts now report PostgreSQL-compatible command tags while preserving shared MySQL affected-row behavior. Coverage includes true, false, NULL, mixed multi-row, qualified predicates, bind variables, exact command tags, and final table state.
Follow-up work is required for unconditional
ON CONFLICT DO UPDATEcommand tags. PostgreSQL counts every executed conflict update as one row, including updates that leave values unchanged. Applying that policy to unconditional updates currently conflicts with MySQL-specific affected-row expectations in the shared enginetests, so the exact fix needs dialect-specific affected-row expectations in the test framework.Fixes #3235
Depends on: