feat: split multi-query requests - #1400
Conversation
e2bb930 to
a4066ea
Compare
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
sgrif
left a comment
sgrif
left a comment
There was a problem hiding this comment.
Would be good to have an integration test that looks like
execute("INSERT ...; BEGIN; INSERT ...")
execute("ROLLBACK")
assert_only_first_insert_visible
| return Err(Error::ImplicitOnDisconnected); | ||
| } | ||
|
|
||
| self.backend.execute("BEGIN").await?; |
There was a problem hiding this comment.
If we're rewriting to the extended protocol, we shouldn't need this explicit transaction control. IMO we should avoid needing to handle it this way so we don't have to worry about handling transaction control statements in the query itself.
There was a problem hiding this comment.
Yeah I agree. It was just more complicated because then we would need to handle PREPARE and EXECUTE sent via Parse, which we currently don't, so the scope increased.
It's actually pretty easy for us right now to start/commit implicit transactions ourselves, we do it already for normal transactions and for 2pc, so this is not a big leap.
But that of course still leaves the problem of a client lib executing PREPARE via Parse unsolved.
There was a problem hiding this comment.
My only thought is that fixing PREPARE/EXECUTE via extended protocol is a relatively contained (and I would hope straightforward) bug fix, while not being able to support BEGIN mid-query is more structural and harder to address later.
But I don't feel strongly about this either way
There was a problem hiding this comment.
I'm starting to come around to your point of view here. Ive been debugging transaction state for a couple days now and it has a lot of interactions with savepoints, 2pc and maybe supporting prepare here isn't worth it ...
There was a problem hiding this comment.
Happy to do the work of supporting it separately from this PR :)
| let stmts = ast | ||
| .ast | ||
| .stmts() | ||
| .map(deparse) | ||
| .collect::<Result<Vec<_>, PgParseRawError>>()?; | ||
| let queries = stmts | ||
| .into_iter() | ||
| .map(|query| query.as_str().to_string()) | ||
| .collect::<Vec<_>>(); |
There was a problem hiding this comment.
We don't need the intermediate vec
| let stmts = ast | |
| .ast | |
| .stmts() | |
| .map(deparse) | |
| .collect::<Result<Vec<_>, PgParseRawError>>()?; | |
| let queries = stmts | |
| .into_iter() | |
| .map(|query| query.as_str().to_string()) | |
| .collect::<Vec<_>>(); | |
| let queries = ast | |
| .ast | |
| .stmts() | |
| .map(|stmt| { | |
| let query = deparse(stmt)?; | |
| Ok::<_, Error>(query.as_str().to_owned()) | |
| }) | |
| .collect::<Result<Vec<_>, _>>>()?; |
There was a problem hiding this comment.
I think I was building something else here and I forgot to remove it
Co-authored-by: Sage Griffin <sage@sagetheprogrammer.com>
2 participants
Using our parser, tell the query engine to re-execute the requests as a simple query pipeline.
Refactor the query engine and remove the junk from
client.rs. Also move request splicing to the query engine. Also, remove multi-set handling, which is no longer necessary.