go.mod: revert comparer change and bump Pebble to 0f785fec58c0

The comparer changes effectively revert cockroachdb#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: cockroachdb#130533

Release note: none.
Epic: none.

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/[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(

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",

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

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=

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{

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 "<nil>\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()))
 		}

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

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.

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)
 			}
 		}
 	}