Fix duplicate key handling after expression index alters

[fulghum]

Adds PostgreSQL regression coverage for duplicate-key errors and ON CONFLICT operations after adding a column to a table with an expression index. The underlying Dolt writer fix is now present on main.

Fixes #3082

[github-actions]

	Main	PR
Total	42090	42090
Successful	19011	19010
Failures	23079	23080
Partial Successes1	5452	5452

	Main	PR
Successful	45.1675%	45.1651%
Failures	54.8325%	54.8349%

${\color{red}Regressions (1)}$

subselect

QUERY:          select count(*) from tenk1 t
where (exists(select 1 from tenk1 k where k.unique1 = t.unique2) or ten < 0);
RECEIVED ERROR: timeout during Receive

Footnotes

1. These are tests that we're marking as Successful, however they do not match the expected output in some way. This is due to small differences, such as different wording on the error messages, or the column names being incorrect while the data itself is correct. ↩

[coffeegoddd]

@fulghum DOLT

read_tests	from_latency	to_latency	percent_change
covering_index_scan_postgres	2.43	2.43	0.0
groupby_scan_postgres	75.82	74.46	-1.79
index_join_postgres	2.18	2.18	0.0
index_join_scan_postgres	1.58	1.58	0.0
index_scan_postgres	475.79	475.79	0.0
oltp_point_select	0.36	0.36	0.0
oltp_read_only	6.32	6.32	0.0
select_random_points	0.7	0.7	0.0
select_random_ranges	1.01	1.01	0.0
table_scan_postgres	475.79	475.79	0.0
types_table_scan_postgres	1213.57	1191.92	-1.78

write_tests	from_latency	to_latency	percent_change
oltp_delete_insert_postgres	6.67	6.67	0.0
oltp_insert	3.3	3.3	0.0
oltp_read_write	13.22	13.22	0.0
oltp_update_index	3.55	3.55	0.0
oltp_update_non_index	3.25	3.25	0.0
oltp_write_only	6.91	6.91	0.0
types_delete_insert_postgres	7.17	7.17	0.0

[itoqa]

Commit: 867c753: 11 test cases ran, 10 passed ✅, 1 additional finding ⚠️.

Summary

Coverage focuses on database schema changes and data integrity: adding nullable fields while preserving existing rows and constraints, rejecting duplicate writes, maintaining transaction safety, and handling conflict updates and inserts. It includes normal write flows, constraint and transaction edge cases, repeated conflicting operations, and recovery behavior across fresh connections.

Safe to merge — the only failure is a pre-existing medium-severity database conflict-update defect that is explicitly not attributable to this PR; the PR introduces no observed regressions or new failures.

Tests run by Ito

Result	Severity	Type	Description
✅	—	Alter	The database rejected the second row with the existing primary-key value, and the original row stayed unchanged.
✅	—	Alter	Adding a nullable time column did not break the unique rule. A second row using the existing name was rejected, and the original row stayed unchanged.
✅	—	General	After a column was added, duplicate primary-key and unique-name inserts were rejected on the first try and on repeat attempts. The original row stayed unchanged, and a separate row with a new id and name was inserted successfully.
✅	—	General	Changing the existing row's name from v1 to v2 worked. A new row using v1 was accepted, a new row using v2 was rejected as a duplicate, and the added extra field stayed NULL.
✅	—	General	Duplicate primary-key and unique-name inserts were rejected, and later checks still found the original row unchanged. A separate valid row was stored once, with no partial data or index damage.
✅	—	General	After a new column was added, duplicate writes were rejected, conflict updates worked, and the final row kept the expected values across fresh database connections.
✅	—	Conflict	The conflicting row was updated to v2, and the new extra column stayed empty as expected.
✅	—	Rev	Adding the new nullable column kept all three existing rows unchanged, with an empty value in that column. Duplicate ID and duplicate text inserts were rejected, while a new row was inserted normally.
✅	—	Rev	A duplicate insert returned an error, and rolling back the transaction left the same database session usable. A later valid insert succeeded, and a committed conflict-ignore insert left the original row unchanged.
✅	—	Writer	The PostgreSQL regression passed. Duplicate primary-key and unique-name writes were rejected, conflict updates and no-op inserts behaved correctly, and the final row was (1, 'v2', NULL).
⚠️		Rev	The conflicting update should report that the new name duplicates another row, preserve both original rows, and allow a later valid conflict update. Instead, the conflicting statement and the later valid update both return table not found: excluded, and the expected final update is never applied.

Additional Findings Details

These findings are unrelated to the current changes but were observed during testing.

🟡 Conflict update rejects valid changes

• Severity: Medium
• Description: The conflicting update should report that the new name duplicates another row, preserve both original rows, and allow a later valid conflict update. Instead, the conflicting statement and the later valid update both return table not found: excluded, and the expected final update is never applied.
• Impact: Database users cannot complete supported ON CONFLICT updates. The intended change is not applied, although the existing rows remain unchanged.
• Steps to Reproduce:
  1. Create a table with an integer primary key and a unique name column.
  2. Create an expression index on lower(name), add a nullable extra column, and insert rows (1, 'v1') and (2, 'v2').
  3. Run an INSERT for id 1 with ON CONFLICT (id) DO UPDATE SET name = 'v2';.
  4. Check that the statement reports a unique-constraint error and that both rows are unchanged.
  5. Run a valid ON CONFLICT (id) DO UPDATE that changes id 1's name, then read both rows again.
• Stub / mock content: The test used a local PostgreSQL-compatible Doltgres server and the documented development database account. No application mocks, route interception, or behavior-changing bypasses were applied.
• Code Analysis: The observed error is consistent with the production translation path. In server/ast/insert.go, nodeInsert handles a non-DO-NOTHING ON CONFLICT clause at lines 45-58 by converting node.OnConflict.Exprs through nodeUpdateExprs and appending the resulting expressions to a Vitess OnDup value. The code does not bind or rewrite PostgreSQL's excluded row reference before delegating the operation. The returned Vitess insert at lines 109-123 passes that OnDup value directly to the go-mysql-server execution layer. supportedOnConflictClause at lines 134-143 accepts this clause whenever it has no arbiter predicate or WHERE condition, so a valid targeted DO UPDATE reaches execution without excluded resolution. The REV-3 SQL evidence reproduced table not found: excluded for both the secondary-conflict statement and the later valid update, while readback showed rows 1 and 2 remained unchanged. The smallest practical fix is to resolve PostgreSQL excluded references when translating supported ON CONFLICT update expressions, or to reject unsupported excluded-dependent expressions with a clear PostgreSQL error; the normal constant assignment in this test must execute successfully. The PR diff only pins github.com/dolthub/dolt/go in go.mod/go.sum and adds testing/go/alter_table_test.go coverage. It does not change server/ast/insert.go or provide enough dependency-level evidence to tie this defect to a specific changed line, so the finding is not attributed to this PR.

Evidence Package

• text/REV-3/conflict-update-evidence.txt — 3 KB

Tip

Reply with @itoqa to send us feedback on this test run.

[itoqa]

History reset (rebase or force-push detected). Starting test narrative over.

Commit: 0ca9215: 10 test cases ran, 6 passed ✅, 4 additional findings ⚠️.

Summary

Coverage spans core data-management behavior: preserving existing records through schema changes, maintaining visible and expression-based indexes, enforcing uniqueness, and keeping later and concurrent writes correct. It also exercises edge cases such as duplicate writes, conflict retries, and standard upsert behavior, with both successful workflows and compatibility gaps identified.

Safe to merge — the observed failures are pre-existing database compatibility issues unrelated to this PR, with no regressions or newly introduced failures attributable to the change. They are worth flagging for later, but do not present a merge blocker for this pull request.

Tests run by Ito

Result	Severity	Type	Description
✅	—	General	The altered-table conflict workflow remains supported with the repository's documented SQL form. The recorded error came from using an unsupported EXCLUDED expression in the test query, while the checked-in regression uses a literal update and completed with the expected row state.
✅	—	Rev	Three existing rows stayed intact after an expression index and a new nullable column were added. Duplicate writes were rejected, a valid fourth row was added, and the new column was empty for every row.
✅	—	Rev	After the name changed from Alpha to Beta, the row was found by searching for Beta and was no longer found by searching for Alpha. The added column stayed empty, and the row appeared exactly once.
✅	—	Rev	The table showed only its normal columns after the new column was added. The expression index stayed intact, and inserting a row through the visible columns worked.
✅	—	Rev	Duplicate rows were rejected, and a valid row written afterward was saved correctly. The final table kept both rows, with one matching result for each indexed name.
✅	—	Writer	The table kept one row after a column was added, and the conflict update changed its name to v2 while the new column stayed empty.
⚠️		General	The first conflict update fails with an error instead of changing the existing row. Repeating the request does not reach the safe retry check because the same unsupported reference fails each time.
⚠️		General	The existing row is not updated from v1 to v2. The required INSERT ... ON CONFLICT (id) DO UPDATE SET name = EXCLUDED.name statement returns ERROR: table not found: excluded; a diagnostic statement using the literal name = 'v2' succeeds, so the failure is specific to the incoming-row reference.
⚠️		Conflict	The conflict update could not use the value from the incoming row. The database returned an error and left the existing name as v1; the expected name was v2.
⚠️		Rev	The existing row should be updated when the new name matches the unique name constraint. Instead, the write stops with a table-not-found error, so the required conflict update does not happen.

Additional Findings Details

These findings are unrelated to the current changes but were observed during testing.

🟠 Conflict retries fail instead of updating the existing row

• Severity: High
• Description: The first conflict update fails with an error instead of changing the existing row. Repeating the request does not reach the safe retry check because the same unsupported reference fails each time.
• Impact: Applications cannot use the standard PostgreSQL conflict-update form when the new value comes from the attempted insert. The update fails instead of changing the existing row, so this workflow cannot complete until the query is rewritten or the defect is fixed.
• Steps to Reproduce:
  1. Create a table with an integer primary key and a unique name column.
  2. Create an expression index on lower(name), add a nullable extra column, and insert one row with id 1 and name v1.
  3. Run INSERT INTO expression_index_alter (id, name) VALUES (1, 'v2') ON CONFLICT (id) DO UPDATE SET name = EXCLUDED.name.
  4. Run the same conflict update again and query the table; the first statement already fails with table not found: excluded, so the retry cannot produce one row named v2.
• Stub / mock content: No stubs, mocks, or bypasses were applied for this test in the recorded run.
• Code Analysis: In server/ast/insert.go:45-58, nodeInsert handles a non-DO-NOTHING ON CONFLICT clause by passing node.OnConflict.Exprs directly to nodeUpdateExprs at lines 48-52. There is no branch that recognizes PostgreSQL's EXCLUDED pseudo-table and maps EXCLUDED.column to the inserted value or the target expression expected by the downstream Vitess ON DUPLICATE KEY UPDATE representation. As a result, EXCLUDED.name is treated as an ordinary table-qualified reference during expression conversion and resolves to the nonexistent table excluded, producing the observed table-not-found error before the conflict update can execute. The smallest practical fix is to translate EXCLUDED-qualified expressions at this boundary, or explicitly reject them with a PostgreSQL-compatible unsupported-feature error if the downstream representation cannot preserve their semantics; the normal supported path should map the reference to the incoming insert value. The PR diff changes go.mod/go.sum and appends testing/go/alter_table_test.go, but does not change this conversion code.

Evidence Package

• text/BF-RAPID-1/BF-RAPID-1-conflict-retry.txt — 2 KB

🟡 Conflict update cannot use incoming values

• Severity: Medium
• Description: The existing row is not updated from v1 to v2. The required INSERT ... ON CONFLICT (id) DO UPDATE SET name = EXCLUDED.name statement returns ERROR: table not found: excluded; a diagnostic statement using the literal name = 'v2' succeeds, so the failure is specific to the incoming-row reference.
• Impact: Applications using a standard upsert with an incoming row value fail instead of updating the existing record. The operation reports an error, but there is no evidence of data loss or corrupted index values.
• Steps to Reproduce:
  1. Create a table with an integer primary key, a unique name column, and an expression index on lower(name).
  2. Add a nullable extra column and insert the row (1, 'v1').
  3. Run an insert for id 1 with name 'v2' using ON CONFLICT (id) DO UPDATE SET name = EXCLUDED.name.
  4. Observe the table-not-found error for excluded instead of a successful update, then compare the visible row and the lower(name) index state.
