builtins: stream consistency checker output

[tbg]

Also makes it resilient to per-Range errors, which now no longer
tank the entire operation.

-- avoid buffering in cli
\set display_format=csv;
-- avoid rows getting buffered at server
set avoid_buffering=true;
-- compute as fast as possible
SET CLUSTER SETTING server.consistency_check.max_rate = '1tb';

SELECT * FROM crdb_internal.check_consistency(false, '', '');

edit: should probably use SET CLUSTER SETTING sql.defaults.results_buffer.size = '1b'; here, somehow the set avoid_buffering=true isn't cutting it entirely

Release justification: improvement for a debugging-related feature
Release note: None

[cockroach-teamcity]

This change is 

[tbg]

NB: probably need to tweak something in the example above to truly disable buffering, see ¹, but that isn't a problem with the PR.

Footnotes

1. https://cockroachlabs.slack.com/archives/C0168LW5THS/p1661984789936559 ↩

[pav-kv]

I wonder if this PR eliminates the need for the "special code" mentioned here:

cockroach/pkg/sql/consistencychecker/consistency_checker.go

Lines 47 to 48 in 544c6ae

	// NB: DistSender has special code to avoid parallelizing the request if
	// we're requesting CHECK_FULL.

Although the sub-requests sent in this PR still may end up cut in a few pieces by DistSender?

[tbg]

I wonder if this PR eliminates the need for the "special code" mentioned here:

In practice yes, but I don't want to back it out to account for cases where the generator reads a range as [a,z) but a rapid sequence of splits cuts it into [a,b), [b,c), ..., [y,z). In that case, we don't want DistSender to send dozens of parallel requests.

TFTR!

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

---

pkg/sql/sem/builtins/generator_builtins.go line 1903 at r1 (raw file):
Done. I don't think wrapping at 80 is worth review feedback in general, though. I do realize it's in our guidelines but I think that is broadly ill-advised. If wrapping is important, gofmt needs to do it; otherwise we should be lenient to reduce drag. (These are my opinions and there's probably someone out there who feels strongly the other way). I feel the same about harmless typos, punctuation, etc, though I have a harder time myself not pointing them out.

Will you do it in this PR? If not, add a TODO / link to an issue?

Good idea, filed this as a starter project for KV: #87503

---

pkg/sql/sem/builtins/generator_builtins.go line 1942 at r1 (raw file):

Previously, pavelkalinnikov (Pavel Kalinnikov) wrote…

How does this request behave if the start and end key get out of sync in presence of range splits/merges? Or, since it's in a Txn, we are fine?

Also, originally, how was this request decomposed into per-range requests? Is it by some magic of DistSender?

It looks like DistSender can't stream the result, hence we are here? But can we push this behaviour as low as that level, so that we don't need to manually deal with meta ranges up here?

DistSender doesn't support streaming (fixing this is a deep change, see kvstreamer, where we preferred bolting on vs rewriting DistSender), so cutting the request into pieces here is a pragmatic way to get it done. DistSender can split and recombine CheckConsistency though, so if we get it wrong here it'll just do that for us, so this will work even if the range boundaries change out from under us. What matters is that we read a consistent snapshot of the range descriptors, so we're going to cover the entire keyspace. I think we could get duplicate results if we have a range [a,c) and [c,d) but it really merged into [a,d) - in that case, we'd ask DistSender to scan the same range twice and I think it would produce two consistency checks for [a,d) in return. That is acceptable.

[pav-kv]

Mostly style nits. Address as many as you like :)

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

---

pkg/sql/sem/builtins/generator_builtins.go line 1837 at r4 (raw file):

	descs []roachpb.RangeDescriptor
	// remainingRows is populated by Next() call peels of the first

This sentence got corrupted, and there is no longer a curRow field mentioned in it, please fix.

---

pkg/sql/sem/builtins/generator_builtins.go line 1840 at r4 (raw file):

	// row and moves it to curRow. When empty, consumes from 'descs' to produce
	// more rows.
	remainingRows      []roachpb.CheckConsistencyResponse_Result

