kvstreamer: fix the usage of the range iterator#94031
kvstreamer: fix the usage of the range iterator#94031craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
This change is |
9660edd to
18bb9b6
Compare
DrewKimball
left a comment
There was a problem hiding this comment.
Reviewable status:
complete! 1 of 0 LGTMs obtained (waiting on @michae2)
6240542 to
5fabb7c
Compare
yuzefovich
left a comment
There was a problem hiding this comment.
We could change RangeIterator interface so that Seek would explicitly return an error instead of invalidating the iterator. However, that would change pretty-nice looking interface where it can be used with the for loop. I looked over how other callers do it, and the streamer is the only one that had an issue and was deviating from the iterator pattern around the for loop, so I just pushed a minor update to bring it more in line with others. PTAL.
Reviewable status:
5fabb7c to
90f6b0f
Compare
DrewKimball
left a comment
There was a problem hiding this comment.
Reviewed all commit messages.
Reviewable status:
pkg/kv/kvclient/kvstreamer/streamer.go line 635 at r2 (raw file):
// This was the last range. Breaking here rather than Seek'ing the // iterator to RKeyMax (and, thus, invalidating it) allows us to // avoid adding a confusingly-looking message into the trace.
[nit] confusingly-looking -> confusing
Previously, after `Seek`ing the range iterator to the next key in the batch of requests in the streamer we forgot to check the validity of the iterator. In particular, this could lead to a crash of the process if `Seek` encountered an error for whatever reason. In practice, I've only observed this when running TPCH with high concurrency when GOMEMLIMIT is set. The bug was introduced in an innocently-looking refactor in 041b104. Epic: None Release note (bug fix): CockroachDB could previously crash in rare circumstances when evaluating lookup and index joins. The bug is present since 22.2.0 release. Temporary workaround without upgrading to the release with this fix is changing the value of undocumented cluster setting `sql.distsql.use_streamer.enabled` to `false`.
90f6b0f to
3125bc7
Compare
yuzefovich
left a comment
There was a problem hiding this comment.
TFTR!
bors r+
Reviewable status:
pkg/kv/kvclient/kvstreamer/streamer.go line 635 at r2 (raw file):
Previously, DrewKimball (Drew Kimball) wrote…
[nit]
confusingly-looking->confusing
Done.
|
Build succeeded: |
Previously, after
Seeking the range iterator to the next key in the batch of requests in the streamer we forgot to check the validity of the iterator. In particular, this could lead to a crash of the process ifSeekencountered an error for whatever reason. In practice, I've only observed this when running TPCH with high concurrency when GOMEMLIMIT is set.The bug was introduced in an innocently-looking refactor in 041b104.
Epic: None
Release note (bug fix): CockroachDB could previously crash in rare circumstances when evaluating lookup and index joins. The bug is present since 22.2.0 release. Temporary workaround without upgrading to the release with this fix is changing the value of undocumented cluster setting
sql.distsql.use_streamer.enabledtofalse.