storage: Refactor pagination for the Get command into the MVCC layer

[KaiSun314]

Informs: #77228

Refactor key and byte pagination for the Get command into the MVCC layer Previously, pagination was done in pkg/kv/kvserver/batcheval/cmd_get.go, but to ensure consistency in where pagination logic is located across all commands, we move the pagination logic for the Get command to the MVCC layer where the pagination logic for most other commands is. This also enables better parameter testing in the storage package since we can leverage e.g. data-driven tests like TestMVCCHistories.

Release note: None

[blathers-crl]

Thank you for contributing to CockroachDB. Please ensure you have followed the guidelines for creating a PR.

My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}

[cockroach-teamcity]

This change is 

[blathers-crl]

Thank you for updating your pull request.

My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}

[knz]

cc @nicktrav

[sumeerbhola]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @KaiSun314)

---

pkg/storage/mvcc.go line 1027 at r2 (raw file):

		//
		// This mirrors the logic in MVCCScan, though the logic in MVCCScan is
		// slightly lower in the stack.

stale comment "This mirrors ..."

[KaiSun314]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @sumeerbhola)

---

pkg/storage/mvcc.go line 1027 at r2 (raw file):

Previously, sumeerbhola wrote…

stale comment "This mirrors ..."

Done.

[sumeerbhola]

The storage changes are fine with me, but added @nvanbenschoten to give final approval.

[nvanbenschoten]

Reviewed 2 of 4 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @KaiSun314 and @sumeerbhola)

---

pkg/storage/mvcc.go line 917 at r3 (raw file):

	// TargetBytes limit.
	AllowEmpty bool
	// Reply is a pointer to the Get response object.

This feels like a layering violation. MVCC is not currently aware of roachpb.Response types. I don't think we want it to be.

Instead of pushing a pointer to the response to be modified into MVCCGet, we could return the information we need to populate this response, like we do with MVCCScan. Then we keep the decision of how to use this information (e.g. to populate a *roachpb.GetResponse) up in kv/kv/server/batcheval.

That might mean creating a MVCCGetResult struct, which feels like a nice change.

[KaiSun314]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @nvanbenschoten and @sumeerbhola)

---

pkg/storage/mvcc.go line 917 at r3 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

This feels like a layering violation. MVCC is not currently aware of roachpb.Response types. I don't think we want it to be.

Instead of pushing a pointer to the response to be modified into MVCCGet, we could return the information we need to populate this response, like we do with MVCCScan. Then we keep the decision of how to use this information (e.g. to populate a *roachpb.GetResponse) up in kv/kv/server/batcheval.

That might mean creating a MVCCGetResult struct, which feels like a nice change.

True agreed, I have migrated to returning a MVCCGetResult struct containing the fields to populate the Get response

[nvanbenschoten]

Reviewed 2 of 21 files at r4, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @KaiSun314 and @sumeerbhola)

---

pkg/storage/mvcc.go line 917 at r3 (raw file):

Previously, KaiSun314 (Kai Sun) wrote…

True agreed, I have migrated to returning a MVCCGetResult struct containing the fields to populate the Get response

Should we include the *roachpb.Value and *roachpb.Intent in the MVCCGetResult as well? That's what we do in MVCCScanResult.

[blathers-crl]

Thank you for updating your pull request.

Before a member of our team reviews your PR, I have some potential action items for you:

• We notice you have more than one commit in your PR. We try break logical changes into separate commits, but commits such as "fix typo" or "address review commits" should be squashed into one commit and pushed with --force
• When CI has completed, please ensure no errors have appeared.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}

[KaiSun314]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @nvanbenschoten and @sumeerbhola)

---

pkg/storage/mvcc.go line 917 at r3 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Should we include the *roachpb.Value and *roachpb.Intent in the MVCCGetResult as well? That's what we do in MVCCScanResult.

Yep that sounds great, added Value *roachpb.Value and Intent *roachpb.Intent to MVCCGetresult

[nvanbenschoten]

Reviewed 19 of 23 files at r5, 4 of 4 files at r6, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @KaiSun314 and @sumeerbhola)

---

-- commits line 9 at r6:
Did you mean for these to be two different commits?

---

pkg/kv/kvserver/replica.go line 1808 at r6 (raw file):

	desc := r.descRLocked()
	descKey := keys.RangeDescriptorKey(desc.StartKey)
	res, err := storage.MVCCGet(ctx, r.Engine(), descKey, hlc.MaxTimestamp,

intentRes

---

pkg/kv/kvserver/replica.go line 1815 at r6 (raw file):

		return false, nil
	}
	getRes, err := storage.MVCCGetAsTxn(

valRes

---

pkg/kv/kvserver/replica_rangefeed.go line 538 at r4 (raw file):

		// Read the previous value from the prev Reader. Unlike the new value
		// (see handleLogicalOpLogRaftMuLocked), this one may be missing.
		prevVal, _, _, err := storage.MVCCGet(

In cases where the variable names before carried some context to help readers understand the code, we should try to incorporate that into the new name. For instance, it would be clearer if this was prevValRes. I left comments about this in a few other places.

[KaiSun314]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @nvanbenschoten and @sumeerbhola)

---

-- commits line 9 at r6:

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Did you mean for these to be two different commits?

Oh yep I wanted to split into two commits to make it easier to read and review. One commit refactors MVCCGetResult into the return values and one commit pushes pagination for Get into MVCC.

---

pkg/kv/kvserver/replica_rangefeed.go line 538 at r4 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

In cases where the variable names before carried some context to help readers understand the code, we should try to incorporate that into the new name. For instance, it would be clearer if this was prevValRes. I left comments about this in a few other places.

Done, incorporated previous variable names into the new name

[nvanbenschoten]

Reviewed 11 of 15 files at r7, 4 of 4 files at r8, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @sumeerbhola)

[KaiSun314]

Thank you so much for the review Nathan and Sumeer!