Skip to content

Commit

Permalink
Merge #128043
Browse files Browse the repository at this point in the history 
128043: storage: bump Pebble and fix Comparer r=RaduBerinde a=RaduBerinde

The `EngineComparer` has a bug when comparing bare suffixes - we call
the normalization function on a slice without the sentinel byte; that
function's logic is based on length so it will not function properly.

Currently, I don't think this bug can manifest in any visible problems
in practice - we compare bare suffixes only for range keys and those
suffixes never have a logical component or synthetic bit. It is
conceivable that we compare those with some actual key suffix (e.g.
when applying masking) but that would only matter if a range key
prefix and an existing key in that range have the same timestamp,
which should never happen.

We now have a separate `CompareSuffixes` function which is used for
bare suffixes. This simplifies the implementation, allows us to add
more assertions, and allows testing the `Compare` function (since the
result can be derived from `Split` and `CompareSuffixes`).

We also improve the comparer testing to include these special cases of
version bytes that must be ignored, and we use the new Pebble
`CheckComparer` test to verify the internal consistency of the
comparer.

Thanks to the more strict checks, several cases of passing
`roachpb.Key`s unencoded (i.e. without the sentinel byte) were found
and fixed.

Pebble changes:

 * [`1b22d0b7`](cockroachdb/pebble@1b22d0b7) base: add CompareSuffixes to Comparer
 * [`bcbe8d78`](cockroachdb/pebble@bcbe8d78) keyspan: use slices.Sort functions
 * [`8af904fa`](cockroachdb/pebble@8af904fa) sstable/block: move Compress, Decompress, DecompressedLen
 * [`21672e31`](cockroachdb/pebble@21672e31) sstable/block: move blockType to block.CompressionIndicator
 * [`c73ffdb8`](cockroachdb/pebble@c73ffdb8) sstable/block: move Compression enum from sstable
 * [`e3782de2`](cockroachdb/pebble@e3782de2) colblk: add keyspan block writer, reader, iterator
 * [`ad3a2f0b`](cockroachdb/pebble@ad3a2f0b) manifest: fix incorrect Annotator pointer aggregation
 * [`4589d053`](cockroachdb/pebble@4589d053) sstable: implement Writer.DeleteRange using RawWriter.EncodeSpan
 * [`6e60d6ba`](cockroachdb/pebble@6e60d6ba) internal/{rangedel,rangekey}: take Span by value
 * [`0ef38b39`](cockroachdb/pebble@0ef38b39) sstable: remove WriterOption
 * [`9cbad0b5`](cockroachdb/pebble@9cbad0b5) db: Add BenchmarkPointDeletedSwath
 * [`e326a015`](cockroachdb/pebble@e326a015) sstable: clean up use-filter-block logic
 * [`ce28942d`](cockroachdb/pebble@ce28942d) sstable: twoLevelIterator: unembed singleLevelIterator
 * [`b6400d63`](cockroachdb/pebble@b6400d63) sstable: rename NewIterWithBlockPropertyFiltersAndContextEtc
 * [`a6580b19`](cockroachdb/pebble@a6580b19) sstable: remove compaction iterator wrappers
 * [`1cc8f4c9`](cockroachdb/pebble@1cc8f4c9) sstable: remove NewIterWithBlockPropertyFilters
 * [`3fded19e`](cockroachdb/pebble@3fded19e) sstable: improve TrivialReaderProvider
 * [`034fd8ef`](cockroachdb/pebble@034fd8ef) db: remove IterOptions.TableFilter
 * [`cde3df0f`](cockroachdb/pebble@cde3df0f) manifest: clean up Level
 * [`368636f0`](cockroachdb/pebble@368636f0) sstable: show filenum when metadata is corrupted
 * [`f8e117d3`](cockroachdb/pebble@f8e117d3) db: fix missing close in TestOverlappingIngestedSSTs
 * [`d3209700`](cockroachdb/pebble@d3209700) deps: update snappy to 43d5d4c
 * [`f0a0e2bb`](cockroachdb/pebble@f0a0e2bb) sstable: split Writer into Writer and RawWriter
 * [`ed42fb43`](cockroachdb/pebble@ed42fb43) db: don't load large filter blocks for flushable ingests
 * [`ca2b210f`](cockroachdb/pebble@ca2b210f) db: strictly enforce prefix in [flushable]BatchIter.SeekPrefixGE
 * [`44e5fb43`](cockroachdb/pebble@44e5fb43) db: store *Comparer on Batch
 * [`e36d078a`](cockroachdb/pebble@e36d078a) manifest: improve `Annotator` interface with generics
 * [`1cedb60f`](cockroachdb/pebble@1cedb60f) sstable: don't skipBackward past empty blocks with hideObsolete
 * [`4cf4f852`](cockroachdb/pebble@4cf4f852) colblk: clarify PrefixBytes comments
 * [`2752abb9`](cockroachdb/pebble@2752abb9) keyspan: add contexts to keyspan iterators
 * [`54e5ba64`](cockroachdb/pebble@54e5ba64) db: use testkeys comparer in TestTracing
 * [`0e232926`](cockroachdb/pebble@0e232926) db: convert TestTracing to datadriven
 * [`89e5644a`](cockroachdb/pebble@89e5644a) db: Add BenchmarkQueueWorkload
 * [`72c3f550`](cockroachdb/pebble@72c3f550) base: make Compare more restrictive
 * [`4f01ec48`](cockroachdb/pebble@4f01ec48) sstable: plumb context to range iterator constructors
 * [`de0e731a`](cockroachdb/pebble@de0e731a) colblk: add facility for cloning an Array to a []T
 * [`a2f6d39a`](cockroachdb/pebble@a2f6d39a) colblk: randomize BundleSizes in block writer randomized test
 * [`b26e6326`](cockroachdb/pebble@b26e6326) colblk: implement a common interface across column data accessors
 * [`6492e866`](cockroachdb/pebble@6492e866) colblk: make decoder function signatures consistent
 * [`dbf9ab67`](cockroachdb/pebble@dbf9ab67) compact: remove unused parameter in pickAutoLPositive

Release note: none.
Fixes: #127914

Co-authored-by: Radu Berinde <[email protected]>
  • Loading branch information
@RaduBerinde
craig[bot] and RaduBerinde committed Aug 1, 2024
2 parents 244f10d + aac2b3e commit 62a0e62
Show file tree
Hide file tree
Showing 13 changed files with 297 additions and 189 deletions.
12 changes: 6 additions & 6 deletions DEPS.bzl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1708,10 +1708,10 @@ def go_deps():
patches = [
"@com_github_cockroachdb_cockroach//build/patches:com_github_cockroachdb_pebble.patch",
],
sha256 = "708d354be196883b6cfd1b013ec3569a39755419daa90307becb2f0a7543de68",
strip_prefix = "github.com/cockroachdb/[email protected]20240718162859-654324f90ba7",
sha256 = "f2a49c1bbd79769d8087ca0575120e8b8e46db42cde659a608e3360b791c1eec",
strip_prefix = "github.com/cockroachdb/[email protected]20240731203440-1b22d0b78ae4",
urls = [
"https://storage.googleapis.com/cockroach-godeps/gomod/github.com/cockroachdb/pebble/com_github_cockroachdb_pebble-v0.0.0-20240718162859-654324f90ba7.zip",
"https://storage.googleapis.com/cockroach-godeps/gomod/github.com/cockroachdb/pebble/com_github_cockroachdb_pebble-v0.0.0-20240731203440-1b22d0b78ae4.zip",
],
)
go_repository(
Expand Down Expand Up @@ -3827,10 +3827,10 @@ def go_deps():
name = "com_github_golang_snappy",
build_file_proto_mode = "disable_global",
importpath = "github.com/golang/snappy",
sha256 = "ea4545ca44ee990554094df6de440386a440a5bd99106e048939409d63beb423",
strip_prefix = "github.com/golang/[email protected].4",
sha256 = "a40a9145f6d7c1b2c356cf024f65e0f9cbf7efe2b89330ef4bb0763859b6fdc9",
strip_prefix = "github.com/golang/[email protected].5-0.20231225225746-43d5d4cd4e0e",
urls = [
"https://storage.googleapis.com/cockroach-godeps/gomod/github.com/golang/snappy/com_github_golang_snappy-v0.0.4.zip",
"https://storage.googleapis.com/cockroach-godeps/gomod/github.com/golang/snappy/com_github_golang_snappy-v0.0.5-0.20231225225746-43d5d4cd4e0e.zip",
],
)
go_repository(
Expand Down
4 changes: 2 additions & 2 deletions build/bazelutil/distdir_files.bzl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -334,7 +334,7 @@ DISTDIR_FILES = {
"https://storage.googleapis.com/cockroach-godeps/gomod/github.com/cockroachdb/gostdlib/com_github_cockroachdb_gostdlib-v1.19.0.zip": "c4d516bcfe8c07b6fc09b8a9a07a95065b36c2855627cb3514e40c98f872b69e",
"https://storage.googleapis.com/cockroach-godeps/gomod/github.com/cockroachdb/logtags/com_github_cockroachdb_logtags-v0.0.0-20230118201751-21c54148d20b.zip": "ca7776f47e5fecb4c495490a679036bfc29d95bd7625290cfdb9abb0baf97476",
"https://storage.googleapis.com/cockroach-godeps/gomod/github.com/cockroachdb/metamorphic/com_github_cockroachdb_metamorphic-v0.0.0-20231108215700-4ba948b56895.zip": "28c8cf42192951b69378cf537be5a9a43f2aeb35542908cc4fe5f689505853ea",
"https://storage.googleapis.com/cockroach-godeps/gomod/github.com/cockroachdb/pebble/com_github_cockroachdb_pebble-v0.0.0-20240718162859-654324f90ba7.zip": "708d354be196883b6cfd1b013ec3569a39755419daa90307becb2f0a7543de68",
"https://storage.googleapis.com/cockroach-godeps/gomod/github.com/cockroachdb/pebble/com_github_cockroachdb_pebble-v0.0.0-20240731203440-1b22d0b78ae4.zip": "f2a49c1bbd79769d8087ca0575120e8b8e46db42cde659a608e3360b791c1eec",
"https://storage.googleapis.com/cockroach-godeps/gomod/github.com/cockroachdb/redact/com_github_cockroachdb_redact-v1.1.5.zip": "11b30528eb0dafc8bc1a5ba39d81277c257cbe6946a7564402f588357c164560",
"https://storage.googleapis.com/cockroach-godeps/gomod/github.com/cockroachdb/returncheck/com_github_cockroachdb_returncheck-v0.0.0-20200612231554-92cdbca611dd.zip": "ce92ba4352deec995b1f2eecf16eba7f5d51f5aa245a1c362dfe24c83d31f82b",
"https://storage.googleapis.com/cockroach-godeps/gomod/github.com/cockroachdb/stress/com_github_cockroachdb_stress-v0.0.0-20220803192808-1806698b1b7b.zip": "3fda531795c600daf25532a4f98be2a1335cd1e5e182c72789bca79f5f69fcc1",
Expand Down Expand Up @@ -542,7 +542,7 @@ DISTDIR_FILES = {
"https://storage.googleapis.com/cockroach-godeps/gomod/github.com/golang/groupcache/com_github_golang_groupcache-v0.0.0-20210331224755-41bb18bfe9da.zip": "b27034e8fc013627543e1ad098cfc65329f2896df3da5cf3266cc9166f93f3a5",
"https://storage.googleapis.com/cockroach-godeps/gomod/github.com/golang/mock/com_github_golang_mock-v1.6.0.zip": "fa25916b546f90da49418f436e3a61e4c5dae898cf3c82b0007b5a6fab74261b",
"https://storage.googleapis.com/cockroach-godeps/gomod/github.com/golang/protobuf/com_github_golang_protobuf-v1.5.3.zip": "93bda6e88d4a0a493a98b481de67a10000a755d15f16a800b49a6b96d1bd6f81",
"https://storage.googleapis.com/cockroach-godeps/gomod/github.com/golang/snappy/com_github_golang_snappy-v0.0.4.zip": "ea4545ca44ee990554094df6de440386a440a5bd99106e048939409d63beb423",
"https://storage.googleapis.com/cockroach-godeps/gomod/github.com/golang/snappy/com_github_golang_snappy-v0.0.5-0.20231225225746-43d5d4cd4e0e.zip": "a40a9145f6d7c1b2c356cf024f65e0f9cbf7efe2b89330ef4bb0763859b6fdc9",
"https://storage.googleapis.com/cockroach-godeps/gomod/github.com/golangci/lint-1/com_github_golangci_lint_1-v0.0.0-20181222135242-d2cdd8c08219.zip": "2806ffd1a35b26a29b4cea86eb5ae421636b317e33e261fc1c20f9cf8fec2db5",
"https://storage.googleapis.com/cockroach-godeps/gomod/github.com/gomodule/redigo/com_github_gomodule_redigo-v1.7.1-0.20190724094224-574c33c3df38.zip": "f665942b590c65e87284d681ea2784d0b9873c644756f4716a9972dc0d8e804e",
"https://storage.googleapis.com/cockroach-godeps/gomod/github.com/gonum/blas/com_github_gonum_blas-v0.0.0-20181208220705-f22b278b28ac.zip": "bfcad082317ace0d0bdc0832f0835d95aaa90f91cf3fce5d2d81ccdd70c38620",
Expand Down
4 changes: 2 additions & 2 deletions go.mod
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ require (
github.com/golang/geo v0.0.0-20200319012246-673a6f80352d
github.com/golang/mock v1.6.0
github.com/golang/protobuf v1.5.3
github.com/golang/snappy v0.0.4
github.com/golang/snappy v0.0.5-0.20231225225746-43d5d4cd4e0e
github.com/google/btree v1.0.1
github.com/google/pprof v0.0.0-20210827144239-02619b876842
github.com/google/uuid v1.5.0
Expand Down Expand Up @@ -126,7 +126,7 @@ require (
github.com/cockroachdb/go-test-teamcity v0.0.0-20191211140407-cff980ad0a55
github.com/cockroachdb/gostdlib v1.19.0
github.com/cockroachdb/logtags v0.0.0-20230118201751-21c54148d20b
github.com/cockroachdb/pebble v0.0.0-20240718162859-654324f90ba7
github.com/cockroachdb/pebble v0.0.0-20240731203440-1b22d0b78ae4
github.com/cockroachdb/redact v1.1.5
github.com/cockroachdb/returncheck v0.0.0-20200612231554-92cdbca611dd
github.com/cockroachdb/stress v0.0.0-20220803192808-1806698b1b7b
Expand Down
7 changes: 4 additions & 3 deletions go.sum
View file
Original file line number Diff line number Diff line change
Expand Up @@ -520,8 +520,8 @@ github.com/cockroachdb/logtags v0.0.0-20230118201751-21c54148d20b h1:r6VH0faHjZe
github.com/cockroachdb/logtags v0.0.0-20230118201751-21c54148d20b/go.mod h1:Vz9DsVWQQhf3vs21MhPMZpMGSht7O/2vFW2xusFUVOs=
github.com/cockroachdb/metamorphic v0.0.0-20231108215700-4ba948b56895 h1:XANOgPYtvELQ/h4IrmPAohXqe2pWA8Bwhejr3VQoZsA=
github.com/cockroachdb/metamorphic v0.0.0-20231108215700-4ba948b56895/go.mod h1:aPd7gM9ov9M8v32Yy5NJrDyOcD8z642dqs+F0CeNXfA=
github.com/cockroachdb/pebble v0.0.0-20240718162859-654324f90ba7 h1:RoAJkOmI4TPJUSm2AjEJflbXs6dIjww5Ca5Bp/NoYHo=
github.com/cockroachdb/pebble v0.0.0-20240718162859-654324f90ba7/go.mod h1:KEqs35rpPXdUg4kfWzjBLWXaxt9mjm9P3UR64K3KsJo=
github.com/cockroachdb/pebble v0.0.0-20240731203440-1b22d0b78ae4 h1:GmM2CnYbCZhW8P16eu+BR+RyEMYbONLnMhAu7eSvwQo=
github.com/cockroachdb/pebble v0.0.0-20240731203440-1b22d0b78ae4/go.mod h1:6WlkohgvnCq8wLcCX2OPgxH0aa2IdwE2lXQv3qAZu9k=
github.com/cockroachdb/redact v1.1.3/go.mod h1:BVNblN9mBWFyMyqK1k3AAiSxhvhfK2oOZZ2lK+dpvRg=
github.com/cockroachdb/redact v1.1.5 h1:u1PMllDkdFfPWaNGMyLD1+so+aq3uUItthCFqzwPJ30=
github.com/cockroachdb/redact v1.1.5/go.mod h1:BVNblN9mBWFyMyqK1k3AAiSxhvhfK2oOZZ2lK+dpvRg=
Expand Down Expand Up @@ -1136,8 +1136,9 @@ github.com/golang/snappy v0.0.0-20170215233205-553a64147049/go.mod h1:/XxbfmMg8l
github.com/golang/snappy v0.0.0-20180518054509-2e65f85255db/go.mod h1:/XxbfmMg8lxefKM7IXC3fBNl/7bRcc72aCRzEWrmP2Q=
github.com/golang/snappy v0.0.1/go.mod h1:/XxbfmMg8lxefKM7IXC3fBNl/7bRcc72aCRzEWrmP2Q=
github.com/golang/snappy v0.0.3/go.mod h1:/XxbfmMg8lxefKM7IXC3fBNl/7bRcc72aCRzEWrmP2Q=
github.com/golang/snappy v0.0.4 h1:yAGX7huGHXlcLOEtBnF4w7FQwA26wojNCwOYAEhLjQM=
github.com/golang/snappy v0.0.4/go.mod h1:/XxbfmMg8lxefKM7IXC3fBNl/7bRcc72aCRzEWrmP2Q=
github.com/golang/snappy v0.0.5-0.20231225225746-43d5d4cd4e0e h1:4bw4WeyTYPp0smaXiJZCNnLrvVBqirQVreixayXezGc=
github.com/golang/snappy v0.0.5-0.20231225225746-43d5d4cd4e0e/go.mod h1:/XxbfmMg8lxefKM7IXC3fBNl/7bRcc72aCRzEWrmP2Q=
github.com/golangci/lint-1 v0.0.0-20181222135242-d2cdd8c08219/go.mod h1:/X8TswGSh1pIozq4ZwCfxS0WA5JGXguxk94ar/4c87Y=
github.com/gomodule/redigo v1.7.1-0.20190724094224-574c33c3df38/go.mod h1:B4C85qUVwatsJoIUNIfCRsp7qO0iAmpGFZ4EELWSbC4=
github.com/gonum/blas v0.0.0-20181208220705-f22b278b28ac/go.mod h1:P32wAyui1PQ58Oce/KYkOqQv8cVw1zAapXOl+dRFGbc=
Expand Down
2 changes: 2 additions & 0 deletions pkg/storage/BUILD.bazel
View file
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,7 @@ go_library(
"@com_github_cockroachdb_pebble//rangekey",
"@com_github_cockroachdb_pebble//replay",
"@com_github_cockroachdb_pebble//sstable",
"@com_github_cockroachdb_pebble//sstable/block",
"@com_github_cockroachdb_pebble//vfs",
"@com_github_cockroachdb_pebble//wal",
"@com_github_cockroachdb_redact//:redact",
Expand Down Expand Up @@ -204,6 +205,7 @@ go_test(
"@com_github_cockroachdb_pebble//objstorage",
"@com_github_cockroachdb_pebble//objstorage/objstorageprovider",
"@com_github_cockroachdb_pebble//sstable",
"@com_github_cockroachdb_pebble//sstable/block",
"@com_github_cockroachdb_pebble//vfs",
"@com_github_cockroachdb_redact//:redact",
"@com_github_kr_pretty//:pretty",
Expand Down
2 changes: 1 addition & 1 deletion pkg/storage/bench_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2124,7 +2124,7 @@ func BenchmarkMVCCScannerWithIntentsAndVersions(b *testing.B) {
opts := DefaultPebbleOptions().MakeWriterOptions(0, format)
writer := sstable.NewWriter(objstorageprovider.NewFileWritable(sstFile), opts)
for _, kv := range kvPairs {
require.NoError(b, writer.Add(
require.NoError(b, writer.Raw().Add(
pebble.MakeInternalKey(kv.key, 0 /* seqNum */, kv.kind), kv.value))
}
require.NoError(b, writer.Close())
Expand Down
2 changes: 1 addition & 1 deletion pkg/storage/engine_key.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ func (k EngineKey) Format(f fmt.State, c rune) {
// The motivation for the sentinel is that we configure the underlying storage
// engine (Pebble) with a Split function that can be used for constructing
// Bloom filters over just the Key field. However, the encoded Key must also
// look like an encoded EngineKey. By splitting, at Key + \x00, the Key looks
// look like an encoded EngineKey. By splitting at Key + \x00, the Key looks
// like an EngineKey with no Version.
const (
sentinel = '\x00'
Expand Down
24 changes: 12 additions & 12 deletions pkg/storage/engine_key_fuzz.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -90,8 +90,19 @@ func FuzzEngineKeysInvariants(f *testing.F) {
return ek, ok1
}

f.Fuzz(func(t *testing.T, a, b []byte) {
f.Fuzz(func(t *testing.T, a []byte, b []byte) {
t.Logf("a = 0x%x; b = 0x%x", a, b)
// We can only pass valid keys to the comparer.
ekA, okA := decodeEngineKey(t, a)
ekB, okB := decodeEngineKey(t, b)
if !okA || !okB {
return
}
errA := ekA.Validate()
errB := ekB.Validate()
if errA != nil || errB != nil {
return
}
cmp := compareEngineKeys(t, a, b)
if cmp == 0 {
return
Expand All @@ -116,17 +127,6 @@ func FuzzEngineKeysInvariants(f *testing.F) {
t.Errorf("Separator(0x%x, 0x%x) = 0x%x; but EngineKeyCompare(0x%x, 0x%x) = %d",
a, b, sep, sep, b, cmp)
}
ekA, okA := decodeEngineKey(t, a)
ekB, okB := decodeEngineKey(t, b)
if !okA || !okB {
return
}
errA := ekA.Validate()
errB := ekB.Validate()
// The below invariants only apply for valid keys.
if errA != nil || errB != nil {
return
}
t.Logf("ekA = %s (Key: 0x%x, Version: 0x%x); ekB = %s (Key: 0x%x, Version: 0x%x)",
ekA, ekA.Key, ekA.Version, ekB, ekB.Key, ekB.Version)

Expand Down
4 changes: 2 additions & 2 deletions pkg/storage/mvcc_history_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -300,7 +300,7 @@ func TestMVCCHistories(t *testing.T) {
}

// Dump rangedels.
if rdIter, err := r.NewRawRangeDelIter(sstable.NoFragmentTransforms); err != nil {
if rdIter, err := r.NewRawRangeDelIter(context.Background(), sstable.NoFragmentTransforms); err != nil {
return err
} else if rdIter != nil {
defer rdIter.Close()
Expand All @@ -325,7 +325,7 @@ func TestMVCCHistories(t *testing.T) {
}

// Dump range keys.
if rkIter, err := r.NewRawRangeKeyIter(sstable.NoFragmentTransforms); err != nil {
if rkIter, err := r.NewRawRangeKeyIter(context.Background(), sstable.NoFragmentTransforms); err != nil {
return err
} else if rkIter != nil {
defer rkIter.Close()
Expand Down
Loading

0 comments on commit 62a0e62

Please sign in to comment.