sql: ensure spans get anonymized in plan collection#34902
sql: ensure spans get anonymized in plan collection#34902craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
This change is |
jordanlewis
left a comment
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @celiala, @jordanlewis, and @knz)
pkg/sql/explain_tree.go, line 77 at r1 (raw file):
}, spans: func(nodeName, fieldName string, index *sqlbase.IndexDescriptor, spans []roachpb.Span) { spanss := sqlbase.PrettySpans(index, spans, 2)
The performance trouble here was that it's expensive to serialize long span strings. It's unfortunate that we're still calling PrettySpans, just to check to see whether the output is - or not. Can you introduce a function spans.FullScan or something that returns a boolean instead?
Now that I look, it's not trivial to write that function, so if you don't feel like it, maybe add a TODO or file an issue.
pkg/sql/walk.go, line 523 at r1 (raw file):
if v.observer.attr != nil { v.observer.attr(name, "from", n.desc.Name) spans := sqlbase.PrettySpans(&n.desc.PrimaryIndex, n.spans, 2)
There is also a call to PrettySpans here.
e513c08 to
f791112
Compare
|
TFYR! bors r+ |
knz
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/explain_tree.go, line 77 at r1 (raw file):
Previously, jordanlewis (Jordan Lewis) wrote…
The performance trouble here was that it's expensive to serialize long span strings. It's unfortunate that we're still calling
PrettySpans, just to check to see whether the output is-or not. Can you introduce a functionspans.FullScanor something that returns a boolean instead?Now that I look, it's not trivial to write that function, so if you don't feel like it, maybe add a TODO or file an issue.
Done.
pkg/sql/walk.go, line 523 at r1 (raw file):
Previously, jordanlewis (Jordan Lewis) wrote…
There is also a call to PrettySpans here.
Done.
Build failed |
f791112 to
0867873
Compare
|
bors r+ |
Release note (bug fix): The logical plans collected for display in the web UI now also hide the details of which key ranges are scanned in table lookups.
0867873 to
6640333
Compare
Canceled |
|
bors r+ |
34806: util/mon: Downgrade "bytes usage increases" log message r=knz a=bdarnell This message can be logged frequently in certain workloads, and its appearance in the info log is not actionable since there is no way to tie it back to the query that caused it. I see no reason to have these messages enabled by default. Release note: None 34886: storage/engine,libroach: 1x write amplification on ingest sst r=dt a=dt This uses the new flag added in facebook/rocksdb#4172 to avoid needing to make global_seq_no-related edits to SSTs, and thus avoid the SST copying those edits forced us to do to preserve the raft log immutability when hard-linking directly to side load SSTs. This is gated on a cluster version that is defined well after the upgrade to rocks 5.17, since using the flag requires that no older versions of RocksDB (<5.16), will ever be used to read these SSTs, as they will lack the seq_no that the older versions need for correctness. Release note (performance improvement): reduce write-amplification during bulk-loading (IMPORT and RESTORE) 34902: sql: ensure spans get anonymized in plan collection r=knz a=knz Fixes #34889. Release note (bug fix): The logical plans collected for display in the web UI now also hide the details of which key ranges are scanned in table lookups. Co-authored-by: Ben Darnell <ben@bendarnell.com> Co-authored-by: David Taylor <tinystatemachine@gmail.com> Co-authored-by: Raphael 'kena' Poss <knz@cockroachlabs.com>
Build succeeded |
Fixes #34889.
Release note (bug fix): The logical plans collected for display in the
web UI now also hide the details of which key ranges are scanned in
table lookups.