objstorage: simplify AttachRemoteObjects

Simplify `AttachRemoteObjects` to not call `sharedSync`. We already call `Sync` as part of ingestion. Add a call `sharedSync` when closing the provider (this should be a no-op in practice, other than tests).
cockroachdb · Jan 23, 2024 · 5b280af · 5b280af
1 parent c4c17a7
commit 5b280af
Show file tree

Hide file tree

Showing 9 changed files with 13 additions and 31 deletions.
diff --git a/objstorage/objstorage.go b/objstorage/objstorage.go
@@ -287,6 +287,9 @@ type Provider interface {
 	CreateExternalObjectBacking(locator remote.Locator, objName string) (RemoteObjectBacking, error)
 
 	// AttachRemoteObjects registers existing remote objects with this provider.
+	//
+	// The objects are not guaranteed to be durable (accessible in case of
+	// crashes) until Sync is called.
 	AttachRemoteObjects(objs []RemoteObjectToAttach) ([]ObjectMetadata, error)
 
 	Close() error

diff --git a/objstorage/objstorageprovider/provider.go b/objstorage/objstorageprovider/provider.go
@@ -456,11 +456,15 @@ func (p *provider) Metrics() sharedcache.Metrics {
 }
 
 func (p *provider) addMetadata(meta objstorage.ObjectMetadata) {
+	p.mu.Lock()
+	defer p.mu.Unlock()
+	p.addMetadataLocked(meta)
+}
+
+func (p *provider) addMetadataLocked(meta objstorage.ObjectMetadata) {
 	if invariants.Enabled {
 		meta.AssertValid()
 	}
-	p.mu.Lock()
-	defer p.mu.Unlock()
 	p.mu.knownObjects[meta.DiskFileNum] = meta
 	if meta.IsRemote() {
 		p.mu.remote.catalogBatch.AddObject(remoteobjcat.RemoteObjectMetadata{

diff --git a/objstorage/objstorageprovider/remote.go b/objstorage/objstorageprovider/remote.go
@@ -126,9 +126,9 @@ func (p *provider) sharedClose() error {
 	if p.st.Remote.StorageFactory == nil {
 		return nil
 	}
-	var err error
+	err := p.sharedSync()
 	if p.remote.cache != nil {
-		err = p.remote.cache.Close()
+		err = firstError(err, p.remote.cache.Close())
 		p.remote.cache = nil
 	}
 	if p.remote.catalog != nil {

diff --git a/objstorage/objstorageprovider/remote_backing.go b/objstorage/objstorageprovider/remote_backing.go
@@ -12,7 +12,6 @@ import (
 	"github.com/cockroachdb/errors"
 	"github.com/cockroachdb/pebble/internal/base"
 	"github.com/cockroachdb/pebble/objstorage"
-	"github.com/cockroachdb/pebble/objstorage/objstorageprovider/remoteobjcat"
 	"github.com/cockroachdb/pebble/objstorage/remote"
 )
 
@@ -271,34 +270,15 @@ func (p *provider) AttachRemoteObjects(
 		}
 	}
 
-	func() {
-		p.mu.Lock()
-		defer p.mu.Unlock()
-		for _, d := range decoded {
-			p.mu.remote.catalogBatch.AddObject(remoteobjcat.RemoteObjectMetadata{
-				FileNum:          d.meta.DiskFileNum,
-				FileType:         d.meta.FileType,
-				CreatorID:        d.meta.Remote.CreatorID,
-				CreatorFileNum:   d.meta.Remote.CreatorFileNum,
-				CleanupMethod:    d.meta.Remote.CleanupMethod,
-				Locator:          d.meta.Remote.Locator,
-				CustomObjectName: d.meta.Remote.CustomObjectName,
-			})
-		}
-	}()
-	if err := p.sharedSync(); err != nil {
-		return nil, err
-	}
-
 	metas := make([]objstorage.ObjectMetadata, len(decoded))
 	for i, d := range decoded {
 		metas[i] = d.meta
 	}
 
 	p.mu.Lock()
 	defer p.mu.Unlock()
-	for _, meta := range metas {
-		p.mu.knownObjects[meta.DiskFileNum] = meta
+	for i := range metas {
+		p.addMetadataLocked(metas[i])
 	}
 	return metas, nil
 }
diff --git a/objstorage/objstorageprovider/testdata/provider/shared_attach b/objstorage/objstorageprovider/testdata/provider/shared_attach
@@ -101,7 +101,6 @@ b3 103
 <remote> create object "eaac-1-000003.sst.ref.2.000103"
 <remote> close writer for "eaac-1-000003.sst.ref.2.000103" after 0 bytes
 <remote> size of object "eaac-1-000003.sst.ref.1.000003": 0
-<local fs> sync: p2/REMOTE-OBJ-CATALOG-000001
 000101 -> remote://61a6-1-000001.sst
 000102 -> remote://a629-1-000002.sst
 000103 -> remote://eaac-1-000003.sst

diff --git a/objstorage/objstorageprovider/testdata/provider/shared_attach_after_unref b/objstorage/objstorageprovider/testdata/provider/shared_attach_after_unref
@@ -45,7 +45,6 @@ p5b1 101
 <remote> create object "d632-5-000001.sst.ref.6.000101"
 <remote> close writer for "d632-5-000001.sst.ref.6.000101" after 0 bytes
 <remote> size of object "d632-5-000001.sst.ref.5.000001": 0
-<local fs> sync: p6/REMOTE-OBJ-CATALOG-000001
 000101 -> remote://d632-5-000001.sst
 
 switch p5

diff --git a/objstorage/objstorageprovider/testdata/provider/shared_attach_multi b/objstorage/objstorageprovider/testdata/provider/shared_attach_multi
@@ -49,7 +49,6 @@ b1 103
 <remote> create object "61a6-1-000001.sst.ref.2.000103"
 <remote> close writer for "61a6-1-000001.sst.ref.2.000103" after 0 bytes
 <remote> size of object "61a6-1-000001.sst.ref.1.000001": 0
-<local fs> sync: p2/REMOTE-OBJ-CATALOG-000001
 000101 -> remote://61a6-1-000001.sst
 000102 -> remote://61a6-1-000001.sst
 000103 -> remote://61a6-1-000001.sst

diff --git a/objstorage/objstorageprovider/testdata/provider/shared_no_ref b/objstorage/objstorageprovider/testdata/provider/shared_no_ref
@@ -134,7 +134,6 @@ attach
 b1 101
 b2 102
 ----
-<local fs> sync: p2/REMOTE-OBJ-CATALOG-000001
 000101 -> remote://61a6-1-000001.sst
 000102 -> remote://61a6-1-000001.sst
 

diff --git a/objstorage/objstorageprovider/testdata/provider/shared_remove b/objstorage/objstorageprovider/testdata/provider/shared_remove
@@ -66,7 +66,6 @@ b2 102
 <remote> create object "a629-1-000002.sst.ref.2.000102"
 <remote> close writer for "a629-1-000002.sst.ref.2.000102" after 0 bytes
 <remote> size of object "a629-1-000002.sst.ref.1.000002": 0
-<local fs> sync: p2/REMOTE-OBJ-CATALOG-000001
 000101 -> remote://61a6-1-000001.sst
 000102 -> remote://a629-1-000002.sst