diff --git a/pkg/sql/insert.go b/pkg/sql/insert.go index f814cf6990c7..16a1a434d25d 100644 --- a/pkg/sql/insert.go +++ b/pkg/sql/insert.go @@ -199,6 +199,7 @@ func (p *planner) Insert( updateCols: updateCols, conflictIndex: *conflictIndex, evaler: helper, + isUpsertAlias: n.OnConflict.IsUpsertAlias(), } } } diff --git a/pkg/sql/tablewriter.go b/pkg/sql/tablewriter.go index cc15322a5d10..9d124cdca030 100644 --- a/pkg/sql/tablewriter.go +++ b/pkg/sql/tablewriter.go @@ -165,8 +165,6 @@ type tableUpsertEvaler interface { // eval returns the values for the update case of an upsert, given the row // that would have been inserted and the existing (conflicting) values. eval(insertRow parser.Datums, existingRow parser.Datums) (parser.Datums, error) - - isIdentityEvaler() bool } // tableUpserter handles writing kvs and forming table rows for upserts. @@ -189,6 +187,7 @@ type tableUpserter struct { ri sqlbase.RowInserter autoCommit bool conflictIndex sqlbase.IndexDescriptor + isUpsertAlias bool // These are set for ON CONFLICT DO UPDATE, but not for DO NOTHING updateCols []sqlbase.ColumnDescriptor @@ -227,17 +226,25 @@ func (tu *tableUpserter) init(txn *client.Txn) error { tu.tableDesc = tu.ri.Helper.TableDesc tu.indexKeyPrefix = sqlbase.MakeIndexKeyPrefix(tu.tableDesc, tu.tableDesc.PrimaryIndex.ID) - // For the fast path, all columns must be specified in the insert. - allColsIdentityExpr := len(tu.ri.InsertCols) == len(tu.tableDesc.Columns) && - // Plus, all columns not in the conflict index must be specified in the - // DO UPDATE clause and be of the form `x = excluded.x`. - len(tu.updateCols) == len(tu.tableDesc.Columns)-len(tu.conflictIndex.ColumnIDs) && - tu.evaler != nil && tu.evaler.isIdentityEvaler() - // When adding or removing a column in a schema change (mutation), the user - // can't specify it, which means we need to do a lookup and so we can't use - // the fast path. When adding or removing an index, same result, so the fast - // path is disabled during all mutations. - if len(tu.tableDesc.Indexes) == 0 && len(tu.tableDesc.Mutations) == 0 && allColsIdentityExpr { + // TODO(dan): The fast path is currently only enabled when the UPSERT alias + // is explicitly selected by the user. It's possible to fast path some + // queries of the form INSERT ... ON CONFLICT, but the utility is low and + // there are lots of edge cases (that caused real correctness bugs #13437 + // #13962). As a result, we've decided to remove this until after 1.0 and + // re-enable it then. See #14482. + enableFastPath := tu.isUpsertAlias && + // Tables with secondary indexes are not eligible for fast path (it + // would be easy to add the new secondary index entry but we can't clean + // up the old one without the previous values). + len(tu.tableDesc.Indexes) == 0 && + // When adding or removing a column in a schema change (mutation), the user + // can't specify it, which means we need to do a lookup and so we can't use + // the fast path. When adding or removing an index, same result, so the fast + // path is disabled during all mutations. + len(tu.tableDesc.Mutations) == 0 && + // For the fast path, all columns must be specified in the insert. + len(tu.ri.InsertCols) == len(tu.tableDesc.Columns) + if enableFastPath { tu.fastPathBatch = tu.txn.NewBatch() tu.fastPathKeys = make(map[string]struct{}) return nil diff --git a/pkg/sql/testdata/logic_test/upsert b/pkg/sql/testdata/logic_test/upsert index 9dc3109126ea..d4f25597eb95 100644 --- a/pkg/sql/testdata/logic_test/upsert +++ b/pkg/sql/testdata/logic_test/upsert @@ -197,3 +197,51 @@ INSERT INTO issue_14052 (a, b) VALUES (1, 1), (2, 2) statement ok UPSERT INTO issue_14052 (a, c) (SELECT a, b from issue_14052) + +statement ok +CREATE TABLE issue_14052_2 ( + id SERIAL PRIMARY KEY, + name VARCHAR(255), + createdAt INT, + updatedAt INT +) + +statement ok +INSERT INTO issue_14052_2 (id, name, createdAt, updatedAt) VALUES + (1, 'original', 1, 1) + +# Make sure the fast path isn't taken (createdAt is not in the ON CONFLICT clause) +statement ok +INSERT INTO issue_14052_2 (id, name, createdAt, updatedAt) VALUES + (1, 'UPDATED', 2, 2) +ON CONFLICT (id) DO UPDATE + SET id = excluded.id, name = excluded.name, updatedAt = excluded.updatedAt + +query ITII +SELECT * FROM issue_14052_2; +---- +1 UPDATED 1 2 + +# Make sure the fast path isn't taken (repeating a column in the ON CONFLICT clause doesn't do anything) +statement ok +INSERT INTO issue_14052_2 (id, name, createdAt, updatedAt) VALUES + (1, 'FOO', 3, 3) +ON CONFLICT (id) DO UPDATE + SET id = excluded.id, name = excluded.name, name = excluded.name, name = excluded.name + +query ITII +SELECT * FROM issue_14052_2; +---- +1 FOO 1 2 + +# Make sure the fast path isn't taken (all clauses in the set must be of the form x = excluded.x) +statement ok +INSERT INTO issue_14052_2 (id, name, createdAt, updatedAt) VALUES + (1, 'BAR', 4, 5) +ON CONFLICT (id) DO UPDATE + SET name = excluded.name, createdAt = excluded.updatedAt, updatedAt = excluded.updatedAt + +query ITII +SELECT * FROM issue_14052_2; +---- +1 BAR 5 5 diff --git a/pkg/sql/upsert.go b/pkg/sql/upsert.go index 00134ab70e27..6e85811a0b17 100644 --- a/pkg/sql/upsert.go +++ b/pkg/sql/upsert.go @@ -38,7 +38,6 @@ type upsertHelper struct { evalExprs []parser.TypedExpr sourceInfo *dataSourceInfo excludedSourceInfo *dataSourceInfo - allExprsIdentity bool curSourceRow parser.Datums curExcludedRow parser.Datums @@ -135,22 +134,6 @@ func (p *planner) makeUpsertHelper( } helper.evalExprs = evalExprs - helper.allExprsIdentity = true - for _, expr := range evalExprs { - ivar, ok := expr.(*parser.IndexedVar) - if !ok { - helper.allExprsIdentity = false - break - } - // Idx on the IndexedVar references a columns from one of the two - // sources. They're ordered table source then excluded source. If any - // expr references a table column, then the fast path doesn't apply. - if ivar.Idx < len(sourceInfo.sourceColumns) { - helper.allExprsIdentity = false - break - } - } - return helper, nil } @@ -179,10 +162,6 @@ func (uh *upsertHelper) eval( return ret, nil } -func (uh *upsertHelper) isIdentityEvaler() bool { - return uh.allExprsIdentity -} - // upsertExprsAndIndex returns the upsert conflict index and the (possibly // synthetic) SET expressions used when a row conflicts. func upsertExprsAndIndex( diff --git a/pkg/sql/upsert_test.go b/pkg/sql/upsert_test.go index e567d2405a80..55523d228a2c 100644 --- a/pkg/sql/upsert_test.go +++ b/pkg/sql/upsert_test.go @@ -74,12 +74,12 @@ func TestUpsertFastPath(t *testing.T) { t.Errorf("expected no begin-txn (1PC) but got %d", s) } - // This should hit the fast path. + // This could hit the fast path, but doesn't right now because of #14482. atomic.StoreUint64(&scans, 0) atomic.StoreUint64(&beginTxn, 0) sqlDB.Exec(`INSERT INTO d.kv VALUES (1, 1) ON CONFLICT (k) DO UPDATE SET v=excluded.v`) - if s := atomic.LoadUint64(&scans); s != 0 { - t.Errorf("expected no scans (the upsert fast path) but got %d", s) + if s := atomic.LoadUint64(&scans); s != 1 { + t.Errorf("expected 1 scans (no upsert fast path) but got %d", s) } if s := atomic.LoadUint64(&beginTxn); s != 0 { t.Errorf("expected no begin-txn (1PC) but got %d", s)