db: double check file reference counts when loading file

Double check the file reference counts before attempting to find/create a table
cache node for a file. Once a file's reference count falls to zero, the file
becomes obsolete and may be deleted at any moment.

Today if we have a race, break this invariant and attempt to load a file with a
nonpositive reference count, it's relatively unlikely that it manifests as an
error. Typically tables remain open in the table cache, allowing the table
cache to serve the request even if the file is no longer linked into the data
directory. Additionally, even if it's not in the table cache presently,
deletion of obsolete files may be delayed due to deletion pacing, hiding the
race.

This commit preemptively asserts on the file reference counts. I opted for not
restricting this invariant check to invariants builds because it's cheap
relative to a table cache lookup, and it's a particularly tricky form of
corruption to debug otherwise.

Informs cockroachdb/cockroach#110645.

flushable_test.go

@@ -59,14 +59,19 @@ func TestIngestedSSTFlushableAPI(t *testing.T) {
 		// We can reuse the ingestLoad function for this test even if we're
 		// not actually ingesting a file.
 		lr, err := ingestLoad(d.opts, d.FormatMajorVersion(), paths, nil, nil, d.cacheID, pendingOutputs, d.objProvider, jobID)
-		meta := lr.localMeta
 		if err != nil {
 			panic(err)
 		}
+		meta := lr.localMeta
 		if len(meta) == 0 {
 			// All of the sstables to be ingested were empty. Nothing to do.
 			panic("empty sstable")
 		}
+		// The table cache requires the *fileMetadata to have a positive
+		// reference count. Fake a reference before we try to load the file.
+		for _, f := range meta {
+			f.Ref()
+		}
 
 		// Verify the sstables do not overlap.
 		if err := ingestSortAndVerify(d.cmp, lr, KeyRange{}); err != nil {

table_cache.go

@@ -730,6 +730,10 @@ func (c *tableCacheShard) findNode(
 			}
 		}()
 	}
+	if refs := meta.Refs(); refs <= 0 {
+		panic(errors.AssertionFailedf("attempting to load file %s with refs=%d from table cache",
+			meta, refs))
+	}
 
 	// Fast-path for a hit in the cache.
 	c.mu.RLock()

table_cache_test.go

@@ -598,6 +598,8 @@ func testTableCacheRandomAccess(t *testing.T, concurrent bool) {
 			rngMu.Unlock()
 			m := &fileMetadata{FileNum: FileNum(fileNum)}
 			m.InitPhysicalBacking()
+			m.Ref()
+			defer m.Unref()
 			iter, _, err := c.newIters(context.Background(), m, nil, internalIterOpts{})
 			if err != nil {
 				errc <- errors.Errorf("i=%d, fileNum=%d: find: %v", i, fileNum, err)
@@ -659,6 +661,7 @@ func testTableCacheFrequentlyUsedInternal(t *testing.T, rangeIter bool) {
 			var err error
 			m := &fileMetadata{FileNum: FileNum(j)}
 			m.InitPhysicalBacking()
+			m.Ref()
 			if rangeIter {
 				iter, err = c.newRangeKeyIter(m, keyspan.SpanIterOptions{})
 			} else {
@@ -708,6 +711,7 @@ func TestSharedTableCacheFrequentlyUsed(t *testing.T) {
 		for _, j := range [...]int{pinned0, i % tableCacheTestNumTables, pinned1} {
 			m := &fileMetadata{FileNum: FileNum(j)}
 			m.InitPhysicalBacking()
+			m.Ref()
 			iter1, _, err := c1.newIters(context.Background(), m, nil, internalIterOpts{})
 			if err != nil {
 				t.Fatalf("i=%d, j=%d: find: %v", i, j, err)
@@ -760,6 +764,7 @@ func testTableCacheEvictionsInternal(t *testing.T, rangeIter bool) {
 		var err error
 		m := &fileMetadata{FileNum: FileNum(j)}
 		m.InitPhysicalBacking()
+		m.Ref()
 		if rangeIter {
 			iter, err = c.newRangeKeyIter(m, keyspan.SpanIterOptions{})
 		} else {
@@ -824,6 +829,7 @@ func TestSharedTableCacheEvictions(t *testing.T) {
 		j := rng.Intn(tableCacheTestNumTables)
 		m := &fileMetadata{FileNum: FileNum(j)}
 		m.InitPhysicalBacking()
+		m.Ref()
 		iter1, _, err := c1.newIters(context.Background(), m, nil, internalIterOpts{})
 		if err != nil {
 			t.Fatalf("i=%d, j=%d: find: %v", i, j, err)
@@ -890,6 +896,8 @@ func TestTableCacheIterLeak(t *testing.T) {
 
 	m := &fileMetadata{FileNum: 0}
 	m.InitPhysicalBacking()
+	m.Ref()
+	defer m.Unref()
 	iter, _, err := c.newIters(context.Background(), m, nil, internalIterOpts{})
 	require.NoError(t, err)
 
@@ -915,6 +923,8 @@ func TestSharedTableCacheIterLeak(t *testing.T) {
 
 	m := &fileMetadata{FileNum: 0}
 	m.InitPhysicalBacking()
+	m.Ref()
+	defer m.Unref()
 	iter, _, err := c1.newIters(context.Background(), m, nil, internalIterOpts{})
 	require.NoError(t, err)
 
@@ -951,6 +961,8 @@ func TestTableCacheRetryAfterFailure(t *testing.T) {
 	fs.setOpenError(true /* enabled */)
 	m := &fileMetadata{FileNum: 0}
 	m.InitPhysicalBacking()
+	m.Ref()
+	defer m.Unref()
 	if _, _, err = c.newIters(context.Background(), m, nil, internalIterOpts{}); err == nil {
 		t.Fatalf("expected failure, but found success")
 	}
@@ -1013,6 +1025,8 @@ func TestTableCacheErrorBadMagicNumber(t *testing.T) {
 
 	m := &fileMetadata{FileNum: testFileNum}
 	m.InitPhysicalBacking()
+	m.Ref()
+	defer m.Unref()
 	if _, _, err = c.newIters(context.Background(), m, nil, internalIterOpts{}); err == nil {
 		t.Fatalf("expected failure, but found success")
 	}
@@ -1103,6 +1117,7 @@ func TestTableCacheClockPro(t *testing.T) {
 		oldHits := cache.hits.Load()
 		m := &fileMetadata{FileNum: FileNum(key)}
 		m.InitPhysicalBacking()
+		m.Ref()
 		v := cache.findNode(m, dbOpts)
 		cache.unrefValue(v)
 
@@ -1112,6 +1127,7 @@ func TestTableCacheClockPro(t *testing.T) {
 			t.Errorf("%d: cache hit mismatch: got %v, want %v\n", line, hit, wantHit)
 		}
 		line++
+		m.Unref()
 	}
 }