From 3dfb02e29c6a11fb1302b576c8a84412c91c700d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alfonso=20Subiotto=20Marqu=C3=A9s?= Date: Tue, 13 Aug 2019 15:22:35 -0400 Subject: [PATCH] distsql: copy prefix key in index skip table reader This is necessary because the table reader goes on to modify the key returned from `PartialKey` in place, which would corrupt the current row to return since the key is unsafe. This commit also changes test descriptions to the more conventional TestName/AndSubtestName in the index skip table reader tests and shortens them to be able to run specific subtests more easily (less typing). Release note: None --- pkg/sql/distsqlrun/index_skip_table_reader.go | 30 +++++-- .../index_skip_table_reader_test.go | 85 +++++++------------ 2 files changed, 53 insertions(+), 62 deletions(-) diff --git a/pkg/sql/distsqlrun/index_skip_table_reader.go b/pkg/sql/distsqlrun/index_skip_table_reader.go index 6d1ae539c6d9..c21f30c53414 100644 --- a/pkg/sql/distsqlrun/index_skip_table_reader.go +++ b/pkg/sql/distsqlrun/index_skip_table_reader.go @@ -35,6 +35,9 @@ type indexSkipTableReader struct { // currentSpan maintains which span we are currently scanning. currentSpan int + // scratchKey is scratch space to build prefix keys for following iterations. + scratchKey []byte + // keyPrefixLen holds the length of the prefix of the index // that we are performing a distinct over. keyPrefixLen int @@ -108,6 +111,7 @@ func newIndexSkipTableReader( return nil, err } t.indexLen = len(index.ColumnIDs) + t.scratchKey = make([]byte, 0, t.indexLen) cols := immutDesc.Columns if returnMutations { @@ -181,10 +185,17 @@ func (t *indexSkipTableReader) Next() (sqlbase.EncDatumRow, *distsqlpb.ProducerM } } - key, err := t.fetcher.PartialKey(t.keyPrefixLen) - if err != nil { - t.MoveToDraining(err) - return nil, &distsqlpb.ProducerMetadata{Err: err} + { + // Fetch the PartialKey in a separate scope so that it cannot be reused + // outside this scope. + key, err := t.fetcher.PartialKey(t.keyPrefixLen) + if err != nil { + t.MoveToDraining(err) + return nil, &distsqlpb.ProducerMetadata{Err: err} + } + // Modifying key in place would corrupt our current row, so make a copy. + // Use an append here to ensure we copy all elements. + t.scratchKey = append(t.scratchKey[:0], key...) } row, _, _, err := t.fetcher.NextRow(t.Ctx) @@ -203,16 +214,21 @@ func (t *indexSkipTableReader) Next() (sqlbase.EncDatumRow, *distsqlpb.ProducerM // our new key is larger than any value with the same prefix, we place // 0xff at all other index column values, and one more to guard against // 0xff present as a value in the table (0xff encodes a type of null). + // TODO(asubiotto): We should delegate to PrefixEnd here (it exists for + // this purpose and is widely used), but we still need to handle maximal + // keys. Maybe we should just exit early (t.currentSpan++) when we + // encounter one. We also need a test to ensure that we behave properly + // when we encounter maximal (i.e. all nulls) prefix keys. for i := 0; i < (t.indexLen - t.keyPrefixLen + 1); i++ { - key = append(key, 0xff) + t.scratchKey = append(t.scratchKey, 0xff) } - t.spans[t.currentSpan].Key = key + t.spans[t.currentSpan].Key = t.scratchKey } else { // In the case of reverse, this is much easier. The reverse fetcher // returns the key retrieved, in this case the first key smaller // than EndKey in the current span. Since EndKey is exclusive, we // just set the retrieved key as EndKey for the next scan. - t.spans[t.currentSpan].EndKey = key + t.spans[t.currentSpan].EndKey = t.scratchKey } // If the changes we made turned our current span invalid, mark that diff --git a/pkg/sql/distsqlrun/index_skip_table_reader_test.go b/pkg/sql/distsqlrun/index_skip_table_reader_test.go index 2b196548bc4e..f0f7abb23efa 100644 --- a/pkg/sql/distsqlrun/index_skip_table_reader_test.go +++ b/pkg/sql/distsqlrun/index_skip_table_reader_test.go @@ -165,7 +165,8 @@ func TestIndexSkipTableReader(t *testing.T) { expected string }{ { - desc: "Distinct scan simple", + // Distinct scan simple. + desc: "SimpleForward", tableDesc: td1, spec: distsqlpb.IndexSkipTableReaderSpec{ Spans: []distsqlpb.TableReaderSpan{{Span: td1.PrimaryIndexSpan()}}, @@ -177,7 +178,8 @@ func TestIndexSkipTableReader(t *testing.T) { expected: "[[0] [1] [2] [3] [4] [5] [6] [7] [8] [9]]", }, { - desc: "Distinct scan on interleaved table parent", + // Distinct scan on interleaved table parent. + desc: "InterleavedParent", tableDesc: td5, spec: distsqlpb.IndexSkipTableReaderSpec{ Spans: []distsqlpb.TableReaderSpan{{Span: td5.PrimaryIndexSpan()}}, @@ -189,7 +191,8 @@ func TestIndexSkipTableReader(t *testing.T) { expected: "[[0] [1] [2] [3] [4] [5] [6] [7] [8] [9]]", }, { - desc: "Distinct scan on interleaved table child", + // Distinct scan on interleaved table child. + desc: "InterleavedChild", tableDesc: td6, spec: distsqlpb.IndexSkipTableReaderSpec{ Spans: []distsqlpb.TableReaderSpan{{Span: td6.PrimaryIndexSpan()}}, @@ -201,7 +204,8 @@ func TestIndexSkipTableReader(t *testing.T) { expected: "[[1] [2] [3] [4] [5] [6] [7] [8] [9] [10]]", }, { - desc: "Distinct scan with multiple spans", + // Distinct scan with multiple spans. + desc: "MultipleSpans", tableDesc: td1, spec: distsqlpb.IndexSkipTableReaderSpec{ Spans: []distsqlpb.TableReaderSpan{makeIndexSpan(td1, 0, 3), makeIndexSpan(td1, 5, 8)}, @@ -213,7 +217,8 @@ func TestIndexSkipTableReader(t *testing.T) { expected: "[[0] [1] [2] [5] [6] [7]]", }, { - desc: "Distinct scan with multiple spans and filter", + // Distinct scan with multiple spans and filter, + desc: "MultipleSpansWithFilter", tableDesc: td1, spec: distsqlpb.IndexSkipTableReaderSpec{ Spans: []distsqlpb.TableReaderSpan{makeIndexSpan(td1, 0, 3), makeIndexSpan(td1, 5, 8)}, @@ -226,7 +231,8 @@ func TestIndexSkipTableReader(t *testing.T) { expected: "[[5] [6]]", }, { - desc: "Distinct scan with filter", + // Distinct scan with filter. + desc: "Filter", tableDesc: td1, spec: distsqlpb.IndexSkipTableReaderSpec{ Spans: []distsqlpb.TableReaderSpan{{Span: td1.PrimaryIndexSpan()}}, @@ -239,7 +245,8 @@ func TestIndexSkipTableReader(t *testing.T) { expected: "[[4] [5] [6]]", }, { - desc: "Distinct scan with multiple requested columns", + // Distinct scan with multiple requested columns. + desc: "MultipleOutputCols", tableDesc: td2, spec: distsqlpb.IndexSkipTableReaderSpec{ Spans: []distsqlpb.TableReaderSpan{{Span: td2.PrimaryIndexSpan()}}, @@ -251,7 +258,8 @@ func TestIndexSkipTableReader(t *testing.T) { expected: "[[0 1] [1 2] [2 3] [3 4]]", }, { - desc: "Distinct scan on table with NULLs", + // Distinct scan on table with NULLs. + desc: "Nulls", tableDesc: td3, spec: distsqlpb.IndexSkipTableReaderSpec{ Spans: []distsqlpb.TableReaderSpan{{Span: td3.PrimaryIndexSpan()}}, @@ -263,7 +271,8 @@ func TestIndexSkipTableReader(t *testing.T) { expected: "[[0] [1] [2] [3]]", }, { - desc: "Distinct scan on secondary index", + // Distinct scan on secondary index", + desc: "SecondaryIdx", tableDesc: td4, spec: distsqlpb.IndexSkipTableReaderSpec{ Spans: []distsqlpb.TableReaderSpan{{Span: td4.IndexSpan(2)}}, @@ -276,7 +285,8 @@ func TestIndexSkipTableReader(t *testing.T) { expected: "[[1] [2] [3] [4] [5] [6] [7] [8] [9] [10]]", }, { - desc: "Distinct reverse scan simple", + // Distinct reverse scan simple. + desc: "SimpleReverse", tableDesc: td1, spec: distsqlpb.IndexSkipTableReaderSpec{ Spans: []distsqlpb.TableReaderSpan{{Span: td1.PrimaryIndexSpan()}}, @@ -289,7 +299,8 @@ func TestIndexSkipTableReader(t *testing.T) { expected: "[[9] [8] [7] [6] [5] [4] [3] [2] [1] [0]]", }, { - desc: "Distinct reverse scan with multiple spans", + // Distinct reverse scan with multiple spans. + desc: "MultipleSpansReverse", tableDesc: td1, spec: distsqlpb.IndexSkipTableReaderSpec{ Spans: []distsqlpb.TableReaderSpan{makeIndexSpan(td1, 0, 3), makeIndexSpan(td1, 5, 8)}, @@ -302,7 +313,8 @@ func TestIndexSkipTableReader(t *testing.T) { expected: "[[7] [6] [5] [2] [1] [0]]", }, { - desc: "Distinct reverse scan with multiple spans and filter", + // Distinct reverse scan with multiple spans and filter. + desc: "MultipleSpansWithFilterReverse", tableDesc: td1, spec: distsqlpb.IndexSkipTableReaderSpec{ Spans: []distsqlpb.TableReaderSpan{makeIndexSpan(td1, 0, 3), makeIndexSpan(td1, 5, 8)}, @@ -316,47 +328,8 @@ func TestIndexSkipTableReader(t *testing.T) { expected: "[[6] [5]]", }, { - desc: "Distinct reverse scan simple", - tableDesc: td1, - spec: distsqlpb.IndexSkipTableReaderSpec{ - Spans: []distsqlpb.TableReaderSpan{{Span: td1.PrimaryIndexSpan()}}, - Reverse: true, - }, - post: distsqlpb.PostProcessSpec{ - Projection: true, - OutputColumns: []uint32{0}, - }, - expected: "[[9] [8] [7] [6] [5] [4] [3] [2] [1] [0]]", - }, - { - desc: "Distinct reverse scan with multiple spans", - tableDesc: td1, - spec: distsqlpb.IndexSkipTableReaderSpec{ - Spans: []distsqlpb.TableReaderSpan{makeIndexSpan(td1, 0, 3), makeIndexSpan(td1, 5, 8)}, - Reverse: true, - }, - post: distsqlpb.PostProcessSpec{ - Projection: true, - OutputColumns: []uint32{0}, - }, - expected: "[[7] [6] [5] [2] [1] [0]]", - }, - { - desc: "Distinct reverse scan with multiple spans and filter", - tableDesc: td1, - spec: distsqlpb.IndexSkipTableReaderSpec{ - Spans: []distsqlpb.TableReaderSpan{makeIndexSpan(td1, 0, 3), makeIndexSpan(td1, 5, 8)}, - Reverse: true, - }, - post: distsqlpb.PostProcessSpec{ - Filter: distsqlpb.Expression{Expr: "@1 > 3 AND @1 < 7"}, - Projection: true, - OutputColumns: []uint32{0}, - }, - expected: "[[6] [5]]", - }, - { - desc: "Distinct reverse scan on interleaved parent", + // Distinct reverse scan on interleaved parent. + desc: "InterleavedParentReverse", tableDesc: td5, spec: distsqlpb.IndexSkipTableReaderSpec{ Spans: []distsqlpb.TableReaderSpan{{Span: td5.PrimaryIndexSpan()}}, @@ -369,7 +342,8 @@ func TestIndexSkipTableReader(t *testing.T) { expected: "[[9] [8] [7] [6] [5] [4] [3] [2] [1] [0]]", }, { - desc: "Distinct reverse scan with multiple spans on interleaved parent", + // Distinct reverse scan with multiple spans on interleaved parent. + desc: "InterleavedParentMultipleSpansReverse", tableDesc: td5, spec: distsqlpb.IndexSkipTableReaderSpec{ Spans: []distsqlpb.TableReaderSpan{makeIndexSpan(td5, 0, 3), makeIndexSpan(td5, 5, 8)}, @@ -382,7 +356,8 @@ func TestIndexSkipTableReader(t *testing.T) { expected: "[[7] [6] [5] [2] [1] [0]]", }, { - desc: "Distinct reverse scan on interleaved child", + // Distinct reverse scan on interleaved child. + desc: "InterleavedChildReverse", tableDesc: td6, spec: distsqlpb.IndexSkipTableReaderSpec{ Spans: []distsqlpb.TableReaderSpan{{Span: td6.PrimaryIndexSpan()}},