kvclient: serve point read-your-own-writes from the buffer

[arulajmani]

This patch teaches the txnWriteBuffer to recognize when a point-read (i.e. a Get request) should be served from its local buffer. In such cases, we eschew sending the request to the KV layer, and instead stitch the result back on the response path.

The approach above is fairly straightforward. The only thing we must take care of is ensuring the correct value is returned based on the Get request's sequence number. The logic here mirrors that of getOne in pebble_mvcc_scanner.go. It's a bit simpler, as we don't have to account for sequence number rollbacks here. Unlike on the server, we'll proactively rollback writes belonging to sequence numbers that have been ignored as part of a savepoint rollback. As a result, we can be sure that any writes in the buffer aren't rolled back.

In a followup patch, we'll add support for Scan/Reverse requests that overlap with a part of the butffer.

Informs #139054

Release note: None

[cockroach-teamcity]

This change is 

[yuzefovich]

Nice! Just some minor comments from me.

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @arulajmani and @stevendanna)

---

pkg/kv/kvclient/kvcoord/txn_interceptor_write_buffer_test.go line 513 at r1 (raw file):

	// Add a few more blind writes to keyA at different sequence numbers.
	for seq, val := range map[enginepb.TxnSeq]string{12: valA12, 14: valA14} {

Hm, since we're iterating over a map, is it guaranteed that we would see seq 12 before seq 14?

---

pkg/kv/kvclient/kvcoord/txn_interceptor_write_buffer_test.go line 601 at r1 (raw file):

	require.Equal(t, mockSender.NumCalled(), numCalled+1)

	// Finally, perform a read on keyA at a lower sequence number than the minimum

nit: should we also do a read on keyB on seq 9, before the delete?

---

pkg/kv/kvclient/kvcoord/txn_interceptor_write_buffer.go line 298 at r1 (raw file):

		case *kvpb.GetRequest:
			// If the key is in the buffer, we can serve the read from the buffer.

nit: shouldn't it be "we must serve ..."? "We can serve" implies it is optional ...

---

pkg/kv/kvclient/kvcoord/txn_interceptor_write_buffer.go line 526 at r1 (raw file):

	id     uint64
	key    roachpb.Key
	endKey roachpb.Key     // used in btree iteration

I know this is off-topic, but it seems wasteful to me that we would define endKey that serves no purpose other than satisfying the contract of the btree generator. This endKey doesn't seem to be meaningful here and also in keyLocks structure, so it seems worth it to specialize the btree generator for these cases when working with just keys as opposed to key spans. Consider adding a TODO to explore this (unless I'm missing something).

(I was looking into this from the perspective of minimizing the memory footprint of bufferedWrite struct - having this endKey in there is extra 24 bytes per buffered write.)

---

pkg/kv/kvclient/kvcoord/txn_interceptor_write_buffer.go line 527 at r1 (raw file):

	key    roachpb.Key
	endKey roachpb.Key     // used in btree iteration
	vals   []bufferedValue // sorted in increasing sequence number order

nit: another potential optimization is having an array of small size that we would use as the start for vals in addToBuffer, i.e. something like

	valsAlloc [1]bufferedValue
	vals      []bufferedValue // sorted in increasing sequence number order

Consider adding a TODO for this too.

---

pkg/kv/kvclient/kvcoord/txn_interceptor_write_buffer.go line 536 at r1 (raw file):

}

// valPtr returns a pointer to a copy of the buffered value.

Why do we need a copy?