From 619f395b88e7089d12d7c4a44eaf409507d7f42c Mon Sep 17 00:00:00 2001 From: Yahor Yuzefovich Date: Wed, 1 Dec 2021 14:26:33 -0800 Subject: [PATCH] roachpb: treat GetRequests as non-directional in BatchRequest.Split This commit updates `BatchRequest.Split` to treat `GetRequest`s as non-directional so that batches with Gets and ReverseScans are not split. Without this fix currently when reading from a secondary index in the reverse direction, we might crash. This was exposed recently by issueing the GetRequests for indexes without STORING clause, but it is present on 21.2 for index with STORING clause. Release note (bug fix): Previously, CockroachDB could crash when reading of a secondary index with STORING clause in reverse direction (because of ORDER BY col DESC). The bug was introduced in 21.2. --- pkg/roachpb/batch.go | 17 ++++++++++++++++- pkg/roachpb/batch_test.go | 3 +++ pkg/sql/logictest/testdata/logic_test/sqllite | 13 +++++++++++++ 3 files changed, 32 insertions(+), 1 deletion(-) diff --git a/pkg/roachpb/batch.go b/pkg/roachpb/batch.go index 4a4a5aa8073f..93b034ffa044 100644 --- a/pkg/roachpb/batch.go +++ b/pkg/roachpb/batch.go @@ -558,7 +558,22 @@ func (ba BatchRequest) Split(canSplitET bool) [][]RequestUnion { // enforcing are that a batch can't mix non-writes with writes. // Checking isRead would cause ConditionalPut and Put to conflict, // which is not what we want. - const mask = isWrite | isAdmin | isReverse + mask := isWrite | isAdmin + if (exFlags&isRange) != 0 && (newFlags&isRange) != 0 { + // The directions of requests in a batch need to be the same because + // the DistSender (in divideAndSendBatchToRanges) will perform a + // single traversal of all requests while advancing the + // RangeIterator. If we were to have requests with different + // directions in a single batch, the DistSender wouldn't know how to + // route the requests (without incurring a noticeable performance + // hit). + // + // At the same time, non-ranged operations are not directional, so + // we need to require the same value for isReverse flag iff the + // existing and the new requests are ranged. For example, this + // allows us to have Gets and ReverseScans in a single batch. + mask |= isReverse + } return (mask & exFlags) == (mask & newFlags) } var parts [][]RequestUnion diff --git a/pkg/roachpb/batch_test.go b/pkg/roachpb/batch_test.go index 02b7c6d03196..bf0ceaa34715 100644 --- a/pkg/roachpb/batch_test.go +++ b/pkg/roachpb/batch_test.go @@ -93,6 +93,9 @@ func TestBatchSplit(t *testing.T) { {[]Request{get, get, get, put, put, get, get}, []int{3, 2, 2}, true}, {[]Request{spl, get, scan, spl, get}, []int{1, 2, 1, 1}, true}, {[]Request{spl, spl, get, spl}, []int{1, 1, 1, 1}, true}, + {[]Request{scan, get, scan, get}, []int{4}, true}, + {[]Request{rv, get, rv, get}, []int{4}, true}, + {[]Request{scan, get, rv, get}, []int{2, 2}, true}, {[]Request{get, scan, get, dr, rv, put, et}, []int{3, 1, 1, 1, 1}, true}, // Same one again, but this time don't allow EndTxn to be split. {[]Request{get, scan, get, dr, rv, put, et}, []int{3, 1, 1, 2}, false}, diff --git a/pkg/sql/logictest/testdata/logic_test/sqllite b/pkg/sql/logictest/testdata/logic_test/sqllite index 0510867b41c5..87770db41e86 100644 --- a/pkg/sql/logictest/testdata/logic_test/sqllite +++ b/pkg/sql/logictest/testdata/logic_test/sqllite @@ -449,3 +449,16 @@ SELECT pk, col0 FROM tab64784 WHERE (col0 IN (SELECT col3 FROM tab64784 WHERE co 4 216 1 213 0 212 + +# Regression test for crashing when intertwining GetRequests and +# ReverseScanRequests (which was mistakenly considered illegal). +statement ok +CREATE TABLE t73103(pk INT PRIMARY KEY, col0 INT, col1 INT, UNIQUE INDEX idx (col0), UNIQUE INDEX idx_storing (col0) STORING (col1)); + +query II +SELECT pk, col0 FROM t73103@idx WHERE col0 <= 63 OR col0 IN (27,11,93,41) ORDER BY 2 DESC +---- + +query II +SELECT pk, col0 FROM t73103@idx_storing WHERE col0 <= 63 OR col0 IN (27,11,93,41) ORDER BY 2 DESC +----