Fix indexed DELETE column resolution
Project: duckdb/duckdb | Language: C++ | Concepts: indexing, refactoring, error-handling | Difficulty: mid
Link: https://github.com/duckdb/duckdb/pull/24203
The problem
When executing DELETE ... USING queries on tables with indexes, DuckDB could crash if the query optimizer reordered columns. This happened because the code assumed column positions would remain unchanged, but join reordering or other optimizations could change the output layout. As a result, the physical delete operator could reference the wrong columns, leading to type mismatches and crashes.
Why it's non-trivial
A naive fix might try to always use the original column indices, but this doesn't work if the optimizer changes the order or number of columns in the output. The correct fix needs to track which expressions correspond to which table columns throughout optimization and resolve their final positions just before execution.
The approach
The fix involves tracking required columns as expressions, not just indices, through optimization and resolving their final positions during physical planning.
1. Update column mapping in physical plan generation:
+ // Convert storage-column-to-expression mappings to storage-column-to-chunk mappings.
+ auto return_columns = std::move(op.return_columns);
+ for (auto &return_column_idx : return_columns) {
+ if (return_column_idx == DConstants::INVALID_INDEX) {
+ continue;
+ }
+ const auto expression_idx = return_column_idx;
+ D_ASSERT(expression_idx < op.expressions.size());
+ return_column_idx = op.expressions[expression_idx]->Cast<BoundReferenceExpression>().Index();
+ }
This code walks through the logical delete's return_columns, which originally referenced expressions, and updates them to reference the correct chunk positions after optimization. This ensures the physical delete operator uses the right columns.
2. Adjust binding logic to handle internal and external tables differently:
+ auto is_duck_table = table.IsDuckTable();
+ if (is_duck_table) {
+ // Bind the row id before the table columns so it remains the first delete expression.
+ BindRowIdColumns(table, get, del->expressions);
+ }
For DuckDB-managed tables, the row id is bound before other columns to ensure its position is stable. This prevents confusion if other columns are added later.
3. Use expressions for return columns, not just indices:
- BindDeleteReturningColumns(table, get, del->return_columns);
+ if (is_duck_table) {
+ BindDeleteReturningColumns(table, get, del->return_columns, del->expressions, get);
+ } else {
+ BindDeleteReturningColumns(table, get, del->return_columns);
+ }
For DuckDB tables, the binding now also passes the expressions, allowing the mapping to be maintained through optimization.
4. Add regression tests:
+# DELETE USING with an indexed table
+
+statement ok
+CREATE TABLE delete_using_indexed(id INTEGER PRIMARY KEY, payload VARCHAR);
+
+statement ok
+INSERT INTO delete_using_indexed SELECT range, range::VARCHAR FROM range(10);
+
+query I
+DELETE FROM delete_using_indexed USING range(100) source(i) WHERE source.i = 0;
+----
+10
+
+query I
+SELECT count(*) FROM delete_using_indexed;
+----
+0
+
+# Verify deleted keys can be inserted again
+statement ok
+INSERT INTO delete_using_indexed SELECT range, range::VARCHAR FROM range(10);
+
+query I rowsort
+DELETE FROM delete_using_indexed USING range(100) source(i) WHERE source.i = 0 RETURNING id;
+----
+0
+1
+2
+3
+4
+5
+6
+7
+8
+9
These new tests ensure the fix works for indexed deletes with and without RETURNING, and that deleted keys can be re-inserted.
From the review thread
No notable review discussion on this PR.
Takeaways
- Always resolve column positions after query optimization, not before, to account for possible reordering or projection changes.
- Track required columns as bound expressions through the optimizer, not just as indices, to ensure correctness.
- Add regression tests for edge cases involving indexes and column mappings to prevent future breakage.