kv: fix racy access to a Transaction proto

[andreimatei]

Fixes #15565

There was a race on accesses to Transaction.ObservedTimestamps because
we ended up sharing the ObservedTimestamps between a Transaction proto
stored in TxnCoordSender's map and the proto stored in the client.Txn.
The sharing happened because the call to
TxnCoordSender.cleanupTxnLocked() from TxnCoordSender.send() made a
shallow copy of the proto.

[cockroach-teamcity]

This change is 

[tamird]

I've made cleanupTxnLocked() take the Transaction proto by ptr as I
think it makes sense to hide the cloning inside it.

Slightly -1 on this; the call to Clone that you added already invites questions so should have a comment. Passing by pointer doesn't do much other than muddy the waters (can I pass nil?) and introduce diff noise.

Second commit LGTM, nice find!

---

Reviewed 3 of 3 files at r1, 2 of 2 files at r2.
Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks pending.

---

pkg/kv/txn_coord_sender.go, line 551 at r1 (raw file):

		return roachpb.NewError(errNoState)
	case txnMeta.txn.Status == roachpb.ABORTED:
		txn := txnMeta.txn.Clone()

💯 on removing these !

---

pkg/kv/txn_coord_sender.go, line 614 at r1 (raw file):

	// The supplied txn may be newer than the one in txnMeta, which is relevant
	// for stats.

As mentioned in the top level comment: I think the Clone needs to be explained here.

---

pkg/roachpb/data.go, line 718 at r2 (raw file):

func (t Transaction) Clone() Transaction {
	mt := t.ObservedTimestamps
	if mt != nil {

👍

---

Comments from Reviewable

[tbg]

Well-spotted on the second commit. Share @tamird's opinion on the first. Thanks for the debugging. Looked horrible from what I saw.

---

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

---

Comments from Reviewable

[andreimatei]

reverted to passing by value

---

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

---

pkg/kv/txn_coord_sender.go, line 614 at r1 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

As mentioned in the top level comment: I think the Clone needs to be explained here.

Done.

---

Comments from Reviewable

[tamird]

The first commit message is now stale.

---

Reviewed 3 of 3 files at r3, 2 of 2 files at r4.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks pending.

---

pkg/kv/txn_coord_sender.go, line 614 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

Done.

s/might have have/might not have/

---

pkg/kv/txn_coord_sender.go, line 615 at r3 (raw file):

	// The supplied txn may be newer than the one in txnMeta, which is relevant
	// for stats. We clone the txn before storing it, as the caller might have
	// have provided a deep-copy, and we don't want to share Transaction's in the

s/Transaction's/Transactions/ (no apostrophe)

---

Comments from Reviewable

[andreimatei]

completed that commit message

---

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

---

pkg/kv/txn_coord_sender.go, line 614 at r1 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

s/might have have/might not have/

Done.

---

pkg/kv/txn_coord_sender.go, line 615 at r3 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

s/Transaction's/Transactions/ (no apostrophe)

Done.

---

Comments from Reviewable

[tamird]

thanks!

---

Reviewed 3 of 3 files at r5, 2 of 2 files at r6.
Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable