Merge #153401 #153526 #153551 #153552 #153553

153401: batcheval: reliably remove GCHint when it expires r=tbg a=tbg

The GCHint would previously only be adjusted when GC would actually do work. However, conceivably (and reproducably), sometimes all of the GC work would be done under a GCThreshold that did not yet allow the GCHint to be collected. This would leave the GCHint dangling, and endlessly queue the range for GC over and over again.

Unconditionally clear the GCHint to avoid this.

The problem could reliably be reproduced using an [experiment]. It no longer reproduces as of this commit.

[experiment]: https://docs.google.com/document/d/1QajDmDPgICvwYeRmrFVKvqVRW_kwWMD5RAe_q6irCXc/edit?tab=t.0

See also this [Slack thread].

Found while investigating https://github.com/cockroachlabs/support/issues/3424.

#153564 was filed as a result of looking further into a log message involved in this investigation leading up to this PR.

[Slack thread]: https://cockroachlabs.slack.com/archives/G01G8LK77DK/p1757580849424569?thread_ts=1757428752.120299&cid=G01G8LK77DK

Epic: none

153526: kvcoord: reset HasBufferedAllPrecedingWrites for multi-batch flush r=miraradeva a=stevendanna

Fixes #153509
Release note: None

153551: roachtest: make gossip chaos test more resilient r=tbg a=tbg

See #153403 (comment).

Via backport:
Closes #153403.

Epic: none


153552: roachtest: make failure in follower-reads more helpful r=tbg a=tbg

... by telling the reader what to look for to dig in.

Improves #153404.

Epic: none

153553: roachtest: bump vmodule in gossip/restart-node-one r=tbg a=tbg

See #153441.

Epic: none

Co-authored-by: Tobias Grieger <[email protected]>
Co-authored-by: Steven Danna <[email protected]>

Author: 3 people

pkg/cmd/roachtest/tests/follower_reads.go

@@ -792,7 +792,8 @@ func verifySQLLatency(
 		}
 	}
 	if permitted := int(.2 * float64(len(perTenSeconds))); len(above) > permitted {
-		t.Fatalf("%d latency values (%v) are above target latency %v, %d permitted",
+		t.Fatalf("%d latency values (%v) are above target latency %v, %d permitted "+
+			`(search the cluster logs for 'SELECT v FROM "".mr_db.test WHERE k = $1' to look at the traces)`,
 			len(above), above, targetLatency, permitted)
 	}
 }

pkg/cmd/roachtest/tests/gossip.go

@@ -29,6 +29,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/roachprod"
 	"github.com/cockroachdb/cockroach/pkg/roachprod/install"
 	"github.com/cockroachdb/cockroach/pkg/roachprod/logger"
+	"github.com/cockroachdb/cockroach/pkg/testutils"
 	"github.com/cockroachdb/cockroach/pkg/util"
 	"github.com/cockroachdb/cockroach/pkg/util/allstacks"
 	"github.com/cockroachdb/cockroach/pkg/util/retry"
@@ -60,7 +61,17 @@ SELECT node_id
 			tBeforePing := timeutil.Now()
 			db := c.Conn(ctx, t.L(), node)
 			defer db.Close()
