engine: refine mvcc logic around moving intent timestamps

[nvanbenschoten]

This commit cleans up comments and logic in mvccResolveWriteIntent.

Release note: None

[cockroach-teamcity]

This change is 

[tbg]

Reviewable status: complete! 0 of 0 LGTMs obtained

---

pkg/storage/engine/mvcc.go, line 2217 at r1 (raw file):

	// after. We treat such intents as uncommitted.
	//
	// A commit with a newer timestamp than the intent means that our timestamp

This can't happen any more, right? We removed snapshot isolation and write at the provisional timestamp now, and we can't commit unless the provisional timestamp was the same for all writes. Assuming, of course, we're not in a migration where such things might still be carried out by other nodes.

[nvanbenschoten]

Reviewable status: complete! 0 of 0 LGTMs obtained

---

pkg/storage/engine/mvcc.go, line 2217 at r1 (raw file):

Previously, tbg (Tobias Grieger) wrote…

This can't happen any more, right? We removed snapshot isolation and write at the provisional timestamp now, and we can't commit unless the provisional timestamp was the same for all writes. Assuming, of course, we're not in a migration where such things might still be carried out by other nodes.

I think it can still happen if we successfully refresh a transaction to avoid restarting and laying down all intents again.

[tbg]

Reviewed 1 of 1 files at r1.
Reviewable status: complete! 1 of 0 LGTMs obtained

---

pkg/storage/engine/mvcc.go, line 2159 at r1 (raw file):

		return false, err
	}
	// For cases where there's no value corresponding to the key we're

What do you think about

nextKeyIter := iter
iter = nil // prevent accidental use

and use nextKeyIter below? Or any other way you can add confidence that we don't accidentally move iter in the future.

---

pkg/storage/engine/mvcc.go, line 2217 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

I think it can still happen if we successfully refresh a transaction to avoid restarting and laying down all intents again.

Ah, of course. I forgot about refreshs. Worth adding to the comment that this is the only situation in which this happens nowadays.

[nvanbenschoten]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @tbg)

---

pkg/storage/engine/mvcc.go, line 2159 at r1 (raw file):

Previously, tbg (Tobias Grieger) wrote…

What do you think about

nextKeyIter := iter
iter = nil // prevent accidental use

and use nextKeyIter below? Or any other way you can add confidence that we don't accidentally move iter in the future.

How's that? We were already doing it in one place below.

We could also take a *Iterator in unsafeNextVersion and nil it out in unsafeNextVersion, but that's kind of a strange contract.

---

pkg/storage/engine/mvcc.go, line 2217 at r1 (raw file):

Previously, tbg (Tobias Grieger) wrote…

Ah, of course. I forgot about refreshs. Worth adding to the comment that this is the only situation in which this happens nowadays.

Done.

[tbg]

Reviewed 1 of 1 files at r2, 2 of 2 files at r3.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale)

[nvanbenschoten]

bors r+

[craig]

Build succeeded

• GitHub CI (Cockroach)