kvstreamer: fix the usage of the range iterator #94031
Conversation
|
This change is |
yuzefovich
force-pushed
the
fix-streamer
branch
from
9660edd to
18bb9b6
Compare
There was a problem hiding this comment.
Reviewable status:
complete! 1 of 0 LGTMs obtained (waiting on @michae2)
yuzefovich
force-pushed
the
fix-streamer
branch
2 times, most recently
from
6240542 to
5fabb7c
Compare
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:
yuzefovich
force-pushed
the
fix-streamer
branch
from
5fabb7c to
90f6b0f
Compare
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`.
yuzefovich
force-pushed
the
fix-streamer
branch
from
90f6b0f to
3125bc7
Compare
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.