Merge #156102

156102: sql/colfetcher: make Pace() call conditional r=dt a=dt

Previously we relied on the nil-safe Pace() call being a no-op when called on nil a Pacer to make pacing conditional on the txn priority as that priority was used to decide to set Pacer to a non-nil value or not. However Pace() is free to elect to perform some form of pacing even when called on a nil Pacer (e.g. by delegating to the runtime or some global state) so the cFetcher should explicitly choose to call Pace() or not call it based on whether or not it wants to be paced, instead of relying on the behavior of Pace to change. This (lack of) contract when calling Pace() is now made explicit in the doc comment on Pace().

Release note: none.
Epic: none.

Co-authored-by: David Taylor <[email protected]>

Author: craig[bot]

pkg/sql/colfetcher/cfetcher.go

@@ -751,8 +751,14 @@ func (cf *cFetcher) setNextKV(kv roachpb.KeyValue) {
 // rows, the Batch.Length is 0.
 func (cf *cFetcher) NextBatch(ctx context.Context) (coldata.Batch, error) {
 	for {
-		if err := cf.pacer.Pace(ctx); err != nil {
-			return nil, err
+		// While Pace() is nil-safe, its contract does not require it be a total
+		// no-op when nil. We, however, are using its nil-ness specifically to
+		// reflect our prior determination of the scan's priority, so this call to
+		// Pace() should be explicitly conditional on Pacer's non-nilness.
+		if cf.pacer != nil {
+			if err := cf.pacer.Pace(ctx); err != nil {
+				return nil, err
+			}
 		}
 		if debugState {
 			log.Dev.Infof(ctx, "State %s", cf.machine.state[0])

pkg/util/admission/pacer.go

@@ -23,9 +23,14 @@ type Pacer struct {
 	cur *ElasticCPUWorkHandle
 }
 
-// Pace will block as needed to pace work that calls it as configured. It is
-// intended to be called in a tight loop, and will attempt to minimize per-call
-// overhead. Non-nil errors are returned only if the context is canceled.
+// Pace will block as needed to pace work that calls it. It is intended to be
+// called in a tight loop, and will attempt to minimize per-call overhead.
+// Non-nil errors are returned only if the context is canceled.
+//
+// It is safe to call Pace() on a nil *Pacer, but it should not be assumed that
+// such a call will always be a no-op: Pace may elect to perform pacing any time
+// it is called, even if the *Pacer on which it is called is nil e.g. by
+// delegating to the Go runtime or other some global pacing.
 func (p *Pacer) Pace(ctx context.Context) error {
 	if p == nil {
 		return nil