• Stub / mock content: No stubs, mocks, or bypasses were applied for this test in the recorded run.
• Code Analysis: In server/ast/insert.go:45-58, nodeInsert handles a supported ON CONFLICT clause by passing node.OnConflict.Exprs directly to nodeUpdateExprs at line 50. server/ast/update_expr.go:24-41 then converts each assignment by calling nodeExpr(ctx, node.Expr) at line 28 and reuses that converted expression in the Vitess assignment. There is no special handling for PostgreSQL's EXCLUDED pseudo-table or its column references. Consequently, EXCLUDED.name is resolved through the ordinary expression/table lookup path and reaches execution as a reference to a table named excluded, matching the observed table not found: excluded error. The PR changes only the Dolt Go module revision in go.mod:9, go.sum checksums, and adds testing/go/alter_table_test.go:1363-1397; none of those changed lines supplies the missing parser/AST translation. The smallest practical fix is to translate EXCLUDED column references to the conflict insert-row representation while converting ON CONFLICT assignments, without changing the hidden expression-index writer logic.

Evidence Package

• text/BF-CROSS-1/BF-CROSS-1-conflict-update-evidence.txt — 3 KB

🟡 Conflict update rejects incoming values

• Severity: Medium
• Description: The conflict update could not use the value from the incoming row. The database returned an error and left the existing name as v1; the expected name was v2.
• Impact: Applications cannot update an existing row with values from a conflicting insert. The update fails and the old value remains, although a literal-value update can work around this case.
• Steps to Reproduce:
  1. Create a table with an integer primary key, a unique name column, and a lower(name) expression index.
  2. Add a nullable extra column and insert the row (1, 'v1').
  3. Run INSERT INTO expression_index_alter (id, name) VALUES (1, 'v2') ON CONFLICT (id) DO UPDATE SET name = EXCLUDED.name.
  4. Check the result and the row value. The statement returns table not found: excluded and the row remains (1, 'v1', NULL), instead of becoming (1, 'v2', NULL).
• Stub / mock content: No stubs, mocks, or bypasses were applied for this test in the recorded run.
• Code Analysis: The failure is in /tmp/output-agent-workspace/repo/server/ast/insert.go, not in the test setup. In nodeInsert, the supported ON CONFLICT branch at lines 48-52 converts node.OnConflict.Exprs by calling nodeUpdateExprs(ctx, node.OnConflict.Exprs). That conversion receives the PostgreSQL EXCLUDED.name reference without a translation step that maps the EXCLUDED pseudo-table to the representation expected by the downstream Vitess INSERT/ON DUPLICATE KEY UPDATE statement. The unresolved reference therefore reaches expression resolution as a normal table reference and produces the observed table not found: excluded error. The same file's literal assignment path works in the diagnostic control query, which isolates the defect to EXCLUDED reference handling rather than conflict detection, the altered column, or the expression index. The smallest practical fix is to translate EXCLUDED column references while converting ON CONFLICT assignments, before appending them to the Vitess OnDup list. The PR diff changes only go.mod, go.sum, and testing/go/alter_table_test.go; it does not change this conversion path, so this is a pre-existing limitation exposed by the requested scenario.

Evidence Package

• text/CONFLICT-1/conflict-update-evidence.txt — 3 KB

🟡 Unique-key conflict update fails

• Severity: Medium
• Description: The existing row should be updated when the new name matches the unique name constraint. Instead, the write stops with a table-not-found error, so the required conflict update does not happen.
• Impact: Applications that use incoming values in a standard upsert may fail to update an existing row. A query rewrite using fixed values may work, but it cannot support updates that must copy values from the attempted insert.
• Steps to Reproduce:
  1. Create a table with id as the primary key and name as a separate UNIQUE column.
  2. Create an expression index on lower(name), add a nullable extra column, and insert one row with name v1.
  3. Insert a new id with the existing name v1 using ON CONFLICT (name) DO UPDATE and assign a column from EXCLUDED.
  4. Observe that the statement fails with table not found: excluded instead of updating the existing row.
