sql: disable fast path for some upsert queries

`INSERT .. ON CONFLICT ..` is temporarily no longer eligible for fast path.

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 cockroachdb#13437 cockroachdb#13962). As
a result, we've decided to remove this until after 1.0 and re-enable it
then. See cockroachdb#14482.

Closes cockroachdb#13962.

pkg/sql/insert.go

@@ -199,6 +199,7 @@ func (p *planner) Insert(
 				updateCols:    updateCols,
 				conflictIndex: *conflictIndex,
 				evaler:        helper,
+				isUpsertAlias: n.OnConflict.IsUpsertAlias(),
 			}
 		}
 	}

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

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

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(

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)