diff --git a/DEPS.bzl b/DEPS.bzl index 242dc3a2a724..43511aeabdb4 100644 --- a/DEPS.bzl +++ b/DEPS.bzl @@ -1848,10 +1848,10 @@ def go_deps(): patches = [ "@com_github_cockroachdb_cockroach//build/patches:com_github_cockroachdb_pebble.patch", ], - sha256 = "23f265df2d72be18f8f4f7b770a1b893c323590f904dee8faa1343376aa5d195", - strip_prefix = "github.com/cockroachdb/pebble@v0.0.0-20240923205819-785dc8d817a4", + sha256 = "4ac5019c53c21899d5815e3570292a94a4d432c119e687ad794ee4417c0dc7e7", + strip_prefix = "github.com/cockroachdb/pebble@v0.0.0-20240926201817-0f785fec58c0", urls = [ - "https://storage.googleapis.com/cockroach-godeps/gomod/github.com/cockroachdb/pebble/com_github_cockroachdb_pebble-v0.0.0-20240923205819-785dc8d817a4.zip", + "https://storage.googleapis.com/cockroach-godeps/gomod/github.com/cockroachdb/pebble/com_github_cockroachdb_pebble-v0.0.0-20240926201817-0f785fec58c0.zip", ], ) go_repository( diff --git a/build/bazelutil/distdir_files.bzl b/build/bazelutil/distdir_files.bzl index 011398fbde17..9f7bc43ab95e 100644 --- a/build/bazelutil/distdir_files.bzl +++ b/build/bazelutil/distdir_files.bzl @@ -348,7 +348,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-20240923205819-785dc8d817a4.zip": "23f265df2d72be18f8f4f7b770a1b893c323590f904dee8faa1343376aa5d195", + "https://storage.googleapis.com/cockroach-godeps/gomod/github.com/cockroachdb/pebble/com_github_cockroachdb_pebble-v0.0.0-20240926201817-0f785fec58c0.zip": "4ac5019c53c21899d5815e3570292a94a4d432c119e687ad794ee4417c0dc7e7", "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", diff --git a/go.mod b/go.mod index 15e87539dad0..3ea34db0ac74 100644 --- a/go.mod +++ b/go.mod @@ -134,7 +134,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-20240923205819-785dc8d817a4 + github.com/cockroachdb/pebble v0.0.0-20240926201817-0f785fec58c0 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 diff --git a/go.sum b/go.sum index 9be520f819b5..9614ce824ae7 100644 --- a/go.sum +++ b/go.sum @@ -547,8 +547,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-20240923205819-785dc8d817a4 h1:oGsvcWxbx8fKwTOICX5g16XvXMAQ8YFAfsxxeHXbGrg= -github.com/cockroachdb/pebble v0.0.0-20240923205819-785dc8d817a4/go.mod h1:LaF8D6p4Ko4EWprBmrcDs7FrmZYgDExF7dJyPvbq3Y4= +github.com/cockroachdb/pebble v0.0.0-20240926201817-0f785fec58c0 h1:vDjqXrbNrsLHwvFTQG+EvhhhSPk2m4F27f9CEj7GUOo= +github.com/cockroachdb/pebble v0.0.0-20240926201817-0f785fec58c0/go.mod h1:LaF8D6p4Ko4EWprBmrcDs7FrmZYgDExF7dJyPvbq3Y4= 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= diff --git a/pkg/kv/kvserver/client_metrics_test.go b/pkg/kv/kvserver/client_metrics_test.go index 232f9c9cfbfd..eb6413c6ce9d 100644 --- a/pkg/kv/kvserver/client_metrics_test.go +++ b/pkg/kv/kvserver/client_metrics_test.go @@ -262,7 +262,7 @@ func TestStoreMetrics(t *testing.T) { }, } stickyServerArgs[i] = base.TestServerArgs{ - CacheSize: 1 << 20, /* 1 MiB */ + CacheSize: 2 << 20, /* 2 MiB */ StoreSpecs: []base.StoreSpec{spec}, Knobs: base.TestingKnobs{ Server: &server.TestingKnobs{ diff --git a/pkg/kv/kvserver/debug_print.go b/pkg/kv/kvserver/debug_print.go index 47dc11837e7b..dc40393cbce0 100644 --- a/pkg/kv/kvserver/debug_print.go +++ b/pkg/kv/kvserver/debug_print.go @@ -239,7 +239,7 @@ func decodeWriteBatch(writeBatch *kvserverpb.WriteBatch) (string, error) { // pebble.KeyKindDeleteSized is the most recent key kind, ensuring that // compilation will fail if it's not. Unfortunately, this doesn't protect // against reusing a currently unused RocksDB key kind. - const _ = uint(pebble.InternalKeyKindDeleteSized - pebble.InternalKeyKindMax) + const _ = uint(pebble.InternalKeyKindExcise - pebble.InternalKeyKindMax) if writeBatch == nil { return "\n", nil @@ -349,6 +349,19 @@ func decodeWriteBatch(writeBatch *kvserverpb.WriteBatch) (string, error) { } v, _ := binary.Uvarint(r.Value()) sb.WriteString(fmt.Sprintf("Delete (Sized at %d): %s\n", v, SprintEngineKey(engineKey))) + + case pebble.InternalKeyKindExcise: + engineStartKey, err := r.EngineKey() + if err != nil { + return sb.String(), err + } + engineEndKey, err := r.EngineEndKey() + if err != nil { + return sb.String(), err + } + sb.WriteString(fmt.Sprintf( + "Excise: [%s, %s)\n", SprintEngineKey(engineStartKey), SprintEngineKey(engineEndKey), + )) default: sb.WriteString(fmt.Sprintf("unsupported key kind: %d\n", r.KeyKind())) } diff --git a/pkg/storage/batch.go b/pkg/storage/batch.go index 14154ca231cb..6bc4fbc46813 100644 --- a/pkg/storage/batch.go +++ b/pkg/storage/batch.go @@ -20,11 +20,11 @@ import ( ) // Ensure that we always update the batch reader to consider any necessary -// updates when a new key kind is introduced. To do this, we assert -// InternalKeyKindMax=23, ensuring that compilation will fail if it's not. -// Unfortunately, this doesn't protect against reusing a currently unused -// RocksDB key kind. -const _ = uint(pebble.InternalKeyKindDeleteSized - pebble.InternalKeyKindMax) +// updates when a new key kind is introduced. To do this, we assert that the +// latest key we considered equals InternalKeyKindMax, ensuring that compilation +// will fail if it's not. Unfortunately, this doesn't protect against reusing a +// currently unused RocksDB key kind. +const _ = uint(pebble.InternalKeyKindExcise - pebble.InternalKeyKindMax) // decodeBatchHeader decodes the header of Pebble batch representation, // returning the parsed header and a batchrepr.Reader into the contents of the diff --git a/pkg/storage/pebble.go b/pkg/storage/pebble.go index d59a3f069ae2..9b304e3fbdb0 100644 --- a/pkg/storage/pebble.go +++ b/pkg/storage/pebble.go @@ -394,18 +394,22 @@ func ShouldUseEFOS(settings *settings.Values) bool { // cockroach suffixes (which are composed of the version and a trailing sentinel // byte); the version can be an MVCC timestamp or a lock key. func EngineSuffixCompare(a, b []byte) int { - // NB: For performance, this routine manually splits the key into the - // user-key and version components rather than using DecodeEngineKey. In - // most situations, use DecodeEngineKey or GetKeyPartFromEngineKey or - // SplitMVCCKey instead of doing this. if len(a) == 0 || len(b) == 0 { // Empty suffixes sort before non-empty suffixes. return cmp.Compare(len(a), len(b)) } - return bytes.Compare( - normalizeEngineSuffixForCompare(b), - normalizeEngineSuffixForCompare(a), - ) + // Here we are not using normalizeEngineKeyVersionForCompare for historical + // reasons, summarized in + // https://github.com/cockroachdb/cockroach/issues/130533. + + // Check and strip off sentinel bytes. + if buildutil.CrdbTestBuild && len(a) != int(a[len(a)-1]) { + panic(errors.AssertionFailedf("malformed suffix: %x", a)) + } + if buildutil.CrdbTestBuild && len(b) != int(b[len(b)-1]) { + panic(errors.AssertionFailedf("malformed suffix: %x", b)) + } + return bytes.Compare(b[:len(b)-1], a[:len(a)-1]) } func checkEngineKey(k []byte) { @@ -447,8 +451,6 @@ func EngineKeySplit(k []byte) int { // EngineKeyCompare compares cockroach keys, including the version (which // could be MVCC timestamps). func EngineKeyCompare(a, b []byte) int { - // TODO(radu): Pebble sometimes passes empty "keys" and we have to tolerate - // them until we fix that. if len(a) == 0 || len(b) == 0 { return cmp.Compare(len(a), len(b)) } @@ -484,8 +486,6 @@ func EngineKeyCompare(a, b []byte) int { // EngineKeyEqual checks for equality of cockroach keys, including the version // (which could be MVCC timestamps). func EngineKeyEqual(a, b []byte) bool { - // TODO(radu): Pebble sometimes passes empty "keys" and we have to tolerate - // them until we fix that. if len(a) == 0 || len(b) == 0 { return len(a) == len(b) } @@ -2320,7 +2320,7 @@ func (p *Pebble) IngestAndExciseFiles( Start: EngineKey{Key: exciseSpan.Key}.Encode(), End: EngineKey{Key: exciseSpan.EndKey}.Encode(), } - return p.db.IngestAndExcise(ctx, paths, shared, external, rawSpan, sstsContainExciseTombstone) + return p.db.IngestAndExcise(ctx, paths, shared, external, rawSpan) } // IngestExternalFiles implements the Engine interface. diff --git a/pkg/storage/pebble_test.go b/pkg/storage/pebble_test.go index f9d276f3585e..da18b5b3a2c5 100644 --- a/pkg/storage/pebble_test.go +++ b/pkg/storage/pebble_test.go @@ -98,24 +98,23 @@ func TestEngineComparer(t *testing.T) { ts3b := appendBytesToTimestamp(ts3, slices.Concat(zeroLogical[:], syntheticBit)) // We group versions by equality and in the expected ordering. - orderedVersions := [][]any{ - {ts1}, // Empty version sorts first. - {ts2, ts2a}, // Higher timestamps sort before lower timestamps. - {ts3, ts3a, ts3b}, - {ts4}, - {ts5}, + orderedVersions := []any{ + ts1, // Empty version sorts first. + ts2a, // Synthetic bit is not ignored when comparing suffixes. + ts2, + ts3b, // Higher timestamps sort before lower timestamps. + ts3a, + ts3, + ts4, + ts5, } // Compare suffixes. - for i := range orderedVersions { - for j := range orderedVersions { - for _, v1 := range orderedVersions[i] { - for _, v2 := range orderedVersions[j] { - result := EngineComparer.CompareSuffixes(encodeVersion(v1), encodeVersion(v2)) - if expected := cmp.Compare(i, j); result != expected { - t.Fatalf("CompareSuffixes(%x, %x) = %d, expected %d", v1, v2, result, expected) - } - } + for i, v1 := range orderedVersions { + for j, v2 := range orderedVersions { + result := EngineComparer.CompareSuffixes(encodeVersion(v1), encodeVersion(v2)) + if expected := cmp.Compare(i, j); result != expected { + t.Fatalf("CompareSuffixes(%x, %x) = %d, expected %d", v1, v2, result, expected) } } }