style nit / promoted content:

IMO in Go brevity is a feature, e.g. in naming. I would name this as rows or resps/results, for example. The "remaining" bit can be inferred from code/comments, and, besides, remainingRows now represents only a prefix of remaining rows to be fully precise (as opposed to, previously, the full set of them), so it's better to make the name purposefully vague than not exactly precise.

Same about the duration field below. Something like lastDur/requestDur/refillDur would do.

[pre-existing] consistencyChecker (note how it stands out from other short field names) I would name as checker/runner, if not ch or cc, or just embed it, as both "consistency" and "check" are inferred from its type and the wrapping type, plus this file is all about consistency checking anyway.

---

pkg/sql/sem/builtins/generator_builtins.go line 1917 at r4 (raw file):

			return err
		}
		if len(desc.StartKey) == 0 {

I don't think I follow this bit, but that's probably due to my limited knowledge about meta ranges for now. I trust this bit works, but just asking questions to understand it. If you think there is a simple/short way to reflect it in the code comments, feel free (e.g. link to some doc / package / file).

I gather that meta1/meta2 are levels of the 2-level "tree" of meta ranges (i.e. ranges that tell us what/where all ranges are). When the whole keyspace is small, we only have one meta1, and (by the looks of it from the comment) also one meta2 (as opposed to not having meta2 at all). Correct?

Can both meta1 and meta2 split, or one of them is always a singleton? It both can split, where is the "global" root of things, i.e. is there some meta0 too?

What is r1, and why can we get a second copy?

Code quote:

		if len(desc.StartKey) == 0 {
			desc.StartKey = keys.MustAddr(keys.LocalMax)
			// Elide potential second copy we might be getting for r1
			// if meta1 and meta2 haven't split.
			// This too should no longer be necessary with IterateRangeDescriptors.
			if len(c.descs) == 1 {
				continue
			}
		}

---

pkg/sql/sem/builtins/generator_builtins.go line 1939 at r4 (raw file):

// c.remainingRows with at least one element.
func (c *checkConsistencyGenerator) maybeRefillRows(ctx context.Context) {
	if len(c.descs) == 0 || len(c.remainingRows) > 0 {

supernit: this line breaks my logical flow a bit, I would swap it as len(c.remainingRows) > 0 || len(c.descs) == 0.
Logically: do I still have things in the current remainingRows? no? ok, then do I have things in c.descs (i.e. can I refill remainingRows)?

---

pkg/sql/sem/builtins/generator_builtins.go line 1943 at r4 (raw file):

		return
	}
	defer func(tBegin time.Time) {

nice pattern

Code quote:

	defer func(tBegin time.Time) {
		c.lastRefillDuration = timeutil.Since(tBegin)
	}(timeutil.Now())

---

pkg/sql/sem/builtins/generator_builtins.go line 1958 at r4 (raw file):

	if err != nil {
		// Emit result as a row, and keep going.
		c.remainingRows = []roachpb.CheckConsistencyResponse_Result{

reuse the old c.remainingRows to avoid an allocation? (size was > 0)

c.remainingRows = append(c.remainingRows[:0], roachpb.CheckConsistencyResponse_Result{
	...
})

Bonus: -1 indentation 😆

---

pkg/sql/sem/builtins/generator_builtins.go line 1969 at r4 (raw file):

	}

	// NB: this could have more than one entry, if a range split in the

Can this ever have 0 entries? If it can then iteration can end prematurely on the first block of size 0.

---

pkg/sql/sem/builtins/generator_builtins.go line 1976 at r4 (raw file):

// Next is part of the tree.ValueGenerator interface.
func (c *checkConsistencyGenerator) Next(ctx context.Context) (bool, error) {
	// Invariant: len(c.remainingRows) > 0.

This invariant is conditional to that this Next doesn't follow another Next that returned false (otherwise invariant does not hold, and this call panics). That's probably fine: the caller should use the type correctly.
Put differently: Next() == false is not idempotent.

In my original suggestion you would have a "truer" invariant though: remainingRows[0] is the previous entry (originally dummy), regardless of the misuse of the type.
Put differently: in my suggestion Next() == false is idempotent.
That's achieved by a len > 1 condition, which you have to do anyway in some form (before or after resizing the slice). So you get this property for free.

---

pkg/sql/sem/builtins/generator_builtins.go line 1978 at r4 (raw file):

	// Invariant: len(c.remainingRows) > 0.
	c.remainingRows = c.remainingRows[1:]
	c.maybeRefillRows(ctx) // if necessary, try to repopulate remainingRows

style nit: seems unnecessary to factor out a method, it's sufficiently small and would be understandable if inlined

---

pkg/sql/sem/builtins/generator_builtins.go line 1990 at r4 (raw file):

	}
	return tree.Datums{
		tree.NewDInt(tree.DInt(c.remainingRows[0].RangeID)),

style nit: consider avoiding the copy-paste of c.remainingRows[0] by introducing a local variable

[tbg]

TFTR! Could you give it another pass? I left the diff in a temp commit so you don't have to figure out what changed.

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

---

pkg/sql/sem/builtins/generator_builtins.go line 1837 at r4 (raw file):

Previously, pavelkalinnikov (Pavel Kalinnikov) wrote…

This sentence got corrupted, and there is no longer a curRow field mentioned in it, please fix.

Done.

---

pkg/sql/sem/builtins/generator_builtins.go line 1840 at r4 (raw file):

Previously, pavelkalinnikov (Pavel Kalinnikov) wrote…

style nit / promoted content:

IMO in Go brevity is a feature, e.g. in naming. I would name this as rows or resps/results, for example. The "remaining" bit can be inferred from code/comments, and, besides, remainingRows now represents only a prefix of remaining rows to be fully precise (as opposed to, previously, the full set of them), so it's better to make the name purposefully vague than not exactly precise.

Same about the duration field below. Something like lastDur/requestDur/refillDur would do.

[pre-existing] consistencyChecker (note how it stands out from other short field names) I would name as checker/runner, if not ch or cc, or just embed it, as both "consistency" and "check" are inferred from its type and the wrapping type, plus this file is all about consistency checking anyway.

I think this is verging into the territory of personal preference. I'm not a fan of unnecessary verbosity but I do think that overly terse variable naming may prevent others from easily absorbing the code.
I happened to rewrite some of this anyway following one of your other suggestions, and brought back a separate field to house the current row, so I took the opportunity to clear up the naming a bit and as a result they are now short.

---

pkg/sql/sem/builtins/generator_builtins.go line 1917 at r4 (raw file):

Previously, pavelkalinnikov (Pavel Kalinnikov) wrote…

I don't think I follow this bit, but that's probably due to my limited knowledge about meta ranges for now. I trust this bit works, but just asking questions to understand it. If you think there is a simple/short way to reflect it in the code comments, feel free (e.g. link to some doc / package / file).

I gather that meta1/meta2 are levels of the 2-level "tree" of meta ranges (i.e. ranges that tell us what/where all ranges are). When the whole keyspace is small, we only have one meta1, and (by the looks of it from the comment) also one meta2 (as opposed to not having meta2 at all). Correct?

Can both meta1 and meta2 split, or one of them is always a singleton? It both can split, where is the "global" root of things, i.e. is there some meta0 too?

What is r1, and why can we get a second copy?

The meta addressing is a true brain teaser and I'd have to re-explain it to myself first before I could write a convincing comment :laughcry:
But here's a paper trail:

cockroach/pkg/upgrade/upgradecluster/cluster.go

Lines 163 to 179 in 32622e1

	// In small enough clusters it's possible for the same range
	// descriptor to be stored in both meta1 and meta2. This
	// happens when some range spans both the meta and the user
	// keyspace. Consider when r1 is [/Min,
	// /System/NodeLiveness); we'll store the range descriptor
	// in both /Meta2/<r1.EndKey> and in /Meta1/KeyMax[1].
	//
	// As part of iterator we'll de-duplicate this descriptor
	// away by checking whether we've seen it before in meta1.
	// Since we're scanning over the meta range in sorted
	// order, it's enough to check against the last range
	// descriptor we've seen in meta1.
	//
	// [1]: See kvserver.rangeAddressing.
	if desc.RangeID == lastRangeIDInMeta1 {
	continue
	}

---

pkg/sql/sem/builtins/generator_builtins.go line 1939 at r4 (raw file):

Previously, pavelkalinnikov (Pavel Kalinnikov) wrote…

supernit: this line breaks my logical flow a bit, I would swap it as len(c.remainingRows) > 0 || len(c.descs) == 0.
Logically: do I still have things in the current remainingRows? no? ok, then do I have things in c.descs (i.e. can I refill remainingRows)?

Done.

---

pkg/sql/sem/builtins/generator_builtins.go line 1958 at r4 (raw file):

Previously, pavelkalinnikov (Pavel Kalinnikov) wrote…

reuse the old c.remainingRows to avoid an allocation? (size was > 0)

c.remainingRows = append(c.remainingRows[:0], roachpb.CheckConsistencyResponse_Result{
	...
})

Bonus: -1 indentation 😆

This has changed now as a byproduct of other changes, but just to respond to your suggestion, I generally try to stay away from trying to be even mildly clever about memory reuse in cases like this where it has no benefit. The difference in memory usage produces zero difference here, but the chance that I'll slip in a dumb bug is always nonzero, and every future reader will spend a little bit more brain juice convincing themselves that the reuse is correct; that is not worth it. Besides, the other, common, path doesn't (can't) reuse.

---

pkg/sql/sem/builtins/generator_builtins.go line 1969 at r4 (raw file):

Previously, pavelkalinnikov (Pavel Kalinnikov) wrote…

Can this ever have 0 entries? If it can then iteration can end prematurely on the first block of size 0.

No, a request always has at least one result.

---

pkg/sql/sem/builtins/generator_builtins.go line 1976 at r4 (raw file):

Previously, pavelkalinnikov (Pavel Kalinnikov) wrote…

This invariant is conditional to that this Next doesn't follow another Next that returned false (otherwise invariant does not hold, and this call panics). That's probably fine: the caller should use the type correctly.
Put differently: Next() == false is not idempotent.

In my original suggestion you would have a "truer" invariant though: remainingRows[0] is the previous entry (originally dummy), regardless of the misuse of the type.
Put differently: in my suggestion Next() == false is idempotent.
That's achieved by a len > 1 condition, which you have to do anyway in some form (before or after resizing the slice). So you get this property for free.

Hmm, good point, Next should be idempotent. I don't see anything in the contract that prevents the caller from invoking it again after a (false, nil) result.
Gave your idea another spin, but I think it is clearer to just bring curRow back (under a new, short, name 😄). Using a slice that is "empty" when its length is one is surprising. It came out better with an extra field.

---

pkg/sql/sem/builtins/generator_builtins.go line 1978 at r4 (raw file):

Previously, pavelkalinnikov (Pavel Kalinnikov) wrote…

style nit: seems unnecessary to factor out a method, it's sufficiently small and would be understandable if inlined

I think it is helpful to keep around as is. Inlining the method leads to more indentation (it will converge back towards the original pattern where all of the refill code goes into the conditional if <no more rows stashed> { ... }. The early-return in maybeRefillRows now takes care of avoiding that.
The flow is also clearer with this method on the side, now you don't have to worry about how new rows are produced to see how they flow out to Values via Next.

[pav-kv]

Still LGTM, with a few optional things.

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

---

pkg/sql/sem/builtins/generator_builtins.go line 1840 at r4 (raw file):

Previously, tbg (Tobias Grieger) wrote…

I think this is verging into the territory of personal preference. I'm not a fan of unnecessary verbosity but I do think that overly terse variable naming may prevent others from easily absorbing the code.
I happened to rewrite some of this anyway following one of your other suggestions, and brought back a separate field to house the current row, so I took the opportunity to clear up the naming a bit and as a result they are now short.

I agree, there is a balance to it. Thanks for addressing, looks stylish now 😆

---

pkg/sql/sem/builtins/generator_builtins.go line 1917 at r4 (raw file):

Previously, tbg (Tobias Grieger) wrote…

The meta addressing is a true brain teaser and I'd have to re-explain it to myself first before I could write a convincing comment :laughcry:
But here's a paper trail:

cockroach/pkg/upgrade/upgradecluster/cluster.go

Lines 163 to 179 in 32622e1

	// In small enough clusters it's possible for the same range
	// descriptor to be stored in both meta1 and meta2. This
	// happens when some range spans both the meta and the user
	// keyspace. Consider when r1 is [/Min,
	// /System/NodeLiveness); we'll store the range descriptor
	// in both /Meta2/<r1.EndKey> and in /Meta1/KeyMax[1].
	//
	// As part of iterator we'll de-duplicate this descriptor
	// away by checking whether we've seen it before in meta1.
	// Since we're scanning over the meta range in sorted
	// order, it's enough to check against the last range
	// descriptor we've seen in meta1.
	//
	// [1]: See kvserver.rangeAddressing.
	if desc.RangeID == lastRangeIDInMeta1 {
	continue
	}

Yeah, looks quite subtle. The code seems ok to me at this point.

Is it true that len(desc.StartKey) == 0 iff it's r1, i.e. it's the first range in the global keyspace? Why do we bump the StartKey though? Do we intentionally skip some part of keyspace by doing so?

---

pkg/sql/sem/builtins/generator_builtins.go line 1969 at r4 (raw file):

Previously, tbg (Tobias Grieger) wrote…

No, a request always has at least one result.

SGTM. Feel free to assert / return error otherwise though, if there is a non-intrusive way to do so.

---

pkg/sql/sem/builtins/generator_builtins.go line 1976 at r4 (raw file):

Previously, tbg (Tobias Grieger) wrote…

Hmm, good point, Next should be idempotent. I don't see anything in the contract that prevents the caller from invoking it again after a (false, nil) result.
Gave your idea another spin, but I think it is clearer to just bring curRow back (under a new, short, name 😄). Using a slice that is "empty" when its length is one is surprising. It came out better with an extra field.

Ack. Having or not having a separate cur is orthogonal to other aspects. It's either cur+next[] playing the role of one slice, or one slice next[] with 0-th element playing the role of cur. It just changes len > 0 checks to len > 1 or vice versa, but all other structural things are independently tweakable (like: in which order to do the checks to make things idempotent, or how to reduce nesting, etc). Agree that cur+next[] is slightly more human-readable.

E.g. my original proposal can be tweaked like this:

// No invariant needed. Returns false idempotently.
if len(c.next) != 0 {
	c.cur, c.next = c.next[0], c.next[1:]
	return true, nil
}
if len(c.descs) == 0 {
	return false, nil
}
// ... the unconditional refill here, no nesting, can call c.refill(ctx) or inline
c.cur, c.next = resp.Result[0], resp.Result[1:]
return true, nil

---

pkg/sql/sem/builtins/generator_builtins.go line 1978 at r4 (raw file):

Previously, tbg (Tobias Grieger) wrote…

I think it is helpful to keep around as is. Inlining the method leads to more indentation (it will converge back towards the original pattern where all of the refill code goes into the conditional if <no more rows stashed> { ... }. The early-return in maybeRefillRows now takes care of avoiding that.
The flow is also clearer with this method on the side, now you don't have to worry about how new rows are produced to see how they flow out to Values via Next.

It doesn't have to converge back to nested blocks etc, can be inlined pretty much verbatim. See another comment with the code snippet.

---

pkg/sql/sem/builtins/generator_builtins.go line 1961 at r8 (raw file):

		ctx, desc.StartKey.AsRawKey(), desc.EndKey.AsRawKey(), c.mode,
	)
	if err != nil {

In relation to the len(resp.Results) == 0 comment thread. If you'd like to protect from that case, could add a check like:

if err == nil && len(resp.Results) == 0 {
	err = <some-internal-error>
}

[tbg]

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

---

pkg/sql/sem/builtins/generator_builtins.go line 1917 at r4 (raw file):

Previously, pavelkalinnikov (Pavel Kalinnikov) wrote…

Yeah, looks quite subtle. The code seems ok to me at this point.

Is it true that len(desc.StartKey) == 0 iff it's r1, i.e. it's the first range in the global keyspace? Why do we bump the StartKey though? Do we intentionally skip some part of keyspace by doing so?

Yes, range one starts with the empty key. But we have a concept of local keys, which live in a parallel plane (like the range descriptor key) and start with some byte. Check https://github.com/cockroachdb/cockroach/blob/2675c7c0481bb33eb9f43f1c2b52c152fca3ae2d/pkg/keys/doc.go. I am so glad that stuff mostly works, it is quite intricate and really shouldn't ever rear its ugly head in any usage of KV like here. Glad you're digging in! These are some of the hard non-distributed bits of CRDB.

---

pkg/sql/sem/builtins/generator_builtins.go line 1976 at r4 (raw file):

Previously, pavelkalinnikov (Pavel Kalinnikov) wrote…

Ack. Having or not having a separate cur is orthogonal to other aspects. It's either cur+next[] playing the role of one slice, or one slice next[] with 0-th element playing the role of cur. It just changes len > 0 checks to len > 1 or vice versa, but all other structural things are independently tweakable (like: in which order to do the checks to make things idempotent, or how to reduce nesting, etc). Agree that cur+next[] is slightly more human-readable.

E.g. my original proposal can be tweaked like this:

// No invariant needed. Returns false idempotently.
if len(c.next) != 0 {
	c.cur, c.next = c.next[0], c.next[1:]
	return true, nil
}
if len(c.descs) == 0 {
	return false, nil
}
// ... the unconditional refill here, no nesting, can call c.refill(ctx) or inline
c.cur, c.next = resp.Result[0], resp.Result[1:]
return true, nil

Yeah, this trades the indentation for having two places where the array is sliced. Honestly, this is all comparable. I'm going to leave as is.

BTW it might be more productive, in the future, if you push a commit with your suggested refactor, then we don't need to tunnel it through a review which is a slow path for both of us. If you don't have it yet, gh makes this really easy: gh pr checkout <NNNN>; <edit>; git commit -m foo; git push. I've done this a few times before when reviewing and thought it worked out well.

Same for nits or small corrections (typos, etc), which cost a lot of time relative to the benefit they provide when they're communicated through github comments. At least I always prefer a few commits that I can then patch in if I can get them.

---

pkg/sql/sem/builtins/generator_builtins.go line 1961 at r8 (raw file):

Previously, pavelkalinnikov (Pavel Kalinnikov) wrote…

In relation to the len(resp.Results) == 0 comment thread. If you'd like to protect from that case, could add a check like:

if err == nil && len(resp.Results) == 0 {
	err = <some-internal-error>
}

I think this kind of assertion is taking it too far, since we'd need it like 1000x throughout the codebase. If anything we could bake it into the KV client (if it isn't there already) but either way introducing it in this PR isn't too useful.

[tbg]

bors r=pavelkalinnikov
TFTR!

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)

[blathers-crl]

Encountered an error creating backports. Some common things that can go wrong:

1. The backport branch might have already existed.
2. There was a merge conflict.
3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.

---

error creating merge commit from b52735c to blathers/backport-release-22.2-87378: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 22.2.x failed. See errors above.

---

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

[tbg]

blathers backport 22.2

[blathers-crl]

Encountered an error creating backports. Some common things that can go wrong:

1. The backport branch might have already existed.
2. There was a merge conflict.
3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.

---

error creating merge commit from b52735c to blathers/backport-release-22.2-87378: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 22.2 failed. See errors above.

---

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