sql: enable 1PC optimization for UPSERT

[petermattis]

When an UPSERT is being run as part of an auto-commit transaction,
commit the transaction when running the batch.

---

This change is 

[petermattis]

Do we have any tests for the auto-commit code path for INSERT? A quick scan didn't reveal anything, but perhaps I just missed it.

[petermattis]

Review status: 0 of 2 files reviewed at latest revision, 1 unresolved discussion, some commit checks pending.

---

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

		}
		if err != nil {
			return sqlbase.ConvertBatchError(tu.ri.helper.tableDesc, tu.fastPathBatch)

Noticed we had this in tableInserter.finalize. Is it needed here too? I suppose for the upsert fast-path we can't get conditional put errors because we're not performing conditional puts.

---

Comments from Reviewable

[knz]

No we don't have separate tests for the INSERT auto-commit apparently.

for the rest.

---

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

---

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

Previously, petermattis (Peter Mattis) wrote…

Noticed we had this in tableInserter.finalize. Is it needed here too? I suppose for the upsert fast-path we can't get conditional put errors because we're not performing conditional puts.

yeah as long as we don't allow upsert/update to update the PKs this check here is not strictly necessary.

---

Comments from Reviewable

[danhhz]

my upsert test is going in as soon as teamcity finishes if you want to piggy back on that test. seems like that would work

---

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

---

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

Previously, knz (kena) wrote…

yeah as long as we don't allow upsert/update to update the PKs this check here is not strictly necessary.

correct. the fast path doesn't use cput and can't change a pk

---

Comments from Reviewable

[petermattis]

Enhanced TestUpsertFastPath to check for 1PC transactions and enabled 1PC transactions for slow-path UPSERT (found by the test!).

PTAL

---

Review status: 1 of 3 files reviewed at latest revision, 1 unresolved discussion.

---

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

Previously, danhhz (Daniel Harrison) wrote…

correct. the fast path doesn't use cput and can't change a pk

Ok, the call to sqlbase.ConvertBatchError was removed here.

---

Comments from Reviewable

[danhhz]

Review status: 1 of 3 files reviewed at latest revision, 3 unresolved discussions, some commit checks pending.

---

pkg/sql/tablewriter.go, line 335 at r2 (raw file):

	}

	if tu.autoCommit {

I think this one is not safe. In theory, flush is called once per N rows processed by upsert (though it seems actually doing this got left as a TODO). This would commit the txn and then try to keep using it, which IIUC is not okay

---

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

	}
	if s := atomic.LoadUint64(&beginTxn); s != 0 {
		t.Errorf("expected no begin-txn (1PC) but got %d", s)

we should probably add one that does expect a begin-txn

---

Comments from Reviewable

[petermattis]

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

---

pkg/sql/tablewriter.go, line 335 at r2 (raw file):

Previously, danhhz (Daniel Harrison) wrote…

I think this one is not safe. In theory, flush is called once per N rows processed by upsert (though it seems actually doing this got left as a TODO). This would commit the txn and then try to keep using it, which IIUC is not okay

I've added a finalize parameter to flush and set it to true from the one call site. Hopefully this will future proof the code here.

---

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

Previously, danhhz (Daniel Harrison) wrote…

we should probably add one that does expect a begin-txn

Done.

---

Comments from Reviewable

[danhhz]

---

Review status: 1 of 3 files reviewed at latest revision, 3 unresolved discussions, some commit checks pending.

---

Comments from Reviewable