Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Browse files
Browse the repository at this point in the history
94438: sql,storage: add support for COL_BATCH_RESPONSE scan format r=yuzefovich a=yuzefovich
This commit introduces a new `COL_BATCH_RESPONSE` scan format for Scans
and ReverseScans which results only in needed columns to be returned
from the KV server. In other words, this commit introduces the ability
to perform the KV projection pushdown.
The main idea of this feature is to use the injected decoding logic from
SQL in order to process each KV and keep only the needed parts (i.e.
necessary SQL columns). Those needed parts are then propagated back to
the KV client as coldata.Batch'es (serialized in the Apache Arrow format).
Here is the outline of all components involved:
```
┌────────────────────────────────────────────────┐
│ SQL │
│________________________________________________│
│ colfetcher.ColBatchDirectScan │
│ │ │
│ ▼ │
│ row.txnKVFetcher │
│ (behind the row.KVBatchFetcher interface) │
└────────────────────────────────────────────────┘
│
▼
┌────────────────────────────────────────────────┐
│ KV Client │
└────────────────────────────────────────────────┘
│
▼
┌────────────────────────────────────────────────┐
│ KV Server │
│________________________________________________│
│ colfetcher.cFetcherWrapper │
│ (behind the storage.CFetcherWrapper interface) │
│ │ │
│ ▼ │
│ colfetcher.cFetcher │
│ │ │
│ ▼ │
│ storage.mvccScanFetchAdapter ────────┐│
│ (behind the storage.NextKVer interface) ││
│ │ ││
│ ▼ ││
│ storage.pebbleMVCCScanner ││
│ (which put's KVs into storage.singleResults) <┘│
└────────────────────────────────────────────────┘
```
On the KV client side, `row.txnKVFetcher` issues Scans and ReverseScans with
the `COL_BATCH_RESPONSE` format and returns the response (which contains the
columnar data) to the `colfetcher.ColBatchDirectScan`.
On the KV server side, we create a `storage.CFetcherWrapper` that asks the
`colfetcher.cFetcher` for the next `coldata.Batch`. The `cFetcher`, in turn,
fetches the next KV, decodes it, and keeps only values for the needed SQL
columns, discarding the rest of the KV. The KV is emitted by the
`mvccScanFetchAdapter` which - via the `singleResults` struct - exposes
access to the current KV that the `pebbleMVCCScanner` is pointing at.
Note that there is an additional "implicit synchronization" between
components that is not shown on this diagram. In particular,
`storage.singleResults.maybeTrimPartialLastRow` must be in sync with the
`colfetcher.cFetcher` which is achieved by
- the `cFetcher` exposing access to the first key of the last incomplete SQL
row via the `FirstKeyOfRowGetter`,
- the `singleResults` using that key as the resume key for the response,
- and the `cFetcher` removing that last partial SQL row when `NextKV()`
returns `partialRow=true`.
This "upstream" link (although breaking the layering a bit) allows us to
avoid a performance penalty for handling the case with multiple column
families. (This case is handled by the `storage.pebbleResults` via tracking
offsets into the `pebbleResults.repr`.)
This code structure deserves some elaboration. First, there is a mismatch
between the "push" mode in which the `pebbleMVCCScanner` operates and the
"pull" mode that the `NextKVer` exposes. The adaption between two different
modes is achieved via the `mvccScanFetcherAdapter` grabbing (when the control
returns to it) the current unstable KV pair from the `singleResults` struct
which serves as a one KV pair buffer that the `pebbleMVCCScanner` `put`s into.
Second, in order be able to use the unstable KV pair without performing a
copy, the `pebbleMVCCScanner` stops at the current KV pair and returns the
control flow (which is exactly what `pebbleMVCCScanner.getOne` does) back to
the `mvccScanFetcherAdapter`, with the adapter advancing the scanner only when
the next KV pair is needed.
There are multiple scenarios which are currently not supported:
- SQL cannot issue Get requests (likely will support in 23.1)
- `TraceKV` option is not supported (likely will support in 23.1)
- user-defined types other than enums are not supported (will _not_
support in 23.1)
- non-default key locking strength as well as SKIP LOCKED wait policy
are not supported (will _not_ support in 23.1).
The usage of this feature is currently disabled by default, but I intend
to enable it by default for multi-tenant setups. The rationale is that
currently there is a large performance hit when enabling it for
single-tenant deployments whereas it offers significant speed up in the
multi-tenant world.
The microbenchmarks [show](https://gist.github.com/yuzefovich/669c295a8a4fdffa6490532284c5a719)
the expected improvement in multi-tenant setups when the tenant runs in
a separate process whenever we don't need to decode all of the columns
from the table.
The TPCH numbers, though, don't show the expected speedup:
```
Q1: before: 11.47s after: 8.84s -22.89%
Q2: before: 0.41s after: 0.29s -27.71%
Q3: before: 7.89s after: 9.68s 22.63%
Q4: before: 4.48s after: 4.52s 0.86%
Q5: before: 10.39s after: 10.35s -0.29%
Q6: before: 33.57s after: 33.41s -0.48%
Q7: before: 23.82s after: 23.81s -0.02%
Q8: before: 3.78s after: 3.76s -0.68%
Q9: before: 28.15s after: 28.03s -0.42%
Q10: before: 5.00s after: 4.98s -0.42%
Q11: before: 2.44s after: 2.44s 0.22%
Q12: before: 34.78s after: 34.65s -0.37%
Q13: before: 3.20s after: 2.94s -8.28%
Q14: before: 3.13s after: 3.21s 2.43%
Q15: before: 16.80s after: 16.73s -0.38%
Q16: before: 1.60s after: 1.65s 2.96%
Q17: before: 0.85s after: 0.96s 13.04%
Q18: before: 16.39s after: 15.47s -5.61%
Q19: before: 13.76s after: 13.01s -5.45%
Q20: before: 55.33s after: 55.12s -0.38%
Q21: before: 24.31s after: 24.31s -0.00%
Q22: before: 1.28s after: 1.41s 10.26%
```
At the moment, `coldata.Batch` that is included into the response is
always serialized into the Arrow format, but I intend to introduce the
local fastpath to avoid that serialization. That work will be done in
a follow-up and should be able to reduce the perf hit for single-tenant
deployments.
A quick note on the TODOs sprinkled in this commit:
- `TODO(yuzefovich)` means that this will be left for 23.2 or later.
- `TODO(yuzefovich, 23.1)` means that it should be addressed in 23.1.
A quick note on testing: this commit randomizes the fact whether the new
infrastructure is used in almost all test builds. Introducing some unit
testing (say, in `storage` package) seems rather annoying since we must
create keys that are valid SQL keys (i.e. have TableID / Index ID
prefix) and need to come with the corresponding
`fetchpb.IndexFetchSpec`. Not having unit tests in the `storage` seems
ok to me given that the "meat" of the work there is still done by the
`pebbleMVCCScanner` which is exercised using the regular Scans.
End-to-end testing is well covered by all of our existing tests which
now runs randomly. I did run the CI multiple times with the new feature
enabled by default with no failure, so I hope that it shouldn't become
flaky.
Addresses: #82323.
Informs: #87610.
Epic: CRDB-14837
Release note: None
95701: gossip: Track latency by nodeID rather than addr r=kvoli,erikgrinaker a=andrewbaptist
Previously the latency to remote nodes was tracked by address rather than the node's id. This could result in a few problems. First, the remote address could be reused across nodes. This could result in incorrect information. Additionally, places that used this information (such as the allocator) needed to unnecessarily map the node id to address just to do a lookup.
Finally in preparation for dialback on heartbeat #84289 the use of the OriginAddr field in the PingRequest will change to be the actual address that a node should use to dial back. Currently this field is not set correctly.
Epic: none
Release note: None
95796: ui: add CPU Time chart do statement details r=maryliag a=maryliag
This commit adds a new chart for CPU time
on Statement Details page.
Part Of #87213
<img width="1508" alt="Screen Shot 2023-01-24 at 6 01 07 PM" src="https://user-images.githubusercontent.com/1017486/214440274-c48d3bb6-ecbe-47a2-861a-0a8407d219c4.png">
Release note (ui change): Add CPU Time chart to Statement Details page.
95832: cdc: remove 'nonsensitive' tag from changefeed description in telemetry logs r=jayshrivastava a=jayshrivastava
Previously, the description field in changefeed telemetry logs was marked as `nonsensitive`. This is incorrect because the description field may contain an SQL statement which is not safe to report. This change removes the `nonsensitive` tag so the field is redacted by default.
Fixes: #95823
Epic: none
Release note: none
95838: logictest: remove smallEngineBlocks randomization r=yuzefovich a=yuzefovich
This metamorphic randomization has caused some flakiness (due to a subset of tests taking very long time) so is now removed. This feature should be tested in a more targeted fashion.
Fixes: #95799.
Fixes: #95829
Release note: None
95840: opt: replace make with dev in test instructions r=mgartner a=mgartner
Epic: None
Release note: None
95842: roachtest: fix parameters passed to require.NoError r=yuzefovich,srosenberg,herkolategan a=renatolabs
When context is passed to an assertion, the parameters *must* be a string format, followed by arguments (as you would in a call to `fmt.Sprintf`). The previous code would panic trying to cast int to string.
Informs #95416
Release note: None
Co-authored-by: Yahor Yuzefovich <[email protected]>
Co-authored-by: Andrew Baptist <[email protected]>
Co-authored-by: maryliag <[email protected]>
Co-authored-by: Jayant Shrivastava <[email protected]>
Co-authored-by: Marcus Gartner <[email protected]>
Co-authored-by: Renato Costa <[email protected]>- Loading branch information
