I don't believe there should be a need for point GC to serialize with newer MVCC range key writes, since the point key stats are only affected by the point/range key directly above them, and that point/range key must be below the GC threshold for the point key to be eligible for GC in the first place (assuming a non-tombstone point key).

This last part about tombstone point keys is interesting. If we have a tombstone and a non-tombstone below the GC threshold, we are able to GC both of them, leaving no remaining version on the key. Based on a reading of ComputeStatsForRangeWithVisitors and MVCCGarbageCollect, I think this is ok, as the presence of a range tombstone does not have an effect on point tombstones directly below it, only point non-tombstones.

We could either build one of these, or we could possibly hack around it by taking out a read/write latch at timestamp hlc.MaxTimestamp which would not serialize with any readers (since there cannot be any readers above that timestamp).

Taking out a read/write latch at timestamp hlc.MaxTimestamp seems like a reasonable solution to me.

Reviewed 5 of 6 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @aayushshah15, @aliher1911, and @arulajmani)

pkg/kv/kvserver/replica_test.go line 8814 at r4 (raw file):

			// Since the GC request does not acquire latches on the keys being GC'ed,
			// they're not guaranteed to wait for these above Puts to get applied on
			// the leaseholder. See AckCommittedEntriesBeforeApplication() the comment

"and the"

This all seems fine to me, thanks for improving it!

@aliher1911 This mostly addresses #84576. I think we should extend some of these benchmarks to include range tombstone GC, and see if we need to either break up or otherwise limit the work we do per request to make sure we're not holding onto the latches for too long. We also need to look at TBI performance below the GC threshold -- at that point, most data is likely compacted down to the lower LSM levels, and so we'll have to rely on skipping blocks rather than entire SSTs, and I don't know if we get as much of a boost from that.

I think this is ok, as the presence of a range tombstone does not have an effect on point tombstones directly below it, only point non-tombstones.

Yeah, that sounds right.

Taking out a read/write latch at timestamp hlc.MaxTimestamp seems like a reasonable solution to me.

Thanks for confirming -- I think that's the only viable solution with only a week left before stability. Not too thrilled about the wide write latches though, we'll just have to make sure they're short-lived and maybe break them up. Are they essentially going to form a barrier in the concurrency manager queue, or is there some way we can avoid head-of-line blockage? This reminds me about the problems with split latches (#81152).

Reviewed 4 of 6 files at r3, 2 of 2 files at r5, 1 of 1 files at r6, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @aayushshah15, @aliher1911, and @arulajmani)

-- commits line 59 at r6:
This is lovely. 🚀

pkg/kv/kvserver/batcheval/cmd_add_sstable.go line 63 at r5 (raw file):

		latchSpans.AddMVCC(spanset.SpanReadOnly, roachpb.Span{Key: l, EndKey: r}, header.Timestamp)
	}
	latchSpans.DisableUndeclaredAccessAssertions()

Any particular reason for this? I'd rather disable the assertions for the one specific access that violates the latches, than the entire request.

pkg/kv/kvserver/batcheval/cmd_recompute_stats.go line 57 at r5 (raw file):

	latchSpans.AddNonMVCC(spanset.SpanReadWrite, roachpb.Span{Key: keys.TransactionKey(rdKey, uuid.Nil)})
	// Disable the assertions which check that all reads were previously declared.
	latchSpans.DisableUndeclaredAccessAssertions()

As an apropos, I have been a bit worried about the non-commutativity of MVCC range tombstones in the context of consistency check stats calculations. As far as I can tell, this should still be safe, because the stats computation is only reading (from a stable Pebble snapshot) and the stats delta of the write will be correct regardless of the base stats. The non-commutativity is only a problem with concurrent writers. Anything I'm missing?

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @aayushshah15, @aliher1911, and @arulajmani)

pkg/kv/kvserver/batcheval/cmd_add_sstable.go line 63 at r5 (raw file):

Yeah, for the AddSSTable case there was only a single read access that needed to circumvent the spanset assertion (and it was safe to do so). For the rest of the accesses, I think we'd rather still keep the assertions enabled, if possible.

Thanks for the reviews!

bors r+

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @aayushshah15, @aliher1911, @arulajmani, @erikgrinaker, and @nvanbenschoten)

pkg/kv/kvserver/replica_test.go line 8814 at r4 (raw file):

Done.

pkg/kv/kvserver/batcheval/cmd_add_sstable.go line 63 at r5 (raw file):

Done.