From d270fa08c2d96c3c73526ba9175fd1a5e20b0227 Mon Sep 17 00:00:00 2001 From: Gemma Shay <75328174+gemma-shay@users.noreply.github.com> Date: Mon, 11 Jul 2022 17:12:32 -0400 Subject: [PATCH 1/9] Update activerecord.go v7 support --- pkg/cmd/roachtest/tests/activerecord.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/cmd/roachtest/tests/activerecord.go b/pkg/cmd/roachtest/tests/activerecord.go index d18f8d52eb4b..0fa42c2fc2c6 100644 --- a/pkg/cmd/roachtest/tests/activerecord.go +++ b/pkg/cmd/roachtest/tests/activerecord.go @@ -27,8 +27,8 @@ import ( var activerecordResultRegex = regexp.MustCompile(`^(?P[^\s]+#[^\s]+) = (?P\d+\.\d+ s) = (?P.)$`) var railsReleaseTagRegex = regexp.MustCompile(`^v(?P\d+)\.(?P\d+)\.(?P\d+)\.?(?P\d*)$`) -var supportedRailsVersion = "6.1.6" -var activerecordAdapterVersion = "v6.1.10" +var supportedRailsVersion = "7.0.3" +var activerecordAdapterVersion = "v7.0.0" // This test runs activerecord's full test suite against a single cockroach node. From 341a77fed10a4bcddfa1a1d8c28ad20109742845 Mon Sep 17 00:00:00 2001 From: Michael Butler Date: Thu, 7 Jul 2022 22:44:59 -0400 Subject: [PATCH 2/9] storage: handle range keys in readAsOfIterator Previously, the readAsOfIterator used in RESTORE could not handle range keys. This PR implements the new SimpleMVCCIterator methods that handle range keys. Further, this patch ensures the readAsOfIterator skips over point keys shadowed by range keys at or below the caller's specified asOf timestamp. Next, Backup needs to be tought about RangeKeys. Informs #71155 Release note: none --- pkg/storage/mvcc_history_test.go | 24 +++ pkg/storage/read_as_of_iterator.go | 99 ++++++++--- .../mvcc_histories/range_key_iter_read_as_of | 168 ++++++++++++++++++ 3 files changed, 267 insertions(+), 24 deletions(-) create mode 100644 pkg/storage/testdata/mvcc_histories/range_key_iter_read_as_of diff --git a/pkg/storage/mvcc_history_test.go b/pkg/storage/mvcc_history_test.go index 15e52ec37beb..02229f264783 100644 --- a/pkg/storage/mvcc_history_test.go +++ b/pkg/storage/mvcc_history_test.go @@ -670,6 +670,7 @@ var commands = map[string]cmd{ "iter_new": {typReadOnly, cmdIterNew}, "iter_new_incremental": {typReadOnly, cmdIterNewIncremental}, // MVCCIncrementalIterator + "iter_new_read_as_of": {typReadOnly, cmdIterNewReadAsOf}, // readAsOfIterator "iter_seek_ge": {typReadOnly, cmdIterSeekGE}, "iter_seek_lt": {typReadOnly, cmdIterSeekLT}, "iter_seek_intent_ge": {typReadOnly, cmdIterSeekIntentGE}, @@ -1396,6 +1397,29 @@ func cmdIterNewIncremental(e *evalCtx) error { return nil } +func cmdIterNewReadAsOf(e *evalCtx) error { + if e.iter != nil { + e.iter.Close() + } + var asOf hlc.Timestamp + if e.hasArg("asOfTs") { + asOf = e.getTsWithName("asOfTs") + } + opts := IterOptions{ + KeyTypes: IterKeyTypePointsAndRanges, + RangeKeyMaskingBelow: asOf} + if e.hasArg("k") { + opts.LowerBound, opts.UpperBound = e.getKeyRange() + } + if len(opts.UpperBound) == 0 { + opts.UpperBound = keys.MaxKey + } + r, closer := metamorphicReader(e, "iter-reader") + iter := &iterWithCloser{r.NewMVCCIterator(MVCCKeyIterKind, opts), closer} + e.iter = NewReadAsOfIterator(iter, asOf) + return nil +} + func cmdIterSeekGE(e *evalCtx) error { key := e.getKey() ts := e.getTs(nil) diff --git a/pkg/storage/read_as_of_iterator.go b/pkg/storage/read_as_of_iterator.go index ce97929a4426..9b51620fc0ba 100644 --- a/pkg/storage/read_as_of_iterator.go +++ b/pkg/storage/read_as_of_iterator.go @@ -16,15 +16,21 @@ import ( ) // ReadAsOfIterator wraps a SimpleMVCCIterator and only surfaces the latest -// valid key of a given MVCC key that is also below the asOf timestamp, if set. -// Further, the iterator does not surface delete tombstones, nor any MVCC keys -// shadowed by delete tombstones below the asOf timestamp, if set. The iterator -// assumes that it will not encounter any write intents. +// valid point key of a given MVCC key that is also below the asOf timestamp, if +// set. Further, the iterator does not surface point or range tombstones, nor +// any MVCC keys shadowed by tombstones below the asOf timestamp, if set. The +// iterator assumes that it will not encounter any write intents. type ReadAsOfIterator struct { iter SimpleMVCCIterator // asOf is the latest timestamp of a key surfaced by the iterator. asOf hlc.Timestamp + + // valid tracks if the current key is valid + valid bool + + // err tracks if iterating to the current key returned an error + err error } var _ SimpleMVCCIterator = &ReadAsOfIterator{} @@ -45,7 +51,7 @@ func (f *ReadAsOfIterator) SeekGE(originalKey MVCCKey) { synthetic := MVCCKey{Key: originalKey.Key, Timestamp: f.asOf} f.iter.SeekGE(synthetic) - if ok := f.advance(); ok && f.UnsafeKey().Less(originalKey) { + if f.advance(); f.valid && f.UnsafeKey().Less(originalKey) { // The following is true: // originalKey.Key == f.UnsafeKey && // f.asOf timestamp (if set) >= current timestamp > originalKey timestamp. @@ -59,7 +65,7 @@ func (f *ReadAsOfIterator) SeekGE(originalKey MVCCKey) { // Valid implements the simpleMVCCIterator. func (f *ReadAsOfIterator) Valid() (bool, error) { - return f.iter.Valid() + return f.valid, f.err } // Next advances the iterator to the next valid MVCC key obeying the iterator's @@ -94,42 +100,87 @@ func (f *ReadAsOfIterator) UnsafeValue() []byte { // HasPointAndRange implements SimpleMVCCIterator. func (f *ReadAsOfIterator) HasPointAndRange() (bool, bool) { - panic("not implemented") + // the ReadAsOfIterator only surfaces point keys; therefore hasPoint is always + // true, unless the iterator is invalid, and hasRange is always false. + return f.valid, false } -// RangeBounds implements SimpleMVCCIterator. +// RangeBounds always returns an empty span, since the iterator never surfaces +// rangekeys. func (f *ReadAsOfIterator) RangeBounds() roachpb.Span { - panic("not implemented") + return roachpb.Span{} } -// RangeKeys implements SimpleMVCCIterator. +// RangeKeys is always empty since this iterator never surfaces rangeKeys. func (f *ReadAsOfIterator) RangeKeys() []MVCCRangeKeyValue { - panic("not implemented") + return []MVCCRangeKeyValue{} +} + +// updateValid updates i.valid and i.err based on the underlying iterator, and +// returns true if valid. +func (f *ReadAsOfIterator) updateValid() bool { + f.valid, f.err = f.iter.Valid() + return f.valid } // advance moves past keys with timestamps later than f.asOf and skips MVCC keys -// whose latest value (subject to f.asOF) has been deleted. Note that advance -// moves past keys above asOF before evaluating tombstones, implying the -// iterator will never call f.iter.NextKey() on a tombstone with a timestamp -// later than f.asOF. -func (f *ReadAsOfIterator) advance() bool { +// whose latest value (subject to f.asOF) has been deleted by a point or range +// tombstone. +func (f *ReadAsOfIterator) advance() { for { - if ok, err := f.Valid(); err != nil || !ok { - // No valid keys found. - return false - } else if !f.asOf.IsEmpty() && f.asOf.Less(f.iter.UnsafeKey().Timestamp) { - // Skip keys above the asOf timestamp. + if ok := f.updateValid(); !ok { + return + } + + if !f.asOf.IsEmpty() && f.asOf.Less(f.iter.UnsafeKey().Timestamp) { + // Skip keys above the asOf timestamp, regardless of type of key (e.g. point or range) + f.iter.Next() + } else if hasPoint, hasRange := f.iter.HasPointAndRange(); !hasPoint && hasRange { + // Bare range keys get surfaced before the point key, even though the + // point key shadows it; thus, because we can infer range key information + // when the point key surfaces, skip over the bare range key, and reason + // about shadowed keys at the surfaced point key. + // + // E.g. Scanning the keys below: + // 2 a2 + // 1 o---o + // a b + // + // would result in two surfaced keys: + // {a-b}@1; + // a2, {a-b}@1 + f.iter.Next() } else if len(f.iter.UnsafeValue()) == 0 { - // Skip to the next MVCC key if we find a tombstone. + // Skip to the next MVCC key if we find a point tombstone. + f.iter.NextKey() + } else if !hasRange { + // On a valid key without a range key + return + } else if f.asOfRangeKeyShadows() { + // The latest range key, as of system time, shadows the latest point key. + // This key is therefore deleted as of system time. f.iter.NextKey() } else { - // On a valid key. - return true + // On a valid key that potentially shadows range key(s) + return } } } +// asOfRangeKeyShadows returns true if there exists a range key at or below the asOf timestamp +// that shadows the latest point key +// +// TODO (msbutler): ensure this function caches range key values (#84379) before +// the 22.2 branch cut, else we face a steep perf cliff for RESTORE with range keys. +func (f *ReadAsOfIterator) asOfRangeKeyShadows() (shadows bool) { + rangeKeys := f.iter.RangeKeys() + if f.asOf.IsEmpty() { + return f.iter.UnsafeKey().Timestamp.LessEq(rangeKeys[0].RangeKey.Timestamp) + } + return HasRangeKeyBetween(rangeKeys, f.asOf, f.iter.UnsafeKey().Timestamp) +} + // NewReadAsOfIterator constructs a ReadAsOfIterator. func NewReadAsOfIterator(iter SimpleMVCCIterator, asOf hlc.Timestamp) *ReadAsOfIterator { return &ReadAsOfIterator{iter: iter, asOf: asOf} diff --git a/pkg/storage/testdata/mvcc_histories/range_key_iter_read_as_of b/pkg/storage/testdata/mvcc_histories/range_key_iter_read_as_of new file mode 100644 index 000000000000..5b49bc68c726 --- /dev/null +++ b/pkg/storage/testdata/mvcc_histories/range_key_iter_read_as_of @@ -0,0 +1,168 @@ +# Tests range key handling in ReadAsOfIterator. Note that the iterator assumes it will not see an +# intent. +# +# Sets up the following dataset, where x is tombstone, o-o is range tombstone +# +# 6 f6 +# 5 o---------------o k5 +# 4 x x d4 f4 g4 x +# 3 o-------o e3 o-------oh3 o---o +# 2 a2 f2 g2 +# 1 o---------------------------------------o +# a b c d e f g h i j k l m n o +# +run ok +put_rangekey k=a end=k ts=1 +put k=a ts=2 v=a2 +del k=a ts=4 +put_rangekey k=b end=d ts=3 +del k=b ts=4 +put k=d ts=4 v=d4 +put k=e ts=3 v=e3 +put k=f ts=2 v=f2 +put k=g ts=2 v=g2 +put_rangekey k=f end=h ts=3 +put k=f ts=4 v=f4 +put_rangekey k=c end=g ts=5 +put k=f ts=6 v=f6 +put k=g ts=4 v=g4 +put k=h ts=3 v=h3 +del k=h ts=4 +put k=k ts=5 v=k5 +put_rangekey k=m end=n ts=3 localTs=2 +---- +>> at end: +rangekey: {a-b}/[1.000000000,0=/] +rangekey: {b-c}/[3.000000000,0=/ 1.000000000,0=/] +rangekey: {c-d}/[5.000000000,0=/ 3.000000000,0=/ 1.000000000,0=/] +rangekey: {d-f}/[5.000000000,0=/ 1.000000000,0=/] +rangekey: {f-g}/[5.000000000,0=/ 3.000000000,0=/ 1.000000000,0=/] +rangekey: {g-h}/[3.000000000,0=/ 1.000000000,0=/] +rangekey: {h-k}/[1.000000000,0=/] +rangekey: {m-n}/[3.000000000,0={localTs=2.000000000,0}/] +data: "a"/4.000000000,0 -> / +data: "a"/2.000000000,0 -> /BYTES/a2 +data: "b"/4.000000000,0 -> / +data: "d"/4.000000000,0 -> /BYTES/d4 +data: "e"/3.000000000,0 -> /BYTES/e3 +data: "f"/6.000000000,0 -> /BYTES/f6 +data: "f"/4.000000000,0 -> /BYTES/f4 +data: "f"/2.000000000,0 -> /BYTES/f2 +data: "g"/4.000000000,0 -> /BYTES/g4 +data: "g"/2.000000000,0 -> /BYTES/g2 +data: "h"/4.000000000,0 -> / +data: "h"/3.000000000,0 -> /BYTES/h3 +data: "k"/5.000000000,0 -> /BYTES/k5 + +# test range keys are ignored if above asOf, even with multiple range keys +run ok +iter_new_read_as_of asOfTs=2 +iter_seek_ge k=a +iter_scan +---- +iter_seek_ge: "a"/2.000000000,0=/BYTES/a2 +iter_scan: "a"/2.000000000,0=/BYTES/a2 +iter_scan: "f"/2.000000000,0=/BYTES/f2 +iter_scan: "g"/2.000000000,0=/BYTES/g2 +iter_scan: . + +# test range key at or below asOf properly shadows keys +run ok +iter_new_read_as_of asOfTs=3 +iter_seek_ge k=a +iter_scan +---- +iter_seek_ge: "a"/2.000000000,0=/BYTES/a2 +iter_scan: "a"/2.000000000,0=/BYTES/a2 +iter_scan: "e"/3.000000000,0=/BYTES/e3 +iter_scan: "h"/3.000000000,0=/BYTES/h3 +iter_scan: . + +# iterate over a few point tombstones at the asOf time +run ok +iter_new_read_as_of asOfTs=4 +iter_seek_ge k=a +iter_scan +---- +iter_seek_ge: "d"/4.000000000,0=/BYTES/d4 +iter_scan: "d"/4.000000000,0=/BYTES/d4 +iter_scan: "e"/3.000000000,0=/BYTES/e3 +iter_scan: "f"/4.000000000,0=/BYTES/f4 +iter_scan: "g"/4.000000000,0=/BYTES/g4 +iter_scan: . + +# iterate over ts 5-7 because the test is cheap +run ok +iter_new_read_as_of asOfTs=5 +iter_seek_ge k=a +iter_scan +---- +iter_seek_ge: "g"/4.000000000,0=/BYTES/g4 +iter_scan: "g"/4.000000000,0=/BYTES/g4 +iter_scan: "k"/5.000000000,0=/BYTES/k5 +iter_scan: . + +# iterate over ts 5-7 because the test is cheap +run ok +iter_new_read_as_of asOfTs=6 +iter_seek_ge k=a +iter_scan +---- +iter_seek_ge: "f"/6.000000000,0=/BYTES/f6 +iter_scan: "f"/6.000000000,0=/BYTES/f6 +iter_scan: "g"/4.000000000,0=/BYTES/g4 +iter_scan: "k"/5.000000000,0=/BYTES/k5 +iter_scan: . + +# iterate over ts 5-7 for completeness +run ok +iter_new_read_as_of asOfTs=7 +iter_seek_ge k=a +iter_scan +---- +iter_seek_ge: "f"/6.000000000,0=/BYTES/f6 +iter_scan: "f"/6.000000000,0=/BYTES/f6 +iter_scan: "g"/4.000000000,0=/BYTES/g4 +iter_scan: "k"/5.000000000,0=/BYTES/k5 +iter_scan: . + + +# test range key handling when asOf is empty +run ok +iter_new_read_as_of +iter_seek_ge k=a +iter_scan +---- +iter_seek_ge: "f"/6.000000000,0=/BYTES/f6 +iter_scan: "f"/6.000000000,0=/BYTES/f6 +iter_scan: "g"/4.000000000,0=/BYTES/g4 +iter_scan: "k"/5.000000000,0=/BYTES/k5 +iter_scan: . + +# seek to a point key shadowed by a range key +run ok +iter_new_read_as_of asOfTs=5 +iter_seek_ge k=d +---- +iter_seek_ge: "g"/4.000000000,0=/BYTES/g4 + +# seek to the start of a range key +run ok +iter_new_read_as_of asOfTs=5 +iter_seek_ge k=c +---- +iter_seek_ge: "g"/4.000000000,0=/BYTES/g4 + +# seek to the same point key, with AsOf empty +run ok +iter_new_read_as_of +iter_seek_ge k=d +---- +iter_seek_ge: "f"/6.000000000,0=/BYTES/f6 + +# attempt seek to the same point key, but ignore the range key because its above AsOf +run ok +iter_new_read_as_of asOfTs=4 +iter_seek_ge k=d +---- +iter_seek_ge: "d"/4.000000000,0=/BYTES/d4 From 0d036d467cc404b0119340df4285b1b07bc94248 Mon Sep 17 00:00:00 2001 From: Andrei Matei Date: Wed, 20 Jul 2022 14:14:45 -0400 Subject: [PATCH 3/9] sql/distsql: delete unused lazyInternalExecutor Release note: None --- pkg/sql/distsql/BUILD.bazel | 1 - pkg/sql/distsql/server.go | 41 ------------------------------------- 2 files changed, 42 deletions(-) diff --git a/pkg/sql/distsql/BUILD.bazel b/pkg/sql/distsql/BUILD.bazel index a7ed7c550d18..df99eda7ad1e 100644 --- a/pkg/sql/distsql/BUILD.bazel +++ b/pkg/sql/distsql/BUILD.bazel @@ -25,7 +25,6 @@ go_library( "//pkg/sql/sessiondata", "//pkg/sql/sessiondatapb", "//pkg/sql/sqltelemetry", - "//pkg/sql/sqlutil", "//pkg/util/envutil", "//pkg/util/log", "//pkg/util/mon", diff --git a/pkg/sql/distsql/server.go b/pkg/sql/distsql/server.go index df1a6c819f02..230c93970554 100644 --- a/pkg/sql/distsql/server.go +++ b/pkg/sql/distsql/server.go @@ -13,7 +13,6 @@ package distsql import ( "context" "io" - "sync" "time" "github.com/cockroachdb/cockroach/pkg/base" @@ -34,7 +33,6 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/sessiondata" "github.com/cockroachdb/cockroach/pkg/sql/sessiondatapb" "github.com/cockroachdb/cockroach/pkg/sql/sqltelemetry" - "github.com/cockroachdb/cockroach/pkg/sql/sqlutil" "github.com/cockroachdb/cockroach/pkg/util/envutil" "github.com/cockroachdb/cockroach/pkg/util/log" "github.com/cockroachdb/cockroach/pkg/util/mon" @@ -683,42 +681,3 @@ func (ds *ServerImpl) FlowStream(stream execinfrapb.DistSQL_FlowStreamServer) er } return err } - -// lazyInternalExecutor is a tree.InternalExecutor that initializes -// itself only on the first call to QueryRow. -type lazyInternalExecutor struct { - // Set when an internal executor has been initialized. - sqlutil.InternalExecutor - - // Used for initializing the internal executor exactly once. - once sync.Once - - // newInternalExecutor must be set when instantiating a lazyInternalExecutor, - // it provides an internal executor to use when necessary. - newInternalExecutor func() sqlutil.InternalExecutor -} - -var _ sqlutil.InternalExecutor = &lazyInternalExecutor{} - -func (ie *lazyInternalExecutor) QueryRowEx( - ctx context.Context, - opName string, - txn *kv.Txn, - opts sessiondata.InternalExecutorOverride, - stmt string, - qargs ...interface{}, -) (tree.Datums, error) { - ie.once.Do(func() { - ie.InternalExecutor = ie.newInternalExecutor() - }) - return ie.InternalExecutor.QueryRowEx(ctx, opName, txn, opts, stmt, qargs...) -} - -func (ie *lazyInternalExecutor) QueryRow( - ctx context.Context, opName string, txn *kv.Txn, stmt string, qargs ...interface{}, -) (tree.Datums, error) { - ie.once.Do(func() { - ie.InternalExecutor = ie.newInternalExecutor() - }) - return ie.InternalExecutor.QueryRow(ctx, opName, txn, stmt, qargs...) -} From 221ac5530bf73221acd09c7cb6365ad19e989d8c Mon Sep 17 00:00:00 2001 From: Andrei Matei Date: Tue, 12 Jul 2022 19:45:28 -0400 Subject: [PATCH 4/9] tracing: delete old field The RecordedSpan.RedactableLogs. This field was unused since 22.1. Release note: None --- pkg/server/node_tenant.go | 10 ++-------- pkg/server/node_tenant_test.go | 1 - pkg/util/tracing/crdbspan.go | 19 +++++++++---------- .../tracing/tracingpb/recorded_span.proto | 6 +----- 4 files changed, 12 insertions(+), 24 deletions(-) diff --git a/pkg/server/node_tenant.go b/pkg/server/node_tenant.go index 0f6a7760c004..8c046e4c76b4 100644 --- a/pkg/server/node_tenant.go +++ b/pkg/server/node_tenant.go @@ -13,7 +13,6 @@ package server import ( "github.com/cockroachdb/cockroach/pkg/roachpb" "github.com/cockroachdb/cockroach/pkg/util/tracing/tracingpb" - "github.com/cockroachdb/errors" "github.com/cockroachdb/redact" ) @@ -22,7 +21,8 @@ const TraceRedactedMarker = redact.RedactableString("verbose trace message redac // redactRecordingForTenant redacts the sensitive parts of log messages in the // recording if the tenant to which this recording is intended is not the system -// tenant (the system tenant gets an. See https://github.com/cockroachdb/cockroach/issues/70407. +// tenant (the system tenant gets an unredacted trace). +// See https://github.com/cockroachdb/cockroach/issues/70407. // The recording is modified in place. // // tenID is the tenant that will receive this recording. @@ -36,12 +36,6 @@ func redactRecordingForTenant(tenID roachpb.TenantID, rec tracingpb.Recording) e sp.TagGroups = nil for j := range sp.Logs { record := &sp.Logs[j] - if record.Message != "" && !sp.RedactableLogs { - // If Message is set, the record should have been produced by a 22.1 - // node that also sets RedactableLogs. - return errors.AssertionFailedf( - "recording has non-redactable span with the Message field set: %s", sp) - } record.Message = record.Message.Redact() } } diff --git a/pkg/server/node_tenant_test.go b/pkg/server/node_tenant_test.go index 180ea60a7991..9192d1cb43bd 100644 --- a/pkg/server/node_tenant_test.go +++ b/pkg/server/node_tenant_test.go @@ -121,7 +121,6 @@ func TestRedactRecordingForTenant(t *testing.T) { TagGroups []tracingpb.TagGroup StartTime time.Time Duration time.Duration - RedactableLogs bool Logs []tracingpb.LogRecord Verbose bool RecordingMode tracingpb.RecordingMode diff --git a/pkg/util/tracing/crdbspan.go b/pkg/util/tracing/crdbspan.go index c2ff400bbc8f..cbc062f32478 100644 --- a/pkg/util/tracing/crdbspan.go +++ b/pkg/util/tracing/crdbspan.go @@ -834,16 +834,15 @@ func (s *crdbSpan) getRecordingNoChildrenLocked( recordingType tracingpb.RecordingType, finishing bool, ) tracingpb.RecordedSpan { rs := tracingpb.RecordedSpan{ - TraceID: s.traceID, - SpanID: s.spanID, - ParentSpanID: s.parentSpanID, - GoroutineID: s.mu.goroutineID, - Operation: s.operation, - StartTime: s.startTime, - Duration: s.mu.duration, - RedactableLogs: true, - Verbose: s.recordingType() == tracingpb.RecordingVerbose, - RecordingMode: s.recordingType().ToProto(), + TraceID: s.traceID, + SpanID: s.spanID, + ParentSpanID: s.parentSpanID, + GoroutineID: s.mu.goroutineID, + Operation: s.operation, + StartTime: s.startTime, + Duration: s.mu.duration, + Verbose: s.recordingType() == tracingpb.RecordingVerbose, + RecordingMode: s.recordingType().ToProto(), } if rs.Duration == -1 { diff --git a/pkg/util/tracing/tracingpb/recorded_span.proto b/pkg/util/tracing/tracingpb/recorded_span.proto index 1cd832c3cf59..d79cfdb89f3a 100644 --- a/pkg/util/tracing/tracingpb/recorded_span.proto +++ b/pkg/util/tracing/tracingpb/recorded_span.proto @@ -101,10 +101,6 @@ message RecordedSpan { google.protobuf.Duration duration = 8 [(gogoproto.nullable) = false, (gogoproto.stdduration) = true]; - // RedactableLogs determines whether the verbose log messages are redactable. - // This field was introduced in the 22.1 release. It can be removed in the 22.2 - // release. On 22.1 this is always set to `true`. - bool redactable_logs = 15; // Events logged in the span. repeated LogRecord logs = 9 [(gogoproto.nullable) = false]; // verbose indicates whether the span was recording in verbose mode at the @@ -141,7 +137,7 @@ message RecordedSpan { // view of the various operations that are being traced as part of a span. map children_metadata = 19 [(gogoproto.nullable) = false]; - reserved 5,10,11; + reserved 5,10,11,15; } message TagGroup { From 066edbec1a8310ecd52928fa8bee9157b7974b3d Mon Sep 17 00:00:00 2001 From: Andrew Werner Date: Thu, 21 Jul 2022 00:58:43 -0400 Subject: [PATCH 5/9] sql/sqlinstance/instancestorage: use CommitInBatch to optimize round-trips By using CommitInBatch we can hit the 1PC optimization and avoid a round-trip to the leaseholder of the range in question. Release note: None --- pkg/sql/sqlinstance/instancestorage/instancestorage.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/pkg/sql/sqlinstance/instancestorage/instancestorage.go b/pkg/sql/sqlinstance/instancestorage/instancestorage.go index 2f32a49d8901..c8fb19d0af82 100644 --- a/pkg/sql/sqlinstance/instancestorage/instancestorage.go +++ b/pkg/sql/sqlinstance/instancestorage/instancestorage.go @@ -100,7 +100,9 @@ func (s *Storage) CreateInstance( log.Warningf(ctx, "failed to encode row for instance id %d: %v", instanceID, err) return err } - return txn.Put(ctx, row.Key, row.Value) + b := txn.NewBatch() + b.Put(row.Key, row.Value) + return txn.CommitInBatch(ctx, b) }) if err != nil { From f61c7db3a8e6783c3ff7abfda62708fa2c6426bc Mon Sep 17 00:00:00 2001 From: Marcus Gartner Date: Thu, 21 Jul 2022 10:48:01 -0400 Subject: [PATCH 6/9] opt: clarify plan gist comment Release note: None --- pkg/sql/opt/exec/explain/plan_gist_factory.go | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/pkg/sql/opt/exec/explain/plan_gist_factory.go b/pkg/sql/opt/exec/explain/plan_gist_factory.go index bad9481926d3..acc08e8e8d62 100644 --- a/pkg/sql/opt/exec/explain/plan_gist_factory.go +++ b/pkg/sql/opt/exec/explain/plan_gist_factory.go @@ -36,10 +36,14 @@ import ( func init() { if numOperators != 58 { - // If this error occurs please make sure the new op is the last one in order - // to not invalidate existing plan gists/hashes. If we are just adding an - // operator at the end there's no need to update version below and we can - // just bump the hardcoded literal here. + // This error occurs when an operator has been added or removed in + // pkg/sql/opt/exec/explain/factory.opt. If an operator is added at the + // end of factory.opt, simply adjust the hardcoded value above. If an + // operator is removed or added anywhere else in factory.opt, increment + // gistVersion below. Note that we currently do not have a mechanism for + // decoding gists of older versions. This means that if gistVersion is + // incremented in a release, upgrading a cluster to that release will + // cause decoding errors for any previously generated plan gists. panic(errors.AssertionFailedf("Operator field changed (%d), please update check and consider incrementing version", numOperators)) } } From a4e457eb0bf4c08efdd0249a621a8f0e58a77ea3 Mon Sep 17 00:00:00 2001 From: Yahor Yuzefovich Date: Thu, 21 Jul 2022 08:16:59 -0700 Subject: [PATCH 7/9] sql: fix recent leak of a context Release note: None --- pkg/sql/conn_executor_exec.go | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/pkg/sql/conn_executor_exec.go b/pkg/sql/conn_executor_exec.go index 9e6e4b02a1b2..af5da5a4dcca 100644 --- a/pkg/sql/conn_executor_exec.go +++ b/pkg/sql/conn_executor_exec.go @@ -279,9 +279,6 @@ func (ex *connExecutor) execStmtInOpenState( ast := parserStmt.AST ctx = withStatement(ctx, ast) - var cancelQuery context.CancelFunc - ctx, cancelQuery = contextutil.WithCancel(ctx) - makeErrEvent := func(err error) (fsm.Event, fsm.EventPayload, error) { ev, payload := ex.makeErrEvent(err, ast) return ev, payload, nil @@ -327,10 +324,9 @@ func (ex *connExecutor) execStmtInOpenState( ex.planner.EvalContext().Placeholders = pinfo } + var cancelQuery context.CancelFunc + ctx, cancelQuery = contextutil.WithCancel(ctx) ex.addActiveQuery(ast, formatWithPlaceholders(ast, ex.planner.EvalContext()), queryID, cancelQuery) - if ex.executorType != executorTypeInternal { - ex.metrics.EngineMetrics.SQLActiveStatements.Inc(1) - } // Make sure that we always unregister the query. It also deals with // overwriting res.Error to a more user-friendly message in case of query @@ -343,10 +339,6 @@ func (ex *connExecutor) execStmtInOpenState( <-doneAfterFunc } } - ex.removeActiveQuery(queryID, ast) - if ex.executorType != executorTypeInternal { - ex.metrics.EngineMetrics.SQLActiveStatements.Dec(1) - } // Detect context cancelation and overwrite whatever error might have been // set on the result before. The idea is that once the query's context is @@ -365,6 +357,12 @@ func (ex *connExecutor) execStmtInOpenState( retPayload = eventNonRetriableErrPayload{err: cancelchecker.QueryCanceledError} } + ex.removeActiveQuery(queryID, ast) + cancelQuery() + if ex.executorType != executorTypeInternal { + ex.metrics.EngineMetrics.SQLActiveStatements.Dec(1) + } + // If the query timed out, we intercept the error, payload, and event here // for the same reasons we intercept them for canceled queries above. // Overriding queries with a QueryTimedOut error needs to happen after @@ -386,6 +384,10 @@ func (ex *connExecutor) execStmtInOpenState( } }(ctx, res) + if ex.executorType != executorTypeInternal { + ex.metrics.EngineMetrics.SQLActiveStatements.Inc(1) + } + p := &ex.planner stmtTS := ex.server.cfg.Clock.PhysicalTime() ex.statsCollector.Reset(ex.applicationStats, ex.phaseTimes) @@ -505,7 +507,7 @@ func (ex *connExecutor) execStmtInOpenState( timeoutTicker = time.AfterFunc( timerDuration, func() { - ex.cancelQuery(queryID) + cancelQuery() queryTimedOut = true doneAfterFunc <- struct{}{} }) From 3f744ce2d6adee81a3390127225bed7ed17e1adb Mon Sep 17 00:00:00 2001 From: Yahor Yuzefovich Date: Thu, 21 Jul 2022 09:29:52 -0700 Subject: [PATCH 8/9] ccl/streamingccl/streamingest: skip TestTenantStreamingPauseResumeIngestion Refs: #84414 Reason: flaky test Generated by bin/skip-test. Release justification: non-production code changes Release note: None --- pkg/ccl/streamingccl/streamingest/stream_replication_e2e_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/pkg/ccl/streamingccl/streamingest/stream_replication_e2e_test.go b/pkg/ccl/streamingccl/streamingest/stream_replication_e2e_test.go index 2cf072739eb1..b2724d33370d 100644 --- a/pkg/ccl/streamingccl/streamingest/stream_replication_e2e_test.go +++ b/pkg/ccl/streamingccl/streamingest/stream_replication_e2e_test.go @@ -356,6 +356,7 @@ func TestTenantStreamingProducerJobTimedOut(t *testing.T) { func TestTenantStreamingPauseResumeIngestion(t *testing.T) { defer leaktest.AfterTest(t)() + skip.WithIssue(t, 84414, "flaky test") defer log.Scope(t).Close(t) // TODO(casper): now this has the same race issue with From 617c32aea4396e5b23bc33fc4abe698267e082d7 Mon Sep 17 00:00:00 2001 From: David Taylor Date: Thu, 21 Jul 2022 15:36:31 +0000 Subject: [PATCH 9/9] logcrash: fix test on arm Release note: none. --- pkg/util/log/logcrash/crash_reporting_test.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/pkg/util/log/logcrash/crash_reporting_test.go b/pkg/util/log/logcrash/crash_reporting_test.go index bf1fd40df91a..f35f2e1db97c 100644 --- a/pkg/util/log/logcrash/crash_reporting_test.go +++ b/pkg/util/log/logcrash/crash_reporting_test.go @@ -16,6 +16,7 @@ import ( "os" "regexp" "runtime" + "strings" "testing" "time" @@ -317,6 +318,7 @@ func TestCrashReportingSafeError(t *testing.T) { t.Run("safeErr", func(t *testing.T) { errStr := redact.Sprintf("%+v", test.err).Redact().StripMarkers() errStr = fileref.ReplaceAllString(errStr, "...$2:NN") + errStr = strings.ReplaceAll(errStr, "asm_arm64.s", "asm_amd64.s") if errStr != test.expErr { diff, _ := difflib.GetUnifiedDiffString(difflib.UnifiedDiff{ A: difflib.SplitLines(test.expErr),