• Stub / mock content: No stubs, mocks, or bypasses were applied for this test in the recorded run.
• Code Analysis: The runtime evidence is a local Doltgres SQL result: setup completed, the EXCLUDED-based unique-target update returned table not found: excluded, and a diagnostic version using a constant assignment succeeded and left one row. This isolates the failure to EXCLUDED expression handling rather than table creation, ADD COLUMN, or row persistence. In server/ast/insert.go:45-56, nodeInsert accepts a supported tree.OnConflict clause, calls nodeUpdateExprs on the update expressions, and emits those expressions as Vitess OnDup entries. The code does not translate PostgreSQL's EXCLUDED relation into a usable row source; line 49 also explicitly documents that the conflict target column names are ignored. The repository's testing/go/enginetest/query_converter_test.go:157-164 independently documents the same limitation: the translation layer uses literal INSERT values because EXCLUDED.col references do not resolve back through the MySQL translation layer. The related regression fixture in testing/go/alter_table_test.go:1363-1395 covers the altered-table state but only uses a constant assignment at lines 1381-1382, so it does not establish EXCLUDED support. The smallest practical fix is to translate EXCLUDED column references into the inserted-row values before producing the Vitess ON DUPLICATE KEY UPDATE expression, and separately reject or correctly apply a conflict target rather than silently ignoring it.

Evidence Package

• text/REV-2/REV-2-on-conflict-excluded-evidence.txt — 3 KB

Tip

Reply with @itoqa to send us feedback on this test run.

[itoqa]

History reset (rebase or force-push detected). Starting test narrative over.

Commit: 6dd9d86: 7 test cases ran, 7 passed ✅.

Summary

Coverage spans normal table changes and data updates, along with edge cases involving duplicate keys, conflicting writes, ignored conflicts, transaction rollback, and continued use of existing data and indexes. The exercised behavior is healthy across schema evolution, data integrity, error handling, and recovery flows.

Safe to merge — the run found no failures attributable to this PR and showed no regressions in the covered data-integrity or recovery behaviors. No merge-blocking issue was identified.

Tests run by Ito

Result	Severity	Type	Description
✅	—	Alter	The altered table rejected duplicate rows, updated the existing row through conflict handling, and kept the final data correct.
✅	—	Alter	Both duplicate writes were rejected with the correct primary-key and unique-key errors, and the original row stayed unchanged.
✅	—	General	After a duplicate insert was rejected, the next update ran successfully and changed the existing row to v2. No extra row was added, and the new column stayed empty.
✅	—	Rev	Adding the new column kept the existing row available, allowed it to be updated, and allowed a second row to be inserted. Both rows had an empty value for the new column, and the lower-case name index remained intact.
✅	—	Rev	The update was rejected because the new name already belonged to another row. Both original rows stayed unchanged, so no data was overwritten.
✅	—	Rev	Trying to add a second row with the same name was ignored. The original row stayed unchanged, and the table still contained exactly one row.
✅	—	Rev	A duplicate write was rejected, the transaction was rolled back, and a later valid insert committed successfully. The table ended with exactly the original row and the new row, with the added column empty for both.

Tip

Reply with @itoqa to send us feedback on this test run.

[zachmu]

LGTM, just one comment about testing coverage

[itoqa]

History reset (rebase or force-push detected). Starting test narrative over.

Commit: 56bd1f0: 7 test cases ran, 7 passed ✅.

Summary

Coverage focuses on database write integrity across normal updates and deletes, duplicate and conflicting inserts, uniqueness enforcement, no-op behavior, nullable fields, and reuse of values after deletion. The exercised edge cases remained healthy, with correct row counts, updates, cleanup, and conflict handling throughout.

Safe to merge — the run found no regressions, new failures, or previously flagged failures attributable to this PR, and all exercised data-integrity behaviors passed. No merge-blocking risk was identified.

Tests run by Ito

Result	Severity	Type	Description
✅	—	General	Duplicate primary-key and unique-value inserts returned the expected errors, and later conflict updates and no-op writes still worked correctly.
✅	—	General	The existing row was updated to name v2, and the two later duplicate inserts did nothing. The table still had one row, and the new extra field stayed empty.
✅	—	Duplicate	Adding a nullable column did not break duplicate checks. Duplicate primary-key and unique-name inserts were rejected, and no extra rows were added.
✅	—	Nothing	Duplicate inserts were ignored in both tested forms, and the table kept its one original row with the updated name and empty extra field.
✅	—	Rev	The database found the existing row with the same name, updated it, and kept the added timestamp value. No second row was created.
✅	—	Rev	Updating the first row changed its name and added timestamp, deleting the second row removed it, and a new row reused the deleted name successfully. The final table had exactly two correct rows with no stale unique or expression-index entry.
✅	—	Update	A duplicate ID updates the existing row instead of creating a second row. The row keeps its empty added field, and the final check finds exactly one row with the new name.

Tip

Reply with @itoqa to send us feedback on this test run.