From e5827be667f3c5add05edb8cec2d130bf7e7d5da Mon Sep 17 00:00:00 2001 From: Alex Giurgiu Date: Fri, 17 Nov 2023 18:32:55 +0200 Subject: [PATCH 1/3] first pass switching backups to concurrent map --- go/cmd/dolt/commands/backup.go | 4 +-- go/libraries/doltcore/env/environment.go | 29 ++++++++++--------- go/libraries/doltcore/env/memory.go | 4 +-- go/libraries/doltcore/env/repo_state.go | 14 ++++----- go/libraries/doltcore/sqle/database.go | 2 +- .../doltcore/sqle/dprocedures/dolt_backup.go | 2 +- .../sqle/dsess/database_session_state.go | 2 +- .../sqle/dsess/session_state_adapter.go | 18 ++++++------ 8 files changed, 39 insertions(+), 36 deletions(-) diff --git a/go/cmd/dolt/commands/backup.go b/go/cmd/dolt/commands/backup.go index 71735eff95..ab26abb7fc 100644 --- a/go/cmd/dolt/commands/backup.go +++ b/go/cmd/dolt/commands/backup.go @@ -208,7 +208,7 @@ func printBackups(dEnv *env.DoltEnv, apr *argparser.ArgParseResults) errhand.Ver return errhand.BuildDError("Unable to get backups from the local directory").AddCause(err).Build() } - for _, r := range backups { + for _, r := range backups.Snapshot() { if apr.Contains(cli.VerboseFlag) { paramStr := make([]byte, 0) if len(r.Params) > 0 { @@ -256,7 +256,7 @@ func syncBackup(ctx context.Context, dEnv *env.DoltEnv, apr *argparser.ArgParseR return errhand.BuildDError("Unable to get backups from the local directory").AddCause(err).Build() } - b, ok := backups[backupName] + b, ok := backups.Get(backupName) if !ok { return errhand.BuildDError("error: unknown backup: '%s' ", backupName).Build() } diff --git a/go/libraries/doltcore/env/environment.go b/go/libraries/doltcore/env/environment.go index 302174c05c..52a11598ac 100644 --- a/go/libraries/doltcore/env/environment.go +++ b/go/libraries/doltcore/env/environment.go @@ -115,12 +115,7 @@ func createRepoState(fs filesys.Filesys) (*RepoState, error) { // deep copy remotes and backups ¯\_(ツ)_/¯ (see commit c59cbead) if repoState != nil { repoState.Remotes = repoState.Remotes.DeepCopy() - - backups := make(map[string]Remote, len(repoState.Backups)) - for n, r := range repoState.Backups { - backups[n] = r - } - repoState.Backups = backups + repoState.Backups = repoState.Backups.DeepCopy() } return repoState, rsErr @@ -863,7 +858,7 @@ func (dEnv *DoltEnv) GetRemotes() (*concurrentmap.Map[string, Remote], error) { // CheckRemoteAddressConflict checks whether any backups or remotes share the given URL. Returns the first remote if multiple match. // Returns NoRemote and false if none match. -func CheckRemoteAddressConflict(absUrl string, remotes *concurrentmap.Map[string, Remote], backups map[string]Remote) (Remote, bool) { +func CheckRemoteAddressConflict(absUrl string, remotes *concurrentmap.Map[string, Remote], backups *concurrentmap.Map[string, Remote]) (Remote, bool) { if remotes != nil { var rm *Remote remotes.Iter(func(key string, value Remote) bool { @@ -878,9 +873,17 @@ func CheckRemoteAddressConflict(absUrl string, remotes *concurrentmap.Map[string } } - for _, r := range backups { - if r.Url == absUrl { - return r, true + if backups != nil { + var rm *Remote + backups.Iter(func(key string, value Remote) bool { + if value.Url == absUrl { + rm = &value + return false + } + return true + }) + if rm != nil { + return *rm, true } } return NoRemote, false @@ -910,7 +913,7 @@ func (dEnv *DoltEnv) AddRemote(r Remote) error { return dEnv.RepoState.Save(dEnv.FS) } -func (dEnv *DoltEnv) GetBackups() (map[string]Remote, error) { +func (dEnv *DoltEnv) GetBackups() (*concurrentmap.Map[string, Remote], error) { if dEnv.RSLoadErr != nil { return nil, dEnv.RSLoadErr } @@ -919,7 +922,7 @@ func (dEnv *DoltEnv) GetBackups() (map[string]Remote, error) { } func (dEnv *DoltEnv) AddBackup(r Remote) error { - if _, ok := dEnv.RepoState.Backups[r.Name]; ok { + if _, ok := dEnv.RepoState.Backups.Get(r.Name); ok { return ErrBackupAlreadyExists } @@ -976,7 +979,7 @@ func (dEnv *DoltEnv) RemoveRemote(ctx context.Context, name string) error { } func (dEnv *DoltEnv) RemoveBackup(ctx context.Context, name string) error { - backup, ok := dEnv.RepoState.Backups[name] + backup, ok := dEnv.RepoState.Backups.Get(name) if !ok { return ErrBackupNotFound } diff --git a/go/libraries/doltcore/env/memory.go b/go/libraries/doltcore/env/memory.go index 888d29ca26..99571db6af 100644 --- a/go/libraries/doltcore/env/memory.go +++ b/go/libraries/doltcore/env/memory.go @@ -209,7 +209,7 @@ func (m MemoryRepoState) SetCWBHeadRef(_ context.Context, r ref.MarshalableRef) } func (m MemoryRepoState) GetRemotes() (*concurrentmap.Map[string, Remote], error) { - return &concurrentmap.Map[string, Remote]{}, nil + return concurrentmap.New[string, Remote](), nil } func (m MemoryRepoState) AddRemote(r Remote) error { @@ -232,7 +232,7 @@ func (m MemoryRepoState) TempTableFilesDir() (string, error) { return os.TempDir(), nil } -func (m MemoryRepoState) GetBackups() (map[string]Remote, error) { +func (m MemoryRepoState) GetBackups() (*concurrentmap.Map[string, Remote], error) { panic("cannot get backups on in memory database") } diff --git a/go/libraries/doltcore/env/repo_state.go b/go/libraries/doltcore/env/repo_state.go index 05f096a610..3cef197600 100644 --- a/go/libraries/doltcore/env/repo_state.go +++ b/go/libraries/doltcore/env/repo_state.go @@ -32,7 +32,7 @@ type RepoStateReader interface { CWBHeadRef() (ref.DoltRef, error) CWBHeadSpec() (*doltdb.CommitSpec, error) GetRemotes() (*concurrentmap.Map[string, Remote], error) - GetBackups() (map[string]Remote, error) + GetBackups() (*concurrentmap.Map[string, Remote], error) GetBranches() (map[string]BranchConfig, error) } @@ -71,7 +71,7 @@ type BranchConfig struct { type RepoState struct { Head ref.MarshalableRef `json:"head"` Remotes *concurrentmap.Map[string, Remote] `json:"remotes"` - Backups map[string]Remote `json:"backups"` + Backups *concurrentmap.Map[string, Remote] `json:"backups"` Branches map[string]BranchConfig `json:"branches"` // |staged|, |working|, and |merge| are legacy fields left over from when Dolt repos stored this info in the repo // state file, not in the DB directly. They're still here so that we can migrate existing repositories forward to the @@ -86,7 +86,7 @@ type RepoState struct { type repoStateLegacy struct { Head ref.MarshalableRef `json:"head"` Remotes *concurrentmap.Map[string, Remote] `json:"remotes"` - Backups map[string]Remote `json:"backups"` + Backups *concurrentmap.Map[string, Remote] `json:"backups"` Branches map[string]BranchConfig `json:"branches"` Staged string `json:"staged,omitempty"` Working string `json:"working,omitempty"` @@ -162,7 +162,7 @@ func CloneRepoState(fs filesys.ReadWriteFS, r Remote) (*RepoState, error) { working: hashStr, Remotes: remotes, Branches: make(map[string]BranchConfig), - Backups: make(map[string]Remote), + Backups: concurrentmap.New[string, Remote](), } err := rs.Save(fs) @@ -184,7 +184,7 @@ func CreateRepoState(fs filesys.ReadWriteFS, br string) (*RepoState, error) { Head: ref.MarshalableRef{Ref: headRef}, Remotes: concurrentmap.New[string, Remote](), Branches: make(map[string]BranchConfig), - Backups: make(map[string]Remote), + Backups: concurrentmap.New[string, Remote](), } err = rs.Save(fs) @@ -224,9 +224,9 @@ func (rs *RepoState) RemoveRemote(r Remote) { } func (rs *RepoState) AddBackup(r Remote) { - rs.Backups[r.Name] = r + rs.Backups.Set(r.Name, r) } func (rs *RepoState) RemoveBackup(r Remote) { - delete(rs.Backups, r.Name) + rs.Backups.Delete(r.Name) } diff --git a/go/libraries/doltcore/sqle/database.go b/go/libraries/doltcore/sqle/database.go index 9fc33cf6de..4282a3f7db 100644 --- a/go/libraries/doltcore/sqle/database.go +++ b/go/libraries/doltcore/sqle/database.go @@ -419,7 +419,7 @@ func (db Database) getTableInsensitive(ctx *sql.Context, head *doltdb.Commit, ds sess, db.RevisionQualifiedName(), concurrentmap.New[string, env.Remote](), map[string]env.BranchConfig{}, - map[string]env.Remote{}) + concurrentmap.New[string, env.Remote]()) ws, err := sess.WorkingSet(ctx, db.RevisionQualifiedName()) if err != nil { return nil, false, err diff --git a/go/libraries/doltcore/sqle/dprocedures/dolt_backup.go b/go/libraries/doltcore/sqle/dprocedures/dolt_backup.go index d88025c9ab..fdc70f13c2 100644 --- a/go/libraries/doltcore/sqle/dprocedures/dolt_backup.go +++ b/go/libraries/doltcore/sqle/dprocedures/dolt_backup.go @@ -224,7 +224,7 @@ func syncBackupViaName(ctx *sql.Context, dbData env.DbData, sess *dsess.DoltSess return err } - b, ok := backups[backupName] + b, ok := backups.Get(backupName) if !ok { return fmt.Errorf("error: unknown backup: '%s'; %v", backupName, backups) } diff --git a/go/libraries/doltcore/sqle/dsess/database_session_state.go b/go/libraries/doltcore/sqle/dsess/database_session_state.go index 1517def35a..0fe0913690 100644 --- a/go/libraries/doltcore/sqle/dsess/database_session_state.go +++ b/go/libraries/doltcore/sqle/dsess/database_session_state.go @@ -43,7 +43,7 @@ type InitialDbState struct { DbData env.DbData Remotes *concurrentmap.Map[string, env.Remote] Branches map[string]env.BranchConfig - Backups map[string]env.Remote + Backups *concurrentmap.Map[string, env.Remote] // If err is set, this InitialDbState is partially invalid, but may be // usable to initialize a database at a revision specifier, for diff --git a/go/libraries/doltcore/sqle/dsess/session_state_adapter.go b/go/libraries/doltcore/sqle/dsess/session_state_adapter.go index 388206ceb6..7242cca5c7 100644 --- a/go/libraries/doltcore/sqle/dsess/session_state_adapter.go +++ b/go/libraries/doltcore/sqle/dsess/session_state_adapter.go @@ -33,7 +33,7 @@ type SessionStateAdapter struct { session *DoltSession dbName string remotes *concurrentmap.Map[string, env.Remote] - backups map[string]env.Remote + backups *concurrentmap.Map[string, env.Remote] branches map[string]env.BranchConfig } @@ -45,7 +45,7 @@ var _ env.RepoStateReader = SessionStateAdapter{} var _ env.RepoStateWriter = SessionStateAdapter{} var _ env.RootsProvider = SessionStateAdapter{} -func NewSessionStateAdapter(session *DoltSession, dbName string, remotes *concurrentmap.Map[string, env.Remote], branches map[string]env.BranchConfig, backups map[string]env.Remote) SessionStateAdapter { +func NewSessionStateAdapter(session *DoltSession, dbName string, remotes *concurrentmap.Map[string, env.Remote], branches map[string]env.BranchConfig, backups *concurrentmap.Map[string, env.Remote]) SessionStateAdapter { if branches == nil { branches = make(map[string]env.BranchConfig) } @@ -92,7 +92,7 @@ func (s SessionStateAdapter) GetRemotes() (*concurrentmap.Map[string, env.Remote return s.remotes, nil } -func (s SessionStateAdapter) GetBackups() (map[string]env.Remote, error) { +func (s SessionStateAdapter) GetBackups() (*concurrentmap.Map[string, env.Remote], error) { return s.backups, nil } @@ -147,7 +147,7 @@ func (s SessionStateAdapter) AddRemote(remote env.Remote) error { } func (s SessionStateAdapter) AddBackup(backup env.Remote) error { - if _, ok := s.backups[backup.Name]; ok { + if _, ok := s.backups.Get(backup.Name); ok { return env.ErrBackupAlreadyExists } @@ -170,7 +170,7 @@ func (s SessionStateAdapter) AddBackup(backup env.Remote) error { return fmt.Errorf("%w: '%s' -> %s", env.ErrRemoteAddressConflict, bac.Name, bac.Url) } - s.backups[backup.Name] = backup + s.backups.Set(backup.Name, backup) repoState.AddBackup(backup) return repoState.Save(fs) } @@ -202,11 +202,11 @@ func (s SessionStateAdapter) RemoveRemote(_ context.Context, name string) error } func (s SessionStateAdapter) RemoveBackup(_ context.Context, name string) error { - backup, ok := s.backups[name] + backup, ok := s.backups.Get(name) if !ok { return env.ErrBackupNotFound } - delete(s.backups, backup.Name) + s.backups.Delete(backup.Name) fs, err := s.session.Provider().FileSystemForDatabase(s.dbName) if err != nil { @@ -218,12 +218,12 @@ func (s SessionStateAdapter) RemoveBackup(_ context.Context, name string) error return err } - backup, ok = repoState.Backups[name] + backup, ok = repoState.Backups.Get(name) if !ok { // sanity check return env.ErrBackupNotFound } - delete(repoState.Backups, name) + repoState.Backups.Delete(name) return repoState.Save(fs) } From 7a36a96af6b9efab25413c87e84b432d19ec235d Mon Sep 17 00:00:00 2001 From: Alex Giurgiu Date: Fri, 17 Nov 2023 18:38:13 +0200 Subject: [PATCH 2/3] initialize backups as well in the RepoState --- go/libraries/doltcore/env/environment_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/go/libraries/doltcore/env/environment_test.go b/go/libraries/doltcore/env/environment_test.go index 7a0bacd349..a482fe0fed 100644 --- a/go/libraries/doltcore/env/environment_test.go +++ b/go/libraries/doltcore/env/environment_test.go @@ -53,7 +53,7 @@ func createTestEnv(isInitialized bool, hasLocalConfig bool) (*DoltEnv, *filesys. initialDirs = append(initialDirs, doltDataDir) mainRef := ref.NewBranchRef(DefaultInitBranch) - repoState := &RepoState{Head: ref.MarshalableRef{Ref: mainRef}, Remotes: concurrentmap.New[string, Remote]()} + repoState := &RepoState{Head: ref.MarshalableRef{Ref: mainRef}, Remotes: concurrentmap.New[string, Remote](), Backups: concurrentmap.New[string, Remote]()} repoStateData, err := json.Marshal(repoState) if err != nil { From 5aed4d14bde95da8709c48cd8925bdbb77f6e6ed Mon Sep 17 00:00:00 2001 From: Alex Giurgiu Date: Sat, 18 Nov 2023 15:15:14 +0200 Subject: [PATCH 3/3] setting default values for repo state remotes and backups. They should never be nil even if the on disk config doesn't have values for them --- go/libraries/doltcore/env/repo_state.go | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/go/libraries/doltcore/env/repo_state.go b/go/libraries/doltcore/env/repo_state.go index 3cef197600..2b48382bc6 100644 --- a/go/libraries/doltcore/env/repo_state.go +++ b/go/libraries/doltcore/env/repo_state.go @@ -112,7 +112,7 @@ type mergeState struct { } func (rs *repoStateLegacy) toRepoState() *RepoState { - return &RepoState{ + newRS := &RepoState{ Head: rs.Head, Remotes: rs.Remotes, Backups: rs.Backups, @@ -121,6 +121,15 @@ func (rs *repoStateLegacy) toRepoState() *RepoState { working: rs.Working, merge: rs.Merge, } + + if newRS.Remotes == nil { + newRS.Remotes = concurrentmap.New[string, Remote]() + } + if newRS.Backups == nil { + newRS.Backups = concurrentmap.New[string, Remote]() + } + + return newRS } func (rs *repoStateLegacy) save(fs filesys.ReadWriteFS) error {