libsql: remote hrana transactions by Horusiath · Pull Request #653 · tursodatabase/libsql

Horusiath · 2023-11-19T23:58:34Z

This PR introduces transactions in libsql client using http+json. It introduces a Hrana concept of HttpStream, which is opened for sequential execution of subsequent requests.

MarinPostma

Reading the code for transactions I'm starting to wonder if it's a good idea to have connection's execute take &self, and be Sync and Send. If you're sharing a connection between threads, you're likely doing something wrong.

On the surface it looks fine from the remote perspective, because a new sqlite connection is created by the server for each execute request, but locally, the connection is backed by the same sqlite connection, so there is an inadequation in the semantics I think. I don't think connection should be shared, and if you really need to, I think the connection itself should be wrapped into a Mutex to synchronize accesses.

The transaction code made that even more evident to me, since we need a mutex around the HTTP stream to serialize requests, when it's clearly a code smell to have sharable access to a running transaction.

MarinPostma · 2023-11-21T09:26:53Z

+        stream.execute(Stmt::new(begin_stmt, false)).await?;
+        Ok(HttpTransaction { stream })


Maybe the tx open should be batched with the first statement to save a roundtrip?

On one side yes. On the other - this may screw happen before relationships as opening the transaction won't actually start a new transaction ie.:

Have a database in state S1.

Open transaction T1 (current db state: S1).

Open transaction T2.

Within T2, modify database state S1→S2.

Read something in T1: since this is first time when actual request is being made - T1 will point to S2, even though T1 was "opened" before T2.

MarinPostma · 2023-11-21T09:58:25Z

