From c91e8796f4b4ea4cba564c0b8b93b781e57bd40b Mon Sep 17 00:00:00 2001 From: Jackson Owens Date: Thu, 14 Sep 2023 14:48:02 -0400 Subject: [PATCH] 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 | 7 ++++++- table_cache.go | 4 ++++ table_cache_test.go | 16 ++++++++++++++++ 3 files changed, 26 insertions(+), 1 deletion(-) diff --git a/flushable_test.go b/flushable_test.go index 71ff86265e..03a0889d68 100644 --- a/flushable_test.go +++ b/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 { diff --git a/table_cache.go b/table_cache.go index 9a040a5197..6bdef93687 100644 --- a/table_cache.go +++ b/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() diff --git a/table_cache_test.go b/table_cache_test.go index 4535f1693a..285aaca7e1 100644 --- a/table_cache_test.go +++ b/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() } }