From cc0dd372b6d10b7cebacbdf3fc19bbb0074af31b Mon Sep 17 00:00:00 2001
From: Nick Travers <travers@cockroachlabs.com>
Date: Thu, 20 Oct 2022 10:39:54 -0700
Subject: [PATCH] manifest: unref files with range keys when version refcnt is
 zero

A manifest version uses reference counting to ensure that obsolete
SSTables are only removed once it is safe to do so (i.e. no iterators or
snapshots are open referencing tables in a older manifest version).

In #1771, a new slice of `LevelMetadata` was added to support range
keys. A slice already existed for point keys (plus range dels).
Currently, when a version's ref count reaches zero, only the files
present in the point key `LevelMetadata` slice have their ref counts
decremented. If a file also contains range keys, the reference count
never becomes zero and the file becomes "zombied", and can never be
deleted from disk.

Decrement the ref counts on files present in the range keys
`LevelMetadata` slice, allowing the ref counts to be correctly zeroed.

Add regression test to exercise the bug observed in
cockroachdb/cockroach#90149.

Fix cockroachdb/cockroach#90149.
---
 compaction_test.go             |  7 +++++++
 db.go                          |  6 ++++++
 internal/manifest/version.go   |  3 +++
 testdata/compaction_tombstones | 37 ++++++++++++++++++++++++++++++++++
 4 files changed, 53 insertions(+)

diff --git a/compaction_test.go b/compaction_test.go
index dfce53edab..8c7832b06e 100644
--- a/compaction_test.go
+++ b/compaction_test.go
@@ -2200,6 +2200,13 @@ func TestCompactionTombstones(t *testing.T) {
 				d.mu.Unlock()
 				return str
 
+			case "close":
+				if err := d.Close(); err != nil {
+					return err.Error()
+				}
+				d = nil
+				return ""
+
 			case "version":
 				d.mu.Lock()
 				s := d.mu.versions.currentVersion().String()
diff --git a/db.go b/db.go
index 3c4488e405..591eccdb65 100644
--- a/db.go
+++ b/db.go
@@ -1316,6 +1316,12 @@ func (d *DB) Close() error {
 	d.compactionSchedulers.Wait()
 	d.mu.Lock()
 
+	// As a sanity check, ensure that there are no zombie tables. A non-zero count
+	// hints at a reference count leak.
+	if ztbls := len(d.mu.versions.zombieTables); ztbls > 0 {
+		err = firstError(err, errors.Errorf("non-zero zombie file count: %d", ztbls))
+	}
+
 	// If the options include a closer to 'close' the filesystem, close it.
 	if d.opts.private.fsCloser != nil {
 		d.opts.private.fsCloser.Close()
diff --git a/internal/manifest/version.go b/internal/manifest/version.go
index a3f0d0d898..6c3bca0f3b 100644
--- a/internal/manifest/version.go
+++ b/internal/manifest/version.go
@@ -890,6 +890,9 @@ func (v *Version) unrefFiles() []*FileMetadata {
 	for _, lm := range v.Levels {
 		obsolete = append(obsolete, lm.release()...)
 	}
+	for _, lm := range v.RangeKeyLevels {
+		obsolete = append(obsolete, lm.release()...)
+	}
 	return obsolete
 }
 
diff --git a/testdata/compaction_tombstones b/testdata/compaction_tombstones
index 39fba3f04b..30be8b71de 100644
--- a/testdata/compaction_tombstones
+++ b/testdata/compaction_tombstones
@@ -291,3 +291,40 @@ range-deletions-bytes-estimate: 0
 maybe-compact
 ----
 (none)
+
+# Regression test for cockroachdb/cockroach#90149, exercising reference counting
+# on tables that contain a mixture of point, range dels and range keys.
+#
+# Place a table in L6 that contains a RANGEKEYDEL. Because this table is in the
+# bottom of the LSM, and there are no range keys below it, the RANGEKEYDEL is
+# eligible for elision (the RANGEDEL too). After the elision, the input table
+# should be deleted. In #90149, the table still had a reference count, and
+# therefore could not be deleted from the filesystem.
+
+define
+L6
+  rangekey:a-b:{(#1,RANGEKEYDEL)}
+  a.SET.2:a
+  b.SET.3:b
+  c.SET.4:c
+  c.RANGEDEL.5:z
+----
+6:
+  000004:[a#2,SET-z#72057594037927935,RANGEDEL]
+
+wait-pending-table-stats
+000004
+----
+num-entries: 3
+num-deletions: 1
+num-range-key-sets: 0
+point-deletions-bytes-estimate: 0
+range-deletions-bytes-estimate: 39
+
+maybe-compact
+----
+[JOB 100] compacted(elision-only) L6 [000004] (1001 B) + L6 [] (0 B) -> L6 [000005] (778 B), in 1.0s (2.0s total), output rate 778 B/s
+
+# Close the DB, asserting that the reference counts balance.
+close
+----