sql: fix UPSERT fast path regression

[danhhz]

And add a test to prevent future regressions.

A second bug was unearthed which caused the upsert fast path to be
incorrect during schema changes. This was caught by the schema change
tests when the regression was fixed.

Closes #13437.

cc @petermattis

---

This change is 

[petermattis]

LGTM

[petermattis]

This is mildly unfortunate as the fast-path is ~2x faster than the slow-path. So adding a column will cause performance to temporarily drop in half? Is there any hope to fix this eventually? Perhaps file an issue.

[petermattis]

I'm mildly surprised this doesn't need to be done atomically.

[danhhz]

Review status: 0 of 4 files reviewed at latest revision, 2 unresolved discussions.

---

pkg/sql/tablewriter.go, line 231 at r1 (raw file):

Previously, petermattis (Peter Mattis) wrote…

This is mildly unfortunate as the fast-path is ~2x faster than the slow-path. So adding a column will cause performance to temporarily drop in half? Is there any hope to fix this eventually? Perhaps file an issue.

IIRC, when you're adding a column, you can't set it as a user while the schema change is being run. Which I think makes it impossible to fast path during that time

---

pkg/sql/upsert_test.go, line 42 at r1 (raw file):

Previously, petermattis (Peter Mattis) wrote…

I'm mildly surprised this doesn't need to be done atomically.

Same. atomic'd it

---

Comments from Reviewable

[petermattis]

---

Review status: 0 of 4 files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.

---

pkg/sql/tablewriter.go, line 231 at r1 (raw file):

Previously, danhhz (Daniel Harrison) wrote…

IIRC, when you're adding a column, you can't set it as a user while the schema change is being run. Which I think makes it impossible to fast path during that time

Ok, thinking about it more this makes sense, but shouldn't the conditional be part of the next line. What do mutations have to do with allColsIdentityExpr. I think you should let the value remain the same but change the following line to:

if len(tu.tableDesc.Indexes) == 0 && len(tu.tableDesc.Mutations) == 0 && allColsIdentityExpr {

---

Comments from Reviewable

[knz]

---

Reviewed 3 of 4 files at r1, 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.

---

pkg/sql/upsert_test.go, line 43 at r2 (raw file):

		if filterArgs.Req.Method() == roachpb.Scan &&
			bytes.Compare(filterArgs.Req.Header().Key, keys.UserTableDataMin) >= 0 {
			atomic.AddUint64(&scans, 1)

That's a very clever approach to testing this. TIL!

---

Comments from Reviewable

[petermattis]

Review status: all files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.

---

pkg/sql/upsert_test.go, line 43 at r2 (raw file):

Previously, knz (kena) wrote…

That's a very clever approach to testing this. TIL!

Yes, I can probably enhance this to verify that we have a 1PC transaction. See #13500.

---

Comments from Reviewable

[danhhz]

Thanks for the reviews!

---

Review status: 3 of 4 files reviewed at latest revision, 2 unresolved discussions.

---

pkg/sql/tablewriter.go, line 231 at r1 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Ok, thinking about it more this makes sense, but shouldn't the conditional be part of the next line. What do mutations have to do with allColsIdentityExpr. I think you should let the value remain the same but change the following line to:

if len(tu.tableDesc.Indexes) == 0 && len(tu.tableDesc.Mutations) == 0 && allColsIdentityExpr {

I was thinking of when the mutation is a column, then it can't be specified as the identity expr by the user. But you're right that indexes don't fall into this. Moved.

I also improved this comment a bit

---

Comments from Reviewable