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

Fixes the restart problem (i.e. rebuilding the datastore from the message log) #34

Merged
merged 2 commits into from
Jun 30, 2023

Conversation

cloutiertyler
Copy link
Contributor

Description of Changes

This makes it so that we appropriate rebuild in memory data structures from the message log after restarting the database.

I don't love the solution, but a general solution is really tricky and we're on a bit of a deadline.

API

  • This is a breaking change to the module API
  • This is a breaking change to the ClientAPI

If the API is breaking, please state below what will break

pub fn rebuild_state_after_replay(&self) -> Result<(), DBError> {
let mut inner = self.inner.lock();

// `build_missing_tables` must be called before indexes.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd be fine with that if the very deep for loop bodies inside got factored out.

.unwrap()
.insert_tables
.get(table_id)
.and_then(|tx_state| tx_state.insert_tables.get(table_id))
Copy link
Contributor

Choose a reason for hiding this comment

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

I have mixed feelings about this pattern. I don't think the ability to reason about the state (tx_state is None or not, corresponding to being currently in a transaction or not) was great, but I think this also muddles it a little further. I don't think it's actionable now, but something that we could want to make clearer in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I hate this whole thing tbh, but I'm just trying to get it to build in time. :/

Some(RowOp::Absent) => {
return Some(DataRef::new(row.clone()));
}
None => {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is an example of my comment about what state we are in, now there's this extra None case in which you are supposed to do the same thing as when you see Some(Absent).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed.

@cloutiertyler cloutiertyler merged commit 2621de2 into master Jun 30, 2023
cloutiertyler added a commit that referenced this pull request Aug 1, 2023
…sage log) (#34)

* Fixes the restart problem (i.e. rebuilding the datastore from the message log)

* Remove logging
cloutiertyler added a commit that referenced this pull request Aug 1, 2023
…sage log) (#34)

* Fixes the restart problem (i.e. rebuilding the datastore from the message log)

* Remove logging
@cloutiertyler cloutiertyler deleted the tyler/restart branch August 1, 2023 21:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants