Skip to content

Commit

Permalink
Merge #131366
Browse files Browse the repository at this point in the history 
131366: go.mod: revert comparer change and bump Pebble to 0f785fec58c0 r=RaduBerinde a=RaduBerinde

The comparer changes effectively revert #128043, which can cause
replica inconsistency during/after upgrades.

Changes:

 * [`0f785fec`](cockroachdb/pebble@0f785fec) metamorphic: abridge failure output
 * [`be56747f`](cockroachdb/pebble@be56747f) db: fix overlap check for flushable ingest excises
 * [`2569414a`](cockroachdb/pebble@2569414a) db: remove race in TestCrashOpenCrashAfterWALCreation
 * [`575f7a04`](cockroachdb/pebble@575f7a04) sstable: support columnar blocks in Layout.Describe
 * [`c88c7471`](cockroachdb/pebble@c88c7471) github: fix code cover publish workflow
 * [`c0fa4a9c`](cockroachdb/pebble@c0fa4a9c) sstable: populate CompareSuffixes on test4bSuffixComparer
 * [`3a76074f`](cockroachdb/pebble@3a76074f) sstable: set IndexPartitions property in columnar sstable writer
 * [`0595c1fb`](cockroachdb/pebble@0595c1fb) colblk: define behavior of KeyWriter.ComparePrev with no previous
 * [`01dcf575`](cockroachdb/pebble@01dcf575) base: make comparer tolerate empty keys
 * [`d73ab80f`](cockroachdb/pebble@d73ab80f) db: allow excises to unconditionally be flushable ingests
 * [`b34a3937`](cockroachdb/pebble@b34a3937) base: allow CompareSuffixes to be stricter than Compare
 * [`90356021`](cockroachdb/pebble@90356021) db: refactor replayWAL to use flushes to make versionEdits

Informs: #130533

Release note: none.
Epic: none.


Co-authored-by: Radu Berinde <[email protected]>
  • Loading branch information
@RaduBerinde
craig[bot] and RaduBerinde committed Sep 27, 2024
2 parents 946ebda + a15c74e commit 630e676
Show file tree
Hide file tree
Showing 9 changed files with 54 additions and 42 deletions.
6 changes: 3 additions & 3 deletions DEPS.bzl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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/[email protected]20240923205819-785dc8d817a4",
sha256 = "4ac5019c53c21899d5815e3570292a94a4d432c119e687ad794ee4417c0dc7e7",
strip_prefix = "github.com/cockroachdb/[email protected]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(
Expand Down
2 changes: 1 addition & 1 deletion build/bazelutil/distdir_files.bzl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
2 changes: 1 addition & 1 deletion go.mod
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions go.sum
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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=
Expand Down
2 changes: 1 addition & 1 deletion pkg/kv/kvserver/client_metrics_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand Down
15 changes: 14 additions & 1 deletion pkg/kv/kvserver/debug_print.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 "<nil>\n", nil
Expand Down Expand Up @@ -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()))
}
Expand Down
10 changes: 5 additions & 5 deletions pkg/storage/batch.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
26 changes: 13 additions & 13 deletions pkg/storage/pebble.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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))
}
Expand Down Expand Up @@ -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)
}
Expand Down Expand Up @@ -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.
Expand Down
29 changes: 14 additions & 15 deletions pkg/storage/pebble_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
}
}
Expand Down

0 comments on commit 630e676

Please sign in to comment.