Repository: cockroachdb/pebble
Pull request #4775 (2025-05-27 18:43:16)

pebble: use ReadPropertiesBlock when accessing reader props
### pebble: close file cache before closing the objstorage provider

It seems that during close, the file cache may attempt to access structures
managed by the objstroage provider during the eviction process, such as sending
to channels used for tracing events. We should close the file cache before the
obj storage provider.

----------------------------------

### pebble: use ReadPropertiesBlock when accessing reader props

Remove remaining reads of `Properites` on the reader struct. This commit also
removes the Properties field from the reader.

Closes: https://github.com/cockroachdb/pebble/issues/4383


----------
Previous: https://github.com/cockroachdb/pebble/pull/4773

xinhaoz: 




*[`objstorage/objstorageprovider/objiotracing/obj_io_tracing_on.go` line 323 at r2](https://reviewable.io/reviews/cockroachdb/pebble/4775#-ORMv7TV1c4gQaqUyVeJ:-ORNdvkn8uoE-PWUrg1Y:b-ybm7wh) ([raw file](https://github.com/cockroachdb/pebble/blob/25b668583ba7d9a6df35a7bfc97fd6198c821555/objstorage/objstorageprovider/objiotracing/obj_io_tracing_on.go#L323)):*
<details><summary><i>Previously, xinhaoz (Xin Hao Zhang) wrote…</i></summary><blockquote>

Ahh yes you're right. Here's the stack trace, I also just dropped the commit since it's not actually fixing what we want: https://github.com/cockroachdb/pebble/actions/runs/15286733028/job/42998483391?pr=4775

It seems that the objprovider is closed before the file cache -- should we just close the file cache first or must it be closed last here? https://github.com/cockroachdb/pebble/blob/2fd9454bed4c2d0eea216dbfe5ae4e8b5629550a/db.go#L1794
</blockquote></details>

Should also be failing on CI now. Also I was unsure if this is an even an issue with objprovider tracing at all, but I couldn't see anything obvious with these changes since we didn't add any reader.close calls here. LMK if anyone sees anything odd  with the file cache interactions here.


<!-- Sent from Reviewable.io -->


Patch:
diff --git a/db.go b/db.go
index 1203e381..869724f5 100644
--- a/db.go
+++ b/db.go
@@ -1791,6 +1791,8 @@ func (d *DB) Close() error {
 		err = firstError(err, errors.Errorf("non-zero zombie blob count: %d", zblobs))
 	}
 
+	err = firstError(err, d.fileCache.Close())
+
 	err = firstError(err, d.objProvider.Close())
 
 	// If the options include a closer to 'close' the filesystem, close it.
@@ -1803,8 +1805,6 @@ func (d *DB) Close() error {
 		err = firstError(err, errors.Errorf("leaked snapshots: %d open snapshots on DB %p", v, d))
 	}
 
-	err = firstError(err, d.fileCache.Close())
-
 	return err
 }
 
diff --git a/file_cache.go b/file_cache.go
index b636c236..990089ae 100644
--- a/file_cache.go
+++ b/file_cache.go
@@ -951,7 +951,11 @@ func (h *fileCacheHandle) getTableProperties(file *tableMetadata) (*sstable.Prop
 	defer v.Unref()
 
 	r := v.Value().mustSSTableReader()
-	return &r.Properties, nil
+	props, err := r.ReadPropertiesBlock(context.TODO(), nil /* buffer pool */)
+	if err != nil {
+		return nil, err
+	}
+	return &props, nil
 }
 
 type fileCacheValue struct {
diff --git a/ingest.go b/ingest.go
index 6893f4ef..98e52b71 100644
--- a/ingest.go
+++ b/ingest.go
@@ -281,17 +281,25 @@ func ingestLoad1(
 			tf, fmv, fmv.MinTableFormat(), fmv.MaxTableFormat(),
 		)
 	}
+
+	if r.Attributes.Has(sstable.AttributeBlobValues) {
+		return nil, keyspan.Span{}, errors.Newf(
+			"pebble: ingesting tables with blob references is not supported")
+	}
+
+	props, err := r.ReadPropertiesBlock(ctx, nil /* buffer pool */)
+	if err != nil {
+		return nil, keyspan.Span{}, err
+	}
+
+	// If this is a columnar block, read key schema name from properties block.
 	if tf.BlockColumnar() {
-		if _, ok := opts.KeySchemas[r.Properties.KeySchemaName]; !ok {
+		if _, ok := opts.KeySchemas[props.KeySchemaName]; !ok {
 			return nil, keyspan.Span{}, errors.Newf(
 				"pebble: table uses key schema %q unknown to the database",
-				r.Properties.KeySchemaName)
+				props.KeySchemaName)
 		}
 	}
-	if r.Properties.NumValuesInBlobFiles > 0 {
-		return nil, keyspan.Span{}, errors.Newf(
-			"pebble: ingesting tables with blob references is not supported")
-	}
 
 	meta = &tableMetadata{}
 	meta.TableNum = tableNum
@@ -308,7 +316,7 @@ func ingestLoad1(
 	// disallowing removal of an open file. Under MemFS, if we don't populate
 	// meta.Stats here, the file will be loaded into the file cache for
 	// calculating stats before we can remove the original link.
-	maybeSetStatsFromProperties(meta.PhysicalMeta(), &r.Properties.CommonProperties, opts.Logger)
+	maybeSetStatsFromProperties(meta.PhysicalMeta(), &props.CommonProperties, opts.Logger)
 
 	{
 		iter, err := r.NewIter(sstable.NoTransforms, nil /* lower */, nil /* upper */, sstable.AssertNoBlobHandles)
diff --git a/internal/base/lazy_value.go b/internal/base/lazy_value.go
index e006c94c..0d6a156b 100644
--- a/internal/base/lazy_value.go
+++ b/internal/base/lazy_value.go
@@ -157,20 +157,15 @@ type LazyValue struct {
 
 // LazyFetcher supports fetching a lazy value.
 //
-// Fetcher and Attribute are to be initialized at creation time. The fields
-// are arranged to reduce the sizeof this struct.
+// The fields are to be initialized at creation time.
 type LazyFetcher struct {
 	// Fetcher, given a handle, returns the value.
 	Fetcher ValueFetcher
-	err     error
-	value   []byte
 	// Attribute includes the short attribute and value length.
 	Attribute AttributeAndLen
 	// BlobFileNum identifies the blob file containing the value. It is only
 	// populated if the value is stored in a blob file.
 	BlobFileNum DiskFileNum
-	fetched     bool
-	callerOwned bool
 }
 
 // ValueFetcher is an interface for fetching a value.
@@ -193,32 +188,12 @@ type ValueFetcher interface {
 
 // Value returns the underlying value.
 func (lv *LazyValue) Value(buf []byte) (val []byte, callerOwned bool, err error) {
-	if lv.Fetcher == nil {
-		return lv.ValueOrHandle, false, nil
-	}
-	// Do the rest of the work in a separate method to attempt mid-stack
-	// inlining of Value(). Unfortunately, this still does not inline since the
-	// cost of 85 exceeds the budget of 80.
-	//
-	// TODO(sumeer): Packing the return values into a struct{[]byte error bool}
-	// causes it to be below the budget. Consider this if we need to recover
-	// more performance. I suspect that inlining this only matters in
-	// micro-benchmarks, and in actual use cases in CockroachDB it will not
-	// matter because there is substantial work done with a fetched value.
-	return lv.fetchValue(context.TODO(), buf)
-}
-
-// INVARIANT: lv.Fetcher != nil
-func (lv *LazyValue) fetchValue(
-	ctx context.Context, buf []byte,
-) (val []byte, callerOwned bool, err error) {
 	f := lv.Fetcher
-	if !f.fetched {
-		f.fetched = true
-		f.value, f.callerOwned, f.err = f.Fetcher.Fetch(ctx,
-			lv.ValueOrHandle, f.BlobFileNum, f.Attribute.ValueLen, buf)
+	if f == nil {
+		return lv.ValueOrHandle, false, nil
 	}
-	return f.value, f.callerOwned, f.err
+	return f.Fetcher.Fetch(context.TODO(),
+		lv.ValueOrHandle, f.BlobFileNum, f.Attribute.ValueLen, buf)
 }
 
 // Len returns the length of the value.
diff --git a/sstable/reader.go b/sstable/reader.go
index 26246dec..666496c3 100644
--- a/sstable/reader.go
+++ b/sstable/reader.go
@@ -65,7 +65,6 @@ type Reader struct {
 	metaindexBH  block.Handle
 	footerBH     block.Handle
 
-	Properties     Properties
 	tableFormat    TableFormat
 	Attributes     Attributes
 	UserProperties map[string]string
@@ -958,7 +957,6 @@ func NewReader(ctx context.Context, f objstorage.Readable, o ReaderOptions) (*Re
 		r.err = err
 		return nil, err
 	}
-	r.Properties = props
 	r.UserProperties = props.UserProperties
 
 	// Set which attributes are in use based on property values.
diff --git a/table_stats.go b/table_stats.go
index ef892af3..60197fca 100644
--- a/table_stats.go
+++ b/table_stats.go
@@ -310,9 +310,13 @@ func (d *DB) loadTableStats(
 
 	err := d.fileCache.withReader(
 		context.TODO(), block.NoReadEnv, meta, func(r *sstable.Reader, env sstable.ReadEnv) (err error) {
-			props := r.Properties.CommonProperties
+			loadedProps, err := r.ReadPropertiesBlock(context.TODO(), nil /* buffer pool */)
+			if err != nil {
+				return err
+			}
+			props := loadedProps.CommonProperties
 			if meta.Virtual {
-				props = r.Properties.GetScaledProperties(meta.TableBacking.Size, meta.Size)
+				props = loadedProps.GetScaledProperties(meta.TableBacking.Size, meta.Size)
 			}
 			stats.NumEntries = props.NumEntries
 			stats.NumDeletions = props.NumDeletions
@@ -504,32 +508,38 @@ func (d *DB) estimateSizesBeneath(
 	keySum += fileProps.RawKeySize
 	valSum += fileProps.RawValueSize
 
-	addPhysicalTableStats := func(r *sstable.Reader, _ sstable.ReadEnv) (err error) {
-		fileSum += file.Size
-		entryCount += r.Properties.NumEntries
-		keySum += r.Properties.RawKeySize
-		valSum += r.Properties.RawValueSize
-		return nil
-	}
-	addVirtualTableStats := func(v *sstable.Reader, _ sstable.ReadEnv) (err error) {
-		fileSum += file.Size
-		entryCount += file.Stats.NumEntries
-		keySum += v.Properties.RawKeySize
-		valSum += v.Properties.RawValueSize
-		return nil
-	}
-
 	for l := level + 1; l < numLevels; l++ {
 		// NB: This is subtle. The addPhysicalTableStats and
 		// addVirtualTableStats functions close over file, which is the loop
 		// variable.
 		for file = range v.Overlaps(l, meta.UserKeyBounds()).All() {
 			var err error
+			var tableMeta *tableMetadata
 			if file.Virtual {
-				err = d.fileCache.withReader(context.TODO(), block.NoReadEnv, file.VirtualMeta(), addVirtualTableStats)
+				tableMeta = file.VirtualMeta()
 			} else {
-				err = d.fileCache.withReader(context.TODO(), block.NoReadEnv, file.PhysicalMeta(), addPhysicalTableStats)
+				tableMeta = file.PhysicalMeta()
 			}
+
+			err = d.fileCache.withReader(context.TODO(), block.NoReadEnv, tableMeta, func(v *sstable.Reader, _ sstable.ReadEnv) (err error) {
+				// TODO(xinhaoz): We should avoid reading the properties block here.
+				// See https://github.com/cockroachdb/pebble/issues/4792.
+				loadedProps, err := v.ReadPropertiesBlock(context.TODO(), nil /* buffer pool */)
+				if err != nil {
+					return err
+				}
+
+				props := loadedProps.CommonProperties
+				if meta.Virtual {
+					props = loadedProps.GetScaledProperties(meta.TableBacking.Size, meta.Size)
+				}
+
+				fileSum += file.Size
+				entryCount += file.Stats.NumEntries
+				keySum += props.RawKeySize
+				valSum += props.RawValueSize
+				return nil
+			})
 			if err != nil {
 				return 0, 0, err
 			}


Test patch:
diff --git a/data_test.go b/data_test.go
index 2ea831ef..32febb7c 100644
--- a/data_test.go
+++ b/data_test.go
@@ -1349,11 +1349,15 @@ func runSSTablePropertiesCmd(t *testing.T, td *datadriven.TestData, d *DB) strin
 	}
 	defer r.Close()
 
-	props := r.Properties.String()
+	loadedProps, err := r.ReadPropertiesBlock(context.Background(), nil /* buffer pool */)
+	if err != nil {
+		return err.Error()
+	}
+	props := loadedProps.String()
 	env := sstable.ReadEnv{}
 	if m != nil && m.Virtual {
 		env.Virtual = m.VirtualParams
-		scaledProps := r.Properties.GetScaledProperties(m.TableBacking.Size, m.Size)
+		scaledProps := loadedProps.GetScaledProperties(m.TableBacking.Size, m.Size)
 		props = scaledProps.String()
 	}
 	if len(td.Input) == 0 {
diff --git a/internal/base/lazy_value_test.go b/internal/base/lazy_value_test.go
index 1d41bb2a..ecfa064e 100644
--- a/internal/base/lazy_value_test.go
+++ b/internal/base/lazy_value_test.go
@@ -68,7 +68,7 @@ func TestLazyValue(t *testing.T) {
 		require.Equal(t, []byte("foo"), getValue(fooLV3, callerOwned))
 		require.Equal(t, 1, numCalls)
 		require.Equal(t, []byte("foo"), getValue(fooLV3, callerOwned))
-		require.Equal(t, 1, numCalls)
+		require.Equal(t, 2, numCalls)
 		require.Equal(t, 3, fooLV3.Len())
 		attr, hasAttr := fooLV3.TryGetShortAttribute()
 		require.True(t, hasAttr)
diff --git a/testdata/compaction/value_separation b/testdata/compaction/value_separation
index 9375886c..79cbb315 100644
--- a/testdata/compaction/value_separation
+++ b/testdata/compaction/value_separation
@@ -140,7 +140,7 @@ Local tables size: 834B
 Compression types: snappy: 1
 Table stats: all loaded
 Block cache: 4 entries (1.6KB)  hit rate: 70.3%
-Table cache: 2 entries (1.1KB)  hit rate: 82.2%
+Table cache: 2 entries (792B)  hit rate: 82.2%
 Range key sets: 0  Tombstones: 0  Total missized tombstones encountered: 0
 Snapshots: 0  earliest seq num: 0
 Table iters: 0
diff --git a/testdata/event_listener b/testdata/event_listener
index 5aae66fa..cc12e5c1 100644
--- a/testdata/event_listener
+++ b/testdata/event_listener
@@ -209,6 +209,7 @@ open: ext/0
 read-at(689, 61): ext/0
 read-at(639, 50): ext/0
 read-at(122, 517): ext/0
+read-at(122, 517): ext/0
 read-at(81, 41): ext/0
 read-at(0, 81): ext/0
 close: ext/0
@@ -273,6 +274,7 @@ open: ext/a
 read-at(689, 61): ext/a
 read-at(639, 50): ext/a
 read-at(122, 517): ext/a
+read-at(122, 517): ext/a
 read-at(81, 41): ext/a
 read-at(0, 81): ext/a
 close: ext/a
@@ -280,6 +282,7 @@ open: ext/b
 read-at(689, 61): ext/b
 read-at(639, 50): ext/b
 read-at(122, 517): ext/b
+read-at(122, 517): ext/b
 read-at(81, 41): ext/b
 read-at(0, 81): ext/b
 close: ext/b
diff --git a/testdata/ingest b/testdata/ingest
index f1d308da..24682869 100644
--- a/testdata/ingest
+++ b/testdata/ingest
@@ -54,8 +54,8 @@ Virtual tables: 0 (0B)
 Local tables size: 569B
 Compression types: snappy: 1
 Table stats: all loaded
-Block cache: 3 entries (1.1KB)  hit rate: 18.2%
-Table cache: 1 entries (840B)  hit rate: 50.0%
+Block cache: 3 entries (1.1KB)  hit rate: 15.4%
+Table cache: 1 entries (480B)  hit rate: 50.0%
 Range key sets: 0  Tombstones: 0  Total missized tombstones encountered: 0
 Snapshots: 0  earliest seq num: 0
 Table iters: 0
diff --git a/testdata/metrics b/testdata/metrics
index 347463a3..7fd7dfb9 100644
--- a/testdata/metrics
+++ b/testdata/metrics
@@ -78,7 +78,7 @@ Local tables size: 755B
 Compression types: snappy: 1
 Table stats: all loaded
 Block cache: 2 entries (795B)  hit rate: 0.0%
-Table cache: 1 entries (840B)  hit rate: 0.0%
+Table cache: 1 entries (480B)  hit rate: 0.0%
 Range key sets: 0  Tombstones: 0  Total missized tombstones encountered: 0
 Snapshots: 0  earliest seq num: 0
 Table iters: 1
@@ -143,7 +143,7 @@ Local tables size: 1.5KB
 Compression types: snappy: 2
 Table stats: all loaded
 Block cache: 2 entries (795B)  hit rate: 33.3%
-Table cache: 2 entries (1.6KB)  hit rate: 66.7%
+Table cache: 2 entries (960B)  hit rate: 66.7%
 Range key sets: 0  Tombstones: 0  Total missized tombstones encountered: 0
 Snapshots: 0  earliest seq num: 0
 Table iters: 2
@@ -191,7 +191,7 @@ Local tables size: 1.5KB
 Compression types: snappy: 2
 Table stats: all loaded
 Block cache: 2 entries (795B)  hit rate: 33.3%
-Table cache: 2 entries (1.6KB)  hit rate: 66.7%
+Table cache: 2 entries (960B)  hit rate: 66.7%
 Range key sets: 0  Tombstones: 0  Total missized tombstones encountered: 0
 Snapshots: 0  earliest seq num: 0
 Table iters: 2
@@ -236,7 +236,7 @@ Local tables size: 1.5KB
 Compression types: snappy: 2
 Table stats: all loaded
 Block cache: 2 entries (795B)  hit rate: 33.3%
-Table cache: 1 entries (840B)  hit rate: 66.7%
+Table cache: 1 entries (480B)  hit rate: 66.7%
 Range key sets: 0  Tombstones: 0  Total missized tombstones encountered: 0
 Snapshots: 0  earliest seq num: 0
 Table iters: 1
@@ -541,7 +541,7 @@ Virtual tables: 0 (0B)
 Local tables size: 11KB
 Compression types: snappy: 15
 Table stats: all loaded
-Block cache: 6 entries (2.3KB)  hit rate: 7.7%
+Block cache: 6 entries (2.3KB)  hit rate: 7.3%
 Table cache: 0 entries (0B)  hit rate: 55.0%
 Range key sets: 0  Tombstones: 0  Total missized tombstones encountered: 0
 Snapshots: 0  earliest seq num: 0
@@ -618,7 +618,7 @@ Virtual tables: 0 (0B)
 Local tables size: 16KB
 Compression types: snappy: 22
 Table stats: all loaded
-Block cache: 6 entries (2.3KB)  hit rate: 7.7%
+Block cache: 6 entries (2.3KB)  hit rate: 7.3%
 Table cache: 0 entries (0B)  hit rate: 55.0%
 Range key sets: 0  Tombstones: 0  Total missized tombstones encountered: 0
 Snapshots: 0  earliest seq num: 0
@@ -1215,7 +1215,7 @@ Compression types: snappy: 5
 Garbage: point-deletions 502B range-deletions 1.5KB
 Table stats: all loaded
 Block cache: 2 entries (774B)  hit rate: 0.0%
-Table cache: 2 entries (1.6KB)  hit rate: 0.0%
+Table cache: 2 entries (960B)  hit rate: 0.0%
 Range key sets: 0  Tombstones: 3  Total missized tombstones encountered: 0
 Snapshots: 0  earliest seq num: 0
 Table iters: 0
@@ -1257,7 +1257,7 @@ Compression types: snappy: 5
 Garbage: point-deletions 502B range-deletions 1.5KB
 Table stats: all loaded
 Block cache: 2 entries (774B)  hit rate: 0.0%
-Table cache: 2 entries (1.6KB)  hit rate: 0.0%
+Table cache: 2 entries (960B)  hit rate: 0.0%
 Range key sets: 0  Tombstones: 3  Total missized tombstones encountered: 0
 Snapshots: 0  earliest seq num: 0
 Table iters: 0