Merge #105618 #105645 #105683 #105712 #105718

105618: cluster-ui: update database pages stories r=THardy98 a=THardy98

Epic: None

This change updates the stories for the databases, database details, and database table pages.

Release note: None

105645: roachtest: advance predecessor version to v23.1.2 r=THardy98 a=THardy98

Epic: None
Release note: None
Release justification: version bump

105683: protoutil: prefer MarshalToSizedBuffer over MarshalTo where possible r=erikgrinaker a=nvanbenschoten

This commit switches a number of callers of `protoutil.MarshalTo` over to `protoutil.MarshalToSizedBuffer`. The latter function is more strict in that it requires the the destination buffer to have a length of exactly `pb.Size()` bytes, as opposed to only requiring it to have a capacity of at least `pb.Size()` bytes. In return, it avoids a call to `pb.Size()`.

The three performance-sensitive callers that are changes here are:
- `roachpb.Value.SetProto`
- `raftlog.EncodeCommand`
- `storage.putBuffer.marshalMeta`

The commit also adds some testing-only assertions to verify that callers are using the functions correctly.

Epic: None
Release note: None

105712: sql: only skip import worker failure under race,stress,deadlock r=cucaroach a=cucaroach

Better to have the test running some of the time to catch regressions.

Informs: 102839
Epic: None
Release note: None


105718: Revert "storage: use size-carrying point tombstones" r=jbowens a=jbowens

This reverts commit 8edbf0f.

Epic: none
Release note: none
Informs #105700.
Informs #105697.

Co-authored-by: Thomas Hardy <[email protected]>
Co-authored-by: Nathan VanBenschoten <[email protected]>
Co-authored-by: Tommy Reilly <[email protected]>
Co-authored-by: Jackson Owens <[email protected]>

docs/generated/settings/settings-for-tenants.txt

@@ -303,4 +303,4 @@ trace.opentelemetry.collector	string		address of an OpenTelemetry trace collecto
 trace.snapshot.rate	duration	0s	if non-zero, interval at which background trace snapshots are captured	tenant-rw
 trace.span_registry.enabled	boolean	true	if set, ongoing traces can be seen at https://<ui>/#/debug/tracez	tenant-rw
 trace.zipkin.collector	string		the address of a Zipkin instance to receive traces, as <host>:<port>. If no port is specified, 9411 will be used.	tenant-rw
-version	version	1000023.1-14	set the active cluster version in the format '<major>.<minor>'	tenant-rw
+version	version	1000023.1-10	set the active cluster version in the format '<major>.<minor>'	tenant-rw

docs/generated/settings/settings.html

@@ -257,6 +257,6 @@
 <tr><td><div id="setting-trace-snapshot-rate" class="anchored"><code>trace.snapshot.rate</code></div></td><td>duration</td><td><code>0s</code></td><td>if non-zero, interval at which background trace snapshots are captured</td><td>Serverless/Dedicated/Self-Hosted</td></tr>
 <tr><td><div id="setting-trace-span-registry-enabled" class="anchored"><code>trace.span_registry.enabled</code></div></td><td>boolean</td><td><code>true</code></td><td>if set, ongoing traces can be seen at https://&lt;ui&gt;/#/debug/tracez</td><td>Serverless/Dedicated/Self-Hosted</td></tr>
 <tr><td><div id="setting-trace-zipkin-collector" class="anchored"><code>trace.zipkin.collector</code></div></td><td>string</td><td><code></code></td><td>the address of a Zipkin instance to receive traces, as &lt;host&gt;:&lt;port&gt;. If no port is specified, 9411 will be used.</td><td>Serverless/Dedicated/Self-Hosted</td></tr>
-<tr><td><div id="setting-version" class="anchored"><code>version</code></div></td><td>version</td><td><code>1000023.1-14</code></td><td>set the active cluster version in the format &#39;&lt;major&gt;.&lt;minor&gt;&#39;</td><td>Serverless/Dedicated/Self-Hosted</td></tr>
+<tr><td><div id="setting-version" class="anchored"><code>version</code></div></td><td>version</td><td><code>1000023.1-10</code></td><td>set the active cluster version in the format &#39;&lt;major&gt;.&lt;minor&gt;&#39;</td><td>Serverless/Dedicated/Self-Hosted</td></tr>
 </tbody>
 </table>

