kv: only scan intents span for QueryIntent request

[andrewbaptist]

Change QueryIntent to only read from the lock table. Previously the request required merging the read from the MVC iterator with the lock table. This should improve performance.

Also, add unit testing for QueryIntent

Fixes #69716

Release note: None

[cockroach-teamcity]

This change is 

[andrewbaptist]

OK, thanks - in this case, the purpose of the *Intent was to cover the hasIntent or not, so I think it is OK.

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

---

pkg/kv/kvserver/batcheval/cmd_query_intent.go line 116 at r3 (raw file):

Previously, andrewbaptist (Andrew Baptist) wrote…

Moved this to engine.go

OK, moved

---

pkg/kv/kvserver/batcheval/cmd_query_intent.go line 54 at r6 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

nit: stray line.

OK, Fixed

---

pkg/kv/kvserver/batcheval/cmd_query_intent.go line 101 at r6 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

This feels like it could be noisy to me. There are cases where an intent not being found is valid. Consider putting this behind a log verbosity level, like log.VEventf(ctx, 2, ...). That way, it will show up in traces and also logs if logging verbosity is dialed up sufficiently for this file.

OK, Changed

---

pkg/storage/engine.go line 891 at r5 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

In go that would be (hasIntent bool, intent *Intent, err error)

I don't think that's quite right. We wouldn't return a pointer to an Intent if we weren't using nullability to indicate whether the intent existed or not. We have two options here, either of which is idiomatic in Go:

• (intent *Intent, err error)
• (ok bool, intent Intent, err error)

OK, leaving as is - this is the reason I returned the pointer, so just wanted to make sure it was idiomatic

---

pkg/storage/engine.go line 921 at r5 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

If you want a pointer to an intent then you are going to have to do something like this. Go won't let you take the address of a return value directly. You first need to assign it to a variable on the stack, then take the address of that, which will force it to escape.

Do we need this? var intent *roachpb.Intent = nil isn't giving you anything. Couldn't we do something like:

intent := roachpb.MakeIntent(meta.Txn, checkKey)
return &intent, nil

OK, thanks!

---

pkg/storage/engine.go line 886 at r6 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

nit: to parallel ScanIntents, I think this should be called GetIntent, not QueryIntent.

OK, changed

---

pkg/storage/engine.go line 886 at r6 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Should we add unit testing around this function as well? Similar to TestScanIntents.

Working, I should have moved the testing after splitting this function into engine.go

---

pkg/storage/engine.go line 903 at r6 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

style nit: Go prefers early returns and minimal outdenting. So instead of:

if valid {
    ... long ...
}
return intent, nil

prefer:

if !valid {
    return nil, nil
}
... long ...

OK, changed - also changed to use nil, err in most places to make it more clear

---

pkg/storage/engine.go line 920 at r6 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

We'll want to validate that meta.Txn != nil, or else throw an errors.AssertionFailedf.

OK, added check

---

pkg/storage/engine.go line 928 at r6 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Generally in Go you shouldn't return a valid value and an error. So here and below, prefer return nil, err.

OK, changed all to make this more clear

---

pkg/kv/kvserver/batcheval/cmd_query_intent_test.go line 45 at r6 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

(throughout this test) Go's style guide prefers proper capitalization and punctuation in comments: https://go.dev/doc/effective_go#commentary.

OK, updated

---

pkg/kv/kvserver/batcheval/cmd_query_intent_test.go line 114 at r6 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

The test never seems to be consulting QueryIntentResponse.FoundIntent. So if !test.errorFlag, it's not really testing anything. Perhaps this is where your question came from on Slack.

Working, I missed checking the response object that comes back.

[andrewbaptist]

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

---

pkg/storage/engine.go line 886 at r6 (raw file):

Previously, andrewbaptist (Andrew Baptist) wrote…

Working, I should have moved the testing after splitting this function into engine.go

OK, added unit test that at least cover the main cases (found/not found). Lets chat if there are easy ways to test a lot of the other error cases.

---

pkg/kv/kvserver/batcheval/cmd_query_intent_test.go line 114 at r6 (raw file):

Previously, andrewbaptist (Andrew Baptist) wrote…

Working, I missed checking the response object that comes back.

OK - I added additional checks against the response object. This covers the main case of the reply.FoundIntent, but doesn't correctly check the WriteTimestamp on the reply. I could probably add that if you think it is worthwhile to do. It gets set in the one case of the transaction write timestamp is less than the intent write timestamp.

[nvanbenschoten]

This is getting close. Nice job!

Reviewed 3 of 3 files at r7, 5 of 5 files at r10, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andrewbaptist, @nvanbenschoten, and @sumeerbhola)

---

