Avoid loading DB for commands where it's not necessary. #8783
Conversation
nicktobey
force-pushed
the
nicktobey/lazy-load
branch
from
9358401 to
5355928
Compare
|
@nicktobey DOLT
|
go/cmd/dolt/dolt.go
Outdated
| // - They require that the database does not exist | ||
| // - They never access the database | ||
| // - They only ever access the database through a CliContext. | ||
| // - They always call dEnv.ReloadDB() before accessing the database for the first time. |
There was a problem hiding this comment.
Commands should avoid using dEnv at all costs. Anywhere we still use them is a place where we use them should be fixed.
Since all of these commands use the CliContext, seems like adding the ReloadDB method there is the right thing to do. For that matter, maybe the CliContext should always load the DB for the first time.
go/cmd/dolt/commands/branch.go
Outdated
| @@ -138,6 +139,10 @@ func (cmd BranchCmd) Exec(ctx context.Context, commandStr string, args []string, | |||
| case apr.Contains(showCurrentFlag): | |||
| return printCurrentBranch(sqlCtx, queryEngine) | |||
| case apr.Contains(datasetsFlag): | |||
| dEnv.ReloadDB(ctx) | |||
There was a problem hiding this comment.
This was a mis in the sql migration. --datasets requires local storage access, and needs to be deprecated.
go/cmd/dolt/commands/show.go
Outdated
| @@ -158,6 +158,10 @@ func (cmd ShowCmd) Exec(ctx context.Context, commandStr string, args []string, d | |||
| cli.PrintErrln("`dolt show --no-pretty` or `dolt show (BRANCHNAME)` is not supported when using old LD_1 storage format.") | |||
| return 1 | |||
| } | |||
| dEnv.ReloadDB(ctx) | |||
There was a problem hiding this comment.
I'll allow this only because we already discussed how busted this is.
go/cmd/dolt/dolt.go
Outdated
| commands.DiffCmd{}, | ||
| commands.ResetCmd{}, | ||
| commands.CleanCmd{}, | ||
| commands.SqlCmd{VersionStr: doltversion.Version}, |
There was a problem hiding this comment.
sql command is kind of special - and it should really have the DB if there is going to be one. The only case where we don't want one is when we are connecting over the wire.
| if dEnv.doltDB == nil { | ||
| LoadDoltDB(ctx, dEnv.FS, dEnv.urlStr, dEnv) | ||
| } | ||
| return dEnv.DoltDB(ctx) |
There was a problem hiding this comment.
huh? Why doesn't the recurrse forever? shouldn't it be return dEnv.doltDb
| for _, namedEnv := range mrEnv.envs { | ||
| dEnv := namedEnv.env | ||
|
|
||
| if dEnv.DoltDB(ctx) == nil { |
There was a problem hiding this comment.
When does this return nil? I'm not really sure what the DoltDb function is supposed to do, but my expectation would be that the LoadDoltDB call in there would succeed and the DoltDB function would return a non-nil value always
| } | ||
| return fkc | ||
| func fkCollection(fks ...DoltDB(ctx).ForeignKey) *doltdb.ForeignKeyCollection { | ||
| fkc, err := doltdb.NewForeignKeyCollection(fks...) |
There was a problem hiding this comment.
what the heck is happening with the formatter here?
| vrw := types.NewValueStore(cs) | ||
| ns := tree.NewNodeStore(cs) | ||
| db = datas.NewTypesDatabase(vrw, ns) | ||
| var db datas.Database |
There was a problem hiding this comment.
formatting again? Not sure why this is happening
| @@ -866,7 +866,7 @@ func (d *DoltSession) ReleaseSavepoint(ctx *sql.Context, tx sql.Transaction, sav | |||
| return nil | |||
| } | |||
|
|
|||
| // GetDoltDB returns the *DoltDB for a given database by name | |||
| // GetDoltDB returns the *doltDB for a given database by name | |||
This PR avoids loading dolt DBs at startup, instead waiting for either
DoltEnv.ReloadDBorMultiRepoEnv.ReloadDBsto be called.Based on the specific subcommand being run:
MultiRepoEnv.ReloadDBsis called prior to command execution if the command will always need DB access.DoltEnv.ReloadDBCommands that make use of
CliContextdon't need to load the DB, as the underlying query engine will load the DB if it needs database access. As a result, commands that are implemented purely in terms of theCliContextwill load the DB when Dolt is runnning an embedded SQL engine, and will skip loading the DB when Dolt is connecting to a running server.