Isn't this changing the semantics of the cleanup function? Before, the semaphore would be released when the iterator was closed. Now, it's released when the registration is canceled. Maybe we could use a sync.Once with the existing iteratorWithCloser structure to avoid needing to pass a new variable through the registration?

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @andreimatei, and @nvanbenschoten)

pkg/kv/kvserver/replica_rangefeed.go, line 204 at r1 (raw file):

maybeDisconnectEmptyRangefeed

Want to remove the p == nil handling from this function now?

pkg/kv/kvserver/replica_rangefeed.go, line 295 at r1 (raw file):

	p := r.rangefeedMu.proc
	if p != nil {
		reg, filter := p.Register(span, startTS, catchupIter, nil, withDiff, stream, errC)

Shouldn't we be passing cleanup here? And down below?

pkg/kv/kvserver/rangefeed/processor.go, line 361 at r1 (raw file):

// provided an error when the registration closes.
//
// If the rangefeed is successfully registered (true is returned), the

Both of these new sentences need a bit of love. This one is missing punctuation and the other is truncated.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @andreimatei)

pkg/kv/kvserver/replica_rangefeed.go, line 176 at r2 (raw file):

		// Finish the iterator limit if we exit before the iterator finishes.
		// The release function will be hooked into the Close method on the
		// iterator below. The sync.Once prevents any races.

Mind spelling out which races?
Also in the commit message pls give a hint that the race was on the iterSemRelease variable.

pkg/kv/kvserver/replica_rangefeed.go, line 177 at r2 (raw file):

		// The release function will be hooked into the Close method on the
		// iterator below. The sync.Once prevents any races.
		var iterSemReleaseOnce sync.Once

I see that we have gotten to this sync.Once after something else has been tried so you can shut me up, but it seems to me that the control flow is simple enough that a sync.Once big hammer shouldn't be necessary; I think ownership of that rate semaphore token is clear. I'm thinking of turning catchUpIterFunc into a proper struct with a Close() that needs to be called by Processor.Register or delegated to the iteratorWithCloser.

Reviewed 4 of 5 files at r2.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner)