Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Split DB loading into two stages. #8852

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Split DB loading into two stages. #8852

wants to merge 3 commits into from

Conversation

nicktobey
Copy link
Contributor

This is a follow up to #8783, which was supposed to defer loading the DB for commands where it wasn't necessary. The main motivation was for situations where the Dolt CLI doesn't need to load a local DB because it connects to an already-running server instead.

In practice, the prior PR didn't actually accomplish much because in order to determine whether Dolt should connect to an already running server, it checks to see if there's a lock on the local DB, and in order to do that it tries to load it.

In order for CLI commands against an already-running server to avoid loading the DB, we need to either:

  • Detect a lock on the DB without loading it
  • Look for a ServerLocalCredsFile before determining whether or not to connect to a remote server.

This PR tries to tackle the former, although in hindsight the latter might be a cleaner, simpler fix.

Basically, this PR allows us to detect locks without loading the DB by splitting the DB load into two phases:
Phase 1: Acquire any necessary exclusive resources (like locks)
Phase 2: Finish loading the DB

We accomplish this by modifying the DBFactory interface. Previously, it had a CreateDB method that fully loads the DB. Now, it has a GetDBLoader method that performs the first phase and returns an instance of the DBLoader interface. DBLoader has a method LoadDB that performs the second phase and returns the fully loaded database.

This allows us to add other methods to DBLoader to inspect qualities of the DB that were ascertained during the first phase, such as whether or not we were able to acquire an exclusive lock.

Notes:

  • This still needs additional tests
  • As written, for multigenerational file stores, we only acquire the lock on the newgen during the first phase, and only acquire the oldgen lock during the second phase. I don't think this can lead to any race conditions: both phases are guarded by the same mutex and their results are cached. But it is potentially confusing and it might be better to acquire both file locks at the same time.

@coffeegoddd
Copy link
Contributor

@nicktobey DOLT

comparing_percentages
100.000000 to 100.000000
version result total
f506927 ok 5937457
version total_tests
f506927 5937457
correctness_percentage
100.0

if s, ok := singletonFileLocks[urlObj.Path]; ok {
lock = s
} else {
lock, err = nbs.AcquireManifestLock(path)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These lifecycle changes seem weird and complicated and potentially wrong for non-journal stores (if those still exist...in that world the file manifest manager only hold the lock while it makes the switch...). Is there an alternative world where we change CreateDB() so that we can request ExeclusiveAccess and if it can't give it to us it can bail early and we can make lazy queryist use that instead?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we're not actually looking for Exclusive. We're looking for non-ReadOnly. Same thought though...

@coffeegoddd
Copy link
Contributor

@nicktobey DOLT

comparing_percentages
100.000000 to 100.000000
version result total
2af0836 ok 5937457
version total_tests
2af0836 5937457
correctness_percentage
100.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants