This change is 

Reviewed 1 of 1 files at r1, 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks pending.

pkg/storage/store.go, line 3878 at r1 (raw file):

			//
			// Inspired by https://github.com/cockroachdb/cockroach/pull/19404.
			runtime.Gosched()

Gosched can hurt performance, especially in the tail latencies, so I wouldn't say there's no harm. That said, this shouldn't be very common so I doubt it will have an impact. Still, I'm not sure it's worth the risk if there's no reason to add this.

Comments from Reviewable

Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks pending.

pkg/storage/store.go, line 3878 at r1 (raw file):

Note that the Gosched is on the err = errRetry path, so I doubt it matters for performance. And, with a loop like this, I can't prove that there's no reason to add this, and unless we can't, I think we should generally add it to avoid this kind of freak deadlock.

Comments from Reviewable

Reviewed 1 of 1 files at r1.
Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks pending.

pkg/storage/replica.go, line 4536 at r2 (raw file):

	rightRng, created, err := r.store.getOrCreateReplica(ctx, split.RightDesc.RangeID, 0, nil)
	if err != nil {
		log.Fatalf(ctx, "while acquiring split lock: %s", err)

Shouldn't you return the error to the caller in order to force error the Raft command? The code in Store.tryGetOrCreateReplica has enough error paths that I can't convince myself none of them will occur.

pkg/storage/store.go, line 3878 at r1 (raw file):

			//
			// Inspired by https://github.com/cockroachdb/cockroach/pull/19404.
			runtime.Gosched()

Was this addition motivated by a real concern. Store.tryGetOrCreateReplica performs multiple lock operations which will definitely provide stop-the-world safe points for the GC.

Comments from Reviewable

Review status: 0 of 3 files reviewed at latest revision, 3 unresolved discussions.

pkg/storage/replica.go, line 4536 at r2 (raw file):

Good point. I definitely want to learn about the errors out there in the wild though, so I added reporting to sentry. PTAL!

pkg/storage/store.go, line 3878 at r1 (raw file):

No, just paranoia given the recent deadlock in TestRefreshPendingCmds. You're right that the abundance of lock acquisition makes this too paranoid. Removed the commit.

Comments from Reviewable

Reviewed 2 of 4 files at r3.
Review status: 2 of 3 files reviewed at latest revision, 3 unresolved discussions, some commit checks pending.

Comments from Reviewable

LGTM, but the PR title is incorrect now.

Review status: 2 of 3 files reviewed at latest revision, 1 unresolved discussion, some commit checks pending.

Comments from Reviewable

TFTRs, everyone! @nvanbenschoten changed the title.

I stumbled upon this PR while cleaning up #38954 and am now confused about why setting forcedErr to the result of maybeAcquireSplitMergeLock is correct. We don't expect to ever see an error here, but if we do, I don't think we can silently let it prevent one of the replicas from applying the split/merge. That's the kind of thing that causes replica inconsistencies. I think the only option we have is to fatal.

Not sure what we were thinking there, Fatal looks like the only option.