storage: always write RANGEKEYDELs in ClearRangeWithHeuristic

[jbowens]

Previously, ClearRangeWithHeuristic applied a heuristic that would switch between writing RANGEKEYUNSETs versus RANGEKEYDELs. When multiple range keys overlap, unsetting each individual range key with RANGEKEYUNSET is strictly worse than writing a single RANGEKEYDEL over the same span.

This commit adapts ClearRangeWithHeuristic to write a single RANGEKEYDEL over the cleared span, but only if there are indeed range keys within the span. Setting a broad RANGEKEYDEL increases the likelihood that data can be removed using a delete-only compaction.

Close #144954.
Epic: none
Release note: none

[cockroach-teamcity]

This change is 

[RaduBerinde]

Just curious - why doesn't the same apply to point deletes?

[jbowens]

Just curious - why doesn't the same apply to point deletes?

If we end up writing point deletes, we won't be able to pursue a delete-only compaction either. But if the heuristic triggers and there were fewer keys than the threshold (64 during replica removals), it was unlikely that we could've found a sstable wholly contained within the key span anyways because there are so few keys. The same logic doesn't apply for range keys, since a single range key can have a very broad key span.

[RaduBerinde]

I see - still, what's the downside of writing a single rangedel vs multiple point dels?

[jbowens]

I see - still, what's the downside of writing a single rangedel vs multiple point dels?

I think the iteration overhead of a rangedel is higher than a small quantity of point dels because
a) we need to load every higher level's overlapping sstable's range deletion block if it exists
b) every point tombstone needs to be compared to the current range deletion in all higher levels' range deletion blocks (this is what cockroachdb/pebble#3600 strived to avoid but I never managed to track down the last of the correctness issues there).

[sumeerbhola]

Reviewed 6 of 6 files at r1, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @jbowens)

---

pkg/storage/engine.go line 1599 at r1 (raw file):

// will switch from clearing individual keys using point tombstones to clearing
// the entire range using Pebble range tombstones (RANGEDELs). Setting
// pointKeyThreshold to 0 disables checking for and clearing point keys. NB: An

This 0 behavior is very confusing, and I don't see any code in production calling it with 0.
Similarly for rangeKeyThreshold. Can we require this threshold to be > 0 and eliminate the shouldClearRangeKeys parameter?

[jbowens]

TFTR!

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

---

pkg/storage/engine.go line 1599 at r1 (raw file):

Previously, sumeerbhola wrote…

This 0 behavior is very confusing, and I don't see any code in production calling it with 0.
Similarly for rangeKeyThreshold. Can we require this threshold to be > 0 and eliminate the shouldClearRangeKeys parameter?

Good call, Done.

[sumeerbhola]

Reviewed 6 of 6 files at r2, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @jbowens)

[jbowens]

TFTR!

bors r=sumeerbhola

[craig]

Build succeeded:

• examples_orms
• linux_amd64_fips_build
• local_roachtest_fips
• check_generated_code
• acceptance
• docker_image_amd64
• unit_tests
• lint
• linux_amd64_build
• local_roachtest