storage: Push pagination for EndTxn into the MVCC layer

[KaiSun314]

Informs: #77228

This PR consists of two parts: adding MVCCPagination and integrating MVCCPagination to push pagination of EndTxn into the MVCC layer.

Part 1: Add MVCCPagination

Implement MVCCPagination which calls a user-inputted function f() until
it returns an error (note that the iterutil.StopIteration() error means
we have iterated through all elements), or until the number of keys hits
the maxKeys limit or the number of bytes hits the targetBytes limit.

MVCCPagination is a general framework that enables key and byte
pagination.

Part 2: Integrate MVCCPagination to Push Pagination of EndTxn Into The MVCC Layer

In this PR, we push key pagination for EndTxn (currently located in
cmd_end_transaction.go) into the MVCC layer (mvcc.go) by integrating
MVCCPagination from the previous commit into EndTxn.

Release note: None

[blathers-crl]

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

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.

I was unable to automatically find a reviewer. You can try CCing one of the following members:

• A person you worked with closely on this PR.
• The person who created the ticket, or a CRDB organization member involved with the ticket (author, commenter, etc.).
• Join our community slack channel and ask on #contributors.
• Try find someone else from here.

_{🦉 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.

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.

I was unable to automatically find a reviewer. You can try CCing one of the following members:

• A person you worked with closely on this PR.
• The person who created the ticket, or a CRDB organization member involved with the ticket (author, commenter, etc.).
• Join our community slack channel and ask on #contributors.
• Try find someone else from here.

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

[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.

I have added a few people who may be able to assist in reviewing:

• @erikgrinaker (commented on kv: Support byte size pagination with intent resolution #77228)
• @nvanbenschoten (author of kv: Support byte size pagination with intent resolution #77228)

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

[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.}

[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
• Please ensure your git commit message contains a release note.
• When CI has completed, please ensure no errors have appeared.

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

[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
• Please ensure your git commit message contains a release note.
• When CI has completed, please ensure no errors have appeared.

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

[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 dev-inf.}

[nvanbenschoten]

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

---

pkg/kv/kvserver/batcheval/cmd_end_transaction.go line 557 at r2 (raw file):

		span := args.LockSpans[i]
		i++
		if err := func() error {

This anonymous function was used to group the error-handling path so we could wrap all errors with "resolving lock at ..". Now that we're calling storage.MVCCPagination, I don't think we need it. Instead, we can just perform the error wrapping on the error return path after the call to storage.MVCCPagination.

---

pkg/kv/kvserver/batcheval/cmd_end_transaction.go line 574 at r2 (raw file):

				// Note that the underlying pebbleIterator will still be reused
				// since readWriter is a pebbleBatch in the typical case.
				ok, _, _, err := storage.MVCCResolveWriteIntent(ctx, readWriter, ms, update,

MVCCResolveWriteIntent returns a resume span. Should we be consulting it?

---

pkg/kv/kvserver/batcheval/cmd_end_transaction.go line 635 at r2 (raw file):

	}

	externalLocks = append(externalLocks, args.LockSpans[i:]...)

Instead of maintaining i separately and reaching into args.LockSpans multiple times, what do you think about something like:

remainingLockSpans := args.LockSpans
f := func(maxKeys, targetBytes int64) (numKeys int64, numBytes int64, resumeSpan *roachpb.Span, err error) {
    if len(remainingLockSpans) == 0 {
        return 0, 0, nil, iterutil.StopIteration()
    }
    span := remainingLockSpans[0]
    remainingLockSpans = remainingLockSpans[1:]
    ...
    if ... {
        externalLocks = append(externalLocks, span)
    }
    ...
}

externalLocks = append(externalLocks, remainingLockSpans...)

---

pkg/storage/mvcc.go line 4089 at r1 (raw file):

}

// MVCCPagination invokes f() until it returns done (i.e. we have iterated

The functions in this package are typically verbs (MVCCScan, MVCCIterate, etc.), so to match, we'll want to name this MVCCPaginate.

---

pkg/storage/mvcc.go line 4090 at r1 (raw file):

// MVCCPagination invokes f() until it returns done (i.e. we have iterated
// through all elements) or an error, or until the number of keys hits the

We should include some of the commentary from MVCCIterate about iterutil.StopIteration.

EDIT: it looks like you added some of this in the second commit. We'll want to move that back to the first.

---

pkg/storage/mvcc.go line 4091 at r1 (raw file):

// MVCCPagination invokes f() until it returns done (i.e. we have iterated
// through all elements) or an error, or until the number of keys hits the
// maxKeys limit or the number of bytes hits the targetBytes limit.

It would be worthwhile calling out that it's up to f() whether it wants to allow the numBytes to exceed targetBytes by up to one entry or whether it wants to terminate iteration before numBytes exceeds targetBytes. See the AllowEmpty option.

Having written that out, I wonder how that choice interacts with the resumeSpan assertion that you added.

---

pkg/storage/mvcc.go line 4105 at r1 (raw file):

		}
		addedKeys, addedBytes, resumeSpan, err := f(maxKeys, targetBytes)
		if err != nil {

Is the assumption that addedKeys, addedBytes, and resumeSpan are all empty in this case? If so, should we assert that?

---

pkg/storage/mvcc.go line 4127 at r1 (raw file):

			}
		}
		if resumeSpan != nil {

I think we had talked about returning the resumeSpan in the past. It's definitely better than nothing because it allows for the assertion, but if we're just going to drop it after the assertion, I wonder whether we should return the resumeReason instead. If we do that, we can tighten up the assertion further. There's also something nice about returning an integer instead of a pointer.

[KaiSun314]

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

---

pkg/kv/kvserver/batcheval/cmd_end_transaction.go line 574 at r2 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

MVCCResolveWriteIntent returns a resume span. Should we be consulting it?

MVCCResolveWriteIntent will only return a resume span with byte pagination, not key pagination. Byte pagination will be added in #95141, and this PR's goal is to just do refactoring with no behaviour changes

---

pkg/kv/kvserver/batcheval/cmd_end_transaction.go line 635 at r2 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Instead of maintaining i separately and reaching into args.LockSpans multiple times, what do you think about something like:

remainingLockSpans := args.LockSpans
f := func(maxKeys, targetBytes int64) (numKeys int64, numBytes int64, resumeSpan *roachpb.Span, err error) {
    if len(remainingLockSpans) == 0 {
        return 0, 0, nil, iterutil.StopIteration()
    }
    span := remainingLockSpans[0]
    remainingLockSpans = remainingLockSpans[1:]
    ...
    if ... {
        externalLocks = append(externalLocks, span)
    }
    ...
}

externalLocks = append(externalLocks, remainingLockSpans...)

Done. Agreed, that is cleaner!

---

pkg/storage/mvcc.go line 4091 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

It would be worthwhile calling out that it's up to f() whether it wants to allow the numBytes to exceed targetBytes by up to one entry or whether it wants to terminate iteration before numBytes exceeds targetBytes. See the AllowEmpty option.

Having written that out, I wonder how that choice interacts with the resumeSpan assertion that you added.

Hmm that's true. I have implemented one possible solution: MVCCPaginate will take in an allowEmpty parameter. If allowEmpty is true, then the resumeReason assertion will not be checked for the targetBytes limit (the maxKeys limit will still be checked regardless of allowEmpty). I was wondering if this would be okay?

---

pkg/storage/mvcc.go line 4127 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

I think we had talked about returning the resumeSpan in the past. It's definitely better than nothing because it allows for the assertion, but if we're just going to drop it after the assertion, I wonder whether we should return the resumeReason instead. If we do that, we can tighten up the assertion further. There's also something nice about returning an integer instead of a pointer.

Done. Agreed, changed to resumeReason, which also works perfectly with the above design to resolve the resumeReason assertion conflicting with the user wishing to terminate iteration before numBytes exceeds targetBytes.

[nvanbenschoten]

Reviewed 4 of 4 files at r3, 1 of 2 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @KaiSun314)

---

pkg/kv/kvserver/batcheval/cmd_end_transaction.go line 573 at r5 (raw file):

	remainingLockSpans := args.LockSpans
	var span roachpb.Span

Why declare span with this scope? It doesn't seem to be used outside of f, unless I'm missing something.

EDIT: I see that we're using this for the error wrapping. If that's the case, then I think we should just push the errors.Wrapf into the closure. There are only 4 error-handling paths. This gives us the opportunity to specialize the error text for each.

---

pkg/kv/kvserver/batcheval/cmd_end_transaction.go line 648 at r5 (raw file):

	}

	numKeys, _, _, err := storage.MVCCPaginate(ctx, maxKeys, 0, false, f)

Could you add /* <param name> */ comments for the literal values passed in here?

---

pkg/storage/mvcc.go line 4091 at r1 (raw file):

Previously, KaiSun314 (Kai Sun) wrote…

Hmm that's true. I have implemented one possible solution: MVCCPaginate will take in an allowEmpty parameter. If allowEmpty is true, then the resumeReason assertion will not be checked for the targetBytes limit (the maxKeys limit will still be checked regardless of allowEmpty). I was wondering if this would be okay?

This seems reasonable to me.

---

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

			}
		}
		if resumeReason == kvpb.RESUME_KEY_LIMIT {

Consider changing this to a switch-case statement so that you can exhaustively test all of the handled resume reasons (RESUME_KEY_LIMIT, RESUME_BYTE_LIMIT, 0) and then assert against the unhandled reasons.

[nvanbenschoten]

thanks Kai!

bors r=nvanbenschoten

Reviewed 3 of 3 files at r6, 2 of 2 files at r7, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @KaiSun314)

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)

[KaiSun314]

Thank you so much for the review and for merging, Nathan!