Skip to content

Commit

Permalink
Merge #66845 #66885
Browse files Browse the repository at this point in the history 
66845: storage: add Reader method to pin iterators at current engine state r=sumeerbhola a=sumeerbhola

This is relevant for read evaluation cases where we want to release
latches before evaluation. The new Reader.PinEngineStateForIterators
method would be called before releasing latches.

This pinning does not apply to iterators with timestamp hints,
similar to how ConsistentIterators does not apply to them. So
this does not help ExportRequest evaluation for incremental backups.
To address this we would want a guarantee from the caller that the
timestamp hints will not change for the lifetime of the Reader that
is making a promise of ConsistentIterators.

Informs #55461,#66485

Release note: None

66885: sql: add ReType to resolveAndRequireType to fix vector engine panic r=cucaroach a=cucaroach

Fixes #66708

The vector engine needs exact type coercion, specifically when piping
computed column values into downstream operators.  Without this fix
the computed column was left as an int64 instead of cast back to the
required int16 type.

Exposed by sqlsmith, kudos to @michae2 for the reduce help

Release note: None

Co-authored-by: sumeerbhola <[email protected]>
Co-authored-by: Tommy Reilly <[email protected]>
  • Loading branch information
@sumeerbhola @cucaroach
3 people committed Jun 28, 2021
3 parents 3e90db4 + e228cf5 + 394c407 commit 858fc12
Show file tree
Hide file tree
Showing 12 changed files with 185 additions and 64 deletions.
16 changes: 11 additions & 5 deletions pkg/kv/kvserver/spanset/batch.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -402,7 +402,7 @@ func (s spanSetReader) Closed() bool {
return s.r.Closed()
}

// ExportMVCCToSst is part of the engine.Reader interface.
// ExportMVCCToSst is part of the storage.Reader interface.
func (s spanSetReader) ExportMVCCToSst(
startKey, endKey roachpb.Key,
startTS, endTS hlc.Timestamp,
Expand Down Expand Up @@ -479,10 +479,16 @@ func (s spanSetReader) NewEngineIterator(opts storage.IterOptions) storage.Engin
}
}

// ConsistentIterators implements the storage.Reader interface.
func (s spanSetReader) ConsistentIterators() bool {
return s.r.ConsistentIterators()
}

// PinEngineStateForIterators implements the storage.Reader interface.
func (s spanSetReader) PinEngineStateForIterators() error {
return s.r.PinEngineStateForIterators()
}

