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

@@ -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

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},

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
+----