manifest: rework blob file reference counting #4806

jbowens · 2025-06-02T21:08:43Z

Rework the reference counting of in-use blob files to decouple the reference counts from the TableMetadatas' blob references. With the introduction of blob file rewrites and replacement, the physical blob file and the logical blob file's references have distinct lifetimes. A physical blob file that has been replaced will need to be removed once all Versions that predate the replacement have been unreferenced.

This commit renames the struct previously known as BlobFileMetadata to PhysicalBlobFile, describing metadata particular to a physical file backing a logical blob file. A new BlobFileMetadata struct is introduced that holds a BlobFileID and a pointer to the PhysicalBlobFile.

This commit additionally adapts the physical blob file referencing to occur through a separate B-Tree of BlobFileMetadata structs. Similar to TableMetadata, references to blob files are maintained by copy-on-write B-Tree nodes which themselves are reference counted. A PhysicalBlobFile's reference count is incremented when a new B-Tree node references a containing BlobFileMetadata, and it's decremented when the B-Tree node's reference count falls to zero. This indirection ensures that a mutation to the set of blob files only performs log(n) work. Since addition and removal of blob files within a version is now modeled directly (as opposed to indirectly via TableMetadata BlobReferences), a blob file may be removed or replaced within the set before all referencing TableMetadata are removed.

Informs #4802.

cockroach-teamcity · 2025-06-02T21:08:51Z

This change is

sumeerbhola

Made an initial pass.

Reviewed 2 of 2 files at r2.
Reviewable status: 2 of 18 files reviewed, 14 unresolved discussions

internal/manifest/version_edit.go line 166 at r2 (raw file):

	// While replaying a MANIFEST, the values are nil. Otherwise the values must
	// not be nil.
	DeletedBlobFiles map[base.BlobFileID]*PhysicalBlobFile

I'm getting confused here on why PhysicalBlobFile is sufficient for NewBlobFiles.

When we create an original blob the PhysicalBlobFile is sufficient since the only additional information provided by the BlobFileMetadata is the FileID which is the same as in the PhysicalBlobFile. But when we rewrite an original or an already rewritten blob file, we need the BlobFileMetadata of the new blob, since we need to preserve that mapping from the original FileID as all the sstables that are not in this version edit use that original FileID.

For deleting a blob file it seems the PhysicalBlobFile is sufficient, since a physical blob file's FileID is never referenced by more than one original blob file FileID. Though perhaps it would be easier to have the full BlobFileMetadata to quickly check there are no dangling references.

internal/manifest/blob_metadata.go line 25 at r2 (raw file):

// A BlobReference describes a sstable's reference to a blob value file.
type BlobReference struct {
	// FileID identifies the referenced blob file.

Consider saying that a BlobReference is immutable. And that this is always the original FileID, i.e., if a value reference gets reused in a compaction it continues to reference the original FileID, even if that FileID has already been rewritten.

internal/manifest/blob_metadata.go line 36 at r2 (raw file):

	// OriginalMetadata is the metadata for the original physical blob file. It
	// is non-nil for blob references contained within active Versions. It is

I am unsure about this "is non-nil" comment, that is inherited from the existing code. I don't see us setting BlobReference.Metadata to nil in TableMetadata.Unref. So isn't it always non-nil?

internal/manifest/blob_metadata.go line 48 at r2 (raw file):

	// TODO(jackson): We should either remove the OriginalMetadata from the
	// BlobReference or use it to infer when a blob file has definitely NOT been
	// replaced and a lookup in Version.BlobFileSet is unnecessary.

I suppose the interpolation we do in BlobReference.EstimatedPhysicalSize can continue to use the original metadata without translation? Is that the only reason this exists? Given our in-person discussion today, I'd lean towards removing this and recovering any performance loss in other ways.

internal/manifest/blob_metadata.go line 79 at r2 (raw file):

// BlobFileMetadata encapsulates a blob file ID used to identify a particular
// blob file, and a reference-counted physical blob file. See the BlobFileSet
// documentation for more details.

Consider saying that different versions can have different BlobFileMetadata structs with the same BlobFileMetadata.FileID, and if so they necessarily refer to different PhysicalBlobFiles.

internal/manifest/blob_metadata.go line 82 at r2 (raw file):

type BlobFileMetadata struct {
	// FileID is the file ID used to identify the blob file within
	// BlobReferences.

The phrase "used to identify the blob file within BlobReferences" could be made more precise. Consider saying these are always original blob file ids, i.e., the same domain as BlobReference.FileID.

internal/manifest/blob_metadata.go line 87 at r2 (raw file):

	//
	// If the blob file has been replaced, Physical.FileNum ≠ FileID.
	Physical *PhysicalBlobFile

Always non-nil, yes?

internal/manifest/blob_metadata.go line 103 at r2 (raw file):

// contains one.
func (m BlobFileMetadata) Ref() {
	if m.Physical != nil {

why should this ever be nil?

internal/manifest/blob_metadata.go line 112 at r2 (raw file):

// the provided obsolete files set.
func (m BlobFileMetadata) Unref(of ObsoleteFilesSet) {
	if m.Physical != nil {

ditto

internal/manifest/blob_metadata.go line 312 at r2 (raw file):

// entries remap a blob file ID to a new blob file. This remapping allows for
// replacement of a physical blob file without replacing the referencing
// TableMetadatas.

Consider saying

the key is BlobFileMetadata.FileID.
whether it is a remapping or not is hidden inside BlobFileMetadata which follows the same structure for both.

internal/manifest/blob_metadata.go line 341 at r2 (raw file):

	// Version's B-Tree should maintain a reference. So a call to remove()
	// should never result in a file's reference count dropping to zero, and we
	// pass assertNoObsoleteFiles{} to assert such.

This is because this is only happening as part of creating the new Version so we are holding a ref to the current (soon to be previous) Version, yes? A comment like this in BulkVersionEdit.Apply is easier to grasp since the context is already there, but providing a bit more context here since this is a standalone function would be helpful.

internal/manifest/blob_metadata.go line 385 at r2 (raw file):

	// files is a map of blob file IDs to *currentBlobFiles, recording metadata
	// about the blob file's active references in the latest Version.
	files map[base.BlobFileID]*currentBlobFile

Worth saying that the key is currentBlobFile.metadata.FileID, so original blob file ids.
Given how often we are saying this, I wonder if we should make it a different type.

internal/manifest/blob_metadata.go line 391 at r2 (raw file):

type currentBlobFile struct {
	metadata BlobFileMetadata

Is this a copy of the struct in the btree for the latest version?

internal/manifest/table_metadata.go line 232 at r2 (raw file):

	if v := m.refs.Add(1); v == 1 {
		m.TableBacking.Ref()
		for _, blobRef := range m.BlobReferences {

I see why this is removed since the BlobFileSet refs on its behalf as the same TableMetadata can ref different blob files in different versions.
Do we do a invariants.Enabled consistency check somewhere that a version's BlobFileSet has exactly the FileIDs that are referenced in the TableMetadatas in that version?

jbowens

TFTR!

Reviewable status: 0 of 19 files reviewed, 14 unresolved discussions (waiting on @sumeerbhola)

internal/manifest/blob_metadata.go line 25 at r2 (raw file):