storage: write a tombstone with math.MaxInt32 when processing a merge

[ajwerner]

Before this PR we would write such a tombstone when detecting that a range had
been merged via a snapshot or via the replica gc queue but curiously not when
merging the range by applying a merge.

Release Justification: Came across this oddity while working on updating tests
for #40751. Maybe is not justified.

Release note: None

[cockroach-teamcity]

This change is 

[ajwerner]

@benesch any interest in chiming in on this one? Am I missing anything? It seems like if we're at this critical phase of a merge we know the RHS has its terminal replica ID.

[nvanbenschoten]

but let's wait for @benesch to chime in.

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @nvanbenschoten)

[benesch]

It's been a while (holy cow, 9 months!) since I've really thought about range merges, so I'm not 1000% sure that this is correct. But it certainly seems legit, and it's not like there was a particular reason that the code looked like this. Rather the GC queue using MaxInt was quick fix when I realized I didn't have NextReplicaID available there, and I didn't think to propagate that change back to the normal subsumption code path.

Reviewed 2 of 2 files at r1.
Reviewable status: complete! 2 of 0 LGTMs obtained (waiting on @ajwerner)

---

pkg/storage/replica_application_state_machine.go, line 571 at r1 (raw file):

		const mustClearRange = false
		if err := rhsRepl.preDestroyRaftMuLocked(
			ctx, b.batch, b.batch, math.MaxInt32, rangeIDLocalOnly, mustClearRange,

Not that it really matters much to me, but it seems like it might be worth leaving a comment here along these lines:

// Use math.MaxInt32 as the nextReplicaID as an extra safeguard against creating
// new replicas of the RHS. This isn't required for correctness, since the merge
// protocol should guarantee that no new replicas of the RHS can ever be
// created, but it doesn't hurt to be careful.

The comment in the GC queue (

cockroach/pkg/storage/replica_gc_queue.go

Lines 331 to 337 in ad4d6f5

	// We don't have the last NextReplicaID for the subsumed range, nor can we
	// obtain it, but that's OK: we can just be conservative and use the maximum
	// possible replica ID. store.RemoveReplica will write a tombstone using
	// this maximum possible replica ID, which would normally be problematic, as
	// it would prevent this store from ever having a new replica of the removed
	// range. In this case, however, it's copacetic, as subsumed ranges _can't_
	// have new replicas.

) should also probably be updated to read "a replica ID of max int is what we do on merges; see runPreApplyTriggers for details" rather than "oh shoot we don't have the NextReplicaID, lol, guess we can just stick maxint here."

[ajwerner]

TFTR!

bors r+

Reviewable status: complete! 0 of 0 LGTMs obtained (and 2 stale) (waiting on @benesch)

---

pkg/storage/replica_application_state_machine.go, line 571 at r1 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

Not that it really matters much to me, but it seems like it might be worth leaving a comment here along these lines:

// Use math.MaxInt32 as the nextReplicaID as an extra safeguard against creating
// new replicas of the RHS. This isn't required for correctness, since the merge
// protocol should guarantee that no new replicas of the RHS can ever be
// created, but it doesn't hurt to be careful.

The comment in the GC queue (

cockroach/pkg/storage/replica_gc_queue.go

Lines 331 to 337 in ad4d6f5

	// We don't have the last NextReplicaID for the subsumed range, nor can we
	// obtain it, but that's OK: we can just be conservative and use the maximum
	// possible replica ID. store.RemoveReplica will write a tombstone using
	// this maximum possible replica ID, which would normally be problematic, as
	// it would prevent this store from ever having a new replica of the removed
	// range. In this case, however, it's copacetic, as subsumed ranges _can't_
	// have new replicas.

) should also probably be updated to read "a replica ID of max int is what we do on merges; see runPreApplyTriggers for details" rather than "oh shoot we don't have the NextReplicaID, lol, guess we can just stick maxint here."

Done.

[craig]

Build failed

• GitHub CI (Cockroach)

[ajwerner]

zerosum-splits flake.

bors r+

Reviewable status: complete! 0 of 0 LGTMs obtained (and 2 stale) (waiting on @benesch)

[craig]

Build succeeded

• GitHub CI (Cockroach)