pebble: use ReadPropertiesBlock when accessing reader props

Remove remaining reads of `Properties` on the reader struct. This commit also
removes the Properties field from the reader.

Closes: #4383
Closes: #4790

There are no more reads of `Properties` so we can remove this field now.

Author: xinhaoz

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 {

file_cache.go

@@ -946,7 +946,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 {

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)

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.

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
 			}

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

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,13 +274,15 @@ 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
 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

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

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