-			require.NoError(t, db.Ping())
+
+			testutils.SucceedsSoon(t, func() error {
+				// Having just shut down a node, the sql user table may be in the
+				// process of failing over, and if we're unlucky and try to open a new
+				// conn here, we can sometimes hit an internal 10s timeout should the
+				// failover take longer than usual.
+				//
+				// See https://github.com/cockroachdb/cockroach/issues/153403#issuecomment-3296381756.
+				return db.Ping()
+			})
+
 			tAfterPing := timeutil.Now()
 			if pingDur := tAfterPing.Sub(tBeforePing); pingDur > 20*time.Second {
 				t.L().Printf("sql connection ready after %.2fs", pingDur.Seconds())
@@ -387,8 +398,10 @@ func runGossipRestartNodeOne(ctx context.Context, t test.Test, c cluster.Cluster
 	// Reduce the scan max idle time to speed up evacuation of node 1.
 	settings := install.MakeClusterSettings(install.NumRacksOption(c.Spec().NodeCount))
 	settings.Env = append(settings.Env, "COCKROACH_SCAN_MAX_IDLE_TIME=5ms")
+	startOpts := option.DefaultStartOpts()
+	startOpts.RoachprodOpts.ExtraArgs = []string{"--vmodule=*=1"} // see #153441
 
-	c.Start(ctx, t.L(), option.DefaultStartOpts(), settings)
+	c.Start(ctx, t.L(), startOpts, settings)
 
 	db := c.Conn(ctx, t.L(), 1)
 	defer db.Close()
@@ -542,7 +555,8 @@ SELECT count(replicas)
 	c.Stop(ctx, t.L(), option.DefaultStopOpts(), c.Node(1))
 	// N.B. Since n1 was initially stripped of all the replicas, we must wait for full replication. Otherwise, the
 	// replica consistency checks may time out.
-	c.Start(ctx, t.L(), option.NewStartOpts(option.WaitForReplication()), install.MakeClusterSettings(), c.Node(1))
+	startOpts.WaitForReplicationFactor = 3
+	c.Start(ctx, t.L(), startOpts, install.MakeClusterSettings(), c.Node(1))
 }
 
 func runCheckLocalityIPAddress(ctx context.Context, t test.Test, c cluster.Cluster) {

pkg/kv/kvclient/kvcoord/txn_interceptor_write_buffer.go

@@ -1826,6 +1826,9 @@ func (twb *txnWriteBuffer) flushBufferAndSendBatch(
 		if err := requireAllFlushedRequestsProcessed(br.Responses); err != nil {
 			return nil, kvpb.NewError(err)
 		}
+
+		// We've written intents now, so this should be false.
+		ba.HasBufferedAllPrecedingWrites = false
 		ba.UpdateTxn(br.Txn)
 
 		return twb.wrapped.SendLocked(ctx, ba)

pkg/kv/kvclient/kvcoord/txn_interceptor_write_buffer_test.go

@@ -2671,6 +2671,49 @@ func TestTxnWriteBufferHasBufferedAllPrecedingWrites(t *testing.T) {
 	}
 }
 
+func TestTxnWriteBufferHasBufferedAllPrecedingWritesSplitFlush(t *testing.T) {
+	ctx := context.Background()
+	twb, mockSender, _ := makeMockTxnWriteBuffer(ctx)
+	txn := makeTxnProto()
+	txn.Sequence = 1
+	keyA, keyB, keyC := roachpb.Key("a"), roachpb.Key("b"), roachpb.Key("c")
+
+	// This Put will be completely buffered.
+	ba := &kvpb.BatchRequest{Header: kvpb.Header{Txn: &txn}}
+	putA := putArgs(keyA, "valA", txn.Sequence)
+	ba.Add(putA)
+
+	br, pErr := twb.SendLocked(ctx, ba)
+	require.Nil(t, pErr)
+	require.NotNil(t, br)
+
+	// Send a DeleteRange with MaxSpanRequestKeys set which will force a flush
+	// using two batches. The first should have HasBufferedAllPrecedingWrites, the
+	// next should not.
+	reqCount := 0
+	mockSender.MockSend(func(ba *kvpb.BatchRequest) (*kvpb.BatchResponse, *kvpb.Error) {
+		require.Equal(t, reqCount == 0, ba.HasBufferedAllPrecedingWrites,
+			"HasBufferedAllPrecedingWrites expected on the first request (and only the first request")
+		reqCount++
+		br = ba.CreateReply()
+		br.Txn = ba.Txn
+		return br, nil
+	})
+
+	txn.Sequence++
+	ba = &kvpb.BatchRequest{Header: kvpb.Header{Txn: &txn}}
+	ba.MaxSpanRequestKeys = 10
+	ba.Add(delRangeArgs(keyB, keyC, txn.Sequence))
+
+	beforeCallCount := mockSender.NumCalled()
+	br, pErr = twb.SendLocked(ctx, ba)
+	require.Nil(t, pErr)
+	require.NotNil(t, br)
+	require.Equal(t, 2, mockSender.NumCalled()-beforeCallCount)
+	require.Len(t, br.Responses, 1)
+	require.IsType(t, &kvpb.DeleteRangeResponse{}, br.Responses[0].GetInner())
+}
+
 // BenchmarkTxnWriteBuffer benchmarks the txnWriteBuffer. The test sets up a
 // transaction with an existing buffer and runs a single batch through
 // SendLocked and flushBufferAndSendBatch. The test varies the state of the

pkg/kv/kvserver/batcheval/cmd_gc.go

@@ -265,9 +265,7 @@ func GC(
 	// Check if optional GC hint on the range is expired (e.g. delete operation is
 	// older than GC threshold) and remove it. Otherwise this range could be
 	// unnecessarily GC'd with high priority again.
-	// We should only do that when we are doing actual cleanup as we want to have
-	// a hint when request is being handled.
-	if len(args.Keys) != 0 || len(args.RangeKeys) != 0 || args.ClearRange != nil {
+	{
 		sl := MakeStateLoader(cArgs.EvalCtx)
 		hint, err := sl.LoadGCHint(ctx, readWriter)
 		if err != nil {

pkg/kv/kvserver/mvcc_gc_queue.go

@@ -783,6 +783,12 @@ func (mgcq *mvccGCQueue) process(
 	if scoreAfter.ShouldQueue {
 		// The scores are very long, so splitting into multiple lines manually for
 		// readability.
+		//
+		// NB: there are likely situations in which this check triggers incorrectly,
+		// for example when the GC hint triggers GC but a protected timestamp
+		// prevents the GC threshold from advancing. In that case, not only did we
+		// run a GC cycle without improving anything, but we also pile up a stats
+		// recomputation. This is hopefully too rare to matter.
 		log.Dev.Infof(ctx, "GC still needed following GC, recomputing MVCC stats")
 		log.Dev.Infof(ctx, "old score %s", r)
 		log.Dev.Infof(ctx, "new score %s", scoreAfter)