kv: allow manual GC that does not advance threshold

Relates to cockroachdb#60585.

This commit updates the GC queue's `process` method to allow GC even if
it determines that doing so would not advance the GC threshold. This
check is retained in the queue's `shouldQueue` method, so this change
does not impact automated queueing decisions. However, it does ensure
that if someone manually enqueues a range in the GC queue with the
`SkipShouldQueue` option, then the range is actually processed. This is
important because the GC threshold is the first thing that the
GC bumps when processing, meaning that the GC threshold alone is not a
definitive indication that there is no more work to perform on a range.
A processing pass that will not bump the threshold can still be useful,
especially with the long context timeout associated with manual
enqueuing.

Release note (ui change): Manually enqueueing a range in the GC queue
now properly respects the SkipShouldQueue option. This can be useful to
force the GC of a specific Range.

pkg/kv/kvserver/gc_queue.go

@@ -158,10 +158,15 @@ func (gcq *gcQueue) shouldQueue(
 	// Consult the protected timestamp state to determine whether we can GC and
 	// the timestamp which can be used to calculate the score.
 	_, zone := repl.DescAndZone()
-	canGC, _, gcTimestamp, _ := repl.checkProtectedTimestampsForGC(ctx, *zone.GC)
+	canGC, _, gcTimestamp, oldThreshold, newThreshold := repl.checkProtectedTimestampsForGC(ctx, *zone.GC)
 	if !canGC {
 		return false, 0
 	}
+	// If performing a GC will not advance the GC threshold, there's no reason
+	// to GC again.
+	if newThreshold.Equal(oldThreshold) {
+		return false, 0
+	}
 	r := makeGCQueueScore(ctx, repl, gcTimestamp, *zone.GC)
 	return r.ShouldQueue, r.FinalScore
 }
