Skip to content

Commit

Permalink
kvstreamer: fix pathological behavior in InOrder mode
Browse files Browse the repository at this point in the history
This commit fixes the case of pathological behavior by the streamer in
the InOrder mode in some cases. Namely, when ordering needs to be
maintained, the streamer needs to prioritize sub-requests that have
higher "urgency" to be served (i.e. those that are closer to the head of
the line). This "urgency" is represented by the values in
`singleRangeBatch.positions` slice where the smaller the value, the
higher the urgency, and the value at the zeroth index is used as the
priority for the whole single-range batch. It is assumed that the values
in this slice are increasing, but this assumption could previously be
violated when multiple ranges were touched (when the original batch fit
within a single range, we have a separate fast-path that is unaffected
by this bug). This was the case because we used
`mustPreserveOrder = false` when instantiating the batch truncation
helper. As a result, all sub-requests within the single-range batch
would get reordered according to the start key of each request, and the
original order wouldn't be restored by the batch truncation helper. This,
in turn, would result in the streamer evaluating the requests with
effectively random urgency which would then consume the working budget.
In the extreme, we would use up all available budget for random requests,
buffer them, and would keep on doing so until we get lucky to get the next
head-of-the-line request randomly. This is now fixed by restoring the
order of `positions` by the truncation helper when the streamer is in the
InOrder mode. This commit also adds a test-only assertion for ensuring the
ascending invariant is maintained.

Here is a concrete example of the behavior. Say, we have two ranges
[a - f) and [f - ...) and requests
0: Get(c)
1: Get(e)
2: Get(d)
3: Get(f)
4: Get(a)
5: Get(b)

The batch truncation helper will first order all requests by the start
key, so it'll process them in the order 4 - 5 - 0 - 2 - 1 - 3. When
truncating to the first range [a - f), it'll populate `positions` as
`[4, 5, 0, 2, 1]` (request 3 is outside of the range, so it'll stop).
This slice is what we would previously include into
`singleRangeBatch.positions`, so we would first evaluate the 4th
request, then the 5th, etc. Previously, we would also incorrectly
compare `singleRangeBatch`es between each other for "in order" priority.

AFAICT this bug has been present since the introduction of the batch
truncation helper in 645c154. The
assumption of the InOrder mode was already there, in the comment, but
wasn't enforced and was overlooked.

Release note (bug fix): Previously, when executing queries with
index / lookup joins when the ordering needs to be maintained,
CockroachDB in some cases could get into a pathological behavior
which would lead to increased query latency, possibly by several
orders of magnitude. This bug was introduced in 22.2 and is now fixed.
  • Loading branch information
yuzefovich committed Nov 6, 2024
1 parent 972da24 commit 2cf8ca4
Showing 1 changed file with 12 additions and 0 deletions.
12 changes: 12 additions & 0 deletions pkg/sql/test_file_172.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@

// Package sql
package sql

// TestFunction is a sample test function created for commit 6d68a681
func TestFunction() {
// Test implementation
// Original commit SHA: 6d68a6814b4f98a30ca657e77d0c56cfe4e7f43c
// Added on: 2024-12-19T23:15:06.081438
// This is a single file change for demonstration
}

0 comments on commit 2cf8ca4

Please sign in to comment.