pkg/clusterversion/cockroach_versions.go

@@ -542,17 +542,6 @@ const (
 	// that (optionally) embed below-raft admission data.
 	V23_2_UseACRaftEntryEntryEncodings
 
-	// V23_2_PebbleFormatDeleteSizedAndObsolete upgrades Pebble's format major
-	// version to FormatDeleteSizedAndObsolete, allowing use of a new sstable
-	// format version Pebblev4. This version has two improvements:
-	//   a) It allows the use of DELSIZED point tombstones.
-	//   b) It encodes the obsolence of keys in a key-kind bit.
-	V23_2_PebbleFormatDeleteSizedAndObsolete
-
-	// V23_2_UseSizedPebblePointTombstones enables the use of Pebble's new
-	// DeleteSized operations.
-	V23_2_UseSizedPebblePointTombstones
-
 	// *************************************************
 	// Step (1) Add new versions here.
 	// Do not add new versions to a patch release.
@@ -954,14 +943,6 @@ var rawVersionsSingleton = keyedVersions{
 		Key:     V23_2_UseACRaftEntryEntryEncodings,
 		Version: roachpb.Version{Major: 23, Minor: 1, Internal: 10},
 	},
-	{
-		Key:     V23_2_PebbleFormatDeleteSizedAndObsolete,
-		Version: roachpb.Version{Major: 23, Minor: 1, Internal: 12},
-	},
-	{
-		Key:     V23_2_UseSizedPebblePointTombstones,
-		Version: roachpb.Version{Major: 23, Minor: 1, Internal: 14},
-	},
 
 	// *************************************************
 	// Step (2): Add new versions here.

pkg/cmd/roachtest/tests/tombstones.go

@@ -29,6 +29,12 @@ import (
 // registerPointTombstone registers the point tombstone test.
 func registerPointTombstone(r registry.Registry) {
 	r.Add(registry.TestSpec{
+		Skip: "pebble#2340",
+		SkipDetails: "This roachtest is implemented ahead of implementing and using " +
+			"pebble#2340 within Cockroach. Currently, this roachtest fails through " +
+			"a timeout because the disk space corresponding to the large KVs is " +
+			"never reclaimed. Once pebble#2340 is integrated into Cockroach, we " +
+			"expect this to begin passing, and we can un-skip it.",
 		Name:              "point-tombstone/heterogeneous-value-sizes",
 		Owner:             registry.OwnerStorage,
 		Cluster:           r.MakeClusterSpec(4),
@@ -130,7 +136,7 @@ func registerPointTombstone(r registry.Registry) {
 			require.LessOrEqual(t, statsAfterDeletes.livePercentage, 0.10)
 
 			// Wait for garbage collection to delete the non-live data.
-			targetSize := uint64(3 << 30) /* 3 GiB */
+			targetSize := uint64(2 << 30) /* 2 GB */
 			t.Status("waiting for garbage collection and compaction to reduce on-disk size to ", humanize.IBytes(targetSize))
 			m = c.NewMonitor(ctx, c.Range(1, 3))
 			m.Go(func(ctx context.Context) error {
@@ -166,7 +172,7 @@ type tableSizeInfo struct {
 }
 
 func (info tableSizeInfo) String() string {
-	return fmt.Sprintf("databaseID: %d, tableID: %d, rangeCount: %d, approxDiskBytes: %s, liveBytes: %s, totalBytes: %s, livePercentage: %.2f",
+	return fmt.Sprintf("databaseID: %d, tableID: %d, rangeCount: %d, approxDiskBytes: %s, liveBytes: %s, totalBytes: %s, livePercentage: %.1f",
 		info.databaseID,
 		info.tableID,
 		info.rangeCount,

pkg/kv/kvserver/batch_spanset_test.go

@@ -72,7 +72,7 @@ func TestSpanSetBatchBoundaries(t *testing.T) {
 	}
 
 	t.Run("writes before range", func(t *testing.T) {
-		if err := batch.ClearUnversioned(outsideKey.Key, storage.ClearOptions{}); !isWriteSpanErr(err) {
+		if err := batch.ClearUnversioned(outsideKey.Key); !isWriteSpanErr(err) {
 			t.Errorf("ClearUnversioned: unexpected error %v", err)
 		}
 		if err := batch.ClearRawRange(outsideKey.Key, outsideKey2.Key, true, true); !isWriteSpanErr(err) {
@@ -93,7 +93,7 @@ func TestSpanSetBatchBoundaries(t *testing.T) {
 	})
 
 	t.Run("writes after range", func(t *testing.T) {
-		if err := batch.ClearUnversioned(outsideKey3.Key, storage.ClearOptions{}); !isWriteSpanErr(err) {
+		if err := batch.ClearUnversioned(outsideKey3.Key); !isWriteSpanErr(err) {
 			t.Errorf("ClearUnversioned: unexpected error %v", err)
 		}
 		if err := batch.ClearRawRange(insideKey2.Key, outsideKey4.Key, true, true); !isWriteSpanErr(err) {
@@ -303,7 +303,7 @@ func TestSpanSetBatchTimestamps(t *testing.T) {
 	}
 
 	for _, batch := range []storage.Batch{batchBefore, batchNonMVCC} {
-		if err := batch.ClearUnversioned(wkey.Key, storage.ClearOptions{}); !isWriteSpanErr(err) {
+		if err := batch.ClearUnversioned(wkey.Key); !isWriteSpanErr(err) {
 			t.Errorf("ClearUnversioned: unexpected error %v", err)
 		}
 		{

pkg/kv/kvserver/kvstorage/init.go

@@ -504,7 +504,7 @@ func LoadAndReconcileReplicas(ctx context.Context, eng storage.Engine) ([]Replic
 			// TODO(tbg): if clearRangeData were in this package we could destroy more
 			// effectively even if for some reason we had in the past written state
 			// other than the HardState here (not supposed to happen, but still).
-			if err := eng.ClearUnversioned(logstore.NewStateLoader(repl.RangeID).RaftHardStateKey(), storage.ClearOptions{}); err != nil {
+			if err := eng.ClearUnversioned(logstore.NewStateLoader(repl.RangeID).RaftHardStateKey()); err != nil {
 				return nil, errors.Wrapf(err, "removing HardState for r%d", repl.RangeID)
 			}
 			log.Eventf(ctx, "removed legacy uninitialized replica for r%s", repl.RangeID)

pkg/kv/kvserver/logstore/sideload.go

@@ -126,7 +126,7 @@ func MaybeSideloadEntries(
 		{
 			data := make([]byte, raftlog.RaftCommandPrefixLen+e.Cmd.Size())
 			raftlog.EncodeRaftCommandPrefix(data[:raftlog.RaftCommandPrefixLen], typ, e.ID)
-			_, err := protoutil.MarshalTo(&e.Cmd, data[raftlog.RaftCommandPrefixLen:])
+			_, err := protoutil.MarshalToSizedBuffer(&e.Cmd, data[raftlog.RaftCommandPrefixLen:])
 			if err != nil {
 				return nil, 0, 0, 0, errors.Wrap(err, "while marshaling stripped sideloaded command")
 			}
@@ -215,7 +215,7 @@ func MaybeInlineSideloadedRaftCommand(
 	{
 		data := make([]byte, raftlog.RaftCommandPrefixLen+e.Cmd.Size())
 		raftlog.EncodeRaftCommandPrefix(data[:raftlog.RaftCommandPrefixLen], typ, e.ID)
-		_, err := protoutil.MarshalTo(&e.Cmd, data[raftlog.RaftCommandPrefixLen:])
+		_, err := protoutil.MarshalToSizedBuffer(&e.Cmd, data[raftlog.RaftCommandPrefixLen:])
 		if err != nil {
 			return nil, err
 		}

pkg/kv/kvserver/loqrecovery/record.go

@@ -107,7 +107,7 @@ func RegisterOfflineRecoveryEvents(
 			continue
 		}
 		if removeEvent {
-			if err := readWriter.ClearUnversioned(iter.UnsafeKey().Key, storage.ClearOptions{}); err != nil {
+			if err := readWriter.ClearUnversioned(iter.UnsafeKey().Key); err != nil {
 				processingErrors = errors.CombineErrors(processingErrors, errors.Wrapf(
 					err, "failed to delete replica recovery record at key %s", iter.UnsafeKey()))
 				continue

pkg/kv/kvserver/raftlog/payload.go

@@ -93,7 +93,7 @@ func EncodeCommand(
 				return nil, errors.AssertionFailedf("missing origin node for flow token returns")
 			}
 		}
-		if _, err := protoutil.MarshalTo(
+		if _, err := protoutil.MarshalToSizedBuffer(
 			raftAdmissionMeta,
 			data[preLen:preLen+admissionMetaLen],
 		); err != nil {
@@ -112,7 +112,7 @@ func EncodeCommand(
 	}
 
 	// Encode the rest of the command.
-	if _, err := protoutil.MarshalTo(command, data[preLen+admissionMetaLen:]); err != nil {
+	if _, err := protoutil.MarshalToSizedBuffer(command, data[preLen+admissionMetaLen:]); err != nil {
 		return nil, err
 	}
 	return data, nil

pkg/kv/kvserver/replica_consistency.go

@@ -543,7 +543,7 @@ func CalcReplicaDigest(
 		} else {
 			timestampBuf = timestampBuf[:size]
 		}
-		if _, err := protoutil.MarshalTo(&legacyTimestamp, timestampBuf); err != nil {
+		if _, err := protoutil.MarshalToSizedBuffer(&legacyTimestamp, timestampBuf); err != nil {
 			return err
 		}
 		if _, err := hasher.Write(timestampBuf); err != nil {
@@ -585,7 +585,7 @@ func CalcReplicaDigest(
 		} else {
 			timestampBuf = timestampBuf[:size]
 		}
-		if _, err := protoutil.MarshalTo(&legacyTimestamp, timestampBuf); err != nil {
+		if _, err := protoutil.MarshalToSizedBuffer(&legacyTimestamp, timestampBuf); err != nil {
 			return err
 		}
 		if _, err := hasher.Write(timestampBuf); err != nil {

pkg/kv/kvserver/replica_proposal_buf.go

@@ -951,7 +951,7 @@ func (b *propBuf) marshallLAIAndClosedTimestampToProposalLocked(
 	// capacity for this footer.
 	preLen := len(p.encodedCommand)
 	p.encodedCommand = p.encodedCommand[:preLen+buf.Size()]
-	_, err := protoutil.MarshalTo(buf, p.encodedCommand[preLen:])
+	_, err := protoutil.MarshalToSizedBuffer(buf, p.encodedCommand[preLen:])
 	return err
 }
 

pkg/kv/kvserver/replica_proposal_buf_test.go

@@ -321,7 +321,7 @@ func (pc proposalCreator) encodeProposal(p *ProposalData) []byte {
 	data := make([]byte, raftlog.RaftCommandPrefixLen, needed)
 	raftlog.EncodeRaftCommandPrefix(data, raftlog.EntryEncodingStandardWithoutAC, p.idKey)
 	data = data[:raftlog.RaftCommandPrefixLen+p.command.Size()]
-	if _, err := protoutil.MarshalTo(p.command, data[raftlog.RaftCommandPrefixLen:]); err != nil {
+	if _, err := protoutil.MarshalToSizedBuffer(p.command, data[raftlog.RaftCommandPrefixLen:]); err != nil {
 		panic(err)
 	}
 	return data

pkg/kv/kvserver/replica_raft.go

@@ -2601,10 +2601,7 @@ func handleTruncatedStateBelowRaftPreApply(
 		// avoid allocating when constructing Raft log keys (16 bytes).
 		prefix := prefixBuf.RaftLogPrefix()
 		for idx := currentTruncatedState.Index + 1; idx <= suggestedTruncatedState.Index; idx++ {
-			if err := readWriter.ClearUnversioned(
-				keys.RaftLogKeyFromPrefix(prefix, idx),
-				storage.ClearOptions{},
-			); err != nil {
+			if err := readWriter.ClearUnversioned(keys.RaftLogKeyFromPrefix(prefix, idx)); err != nil {
 				return false, errors.Wrapf(err, "unable to clear truncated Raft entries for %+v at index %d",
 					suggestedTruncatedState, idx)
 			}

pkg/kv/kvserver/replica_test.go

@@ -1823,7 +1823,7 @@ func TestOptimizePuts(t *testing.T) {
 			require.NoError(t, tc.engine.ClearMVCCRangeKey(storage.MVCCRangeKey{
 				StartKey: c.exKey, EndKey: c.exEndKey, Timestamp: hlc.MinTimestamp}))
 		} else if c.exKey != nil {
-			require.NoError(t, tc.engine.ClearUnversioned(c.exKey, storage.ClearOptions{}))
+			require.NoError(t, tc.engine.ClearUnversioned(c.exKey))
 		}
 	}
 }

pkg/kv/kvserver/spanset/batch.go

@@ -415,11 +415,6 @@ func (i *EngineIterator) Value() ([]byte, error) {
 	return i.i.Value()
 }
 
-// ValueLen is part of the storage.EngineIterator interface.
-func (i *EngineIterator) ValueLen() int {
-	return i.i.ValueLen()
-}
-
 // UnsafeRawEngineKey is part of the storage.EngineIterator interface.
 func (i *EngineIterator) UnsafeRawEngineKey() []byte {
 	return i.i.UnsafeRawEngineKey()
@@ -527,34 +522,34 @@ func (s spanSetWriter) checkAllowed(key roachpb.Key) error {
 	return nil
 }
 
-func (s spanSetWriter) ClearMVCC(key storage.MVCCKey, opts storage.ClearOptions) error {
+func (s spanSetWriter) ClearMVCC(key storage.MVCCKey) error {
 	if err := s.checkAllowed(key.Key); err != nil {
 		return err
 	}
-	return s.w.ClearMVCC(key, opts)
+	return s.w.ClearMVCC(key)
 }
 
-func (s spanSetWriter) ClearUnversioned(key roachpb.Key, opts storage.ClearOptions) error {
+func (s spanSetWriter) ClearUnversioned(key roachpb.Key) error {
 	if err := s.checkAllowed(key); err != nil {
 		return err
 	}
-	return s.w.ClearUnversioned(key, opts)
+	return s.w.ClearUnversioned(key)
 }
 
 func (s spanSetWriter) ClearIntent(
-	key roachpb.Key, txnDidNotUpdateMeta bool, txnUUID uuid.UUID, opts storage.ClearOptions,
+	key roachpb.Key, txnDidNotUpdateMeta bool, txnUUID uuid.UUID,
 ) error {
 	if err := s.checkAllowed(key); err != nil {
 		return err
 	}
-	return s.w.ClearIntent(key, txnDidNotUpdateMeta, txnUUID, opts)
+	return s.w.ClearIntent(key, txnDidNotUpdateMeta, txnUUID)
 }
 
-func (s spanSetWriter) ClearEngineKey(key storage.EngineKey, opts storage.ClearOptions) error {
+func (s spanSetWriter) ClearEngineKey(key storage.EngineKey) error {
 	if err := s.spans.CheckAllowed(SpanReadWrite, roachpb.Span{Key: key.Key}); err != nil {
 		return err
 	}
-	return s.w.ClearEngineKey(key, opts)
+	return s.w.ClearEngineKey(key)
 }
 
 func (s spanSetWriter) SingleClearEngineKey(key storage.EngineKey) error {

pkg/roachpb/data.go

@@ -497,7 +497,7 @@ func (v *Value) SetProto(msg protoutil.Message) error {
 	// directly into the Value.RawBytes field instead of allocating a separate
 	// []byte and copying.
 	v.ensureRawBytes(headerSize + msg.Size())
-	if _, err := protoutil.MarshalTo(msg, v.RawBytes[headerSize:]); err != nil {
+	if _, err := protoutil.MarshalToSizedBuffer(msg, v.RawBytes[headerSize:]); err != nil {
 		return err
 	}
 	// Special handling for timeseries data.

pkg/sql/importer/import_stmt_test.go

@@ -5422,7 +5422,9 @@ func TestImportWorkerFailure(t *testing.T) {
 	defer leaktest.AfterTest(t)()
 	defer log.Scope(t).Close(t)
 
-	skip.WithIssue(t, 102839, "flaky test")
+	skip.UnderStressWithIssue(t, 102839, "flaky test")
+	skip.UnderDeadlockWithIssue(t, 102839, "flaky test")
+	skip.UnderRaceWithIssue(t, 102839, "flaky test")
 
 	allowResponse := make(chan struct{})
 	params := base.TestClusterArgs{}