+    ) -> Result<BatchResult> {
+        let mut batch = Batch::new();
+        for stmt in stmts.into_iter() {
+            batch.step(None, stmt);


I believe that there should be a conditional execution on the previous step for batches, and a conditional rollback at the end if the last step wasn't a success

This is how it's realised in libsql-client-rs. I'm also using libsql-client-ts for reference, and have problems with that implementation: namely every batch starts with BEGIN ... statement, yet END/COMMIT/ROLLBACK are not attached to batch.

MarinPostma · 2023-11-21T10:18:56Z

+#[derive(Serialize, Debug)]
+pub struct SequenceStreamReq {
+    pub sql: Option<String>,
+    pub sql_id: Option<i32>,
+}
+
+#[derive(Serialize, Debug)]
+pub struct DescribeStreamReq {
+    pub sql: Option<String>,
+    pub sql_id: Option<i32>,
+}
+
+#[derive(Serialize, Debug)]
+pub struct StoreSqlStreamReq {
+    pub sql: String,
+    pub sql_id: i32,
+}
+
+#[derive(Serialize, Debug)]
+pub struct CloseSqlStreamReq {
+    pub sql_id: i32,
+}


we really need to factorize the proto types with libsql-server

Not important now but agreed, it would be nice to centralize these structs and we should prob do the same for protobuf (both hrana and gRPC).

LucioFranco

Overall, really nice work! I left a few comments and some questions.

LucioFranco · 2023-11-21T15:17:10Z

+where
+    T: for<'a> HttpSend<'a>;


Unless the struct refers to an associated type on the trait then we shouldn't have this bound here, it makes defining structs that use this harder.

I didn't defined it because I wanted it to be fancy. I defined it this way, because it was the only way I've found to make borrow checker cooperate. The use case also seems to fit the criteria of this language feature. If there's a better way I'm open for suggestions.

What I mean is that if your struct foo doesn't use the trait inside of the struct def you don't need to bound T by the trait. You can just leave it as T. This will then flow throughout the rest of the code since you no longer need to bound T: Trait for each struct that embeds the struct that contains T.

Trait bounds should only be set at the impl of where the function is used. The only time you want to bound the struct generic is if you are using an associated type with in the struct.

LucioFranco · 2023-11-21T15:21:57Z

+#[derive(Serialize, Debug)]
+pub struct SequenceStreamReq {
+    pub sql: Option<String>,
+    pub sql_id: Option<i32>,
+}
+
+#[derive(Serialize, Debug)]
+pub struct DescribeStreamReq {
+    pub sql: Option<String>,
+    pub sql_id: Option<i32>,
+}
+
+#[derive(Serialize, Debug)]
+pub struct StoreSqlStreamReq {
+    pub sql: String,
+    pub sql_id: i32,
+}
+
+#[derive(Serialize, Debug)]
+pub struct CloseSqlStreamReq {
+    pub sql_id: i32,
+}


Not important now but agreed, it would be nice to centralize these structs and we should prob do the same for protobuf (both hrana and gRPC).

LucioFranco · 2023-11-27T20:05:32Z

+pub(super) fn stmts_to_batch(protocol_v3: bool, stmts: impl IntoIterator<Item = Stmt>) -> Batch {
+    let mut batch = Batch::new();
+    let mut step = -1;
+    for stmt in stmts.into_iter() {


could use https://doc.rust-lang.org/stable/std/iter/trait.Iterator.html#method.enumerate maybe here to do the counter for you so you don't need to do that logic manually.

Since step always refer to a previous step, I find it easier to read/write than using enumerate.

LucioFranco · 2023-11-27T20:51:50Z

+    Sql(String),
+    SqlId(SqlId),


What about String and Remote? Instead of these two names that seem quite similar?

I named them this way, since they map directly onto Hrana protocol fields.

LucioFranco · 2023-11-27T20:54:08Z

+            TransactionBehavior::Deferred => "BEGIN DEFERRED",
+            TransactionBehavior::Immediate => "BEGIN IMMEDIATE",
+            TransactionBehavior::Exclusive => "BEGIN EXCLUSIVE",
+            TransactionBehavior::ReadOnly => "BEGIN READONLY",


We probably need to map this to DEFERRED since I don't think the server supports this syntax. We should make sure we test this.

I was suspecting that since replication client supports this (this code is copied from there), it should be supported in hrana as well.

No clue, we should test that it actually works I have a feeling it might not but I could also be wrong.

LucioFranco · 2023-11-27T20:57:41Z

+    T: for<'a> HttpSend<'a>,
+{
+    pub async fn query(&self, sql: &str, params: impl IntoParams) -> crate::Result<Rows> {
+        tracing::trace!("querying `{}`", sql);


nit: could prefix the logs here like so to make it easier to debug in the future.

Suggested change

tracing::trace!("querying `{}`", sql);

tracing::trace!("txn querying `{}`", sql);

… execution

Horusiath · 2023-11-28T23:47:04Z

If you don't mind guys, unless you have some strong opinions and must-fix remarks about existing code, we could resolve remaining questions in subsequent PRs. I don't think it's good to have this PR rot. I've created a #723 for topics opened but not covered here.

LucioFranco

I am approving, I added a few more items in the follow up issue.

Horusiath requested review from LucioFranco and psarna November 19, 2023 23:58

Horusiath force-pushed the libsql-transactions branch from 52f7e4b to 4a7a854 Compare November 20, 2023 01:16

Horusiath marked this pull request as ready for review November 20, 2023 21:50

MarinPostma reviewed Nov 21, 2023

View reviewed changes

Comment thread libsql/src/hrana/hyper.rs Outdated

LucioFranco suggested changes Nov 27, 2023

View reviewed changes

Horusiath added 10 commits November 28, 2023 10:05

introduced concepts of streams and transactions to Hrana HTTP

0c9b19a

basic transaction features compile (not working yet)

6a86928

fixed transaction tests

fc29986

skip statement preparation in transaction execute

e87d9dc

introduced distinction between stream- and connection-based statement…

0ae1e45

… execution

add futures to hrana feature flag

cb21046

added hrana transactions to wasm

40dcb5e

moved Conn impl from HttpTransaction to HttpStream

fb31a1a

added conditionals for batch statement execution

c80ce93

modified libsql Conn trait: async is_autocommit + removed close

0a0e012

Horusiath force-pushed the libsql-transactions branch from 7dcfad7 to 0a0e012 Compare November 28, 2023 01:05

Horusiath deployed to external November 28, 2023 01:05 — with GitHub Actions Active

added optional close action to libsql::Transaction

8b1c490

Horusiath requested review from LucioFranco and MarinPostma November 28, 2023 12:13

Horusiath mentioned this pull request Nov 28, 2023

Libsql client transactions: remarks and todos #723

Open

LucioFranco changed the title ~~Libsql transactions~~ libsql: remote hrana transactions Nov 29, 2023

Merge branch 'main' into libsql-transactions

dcd07bf

LucioFranco approved these changes Nov 29, 2023

View reviewed changes

LucioFranco enabled auto-merge November 29, 2023 16:31

LucioFranco added this pull request to the merge queue Nov 29, 2023

Merged via the queue into tursodatabase:main with commit 6b5606a Nov 29, 2023

Horusiath mentioned this pull request Dec 5, 2023

make libsql Conn::is_autocommit synchronous #759

Merged

		stream.execute(Stmt::new(begin_stmt, false)).await?;
		Ok(HttpTransaction { stream })

	tracing::trace!("querying `{}`", sql);
	tracing::trace!("txn querying `{}`", sql);

Conversation

Horusiath commented Nov 19, 2023

Uh oh!

MarinPostma left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Horusiath Nov 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

LucioFranco left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Horusiath Nov 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Horusiath commented Nov 28, 2023

Uh oh!

LucioFranco left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Horusiath Nov 21, 2023 •

edited

Loading

Horusiath Nov 28, 2023 •

edited

Loading