Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

storage: randomly clobber UnsafeKey() and UnsafeValue() in pebbleIterator #86657

Closed
erikgrinaker opened this issue Aug 23, 2022 · 1 comment · Fixed by #96685
Closed

storage: randomly clobber UnsafeKey() and UnsafeValue() in pebbleIterator #86657

erikgrinaker opened this issue Aug 23, 2022 · 1 comment · Fixed by #96685
Assignees
@jbowens
Labels
A-storage Relating to our storage engine (Pebble) on-disk storage. C-cleanup Tech debt, refactors, loose ends, etc. Solution not expected to significantly change behavior. O-qa T-storage Storage Team

Comments

@erikgrinaker
Copy link
Contributor

erikgrinaker commented Aug 23, 2022 •
edited by exalate-issue-sync bot
Loading

In #86440, we removed the unsafeMVCCIterator wrapping inside intentInterleavingIter in order to use a concrete pebbleIterator for performance. However, we lose the test coverage that unsafeMVCCIterator provided, by randomly clobbering unsafe byte slices. We should consider moving this into pebbleIterator itself, in addition to the unsafeMVCCIterator which is useful to wrap higher-level iterators.

#86440 (review)

Jira issue: CRDB-18861
@blathers-crl
Copy link

blathers-crl bot commented Aug 23, 2022

cc @cockroachdb/replication

@exalate-issue-sync exalate-issue-sync bot added T-storage Storage Team and removed T-kv-replication labels Oct 4, 2022
@blathers-crl blathers-crl bot added the A-storage Relating to our storage engine (Pebble) on-disk storage. label Oct 4, 2022
@jbowens jbowens added the C-cleanup Tech debt, refactors, loose ends, etc. Solution not expected to significantly change behavior. label Oct 10, 2022
@jbowens jbowens self-assigned this Feb 6, 2023
@jbowens jbowens mentioned this issue Feb 6, 2023
Merged
jbowens added a commit to jbowens/cockroach that referenced this issue Feb 8, 2023
@jbowens
storage/pebbleiter: mangle unsafe buffers between positioning methods
88392b2 
Randomly clobber the key and value buffers in order to ensure lifetimes are
respected. This commit extends the `pebbleiter.assertionIter` type used in
`crdb_test` builds that sits between pkg/storage and the pebble.Iterator type.
It now will randomly zero the buffers used to return the previous iterator
position's key and value, to ensure code isn't improperly relying on the
stability of the these keys.

Epic: None
Close cockroachdb#86657.
Release note: None
craig bot pushed a commit that referenced this issue Feb 9, 2023
@dhartunian @jbowens
@xinhaoz @rafiss
Merge #96109 #96685 #96832 #96893
faeebee 
96109: metrics: introduce CounterFloat64 and use for tenant RUs r=RaduBerinde,tbg a=dhartunian

Previously, we were prevented from using float64s directly in metric counters since counters were limited to ints. This led to the use of Gauges in situations where Counters would be preferable since we didn't have code to help manage a monotonically increasing float64 value.

This commit introduces some helpers around atomically adding float64s together and increasing one monotonically. Those primitives are composed into a `CounterFloat64` type further used to construct `AggCounterFloat64` which can be used in place of `AggCounter` to track per-tenant metrics.

The two `GaugeFloat64` types used for tenant `totalRU` and `totalKVRU` metrics are replaced with the new `CounterFloat64` type to properly reflect the fact that these are monotonically increasing values. This helps Prometheus when scraping these metrics to correctly account for missing data if necessary.

Resolves #68291

Epic: CRDB0-14536

Release note: None

96685: storage/pebbleiter: mangle unsafe buffers during positioning r=jbowens a=jbowens

**storage/pebbleiter: mangle unsafe buffers between positioning methods**

Randomly clobber the key and value buffers in order to ensure lifetimes are respected. This commit extends the `pebbleiter.assertionIter` type used in `crdb_test` builds that sits between pkg/storage and the pebble.Iterator type. It now will randomly zero the buffers used to return the previous iterator position's key and value, to ensure code isn't improperly relying on the stability of the these keys.

**storage: fix UnsafeKey() usage of invalid iterator**

Previously, CheckSSTConflicts would improperly call UnsafeKey on an exhausted
sstIter. This could result in undefined, inconsistent behavior as UnsafeKey
may point to arbitrary data once the iterator is exhausted.

**kv/kvserver/spanset: fix read of invalid iterator's UnsafeKey**

Previously, the spanset.MVCCIterator implementation would retrieve the
contained iterator's UsafeKey() on an invalid iterator in Next, NextKey and
Prev. This retrieval was harmless because it always checked the validity of the
iterator before using it, but recent assertions in test builds prevent this
usage.

Epic: None
Close #86657.
Informs #94141.
Release note: None

96832: ui: txn insights details fixes r=maryliag a=xinhaoz

See individual commits.

<img width="1115" alt="image" src="https://user-images.githubusercontent.com/20136951/217676969-94de02ba-1499-43c3-a542-ece3e5829a6c.png">
<img width="1911" alt="image" src="https://user-images.githubusercontent.com/20136951/217677020-c0a20035-f80b-472d-943d-a2bfd0c92b13.png">


96893: roachtest: test with sqlalchemy 2.0 r=rafiss a=rafiss

fixes #96880
backports for #96859 and #96856

Release note: None

Co-authored-by: David Hartunian <[email protected]>
Co-authored-by: Jackson Owens <[email protected]>
Co-authored-by: Xin Hao Zhang <[email protected]>
Co-authored-by: Rafi Shamim <[email protected]>
@craig craig bot closed this as completed in 8ee03c4 Feb 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@jbowens jbowens

Labels
A-storage Relating to our storage engine (Pebble) on-disk storage. C-cleanup Tech debt, refactors, loose ends, etc. Solution not expected to significantly change behavior. O-qa T-storage Storage Team
Projects
Storage
Archived in project
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

storage/pebbleiter: mangle unsafe buffers during positioning jbowens/cockroach
2 participants
@erikgrinaker @jbowens