pkg/kv/kvserver/batcheval/cmd_query_intent.go line 87 at r10 (raw file):

	if intentPtr != nil {
		intent := *intentPtr

This doesn't feel necessary. Consider leaving intentPtr called intent and never explicitly dereferencing. Go implicitly dereferences when accessing fields and methods.

---

pkg/kv/kvserver/batcheval/cmd_query_intent.go line 121 at r10 (raw file):

				if ownTxn {
					reply.Txn = h.Txn.Clone()
					reply.Txn.WriteTimestamp.Forward((intent).Txn.WriteTimestamp)

nit: why the parenthesis?

---

pkg/storage/engine.go line 891 at r10 (raw file):

// intent, missing transaction on the intent or multiple intents for this key.
func GetIntent(reader Reader, key roachpb.Key) (*roachpb.Intent, error) {
	// translate to a key in the lock space

Same point about the the capitalization and punctuation in comments.

---

pkg/storage/engine.go line 923 at r10 (raw file):

	}
	if meta.Txn == nil {
		return nil, errors.AssertionFailedf("Txn is null for key %v, intent %v", key, meta)

nit: error messages are generally lower-case

---

pkg/kv/kvserver/batcheval/cmd_query_intent_test.go line 128 at r10 (raw file):

			switch test.response {
			case Error:
				require.Error(t, err)

Should we assert on the contents of the error?

---

pkg/storage/engine_test.go line 1636 at r10 (raw file):

				require.NoError(t, err)
				if test.found {
					require.NotNil(t, intent)

Let's also test the contents of the intent. The txn should be correct (maybe just check the txn.ID) and the key should be correct.

[andrewbaptist]

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

---

pkg/kv/kvserver/batcheval/cmd_query_intent.go line 87 at r10 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

This doesn't feel necessary. Consider leaving intentPtr called intent and never explicitly dereferencing. Go implicitly dereferences when accessing fields and methods.

OK, fixed - I didn't realize about the implict deference - but makes this much cleaner

---

pkg/kv/kvserver/batcheval/cmd_query_intent.go line 121 at r10 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

nit: why the parenthesis?

OK removed

---

pkg/storage/engine.go line 891 at r10 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Same point about the the capitalization and punctuation in comments.

OK, fixed

---

pkg/storage/engine.go line 923 at r10 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

nit: error messages are generally lower-case

OK, changed

---

pkg/kv/kvserver/batcheval/cmd_query_intent_test.go line 128 at r10 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Should we assert on the contents of the error?

OK, I added a check on the type of the errors - it is a little hacky though, so if you see a better way to do this I can change it.

---

pkg/storage/engine_test.go line 1636 at r10 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Let's also test the contents of the intent. The txn should be correct (maybe just check the txn.ID) and the key should be correct.

OK, Good idea - added (and check key to be safe)

[nvanbenschoten]

other than the proposed cleanup to avoid reflection in cmd_query_intent_test.go.

Reviewed 4 of 4 files at r11, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @andrewbaptist, @nvanbenschoten, and @sumeerbhola)

---

pkg/kv/kvserver/batcheval/cmd_query_intent_test.go line 128 at r10 (raw file):

Previously, andrewbaptist (Andrew Baptist) wrote…

OK, I added a check on the type of the errors - it is a little hacky though, so if you see a better way to do this I can change it.

I think you can clean this up with either require.IsType or require.ErrorIs. That should allow you to remove all use f reflection.

---

pkg/kv/kvserver/batcheval/cmd_query_intent_test.go line 72 at r11 (raw file):

	// the way that type information is lost, this seemed like the only way to do
	// this.
	Success := reflect.TypeOf(1)

nit: local variables are rarely capitalized in Go. Capitalization is used to control package visibility.

[andrewbaptist]

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

---

pkg/kv/kvserver/batcheval/cmd_query_intent_test.go line 128 at r10 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

I think you can clean this up with either require.IsType or require.ErrorIs. That should allow you to remove all use f reflection.

OK, I was able to get this working with IsType - It was a little different than I thought at first but glad you pushed me down this path.

---

pkg/kv/kvserver/batcheval/cmd_query_intent_test.go line 72 at r11 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

nit: local variables are rarely capitalized in Go. Capitalization is used to control package visibility.

OK, fixed

[nvanbenschoten]

Reviewed 1 of 1 files at r12, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @nvanbenschoten and @sumeerbhola)

[andrewbaptist]

bors r=nvanbenschoten

[craig]

Build failed (retrying...):

• GitHub CI (Cockroach)

[craig]

Canceled.

[andrewbaptist]

bors r=nvanbenschoten

[craig]

Build succeeded:

• GitHub CI (Cockroach)