diff --git a/pkg/ccl/backupccl/restore_data_processor_test.go b/pkg/ccl/backupccl/restore_data_processor_test.go index c0779b55483b..4176a39a9c27 100644 --- a/pkg/ccl/backupccl/restore_data_processor_test.go +++ b/pkg/ccl/backupccl/restore_data_processor_test.go @@ -122,7 +122,7 @@ func slurpSSTablesLatestKey( } else if !ok || !it.UnsafeKey().Less(end) { break } - val, err := storage.DecodeMVCCValue(it.Value()) + val, err := storage.DecodeMVCCValueAndErr(it.Value()) if err != nil { t.Fatal(err) } diff --git a/pkg/ccl/streamingccl/replicationutils/utils.go b/pkg/ccl/streamingccl/replicationutils/utils.go index b05b41255a2a..790b16069b7d 100644 --- a/pkg/ccl/streamingccl/replicationutils/utils.go +++ b/pkg/ccl/streamingccl/replicationutils/utils.go @@ -70,9 +70,13 @@ func ScanSST( } else if !valid { break } + v, err := pointIter.Value() + if err != nil { + return err + } if err = mvccKeyValOp(storage.MVCCKeyValue{ Key: pointIter.Key(), - Value: pointIter.Value(), + Value: v, }); err != nil { return err } diff --git a/pkg/ccl/streamingccl/streamingest/replication_random_client_test.go b/pkg/ccl/streamingccl/streamingest/replication_random_client_test.go index 2663dbec3c46..8e2129547a23 100644 --- a/pkg/ccl/streamingccl/streamingest/replication_random_client_test.go +++ b/pkg/ccl/streamingccl/streamingest/replication_random_client_test.go @@ -351,10 +351,12 @@ func assertExactlyEqualKVs( // Since the iterator goes from latest to older versions, we compare // starting from the end of the slice that is sorted by timestamp. latestVersionInChain := valueTimestampTuples[len(valueTimestampTuples)-1] + v, err := it.Value() + require.NoError(t, err) require.Equal(t, roachpb.KeyValue{ Key: it.Key().Key, Value: roachpb.Value{ - RawBytes: it.Value(), + RawBytes: v, Timestamp: it.Key().Timestamp, }, }, latestVersionInChain) diff --git a/pkg/ccl/streamingccl/streamingest/stream_ingestion_processor_test.go b/pkg/ccl/streamingccl/streamingest/stream_ingestion_processor_test.go index 6d8f06db6c05..86fcdf3304bf 100644 --- a/pkg/ccl/streamingccl/streamingest/stream_ingestion_processor_test.go +++ b/pkg/ccl/streamingccl/streamingest/stream_ingestion_processor_test.go @@ -404,10 +404,12 @@ func assertEqualKVs( // Since the iterator goes from latest to older versions, we compare // starting from the end of the slice that is sorted by timestamp. latestVersionInChain := valueTimestampTuples[len(valueTimestampTuples)-1] + v, err := it.Value() + require.NoError(t, err) require.Equal(t, roachpb.KeyValue{ Key: it.Key().Key, Value: roachpb.Value{ - RawBytes: it.Value(), + RawBytes: v, Timestamp: it.Key().Timestamp, }, }, latestVersionInChain) diff --git a/pkg/cmd/roachtest/tests/quit.go b/pkg/cmd/roachtest/tests/quit.go index 9edf2acb9a60..349ef15f7bab 100644 --- a/pkg/cmd/roachtest/tests/quit.go +++ b/pkg/cmd/roachtest/tests/quit.go @@ -181,7 +181,7 @@ func (q *quitTest) createRanges(ctx context.Context) { db := q.c.Conn(ctx, q.t.L(), 1) defer db.Close() if _, err := db.ExecContext(ctx, fmt.Sprintf(` -CREATE TABLE t(x, y, PRIMARY KEY(x)) AS SELECT @1, 1 FROM generate_series(1,%[1]d)`, +CREATE TABLE t(x, y, PRIMARY KEY(x)) AS SELECT i, 1 FROM generate_series(1,%[1]d) g(i)`, numRanges)); err != nil { q.Fatal(err) } diff --git a/pkg/kv/kvclient/kvstreamer/streamer.go b/pkg/kv/kvclient/kvstreamer/streamer.go index 8da411f86209..005ba3848a10 100644 --- a/pkg/kv/kvclient/kvstreamer/streamer.go +++ b/pkg/kv/kvclient/kvstreamer/streamer.go @@ -532,7 +532,10 @@ func (s *Streamer) Enqueue(ctx context.Context, reqs []roachpb.RequestUnion) (re } var reqsKeysScratch []roachpb.Key var newNumRangesPerScanRequestMemoryUsage int64 - for { + for ; ; ri.Seek(ctx, rs.Key, scanDir) { + if !ri.Valid() { + return ri.Error() + } // Find all requests that touch the current range. var singleRangeReqs []roachpb.RequestUnion var positions []int @@ -626,11 +629,12 @@ func (s *Streamer) Enqueue(ctx context.Context, reqs []roachpb.RequestUnion) (re requestsToServe = append(requestsToServe, r) s.enqueuedSingleRangeRequests += len(singleRangeReqs) - if !ri.NeedAnother(rs) { - // This was the last range. + if allRequestsAreWithinSingleRange || !ri.NeedAnother(rs) { + // 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 confusing message into the trace. break } - ri.Seek(ctx, rs.Key, scanDir) } if streamerLocked { diff --git a/pkg/kv/kvnemesis/engine.go b/pkg/kv/kvnemesis/engine.go index 4ddc045e58f4..2fa72432217c 100644 --- a/pkg/kv/kvnemesis/engine.go +++ b/pkg/kv/kvnemesis/engine.go @@ -89,7 +89,11 @@ func (e *Engine) Get(key roachpb.Key, ts hlc.Timestamp) roachpb.Value { return roachpb.Value{} } var valCopy []byte - e.b, valCopy = e.b.Copy(iter.Value(), 0 /* extraCap */) + v, err := iter.ValueAndErr() + if err != nil { + panic(err) + } + e.b, valCopy = e.b.Copy(v, 0 /* extraCap */) mvccVal, err := storage.DecodeMVCCValue(valCopy) if err != nil { panic(err) @@ -128,7 +132,11 @@ func (e *Engine) Iterate( hasPoint, _ := iter.HasPointAndRange() var keyCopy, valCopy []byte e.b, keyCopy = e.b.Copy(iter.Key(), 0 /* extraCap */) - e.b, valCopy = e.b.Copy(iter.Value(), 0 /* extraCap */) + v, err := iter.ValueAndErr() + if err != nil { + fn(nil, nil, hlc.Timestamp{}, nil, err) + } + e.b, valCopy = e.b.Copy(v, 0 /* extraCap */) if hasPoint { key, err := storage.DecodeMVCCKey(keyCopy) if err != nil { diff --git a/pkg/kv/kvserver/gc/gc_old_test.go b/pkg/kv/kvserver/gc/gc_old_test.go index f99be5f77c8f..fa16af692978 100644 --- a/pkg/kv/kvserver/gc/gc_old_test.go +++ b/pkg/kv/kvserver/gc/gc_old_test.go @@ -193,7 +193,11 @@ func runGCOld( expBaseKey = iterKey.Key if !iterKey.IsValue() { keys = []storage.MVCCKey{iter.Key()} - vals = [][]byte{iter.Value()} + v, err := iter.Value() + if err != nil { + return Info{}, err + } + vals = [][]byte{v} continue } // An implicit metadata. @@ -203,8 +207,12 @@ func runGCOld( // determine that there is no intent. vals = [][]byte{nil} } + v, err := iter.Value() + if err != nil { + return Info{}, err + } keys = append(keys, iter.Key()) - vals = append(vals, iter.Value()) + vals = append(vals, v) } // Handle last collected set of keys/vals. processKeysAndValues() diff --git a/pkg/kv/kvserver/gc/gc_random_test.go b/pkg/kv/kvserver/gc/gc_random_test.go index 0acbe5611300..f6b3d74fd97f 100644 --- a/pkg/kv/kvserver/gc/gc_random_test.go +++ b/pkg/kv/kvserver/gc/gc_random_test.go @@ -495,7 +495,8 @@ func getExpectationsGenerator( p, r := it.HasPointAndRange() if p { k := it.Key() - v := it.Value() + v, err := it.Value() + require.NoError(t, err) if len(baseKey) == 0 { baseKey = k.Key // We are only interested in range tombstones covering current point, diff --git a/pkg/kv/kvserver/rditer/replica_data_iter.go b/pkg/kv/kvserver/rditer/replica_data_iter.go index 43087b9a7370..07887ca6bdbd 100644 --- a/pkg/kv/kvserver/rditer/replica_data_iter.go +++ b/pkg/kv/kvserver/rditer/replica_data_iter.go @@ -334,7 +334,7 @@ func (ri *ReplicaMVCCDataIterator) Key() storage.MVCCKey { } // Value returns the current value. Only called in tests. -func (ri *ReplicaMVCCDataIterator) Value() []byte { +func (ri *ReplicaMVCCDataIterator) Value() ([]byte, error) { return ri.it.Value() } diff --git a/pkg/kv/kvserver/replica_raft.go b/pkg/kv/kvserver/replica_raft.go index 543ab5be689c..7e0d897bcea9 100644 --- a/pkg/kv/kvserver/replica_raft.go +++ b/pkg/kv/kvserver/replica_raft.go @@ -2301,9 +2301,13 @@ func (r *Replica) printRaftTail( if err != nil { return sb.String(), err } + v, err := it.Value() + if err != nil { + return sb.String(), err + } kv := storage.MVCCKeyValue{ Key: mvccKey, - Value: it.Value(), + Value: v, } sb.WriteString(truncateEntryString(SprintMVCCKeyValue(kv, true /* printKey */), 2000)) sb.WriteRune('\n') diff --git a/pkg/kv/kvserver/spanset/batch.go b/pkg/kv/kvserver/spanset/batch.go index 9a4685b9d770..653b168c9d63 100644 --- a/pkg/kv/kvserver/spanset/batch.go +++ b/pkg/kv/kvserver/spanset/batch.go @@ -144,7 +144,7 @@ func (i *MVCCIterator) Key() storage.MVCCKey { } // Value is part of the storage.MVCCIterator interface. -func (i *MVCCIterator) Value() []byte { +func (i *MVCCIterator) Value() ([]byte, error) { return i.i.Value() } @@ -391,7 +391,7 @@ func (i *EngineIterator) EngineKey() (storage.EngineKey, error) { } // Value is part of the storage.EngineIterator interface. -func (i *EngineIterator) Value() []byte { +func (i *EngineIterator) Value() ([]byte, error) { return i.i.Value() } diff --git a/pkg/sql/catalog/lease/kv_writer_test.go b/pkg/sql/catalog/lease/kv_writer_test.go index a7458cf1150e..898afed9c167 100644 --- a/pkg/sql/catalog/lease/kv_writer_test.go +++ b/pkg/sql/catalog/lease/kv_writer_test.go @@ -159,10 +159,12 @@ func getRawHistoryKVs( k := it.Key() suffix, _, err := codec.DecodeTablePrefix(k.Key) require.NoError(t, err) + v, err := it.Value() + require.NoError(t, err) row := roachpb.KeyValue{ Key: suffix, Value: roachpb.Value{ - RawBytes: it.Value(), + RawBytes: v, }, } row.Value.ClearChecksum() diff --git a/pkg/sql/logictest/testdata/logic_test/txn b/pkg/sql/logictest/testdata/logic_test/txn index 0a9aff71701f..bd5a179e4b46 100644 --- a/pkg/sql/logictest/testdata/logic_test/txn +++ b/pkg/sql/logictest/testdata/logic_test/txn @@ -1028,6 +1028,59 @@ SELECT nextval('a') statement error cannot execute setval\(\) in a read-only transaction SELECT setval('a', 2) +statement error cannot execute CREATE ROLE in a read-only transaction +CREATE ROLE my_user + +statement error cannot execute ALTER ROLE in a read-only transaction +ALTER ROLE testuser SET default_int_size = 4 + +statement error cannot execute DROP ROLE in a read-only transaction +DROP ROLE testuser + +statement error cannot execute SET CLUSTER SETTING in a read-only transaction +SET CLUSTER SETTING sql.auth.change_own_password.enabled = true + +statement error cannot execute GRANT in a read-only transaction +GRANT admin TO testuser + +statement error cannot execute REVOKE in a read-only transaction +REVOKE admin FROM testuser + +statement error cannot execute GRANT in a read-only transaction +GRANT CONNECT ON DATABASE test TO testuser + +statement error cannot execute create_tenant\(\) in a read-only transaction +SELECT crdb_internal.create_tenant(3) + +statement error cannot execute rename_tenant\(\) in a read-only transaction +SELECT crdb_internal.rename_tenant(3, 'new') + +statement error cannot execute destroy_tenant\(\) in a read-only transaction +SELECT crdb_internal.destroy_tenant(3) + +# SET session variable should work in a read-only txn. +statement ok +SET intervalstyle = 'postgres' + +statement ok +SET SESSION CHARACTERISTICS AS TRANSACTION PRIORITY NORMAL + +statement ok +SET SESSION AUTHORIZATION DEFAULT + +statement ok +BEGIN + +# DECLARE and FETCH CURSOR should work in a read-only txn. +statement ok +DECLARE foo CURSOR FOR SELECT 1 + +statement ok +FETCH 1 foo + +statement ok +COMMIT + query T SHOW TRANSACTION STATUS ---- diff --git a/pkg/sql/row/fetcher_mvcc_test.go b/pkg/sql/row/fetcher_mvcc_test.go index 1a0cb242f57d..1802efbf0f9e 100644 --- a/pkg/sql/row/fetcher_mvcc_test.go +++ b/pkg/sql/row/fetcher_mvcc_test.go @@ -57,7 +57,7 @@ func slurpUserDataKVs(t testing.TB, e storage.Engine) []roachpb.KeyValue { if !it.UnsafeKey().IsValue() { return errors.Errorf("found intent key %v", it.UnsafeKey()) } - mvccValue, err := storage.DecodeMVCCValue(it.Value()) + mvccValue, err := storage.DecodeMVCCValueAndErr(it.Value()) if err != nil { t.Fatal(err) } diff --git a/pkg/sql/sem/tree/stmt.go b/pkg/sql/sem/tree/stmt.go index 4b8ab0c82a06..87dda5f2b877 100644 --- a/pkg/sql/sem/tree/stmt.go +++ b/pkg/sql/sem/tree/stmt.go @@ -116,7 +116,7 @@ type canModifySchema interface { // CanModifySchema returns true if the statement can modify // the database schema. func CanModifySchema(stmt Statement) bool { - if stmt.StatementReturnType() == DDL { + if stmt.StatementReturnType() == DDL || stmt.StatementType() == TypeDDL { return true } scm, ok := stmt.(canModifySchema) @@ -125,6 +125,10 @@ func CanModifySchema(stmt Statement) bool { // CanWriteData returns true if the statement can modify data. func CanWriteData(stmt Statement) bool { + if stmt.StatementType() == TypeDCL { + // Commands like GRANT and REVOKE modify system tables. + return true + } switch stmt.(type) { // Normal write operations. case *Insert, *Delete, *Update, *Truncate: @@ -132,6 +136,9 @@ func CanWriteData(stmt Statement) bool { // Import operations. case *CopyFrom, *Import, *Restore: return true + // Backup creates a job and allows you to write into userfiles. + case *Backup: + return true // CockroachDB extensions. case *Split, *Unsplit, *Relocate, *RelocateRange, *Scatter: return true @@ -477,7 +484,7 @@ func (*AlterSequence) StatementTag() string { return "ALTER SEQUENCE" } func (*AlterRole) StatementReturnType() StatementReturnType { return Ack } // StatementType implements the Statement interface. -func (*AlterRole) StatementType() StatementType { return TypeDDL } +func (*AlterRole) StatementType() StatementType { return TypeDCL } // StatementTag returns a short string identifying the type of statement. func (*AlterRole) StatementTag() string { return "ALTER ROLE" } @@ -488,7 +495,7 @@ func (*AlterRole) hiddenFromShowQueries() {} func (*AlterRoleSet) StatementReturnType() StatementReturnType { return Ack } // StatementType implements the Statement interface. -func (*AlterRoleSet) StatementType() StatementType { return TypeDDL } +func (*AlterRoleSet) StatementType() StatementType { return TypeDCL } // StatementTag returns a short string identifying the type of statement. func (*AlterRoleSet) StatementTag() string { return "ALTER ROLE" } @@ -844,7 +851,7 @@ func (*CreateType) modifiesSchema() bool { return true } func (*CreateRole) StatementReturnType() StatementReturnType { return Ack } // StatementType implements the Statement interface. -func (*CreateRole) StatementType() StatementType { return TypeDDL } +func (*CreateRole) StatementType() StatementType { return TypeDCL } // StatementTag returns a short string identifying the type of statement. func (*CreateRole) StatementTag() string { return "CREATE ROLE" } @@ -912,7 +919,7 @@ func (d *Discard) StatementTag() string { func (n *DeclareCursor) StatementReturnType() StatementReturnType { return Ack } // StatementType implements the Statement interface. -func (*DeclareCursor) StatementType() StatementType { return TypeDCL } +func (*DeclareCursor) StatementType() StatementType { return TypeDML } // StatementTag returns a short string identifying the type of statement. func (*DeclareCursor) StatementTag() string { return "DECLARE CURSOR" } @@ -975,7 +982,7 @@ func (*DropSequence) StatementTag() string { return "DROP SEQUENCE" } func (*DropRole) StatementReturnType() StatementReturnType { return Ack } // StatementType implements the Statement interface. -func (*DropRole) StatementType() StatementType { return TypeDDL } +func (*DropRole) StatementType() StatementType { return TypeDCL } // StatementTag returns a short string identifying the type of statement. func (*DropRole) StatementTag() string { return "DROP ROLE" } @@ -1342,7 +1349,7 @@ func (*SelectClause) StatementTag() string { return "SELECT" } func (*SetVar) StatementReturnType() StatementReturnType { return Ack } // StatementType implements the Statement interface. -func (*SetVar) StatementType() StatementType { return TypeDCL } +func (*SetVar) StatementType() StatementType { return TypeDML } // StatementTag returns a short string identifying the type of statement. func (n *SetVar) StatementTag() string { @@ -1365,7 +1372,7 @@ func (*SetClusterSetting) StatementTag() string { return "SET CLUSTER SETTING" } func (*SetTransaction) StatementReturnType() StatementReturnType { return Ack } // StatementType implements the Statement interface. -func (*SetTransaction) StatementType() StatementType { return TypeDCL } +func (*SetTransaction) StatementType() StatementType { return TypeTCL } // StatementTag returns a short string identifying the type of statement. func (*SetTransaction) StatementTag() string { return "SET TRANSACTION" } @@ -1395,7 +1402,7 @@ func (*SetZoneConfig) StatementTag() string { return "CONFIGURE ZONE" } func (*SetSessionAuthorizationDefault) StatementReturnType() StatementReturnType { return Ack } // StatementType implements the Statement interface. -func (*SetSessionAuthorizationDefault) StatementType() StatementType { return TypeDCL } +func (*SetSessionAuthorizationDefault) StatementType() StatementType { return TypeDML } // StatementTag returns a short string identifying the type of statement. func (*SetSessionAuthorizationDefault) StatementTag() string { return "SET" } @@ -1404,7 +1411,7 @@ func (*SetSessionAuthorizationDefault) StatementTag() string { return "SET" } func (*SetSessionCharacteristics) StatementReturnType() StatementReturnType { return Ack } // StatementType implements the Statement interface. -func (*SetSessionCharacteristics) StatementType() StatementType { return TypeDCL } +func (*SetSessionCharacteristics) StatementType() StatementType { return TypeDML } // StatementTag returns a short string identifying the type of statement. func (*SetSessionCharacteristics) StatementTag() string { return "SET" } diff --git a/pkg/sql/tenant.go b/pkg/sql/tenant.go index e224cf54728f..9737d0cf9215 100644 --- a/pkg/sql/tenant.go +++ b/pkg/sql/tenant.go @@ -321,6 +321,9 @@ func getAvailableTenantID( func (p *planner) CreateTenant( ctx context.Context, name roachpb.TenantName, ) (roachpb.TenantID, error) { + if p.EvalContext().TxnReadOnly { + return roachpb.TenantID{}, readOnlyError("create_tenant()") + } const op = "create tenant" if err := p.RequireAdminRole(ctx, op); err != nil { return roachpb.TenantID{}, err @@ -343,6 +346,9 @@ func (p *planner) CreateTenant( func (p *planner) CreateTenantWithID( ctx context.Context, tenantID uint64, tenantName roachpb.TenantName, ) error { + if p.EvalContext().TxnReadOnly { + return readOnlyError("create_tenant()") + } if err := p.RequireAdminRole(ctx, "create tenant"); err != nil { return err } @@ -552,6 +558,10 @@ func (p *planner) DestroyTenantByID( } func (p *planner) validateDestroyTenant(ctx context.Context) error { + if p.EvalContext().TxnReadOnly { + return readOnlyError("destroy_tenant()") + } + const op = "destroy" if err := p.RequireAdminRole(ctx, "destroy tenant"); err != nil { return err @@ -810,6 +820,10 @@ func TestingUpdateTenantRecord( func (p *planner) RenameTenant( ctx context.Context, tenantID uint64, tenantName roachpb.TenantName, ) error { + if p.EvalContext().TxnReadOnly { + return readOnlyError("rename_tenant()") + } + if err := tenantName.IsValid(); err != nil { return pgerror.WithCandidateCode(err, pgcode.Syntax) } diff --git a/pkg/storage/batch_test.go b/pkg/storage/batch_test.go index 96ce2b5f15be..15487975ad8e 100644 --- a/pkg/storage/batch_test.go +++ b/pkg/storage/batch_test.go @@ -752,8 +752,12 @@ func TestBatchIteration(t *testing.T) { if !reflect.DeepEqual(iter.Key(), k1) { t.Fatalf("expected %s, got %s", k1, iter.Key()) } - if !reflect.DeepEqual(iter.Value(), v1) { - t.Fatalf("expected %s, got %s", v1, iter.Value()) + checkValErr := func(v []byte, err error) []byte { + require.NoError(t, err) + return v + } + if !reflect.DeepEqual(checkValErr(iter.Value()), v1) { + t.Fatalf("expected %s, got %s", v1, checkValErr(iter.Value())) } iter.Next() if ok, err := iter.Valid(); !ok { @@ -762,8 +766,8 @@ func TestBatchIteration(t *testing.T) { if !reflect.DeepEqual(iter.Key(), k2) { t.Fatalf("expected %s, got %s", k2, iter.Key()) } - if !reflect.DeepEqual(iter.Value(), v2) { - t.Fatalf("expected %s, got %s", v2, iter.Value()) + if !reflect.DeepEqual(checkValErr(iter.Value()), v2) { + t.Fatalf("expected %s, got %s", v2, checkValErr(iter.Value())) } iter.Next() if ok, err := iter.Valid(); err != nil { @@ -780,8 +784,8 @@ func TestBatchIteration(t *testing.T) { if !reflect.DeepEqual(iter.Key(), k2) { t.Fatalf("expected %s, got %s", k2, iter.Key()) } - if !reflect.DeepEqual(iter.Value(), v2) { - t.Fatalf("expected %s, got %s", v2, iter.Value()) + if !reflect.DeepEqual(checkValErr(iter.Value()), v2) { + t.Fatalf("expected %s, got %s", v2, checkValErr(iter.Value())) } iter.Prev() @@ -791,8 +795,8 @@ func TestBatchIteration(t *testing.T) { if !reflect.DeepEqual(iter.Key(), k1) { t.Fatalf("expected %s, got %s", k1, iter.Key()) } - if !reflect.DeepEqual(iter.Value(), v1) { - t.Fatalf("expected %s, got %s", v1, iter.Value()) + if !reflect.DeepEqual(checkValErr(iter.Value()), v1) { + t.Fatalf("expected %s, got %s", v1, checkValErr(iter.Value())) } } diff --git a/pkg/storage/engine.go b/pkg/storage/engine.go index 190836141144..9873ed2234ac 100644 --- a/pkg/storage/engine.go +++ b/pkg/storage/engine.go @@ -278,7 +278,7 @@ type MVCCIterator interface { // this seems avoidable, and we should consider cleaning up the callers. UnsafeRawMVCCKey() []byte // Value is like UnsafeValue, but returns memory owned by the caller. - Value() []byte + Value() ([]byte, error) // ValueProto unmarshals the value the iterator is currently // pointing to using a protobuf decoder. ValueProto(msg protoutil.Message) error @@ -351,7 +351,7 @@ type EngineIterator interface { UnsafeValue() ([]byte, error) // Value returns the current value as a byte slice. // REQUIRES: latest positioning function returned valid=true. - Value() []byte + Value() ([]byte, error) // GetRawIter is a low-level method only for use in the storage package, // that returns the underlying pebble Iterator. GetRawIter() *pebble.Iterator @@ -1248,7 +1248,7 @@ func ScanIntents( return nil, err } intents = append(intents, roachpb.MakeIntent(meta.Txn, lockedKey)) - intentBytes += int64(len(lockedKey)) + int64(len(iter.Value())) + intentBytes += int64(len(lockedKey)) + int64(len(v)) } if err != nil { return nil, err @@ -1488,7 +1488,11 @@ func iterateOnReader( var kv MVCCKeyValue if hasPoint, _ := it.HasPointAndRange(); hasPoint { - kv = MVCCKeyValue{Key: it.Key(), Value: it.Value()} + v, err := it.Value() + if err != nil { + return err + } + kv = MVCCKeyValue{Key: it.Key(), Value: v} } if !it.RangeBounds().Key.Equal(rangeKeys.Bounds.Key) { rangeKeys = it.RangeKeys().Clone() @@ -1675,7 +1679,11 @@ func assertMVCCIteratorInvariants(iter MVCCIterator) error { if err != nil { return err } - if v := iter.Value(); !bytes.Equal(v, u) { + v, err := iter.Value() + if err != nil { + return err + } + if !bytes.Equal(v, u) { return errors.AssertionFailedf("Value %x does not match UnsafeValue %x at %s", v, u, key) } diff --git a/pkg/storage/intent_interleaving_iter.go b/pkg/storage/intent_interleaving_iter.go index bc4977143e0c..3bd0a2e64ec0 100644 --- a/pkg/storage/intent_interleaving_iter.go +++ b/pkg/storage/intent_interleaving_iter.go @@ -984,7 +984,7 @@ func (i *intentInterleavingIter) Key() MVCCKey { return key } -func (i *intentInterleavingIter) Value() []byte { +func (i *intentInterleavingIter) Value() ([]byte, error) { if i.isCurAtIntentIter() { return i.intentIter.Value() } diff --git a/pkg/storage/intent_interleaving_iter_test.go b/pkg/storage/intent_interleaving_iter_test.go index 4022f2447709..b36f8074a8d4 100644 --- a/pkg/storage/intent_interleaving_iter_test.go +++ b/pkg/storage/intent_interleaving_iter_test.go @@ -148,7 +148,11 @@ func checkAndOutputIter(iter MVCCIterator, b *strings.Builder) { fmt.Fprintf(b, "output: unable to fetch value: %s\n", err.Error()) return } - v2 := iter.Value() + v2, err := iter.Value() + if err != nil { + fmt.Fprintf(b, "output: unable to fetch value: %s\n", err.Error()) + return + } if !bytes.Equal(v1, v2) { fmt.Fprintf(b, "output: value: %x != %x\n", v1, v2) return diff --git a/pkg/storage/mvcc.go b/pkg/storage/mvcc.go index e34367118698..240bc30f645e 100644 --- a/pkg/storage/mvcc.go +++ b/pkg/storage/mvcc.go @@ -1823,7 +1823,10 @@ func mvccPutInternal( // NOTE: we use Value instead of UnsafeValue so that we can move the // iterator below without invalidating this byte slice. - curProvValRaw = iter.Value() + curProvValRaw, err = iter.Value() + if err != nil { + return false, err + } curIntentVal, err := DecodeMVCCValue(curProvValRaw) if err != nil { return false, err diff --git a/pkg/storage/mvcc_history_metamorphic_iterator_test.go b/pkg/storage/mvcc_history_metamorphic_iterator_test.go index ef2edf7ad506..47e6c2cccaee 100644 --- a/pkg/storage/mvcc_history_metamorphic_iterator_test.go +++ b/pkg/storage/mvcc_history_metamorphic_iterator_test.go @@ -380,7 +380,7 @@ func (m *metamorphicMVCCIterator) UnsafeRawMVCCKey() []byte { return m.it.(storage.MVCCIterator).UnsafeRawMVCCKey() } -func (m *metamorphicMVCCIterator) Value() []byte { +func (m *metamorphicMVCCIterator) Value() ([]byte, error) { return m.it.(storage.MVCCIterator).Value() } diff --git a/pkg/storage/mvcc_incremental_iterator_test.go b/pkg/storage/mvcc_incremental_iterator_test.go index e997e6c68336..dddf6827cebf 100644 --- a/pkg/storage/mvcc_incremental_iterator_test.go +++ b/pkg/storage/mvcc_incremental_iterator_test.go @@ -1357,7 +1357,9 @@ func TestMVCCIncrementalIteratorIntentStraddlesSStables(t *testing.T) { ek, err := it.EngineKey() require.NoError(t, err) require.NoError(t, err) - if err := sst.PutEngineKey(ek, it.Value()); err != nil { + v, err := it.Value() + require.NoError(t, err) + if err := sst.PutEngineKey(ek, v); err != nil { t.Fatal(err) } valid, err = it.NextEngineKey() @@ -1483,7 +1485,11 @@ func collectMatchingWithMVCCIterator( } ts := iter.Key().Timestamp if (ts.Less(end) || end == ts) && start.Less(ts) { - expectedKVs = append(expectedKVs, MVCCKeyValue{Key: iter.Key(), Value: iter.Value()}) + v, err := iter.Value() + if err != nil { + t.Fatal(err) + } + expectedKVs = append(expectedKVs, MVCCKeyValue{Key: iter.Key(), Value: v}) } iter.Next() } diff --git a/pkg/storage/mvcc_test.go b/pkg/storage/mvcc_test.go index 3549ee66617b..0f9353a4495c 100644 --- a/pkg/storage/mvcc_test.go +++ b/pkg/storage/mvcc_test.go @@ -6773,7 +6773,7 @@ func mvccGetRawWithError(t *testing.T, r Reader, key MVCCKey) ([]byte, error) { if ok, err := iter.Valid(); err != nil || !ok { return nil, err } - return iter.Value(), nil + return iter.Value() } func TestMVCCLookupRangeKeyValue(t *testing.T) { diff --git a/pkg/storage/pebble_iterator.go b/pkg/storage/pebble_iterator.go index 79152d47063d..8081339074c3 100644 --- a/pkg/storage/pebble_iterator.go +++ b/pkg/storage/pebble_iterator.go @@ -667,12 +667,14 @@ func (p *pebbleIterator) EngineKey() (EngineKey, error) { } // Value implements the MVCCIterator and EngineIterator interfaces. -func (p *pebbleIterator) Value() []byte { - // TODO(sumeer): Do not ignore error. - value, _ := p.UnsafeValue() +func (p *pebbleIterator) Value() ([]byte, error) { + value, err := p.UnsafeValue() + if err != nil { + return nil, err + } valueCopy := make([]byte, len(value)) copy(valueCopy, value) - return valueCopy + return valueCopy, nil } // ValueProto implements the MVCCIterator interface. diff --git a/pkg/storage/pebble_test.go b/pkg/storage/pebble_test.go index 1a378a287451..3f69651e457d 100644 --- a/pkg/storage/pebble_test.go +++ b/pkg/storage/pebble_test.go @@ -1083,7 +1083,9 @@ func TestPebbleFlushCallbackAndDurabilityRequirement(t *testing.T) { require.NoError(t, err) require.Equal(t, v != nil, valid) if valid { - require.Equal(t, v, iter.Value()) + value, err := iter.Value() + require.NoError(t, err) + require.Equal(t, v, value) } return v } diff --git a/pkg/storage/point_synthesizing_iter.go b/pkg/storage/point_synthesizing_iter.go index 803289efc4c7..d7adcd5ff0a1 100644 --- a/pkg/storage/point_synthesizing_iter.go +++ b/pkg/storage/point_synthesizing_iter.go @@ -697,11 +697,15 @@ func (i *PointSynthesizingIter) UnsafeRawMVCCKey() []byte { } // Value implements MVCCIterator. -func (i *PointSynthesizingIter) Value() []byte { - if v, _ := i.UnsafeValue(); v != nil { - return append([]byte{}, v...) +func (i *PointSynthesizingIter) Value() ([]byte, error) { + v, err := i.UnsafeValue() + if err != nil { + return nil, err } - return nil + if v != nil { + return append([]byte{}, v...), nil + } + return nil, nil } // UnsafeValue implements MVCCIterator. diff --git a/pkg/testutils/storageutils/mvcc.go b/pkg/testutils/storageutils/mvcc.go index 5b6729370b7a..ea883834a2d0 100644 --- a/pkg/testutils/storageutils/mvcc.go +++ b/pkg/testutils/storageutils/mvcc.go @@ -33,5 +33,5 @@ func MVCCGetRawWithError(t *testing.T, r storage.Reader, key storage.MVCCKey) ([ if ok, err := iter.Valid(); err != nil || !ok { return nil, err } - return iter.Value(), nil + return iter.Value() }