-
Notifications
You must be signed in to change notification settings - Fork 344
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
schema migration #1110
schema migration #1110
Conversation
e258feb
to
162fbd9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since it's still a draft, just few points. The overall design seems pretty solid and aligned with what we were discussing in previous weeks. Please let me know when this is ready to full review.
libsql-server/src/database/schema.rs
Outdated
.await?; | ||
self.connection | ||
.execute_program(pgm, ctx, response_builder, replication_index) | ||
dbg!(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Committed by accident?
3 => Self::Run, | ||
4 => Self::Success, | ||
5 => Self::Failure, | ||
_ => panic!(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_ => panic!(), | |
_ => unreachable!(), |
3 => Self::WaitingRun, | ||
4 => Self::RunSuccess, | ||
5 => Self::RunFailure, | ||
_ => panic!(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_ => panic!(), | |
_ => unreachable!(), |
builder: B, | ||
) -> (Result<B, Error>, MigrationTaskStatus) { | ||
// todo error handling is sketchy, improve | ||
let mut savepoint = conn.savepoint().unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this is also a place for bottomless savepoint (see #1085) in order to guarantee, that in case of crashes we can restore db to a state where the dry run succeeded. However in a current API it operates on the Database
type not a Connection
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's discuss that
162fbd9
to
1c06866
Compare
1c06866
to
23838ee
Compare
implements the happy path of schema migration. do not use this feature yet.
TODO:
include!
that file for clarityenqueued_tasks
is updated corectly after a task is popped when a tasks status changed (maybe use a trigger?)