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.

Sign up for GitHub

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

Jump to bottom

Jeremie/notify sql #1198

Merged
merged 8 commits into from
Jun 3, 2024
Merged

Jeremie/notify sql #1198

merged 8 commits into from
Jun 3, 2024

Conversation

lcodes
Copy link
Contributor

@lcodes lcodes commented May 3, 2024 •
edited by Centril

Description of Changes

  • Added a new parameter on execute_sql to bring ModuleSubscriptions into scope.
  • Augmented Code::Pass with an optional Update structure to thread inserts/deletes back into execute_sql where they can be broadcast as an event to subscribers.
  • Updated tests to reflect these changes.

Expected complexity level and risk

2? Still a few things I'm unsure of:

  • Changed the return from evaluating delete from a count to an Update structure holding the count, but (afaik) the count wasn't used.
  • The new Update structure is almost identical to the one from core it then converts into, would it be preferable to move that one instead?
  • Moved ast transform and collect_result outside the transaction scope in execute_sql. Assuming these changes are safe.

Testing

Still need to add tests to validate that changes broadcast correctly.

@lcodes lcodes requested a review from Centril May 3, 2024 02:07
lcodes
lcodes commented May 3, 2024
View reviewed changes
crates/core/src/sql/execute.rs Outdated
collect_result(&mut tables, &mut updates, code?.into())?;

if !reads && !updates.is_empty() {
let event = ModuleEvent {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Left most of the values to defaults here, assuming these relate to reflecting the state of a reducer/client which aren't present in this case.

Only handling the committed case, as collect_result short-circuits the function on error.

lcodes
lcodes commented May 3, 2024
View reviewed changes
crates/core/src/vm.rs Outdated
@@ -447,10 +447,17 @@ impl<'db, 'tx> DbProgram<'db, 'tx> {
// TODO: How do we deal with mutating values?
Table::MemTable(_) => Err(ErrorVm::Other(anyhow::anyhow!("How deal with mutating values?"))),
Table::DbTable(x) => {
let inserts = rows.clone();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would prefer not to clone here, but db.insert wants a row, not a &row. Is it worth extending the refactor in there too?

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's leave a todo for now. This code shouldn't be hot.

lcodes
lcodes commented May 3, 2024
View reviewed changes
crates/core/src/vm.rs Outdated
@@ -518,15 +533,15 @@ impl<'db, 'tx> DbProgram<'db, 'tx> {
}
}
}
Ok(Code::Pass)
Ok(Code::Pass(None)) // TODO going to assume dropping a table doesn't need to broadcast its entire contents as a notify
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Assumption here is deleting a table a reducer/subscriber depends on is either undefined behavior or breaking things up.

Copy link
Contributor

Choose a reason for hiding this comment

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

Correct; modifying the schema which clients depend on is unsupported.

lcodes
lcodes commented May 3, 2024
View reviewed changes
crates/core/src/vm.rs Outdated
}
TxMode::Tx(_) => unreachable!("mutable operation is invalid with read tx"),
}
}

fn _set_config(&mut self, name: String, value: AlgebraicValue) -> Result<Code, ErrorVm> {
self.db.set_config(&name, value)?;
Ok(Code::Pass)
Ok(Code::Pass(None)) // TODO can subscriptions watch config values?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unsure how to broadcast these, as there's no "sql update" happening here. Is there a path to notify config changes?

Copy link
Contributor

Choose a reason for hiding this comment

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

Clients should only care about inserts and deletes in non-system tables; anything else doesn't need to be broadcast.

Centril
Centril reviewed May 3, 2024
View reviewed changes
crates/core/src/util/slow.rs Outdated Show resolved Hide resolved
crates/core/src/vm.rs Outdated Show resolved Hide resolved
crates/core/src/vm.rs Outdated Show resolved Hide resolved
crates/sqltest/src/space.rs Show resolved Hide resolved
crates/core/src/sql/execute.rs Outdated Show resolved Hide resolved
crates/vm/src/expr.rs Outdated Show resolved Hide resolved
crates/core/src/sql/execute.rs Outdated Show resolved Hide resolved
@bfops bfops added the release-any To be landed in any release window label May 13, 2024
Centril
Centril approved these changes May 17, 2024
View reviewed changes
Copy link
Contributor

@Centril Centril left a comment

Choose a reason for hiding this comment

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

Looks good with the latest changes.

@Centril
Copy link
Contributor

Centril commented May 20, 2024

(Needs a rebase though)

lcodes
lcodes commented May 29, 2024
View reviewed changes
crates/client-api/src/routes/database.rs Outdated
let host = worker_ctx.host_controller();
let module_host = host.get_or_launch_module_host(database.clone(), instance_id).await.map_err(|e| {
log::warn!("{}", e);
(StatusCode::INTERNAL_SERVER_ERROR, "Internal Server Error")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay this is a bit aggressive, but I think this is the wanted behavior

Copy link
Contributor

Choose a reason for hiding this comment

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

Why warn! and not use log_and_500 here?

@Centril Centril mentioned this pull request May 29, 2024
Merged
@Centril
Copy link
Contributor

Centril commented May 29, 2024

Looks like you need to cargo fmt also :)

Centril
Centril approved these changes May 30, 2024
View reviewed changes
Copy link
Contributor

@Centril Centril left a comment

Choose a reason for hiding this comment

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

Looks good, but please also consider #1198 (comment) before deciding to merge. But I'm happy for this to merge either way.

@cloutiertyler cloutiertyler added release-0.10 and removed release-any To be landed in any release window labels Jun 3, 2024
@lcodes lcodes added this pull request to the merge queue Jun 3, 2024
Merged via the queue into master with commit da23401 Jun 3, 2024
6 checks passed
@lcodes lcodes deleted the jeremie/notify-sql branch June 3, 2024 23:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gefjon gefjon gefjon left review comments

@Centril Centril Centril approved these changes

Assignees
No one assigned
Labels
backward-compatible release-0.10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@lcodes @Centril @gefjon @bfops @cloutiertyler