Invalidate all sessions' db state for drop database
#5556
Conversation
…is an InMemFilesystem
| // TODO: this might be a better fit as a function on SessionManager | ||
| // TODO: Instead of deleting the db state, could we just mark it as invalid and continue using that same instance | ||
| // in memory, even when we create a new db with the same name? reusing for creation of a new db might be tricky. | ||
| runningServer := sqlserver.GetRunningServer() |
There was a problem hiding this comment.
This global is super spooky! Can we thread the server through when we construct the database provider for the engine? Definitely not related to this PR per se, but seeing this global makes me want to immediately go try to remove it...
There was a problem hiding this comment.
Thanks for taking a look! Yeah... looks like we could change sqlengine.go and server.go a little bit to set the MySQL server directly into DoltDatabaseProvider so that it doesn't have to reach into that global var. I'll push a commit with that and we can see how it looks.
There was a problem hiding this comment.
Well... this turned out to be a little messier than I was expecting... It looks like we're always using DoltDatabaseProvider as a struct for some reason, and not as a pointer to a struct. The DoltDatabaseProvider struct gets created when we create the engine and then passed (as a copy) into GMS. After that, the sqlEngine is instantiated and we can start the sql-server at which point we get the server reference that we want to set in the provider, but we can't change the provider registered in GMS, because a copy was passed in, and we don't have a reference to that memory.
I'm happy to change DoltDatabaseProvider to be used as a pointer (seems like a good change to make for several reasons), but it feels like that might be cleaner in a separate PR since it'll be more than just a couple real quick changes like I thought this was going to be.
There was a problem hiding this comment.
Thanks for the explanation and investigating this. I agree this doesn't need to be addressed here :).
… into fulghum/drop-database
|
|
||
| // We need to invalidate this database state for EVERY session, even if other sessions aren't actively | ||
| // using this database, since they could still reference it with a db-qualified table name. | ||
| err := dsess.RemoveDbState(ctx, name) |
There was a problem hiding this comment.
This is all a bit crunchy. Probably fine to check in for now, but it seems like this is a sign of missing abstractions on the GMS side.
Historically the Session interface was really underdefined for how much it was responsible for, and this kind of thing is a similar situation. We really want to engine to orchestrate dropping the database, including things like 1) preventing new connections to that DB once the drop operation begins, 2) cleanly invalidating any sessions / transactions that have that DB in scope. Might need to define a SessionProvider / SessionManager interface for this to work cleanly.
There was a problem hiding this comment.
Totally agree. I briefly thought about pushing some of this down to SessionManager (seems appropriate), but the state is at the Dolt layer of course, so it needed a callback hook and felt like the scope was increasing. You're right that it definitely smells like the abstraction is not ideal here.
3 participants
Fix for: #5349
When a database is deleted, the cached dbState held by every session needs to be invalidated. Otherwise sessions think the deleted database still exists and are can still reference the deleted db's data, which does not match MySQL's behavior. If a new database is then created with the same name, then sessions will still use the old db info and aren't able to see the new database state.
This change means that a session's
dbStatemember now needs to be accessed concurrently, so I've also added synchronization using the exiting session mutex.Related GMS PR: dolthub/go-mysql-server#1654