// GetDBEngine recursively searches for the underlying rocksDB engine.
func GetDBEngine(reader storage.Reader, span roachpb.Span) storage.Reader {
switch v := reader.(type) {
Expand All @@ -495,7 +501,7 @@ func GetDBEngine(reader storage.Reader, span roachpb.Span) storage.Reader {
}
}

// getSpanReader is a getter to access the engine.Reader field of the
// getSpanReader is a getter to access the storage.Reader field of the
// spansetReader.
func getSpanReader(r ReadWriter, span roachpb.Span) storage.Reader {
if err := r.spanSetReader.spans.CheckAllowed(SpanReadOnly, span); err != nil {
Expand Down Expand Up @@ -700,7 +706,7 @@ func makeSpanSetReadWriterAt(rw storage.ReadWriter, spans *SpanSet, ts hlc.Times
}
}

// NewReadWriterAt returns an engine.ReadWriter that asserts access of the
// NewReadWriterAt returns a storage.ReadWriter that asserts access of the
// underlying ReadWriter against the given SpanSet at a given timestamp.
// If zero timestamp is provided, accesses are considered non-MVCC.
func NewReadWriterAt(rw storage.ReadWriter, spans *SpanSet, ts hlc.Timestamp) storage.ReadWriter {
Expand Down Expand Up @@ -734,7 +740,7 @@ func (s spanSetBatch) Repr() []byte {
return s.b.Repr()
}

// NewBatch returns an engine.Batch that asserts access of the underlying
// NewBatch returns a storage.Batch that asserts access of the underlying
// Batch against the given SpanSet. We only consider span boundaries, associated
// timestamps are not considered.
func NewBatch(b storage.Batch, spans *SpanSet) storage.Batch {
Expand All @@ -746,7 +752,7 @@ func NewBatch(b storage.Batch, spans *SpanSet) storage.Batch {
}
}

// NewBatchAt returns an engine.Batch that asserts access of the underlying
// NewBatchAt returns an storage.Batch that asserts access of the underlying
// Batch against the given SpanSet at the given timestamp.
// If the zero timestamp is used, all accesses are considered non-MVCC.
func NewBatchAt(b storage.Batch, spans *SpanSet, ts hlc.Timestamp) storage.Batch {
Expand Down
4 changes: 2 additions & 2 deletions pkg/sql/opt/exec/execbuilder/testdata/hash_sharded_index
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ vectorized: true
└── • render
│ columns: (crdb_internal_a_shard_11_comp, column1)
│ estimated row count: 2
│ render crdb_internal_a_shard_11_comp: mod(fnv32(COALESCE(column1::STRING, '')), 11)
│ render crdb_internal_a_shard_11_comp: mod(fnv32(COALESCE(column1::STRING, '')), 11)::INT4
│ render column1: column1
│
└── • values
Expand Down Expand Up @@ -63,7 +63,7 @@ vectorized: true
└── • render
│ columns: (crdb_internal_a_shard_12_comp, rowid_default, column1)
│ estimated row count: 2
│ render crdb_internal_a_shard_12_comp: mod(fnv32(COALESCE(column1::STRING, '')), 12)
│ render crdb_internal_a_shard_12_comp: mod(fnv32(COALESCE(column1::STRING, '')), 12)::INT4
│ render rowid_default: unique_rowid()
│ render column1: column1
│
Expand Down
2 changes: 1 addition & 1 deletion pkg/sql/opt/optbuilder/scope.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -458,7 +458,7 @@ func (s *scope) resolveAndRequireType(expr tree.Expr, desired *types.T) tree.Typ
if err != nil {
panic(err)
}
return s.ensureNullType(texpr, desired)
return tree.ReType(s.ensureNullType(texpr, desired), desired)
}

// ensureNullType tests the type of the given expression. If types.Unknown, then
Expand Down
4 changes: 2 additions & 2 deletions pkg/sql/opt/optbuilder/testdata/insert
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1168,13 +1168,13 @@ insert decimals
│ │ │ │ │ ├── columns: column1:7!null column2:8
│ │ │ │ │ └── (1.1, ARRAY[0.95,NULL,15])
│ │ │ │ └── projections
│ │ │ │ └── 1.23 [as=c_default:9]
│ │ │ │ └── 1.23::DECIMAL(10,1) [as=c_default:9]
│ │ │ └── projections
│ │ │ ├── crdb_internal.round_decimal_values(column1:7, 0) [as=a:10]
│ │ │ ├── crdb_internal.round_decimal_values(column2:8, 1) [as=b:11]
│ │ │ └── crdb_internal.round_decimal_values(c_default:9, 1) [as=c_default:12]
│ │ └── projections
│ │ └── a:10 + c_default:12 [as=d_comp:13]
│ │ └── (a:10 + c_default:12::DECIMAL)::DECIMAL(10,1) [as=d_comp:13]
│ └── projections
│ └── crdb_internal.round_decimal_values(d_comp:13, 1) [as=d_comp:14]
└── projections
Expand Down
4 changes: 2 additions & 2 deletions pkg/sql/opt/optbuilder/testdata/update
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1631,15 +1631,15 @@ update decimals
│ │ │ │ │ ├── columns: a:7!null b:8 c:9 d:10 crdb_internal_mvcc_timestamp:11 tableoid:12
│ │ │ │ │ └── computed column expressions
│ │ │ │ │ └── d:10
│ │ │ │ │ └── a:7::DECIMAL + c:9::DECIMAL
│ │ │ │ │ └── (a:7::DECIMAL + c:9::DECIMAL)::DECIMAL(10,1)
│ │ │ │ └── projections
│ │ │ │ ├── 1.1 [as=a_new:13]
│ │ │ │ └── ARRAY[0.95,NULL,15] [as=b_new:14]
│ │ │ └── projections
│ │ │ ├── crdb_internal.round_decimal_values(a_new:13, 0) [as=a_new:15]
│ │ │ └── crdb_internal.round_decimal_values(b_new:14, 1) [as=b_new:16]
│ │ └── projections
│ │ └── a_new:15 + c:9::DECIMAL [as=d_comp:17]
│ │ └── (a_new:15 + c:9::DECIMAL)::DECIMAL(10,1) [as=d_comp:17]
│ └── projections
│ └── crdb_internal.round_decimal_values(d_comp:17, 1) [as=d_comp:18]
└── projections
Expand Down
52 changes: 52 additions & 0 deletions pkg/sql/opt/optbuilder/testdata/update-col-cast-bug
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
exec-ddl
create table t (a int2, b int2, c int2 as (a + b) virtual)
----

build format=show-types
update t set a = (with cte as (select 1:::int8) select t.c from cte limit 1)
----
with &1 (cte)
├── project
│ ├── columns: int8:13(int!null)
│ ├── values
│ │ └── () [type=tuple]
│ └── projections
│ └── 1 [as=int8:13, type=int]
└── update t
├── columns: <none>
├── fetch columns: a:7(int2) b:8(int2) t.c:9(int2) rowid:10(int)
├── update-mapping:
│ ├── a_new:16 => a:1
│ └── c_comp:17 => t.c:3
└── project
├── columns: c_comp:17(int2) a:7(int2) b:8(int2) t.c:9(int2) rowid:10(int!null) crdb_internal_mvcc_timestamp:11(decimal) tableoid:12(oid) a_new:16(int2)
├── project
│ ├── columns: a_new:16(int2) a:7(int2) b:8(int2) t.c:9(int2) rowid:10(int!null) crdb_internal_mvcc_timestamp:11(decimal) tableoid:12(oid)
│ ├── project
│ │ ├── columns: t.c:9(int2) a:7(int2) b:8(int2) rowid:10(int!null) crdb_internal_mvcc_timestamp:11(decimal) tableoid:12(oid)
│ │ ├── scan t
│ │ │ ├── columns: a:7(int2) b:8(int2) rowid:10(int!null) crdb_internal_mvcc_timestamp:11(decimal) tableoid:12(oid)
│ │ │ └── computed column expressions
│ │ │ └── t.c:9
│ │ │ └── (a:7::INT8 + b:8::INT8)::INT2 [type=int2]
│ │ └── projections
│ │ └── (a:7::INT8 + b:8::INT8)::INT2 [as=t.c:9, type=int2]
│ └── projections
│ └── subquery [as=a_new:16, type=int2]
│ └── max1-row
│ ├── columns: c:15(int2)
│ └── limit
│ ├── columns: c:15(int2)
│ ├── project
│ │ ├── columns: c:15(int2)
│ │ ├── limit hint: 1.00
│ │ ├── with-scan &1 (cte)
│ │ │ ├── columns: int8:14(int!null)
│ │ │ ├── mapping:
│ │ │ │ └── int8:13(int) => int8:14(int)
│ │ │ └── limit hint: 1.00
│ │ └── projections
│ │ └── t.c:9 [as=c:15, type=int2]
│ └── 1 [type=int]
└── projections
└── (a_new:16::INT8 + b:8::INT8)::INT2 [as=c_comp:17, type=int2]
24 changes: 12 additions & 12 deletions pkg/sql/opt/optbuilder/testdata/upsert
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1722,13 +1722,13 @@ upsert decimals
│ │ │ │ │ │ │ │ │ ├── columns: column1:7!null column2:8
│ │ │ │ │ │ │ │ │ └── (1.1, ARRAY[0.95])
│ │ │ │ │ │ │ │ └── projections
│ │ │ │ │ │ │ │ └── 1.23 [as=c_default:9]
│ │ │ │ │ │ │ │ └── 1.23::DECIMAL(10,1) [as=c_default:9]
│ │ │ │ │ │ │ └── projections
│ │ │ │ │ │ │ ├── crdb_internal.round_decimal_values(column1:7, 0) [as=a:10]
│ │ │ │ │ │ │ ├── crdb_internal.round_decimal_values(column2:8, 1) [as=b:11]
│ │ │ │ │ │ │ └── crdb_internal.round_decimal_values(c_default:9, 1) [as=c_default:12]
│ │ │ │ │ │ └── projections
│ │ │ │ │ │ └── a:10 + c_default:12 [as=d_comp:13]
│ │ │ │ │ │ └── (a:10 + c_default:12::DECIMAL)::DECIMAL(10,1) [as=d_comp:13]
│ │ │ │ │ └── projections
│ │ │ │ │ └── crdb_internal.round_decimal_values(d_comp:13, 1) [as=d_comp:14]
│ │ │ │ └── aggregations
Expand All @@ -1742,11 +1742,11 @@ upsert decimals
│ │ │ │ ├── columns: decimals.a:15!null decimals.b:16 c:17 d:18 crdb_internal_mvcc_timestamp:19 tableoid:20
│ │ │ │ └── computed column expressions
│ │ │ │ └── d:18
│ │ │ │ └── decimals.a:15::DECIMAL + c:17::DECIMAL
│ │ │ │ └── (decimals.a:15::DECIMAL + c:17::DECIMAL)::DECIMAL(10,1)
│ │ │ └── filters
│ │ │ └── a:10 = decimals.a:15
│ │ └── projections
│ │ └── decimals.a:15::DECIMAL + c:17::DECIMAL [as=d_comp:21]
│ │ └── (decimals.a:15::DECIMAL + c:17::DECIMAL)::DECIMAL(10,1) [as=d_comp:21]
│ └── projections
│ ├── CASE WHEN decimals.a:15 IS NULL THEN a:10 ELSE decimals.a:15 END [as=upsert_a:22]
│ ├── CASE WHEN decimals.a:15 IS NULL THEN c_default:12 ELSE c:17 END [as=upsert_c:23]
Expand Down Expand Up @@ -1794,13 +1794,13 @@ upsert decimals
│ │ │ │ │ │ │ │ │ └── (1.1,)
│ │ │ │ │ │ │ │ └── projections
│ │ │ │ │ │ │ │ ├── NULL::DECIMAL(5,1)[] [as=b_default:8]
│ │ │ │ │ │ │ │ └── 1.23 [as=c_default:9]
│ │ │ │ │ │ │ │ └── 1.23::DECIMAL(10,1) [as=c_default:9]
│ │ │ │ │ │ │ └── projections
│ │ │ │ │ │ │ ├── crdb_internal.round_decimal_values(column1:7, 0) [as=a:10]
│ │ │ │ │ │ │ ├── crdb_internal.round_decimal_values(b_default:8, 1) [as=b_default:11]
│ │ │ │ │ │ │ └── crdb_internal.round_decimal_values(c_default:9, 1) [as=c_default:12]
│ │ │ │ │ │ └── projections
│ │ │ │ │ │ └── a:10 + c_default:12 [as=d_comp:13]
│ │ │ │ │ │ └── (a:10 + c_default:12::DECIMAL)::DECIMAL(10,1) [as=d_comp:13]
│ │ │ │ │ └── projections
│ │ │ │ │ └── crdb_internal.round_decimal_values(d_comp:13, 1) [as=d_comp:14]
│ │ │ │ └── aggregations
Expand All @@ -1814,11 +1814,11 @@ upsert decimals
│ │ │ │ ├── columns: decimals.a:15!null b:16 c:17 d:18 crdb_internal_mvcc_timestamp:19 tableoid:20
│ │ │ │ └── computed column expressions
│ │ │ │ └── d:18
│ │ │ │ └── decimals.a:15::DECIMAL + c:17::DECIMAL
│ │ │ │ └── (decimals.a:15::DECIMAL + c:17::DECIMAL)::DECIMAL(10,1)
│ │ │ └── filters
│ │ │ └── a:10 = decimals.a:15
│ │ └── projections
│ │ └── decimals.a:15::DECIMAL + c:17::DECIMAL [as=d_comp:21]
│ │ └── (decimals.a:15::DECIMAL + c:17::DECIMAL)::DECIMAL(10,1) [as=d_comp:21]
│ └── projections
│ ├── CASE WHEN decimals.a:15 IS NULL THEN a:10 ELSE decimals.a:15 END [as=upsert_a:22]
│ ├── CASE WHEN decimals.a:15 IS NULL THEN b_default:11 ELSE b:16 END [as=upsert_b:23]
Expand Down Expand Up @@ -1874,13 +1874,13 @@ upsert decimals
│ │ │ │ │ │ │ │ │ │ │ ├── columns: column1:7!null column2:8
│ │ │ │ │ │ │ │ │ │ │ └── (1.1, ARRAY[0.95])
│ │ │ │ │ │ │ │ │ │ └── projections
│ │ │ │ │ │ │ │ │ │ └── 1.23 [as=c_default:9]
│ │ │ │ │ │ │ │ │ │ └── 1.23::DECIMAL(10,1) [as=c_default:9]
│ │ │ │ │ │ │ │ │ └── projections
│ │ │ │ │ │ │ │ │ ├── crdb_internal.round_decimal_values(column1:7, 0) [as=a:10]
│ │ │ │ │ │ │ │ │ ├── crdb_internal.round_decimal_values(column2:8, 1) [as=b:11]
│ │ │ │ │ │ │ │ │ └── crdb_internal.round_decimal_values(c_default:9, 1) [as=c_default:12]
│ │ │ │ │ │ │ │ └── projections
│ │ │ │ │ │ │ │ └── a:10 + c_default:12 [as=d_comp:13]
│ │ │ │ │ │ │ │ └── (a:10 + c_default:12::DECIMAL)::DECIMAL(10,1) [as=d_comp:13]
│ │ │ │ │ │ │ └── projections
│ │ │ │ │ │ │ └── crdb_internal.round_decimal_values(d_comp:13, 1) [as=d_comp:14]
│ │ │ │ │ │ └── aggregations
Expand All @@ -1894,15 +1894,15 @@ upsert decimals
│ │ │ │ │ │ ├── columns: decimals.a:15!null decimals.b:16 c:17 d:18 crdb_internal_mvcc_timestamp:19 tableoid:20
│ │ │ │ │ │ └── computed column expressions
│ │ │ │ │ │ └── d:18
│ │ │ │ │ │ └── decimals.a:15::DECIMAL + c:17::DECIMAL
│ │ │ │ │ │ └── (decimals.a:15::DECIMAL + c:17::DECIMAL)::DECIMAL(10,1)
│ │ │ │ │ └── filters
│ │ │ │ │ └── a:10 = decimals.a:15
│ │ │ │ └── projections
│ │ │ │ └── ARRAY[0.99] [as=b_new:21]
│ │ │ └── projections
│ │ │ └── crdb_internal.round_decimal_values(b_new:21, 1) [as=b_new:22]
│ │ └── projections
│ │ └── decimals.a:15::DECIMAL + c:17::DECIMAL [as=d_comp:23]
│ │ └── (decimals.a:15::DECIMAL + c:17::DECIMAL)::DECIMAL(10,1) [as=d_comp:23]
│ └── projections
│ ├── CASE WHEN decimals.a:15 IS NULL THEN a:10 ELSE decimals.a:15 END [as=upsert_a:24]
│ ├── CASE WHEN decimals.a:15 IS NULL THEN b:11 ELSE b_new:22 END [as=upsert_b:25]
Expand Down
2 changes: 1 addition & 1 deletion pkg/sql/opt/xform/testdata/external/trading-mutation
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1602,7 +1602,7 @@ update cardsinfo [as=ci]
│ └── const-agg [as=q:37, outer=(37)]
│ └── q:37
└── projections
├── crdb_internal.round_decimal_values(buyprice:23::DECIMAL - discount:25::DECIMAL, 4) [as=discountbuyprice_comp:53, outer=(23,25), immutable]
├── crdb_internal.round_decimal_values((buyprice:23::DECIMAL - discount:25::DECIMAL)::DECIMAL(10,4), 4) [as=discountbuyprice_comp:53, outer=(23,25), immutable]
├── CAST(NULL AS STRING) [as=notes_default:50]
├── 0 [as=oldinventory_default:51]
└── COALESCE(sum_int:47, 0) [as=actualinventory_new:49, outer=(47)]
Loading

0 comments on commit 858fc12

Please sign in to comment.