kv: add ReturnRawMVCCValue to Get, Scan and ReverseScan requests

[stevendanna]

Logical Data Replication (LDR) and Import now both write values into the MVCCValueHeader of keys. We would eventually like to be able to read these values from SQL. In the case of LDR this will allow the SQL-write-path to continue to function even after the OriginTimetamp column is moved out of the user's table schema and into the MVCCValueHeader.

Here, we take the first step in that direction by adding a ReturnRawMVCCValue option to GetRequest, ScanRequest, and ReverseScanRequest.

Future work will modify the row and column fetchers to pass this option and correctly handle the results.

Epic: none
Release note: None

[cockroach-teamcity]

This change is 

[tbg]

Good on the high level - I'd like to leave the first detailed pass to @miraradeva.

As far as approach - @nvanbenschoten suggested¹ the alternative of putting the header into a new field on roachpb.Value. Both seem sensible and I don't have strong opinions on which one is better "on principle" and "in practice" (though I assume you'll prefer the RawBytes option for ergonomics, but it would be good to explain how the other change makes it more difficult). In short, just making sure we're not going down a one-way road here that we later regret.

I would also be interested in adding a way that a) always writes headers and b) always returns them from all operations in tests. Will CRDB work in that mode? Will it be roughly perform the same on performance? Both should be true. I'd like to think of the decision to opt into headerful RawBytes as something we can consider making the default later - if we discover roadblocks to this, it should give us pause.

I'll also ask for an audit of all RawBytes access (once we've committed to that fork in the road). I remember your precursor PR had some TODOs about some RawBytes being randomly generated in tests; you should fix these "just because" (replace with a RandomValidRawBytes helper or the like). But you also mentioned in a conversation that bulk ops tend to create RawBytes; this should be fine. I wonder what other categories of RawBytes access there are, and if any of them are problematic. We also need to anticipate future RawBytes access and make clear what goes and what doesn't. (In my understand, direct access to RawBytes that come from KV should be rare).

Footnotes

1. https://cockroachlabs.slack.com/archives/G01G8LK77DK/p1727293622908329?thread_ts=1727251232.773059&cid=G01G8LK77DK ↩

[tbg]

I don't think we've ever considered whether inline values can have an MVCCValueHeader. I think there's no reason to assume they couldn't, but it's probably a door we should close until we decide that we want to open it explicitly. However if for whatever reasons inline puts with a header are out there, we want that to keep working. So my vote would be to see (perhaps by adding an assertion plus an audit) whether there is a way for inline writes to end up with a header. If not - great, shut the door to it by adding an error check on the write path and an non-fatal and perhaps testbuild-only assertion on the read path.
I wouldn't do this in this PR though. You could file an issue.

[stevendanna]

Both seem sensible and I don't have strong opinions on which one is better "on principle" and "in practice" (though I assume you'll prefer the RawBytes option for ergonomics, but it would be good to explain how the other change makes it more difficult). In short, just making sure we're not going down a one-way road here that we later regret.

A concern on top of ergonomics is increasing the size of roachpb.Value structs unnecessarily. I suppose the extra field is a bit less of a one-way door since callers are always free to ignore a field they don't understand. I don't think using RawBytes is a one way door, but if we do want to move from that to an extra field later, I think it just implies that callers would still need to understand both for at least 2 release cycles.

a) always writes headers and b) always returns them from all operations in tests. Will CRDB work in that mode?

We have the former in a metamorphic variable. I had the latter during testing. Let me enable this on this branch and see how many roadblocks we hit.

Will it be roughly perform the same on performance? Both should be true. I'd like to think of the decision to opt into headerful RawBytes as something we can consider making the default later - if we discover roadblocks to this, it should give us pause.

I don't think the performance will be the same since there is a cost to any key that has an MVCC Value header. I've opened #131359 to address at least one issue related to that.

I'll also ask for an audit of all RawBytes access (once we've committed to that fork in the road). I remember your precursor PR had some TODOs about some RawBytes being randomly generated in tests; you should fix these "just because" (replace with a RandomValidRawBytes helper or the like)

Perhaps I should pull in the second commit that updates roachpb.Value's methods to this review. It might make things a bit easier. I almost surely need to do that if we want a metamorphic mode that always sets the ReturnRawMVCCValues option.

As for the audit, I don't think it'll be too bad, I did an incomplete look initial and found that outside of tests, roachpb.Value methods, and kvnemesis there were roughly 144 references to RawBytes. 52 of these were length or nil comparisons. Of the remaining 92, the vast majority are simply copies/assignments as passing along the data untouched, so my guess is we have just a few dozens of callsites to look at with a more caring eye.

[miraradeva]

I'm trying to understand the difference between the two approaches better.

A concern on top of ergonomics is increasing the size of roachpb.Value structs unnecessarily.

I get the part about increasing the roachpb.Value struct size. What exactly makes this approach more ergonomic? Is the difficulty in populating the new field in all the possible cases in pebbleMVCCScanner.getOne?

If we go with the separate field, we don't need to worry about auditing the RawBytes use cases, right?

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

---

pkg/storage/pebble_mvcc_scanner.go line 437 at r2 (raw file):

	skipLocked       bool
	tombstones       bool
	rawMVCCValues    bool

Maybe add a comment here?

---

pkg/storage/pebble_mvcc_scanner.go line 1050 at r2 (raw file):

				return false, false
			}
			return p.add(ctx, p.curUnsafeKey.Key, p.keyBuf, p.curUnsafeValue.Value.RawBytes, intentValueRaw)

Can you add a unit test for the intent case?

---

pkg/storage/pebble_mvcc_scanner.go line 1219 at r2 (raw file):

}

// Adds the specified key and value to the result set, excluding tombstones

Maybe add to this comment something about the new argument?

---

pkg/storage/pebble_mvcc_scanner.go line 1343 at r2 (raw file):

	}

	return p.add(ctx, key, p.keyBuf, value.Value.RawBytes, version.Value)

Can you add a unit test for the rangekey case too?