db: avoid mutex-protected I/O in Ingest and NewEventuallyFileOnlySnapshot

[jbowens]

Creation of an eventually file-only snapshot requires inspecting the flushable queue to look for overlap with the key ranges of the new snapshot. This is performed while holding the DB mutex to avoid racing with other goroutines that may mutate the flushable queue. Previously, if ingested sstables were queued as flushable ingests, checking for overlap could perform read I/O while holding the database mutex.

Similarly, ingestion requires determining which ingested sstables overlap any flushables. This is used for correctness (to ensure we sequence the sstable higher in the LSM than any overlapping flushables) and for ingest statistics that we return to the caller. Previously, if ingested sstables were already in the queue as flushable ingests, checking for overlap could perform read I/O while holding both the database mutex and the commit pipeline mutex.

This commit refactors these overlap checks to use a new computePossibleOverlaps method of the flushable interface. This allows the ingestedFlushable implementation to avoid I/O and determine overlap using simple file boundary comparisons. This required some gymnastics to support the IngestAndExcise case that requires knowledge of which files overlapped flushables and must detect overlaps with both ingested sstables and an excise span.

[cockroach-teamcity]

This change is 

[jbowens]

I'm thinking a bit about how to test.

Reviewable status: 0 of 9 files reviewed, all discussions resolved (waiting on @sumeerbhola)

[itsbilal]

LGTM, thanks for doing this!

[itsbilal]

(not relevant to this PR so feel free to ignore) this seems like an abuse of a FileMetadata. Maybe we should use KeyRange instead?

[sumeerbhola]

very minor comments only

Reviewed 4 of 9 files at r1, 3 of 4 files at r2, all commit messages.
Reviewable status: 7 of 9 files reviewed, 7 unresolved discussions (waiting on @itsbilal and @jbowens)

---

flushable.go line 45 at r2 (raw file):

type bounded interface {
	InternalKeyBounds() (InternalKey, InternalKey)

can you add a comment that these are both inclusive.

---

flushable.go line 303 at r2 (raw file):

// computePossibleOverlapsGenericImpl is an implemention of the flushable
// interface's computePossibleOverlaps function for flushable implementations

nit: perhaps qualify as "... flushable implementations with only in-memory state that do not ..."

---

flushable.go line 319 at r2 (raw file):

		kr := internalKeyRange{s, l}
		if overlapWithIterator(iter, &rangeDelIter, rkeyIter, kr, cmp) {
			fn(b)

its a bit odd that this interface does not tell the callee whether the caller has ensured that there is overlap (like this case), or that this could be a false positive. I suppose that is because the current callees never try to eliminate false positives?

---

ingest.go line 1352 at r2 (raw file):

	}

	// metaFlushableOverlaps is a slice parallel to meta indicating which of the

stale comment (no longer a slice)

---

ingest.go line 1396 at r2 (raw file):

				switch v := b.(type) {
				case *fileMetadata:
					metaFlushableOverlaps[v.FileNum] = true

so are we accepting some false positives?
And is this considered ok since overlap between an ingested flushable (caused by addsstable or range snapshot) and a new ingest will likely be a true positive (in the CRDB context)?

---

snapshot.go line 287 at r2 (raw file):

	for i := range d.mu.mem.queue {
		d.mu.mem.queue[i].computePossibleOverlaps(func(bounded) {
			isFileOnly = false

should there be a bool return value from this func to avoid continuing to check the remaining key ranges?

[jbowens]

Reviewable status: 1 of 9 files reviewed, 6 unresolved discussions (waiting on @itsbilal and @sumeerbhola)

---

db.go line 1746 at r1 (raw file):

Previously, itsbilal (Bilal Akhtar) wrote…

(not relevant to this PR so feel free to ignore) this seems like an abuse of a FileMetadata. Maybe we should use KeyRange instead?

Done.

---

flushable.go line 45 at r2 (raw file):

Previously, sumeerbhola wrote…

can you add a comment that these are both inclusive.

Done.

---

flushable.go line 303 at r2 (raw file):

Previously, sumeerbhola wrote…

nit: perhaps qualify as "... flushable implementations with only in-memory state that do not ..."

Done.

---

flushable.go line 319 at r2 (raw file):

Previously, sumeerbhola wrote…

its a bit odd that this interface does not tell the callee whether the caller has ensured that there is overlap (like this case), or that this could be a false positive. I suppose that is because the current callees never try to eliminate false positives?

Yeah, that's right

---

ingest.go line 1352 at r2 (raw file):

Previously, sumeerbhola wrote…

stale comment (no longer a slice)

Done.

---

ingest.go line 1396 at r2 (raw file):

Previously, sumeerbhola wrote…

so are we accepting some false positives?
And is this considered ok since overlap between an ingested flushable (caused by addsstable or range snapshot) and a new ingest will likely be a true positive (in the CRDB context)?

Yeah, that's right—also even if it were a true false positive, it's unclear which is more disruptive: blocking all commits to determine whether there's overlap, or the cost of handling this ingestion as a flushable ingest. Added some of this in a comment here.

---

snapshot.go line 287 at r2 (raw file):

Previously, sumeerbhola wrote…

should there be a bool return value from this func to avoid continuing to check the remaining key ranges?

added a shouldContinue enum return value to control it

[sumeerbhola]

Reviewed 1 of 9 files at r1, 7 of 7 files at r3, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @itsbilal and @jbowens)

---

flushable.go line 48 at r3 (raw file):

const (
	continueIteration shouldContinue = true
	stopIteration

https://go.dev/play/p/NA16uxyR-ug claims this will also be true. Even if correct, relying on some default here is confusing (at least to me). If incorrect, I wonder why this didn't trip up a unit test.

[jbowens]

Reviewable status: 8 of 11 files reviewed, 7 unresolved discussions (waiting on @itsbilal and @sumeerbhola)

---

flushable.go line 48 at r3 (raw file):

Previously, sumeerbhola wrote…

https://go.dev/play/p/NA16uxyR-ug claims this will also be true. Even if correct, relying on some default here is confusing (at least to me). If incorrect, I wonder why this didn't trip up a unit test.

Oof, good catch. Yeah, this was a bug. It wasn't caught by any unit tests because continuing ignoring a stopIteration value is okay in all use cases, just less efficient. I fixed this and added a memtable unit test that could've caught it.

[sumeerbhola]

Reviewed 3 of 3 files at r5, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @itsbilal)

[jbowens]

TFTRs!