@@ -442,7 +447,7 @@ func (gcq *gcQueue) process(
 	// Consult the protected timestamp state to determine whether we can GC and
 	// the timestamp which can be used to calculate the score and updated GC
 	// threshold.
-	canGC, cacheTimestamp, gcTimestamp, newThreshold := repl.checkProtectedTimestampsForGC(ctx, *zone.GC)
+	canGC, cacheTimestamp, gcTimestamp, _, newThreshold := repl.checkProtectedTimestampsForGC(ctx, *zone.GC)
 	if !canGC {
 		return false, nil
 	}

pkg/kv/kvserver/replica_protected_timestamp.go

@@ -220,20 +220,20 @@ func (r *Replica) protectedTimestampRecordCurrentlyApplies(
 	return false, read.readAt.Less(args.RecordAliveAt), nil
 }
 
-// checkProtectedTimestampsForGC determines whether the Replica can run GC.
-// If the Replica can run GC, this method returns the latest timestamp which
-// can be used to determine a valid new GCThreshold. The policy is passed in
-// rather than read from the replica state to ensure that the same value used
-// for this calculation is used later.
+// checkProtectedTimestampsForGC determines whether the Replica can run GC. If
+// the Replica can run GC, this method returns the latest timestamp which can be
+// used to determine a valid new GCThreshold. The policy is passed in rather
+// than read from the replica state to ensure that the same value used for this
+// calculation is used later.
 //
-// In the case that GC can proceed, three timestamps are returned: The timestamp
+// In the case that GC can proceed, four timestamps are returned: The timestamp
 // corresponding to the state of the cache used to make the determination (used
 // for markPendingGC when actually performing GC), the timestamp used as the
-// basis to calculate the new gc threshold (used for scoring and reporting), and
-// the new gc threshold itself.
+// basis to calculate the new gc threshold (used for scoring and reporting), the
+// old gc threshold, and the new gc threshold.
 func (r *Replica) checkProtectedTimestampsForGC(
 	ctx context.Context, policy zonepb.GCPolicy,
-) (canGC bool, cacheTimestamp, gcTimestamp, newThreshold hlc.Timestamp) {
+) (canGC bool, cacheTimestamp, gcTimestamp, oldThreshold, newThreshold hlc.Timestamp) {
 
 	// We may be reading the protected timestamp cache while we're holding
 	// the Replica.mu for reading. If we do so and find newer state in the cache
@@ -247,10 +247,10 @@ func (r *Replica) checkProtectedTimestampsForGC(
 	defer r.mu.RUnlock()
 	defer read.clearIfNotNewer(r.mu.cachedProtectedTS)
 
-	gcThreshold := *r.mu.state.GCThreshold
+	oldThreshold = *r.mu.state.GCThreshold
 	lease := *r.mu.state.Lease
 
-	// earliestValidRecord is the record with the earliest timestamp which is
+	// read.earliestRecord is the record with the earliest timestamp which is
 	// greater than the existing gcThreshold.
 	read = r.readProtectedTimestampsRLocked(ctx, nil)
 	gcTimestamp = read.readAt
@@ -266,18 +266,12 @@ func (r *Replica) checkProtectedTimestampsForGC(
 	if gcTimestamp.Less(lease.Start) {
 		log.VEventf(ctx, 1, "not gc'ing replica %v due to new lease %v started after %v",
 			r, lease, gcTimestamp)
-		return false, hlc.Timestamp{}, hlc.Timestamp{}, hlc.Timestamp{}
+		return false, hlc.Timestamp{}, hlc.Timestamp{}, hlc.Timestamp{}, hlc.Timestamp{}
 	}
 
 	newThreshold = gc.CalculateThreshold(gcTimestamp, policy)
 
-	// If we've already GC'd right up to this record, there's no reason to
-	// gc again.
-	if newThreshold.Equal(gcThreshold) {
-		return false, hlc.Timestamp{}, hlc.Timestamp{}, hlc.Timestamp{}
-	}
-
-	return true, read.readAt, gcTimestamp, newThreshold
+	return true, read.readAt, gcTimestamp, oldThreshold, newThreshold
 }
 
 // markPendingGC is called just prior to sending the GC request to increase the

pkg/kv/kvserver/replica_protected_timestamp_test.go

@@ -315,7 +315,7 @@ func TestCheckProtectedTimestampsForGC(t *testing.T) {
 			name: "lease is too new",
 			test: func(t *testing.T, r *Replica, mt *manualCache) {
 				r.mu.state.Lease.Start = r.store.Clock().Now()
-				canGC, _, gcTimestamp, _ := r.checkProtectedTimestampsForGC(ctx, makePolicy(10))
+				canGC, _, gcTimestamp, _, _ := r.checkProtectedTimestampsForGC(ctx, makePolicy(10))
 				require.False(t, canGC)
 				require.Zero(t, gcTimestamp)
 			},
@@ -337,7 +337,7 @@ func TestCheckProtectedTimestampsForGC(t *testing.T) {
 				})
 				// We should allow gc to proceed with the normal new threshold if that
 				// threshold is earlier than all of the records.
-				canGC, _, gcTimestamp, _ := r.checkProtectedTimestampsForGC(ctx, makePolicy(10))
+				canGC, _, gcTimestamp, _, _ := r.checkProtectedTimestampsForGC(ctx, makePolicy(10))
 				require.True(t, canGC)
 				require.Equal(t, mt.asOf, gcTimestamp)
 			},
@@ -362,8 +362,9 @@ func TestCheckProtectedTimestampsForGC(t *testing.T) {
 				// We should allow gc to proceed up to the timestamp which precedes the
 				// protected timestamp. This means we expect a GC timestamp 10 seconds
 				// after ts.Prev() given the policy.
-				canGC, _, gcTimestamp, _ := r.checkProtectedTimestampsForGC(ctx, makePolicy(10))
+				canGC, _, gcTimestamp, oldThreshold, newThreshold := r.checkProtectedTimestampsForGC(ctx, makePolicy(10))
 				require.True(t, canGC)
+				require.False(t, newThreshold.Equal(oldThreshold))
 				require.Equal(t, ts.Prev().Add(10*time.Second.Nanoseconds(), 0), gcTimestamp)
 			},
 		},
@@ -386,11 +387,14 @@ func TestCheckProtectedTimestampsForGC(t *testing.T) {
 						},
 					},
 				})
-				// We should not allow GC if the threshold is already the predecessor
-				// of the earliest valid record.
-				canGC, _, gcTimestamp, _ := r.checkProtectedTimestampsForGC(ctx, makePolicy(10))
-				require.False(t, canGC)
-				require.Zero(t, gcTimestamp)
+				// We should allow GC even if the threshold is already the
+				// predecessor of the earliest valid record. However, the GC
+				// queue does not enqueue ranges in such cases, so this is only
+				// applicable to manually enqueued ranges.
+				canGC, _, gcTimestamp, oldThreshold, newThreshold := r.checkProtectedTimestampsForGC(ctx, makePolicy(10))
+				require.True(t, canGC)
+				require.True(t, newThreshold.Equal(oldThreshold))
+				require.Equal(t, th.Add(10*time.Second.Nanoseconds(), 0), gcTimestamp)
 			},
 		},
 		{
@@ -411,7 +415,7 @@ func TestCheckProtectedTimestampsForGC(t *testing.T) {
 						},
 					},
 				})
-				canGC, _, gcTimestamp, _ := r.checkProtectedTimestampsForGC(ctx, makePolicy(10))
+				canGC, _, gcTimestamp, _, _ := r.checkProtectedTimestampsForGC(ctx, makePolicy(10))
 				require.True(t, canGC)
 				require.Equal(t, mt.asOf, gcTimestamp)
 			},