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

Expose transaction_depth through get_transaction_depth() method #3427

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

mpyw
Copy link

@mpyw mpyw commented Aug 11, 2024

Issue

Description

This PR introduces the get_transaction_depth() method for exposing the current transaction depth in various database connections.

  • MySqlConnection and PgConnection now implement the TransactionDepth::get_transaction_depth() method to synchronously retrieve the transaction depth.
  • SqliteConnection implements the AsyncTransactionDepth::get_transaction_depth() method to retrieve the transaction depth asynchronously by acquiring a lock (as the existing implementation requires a lock to access the transaction level).
  • AnyConnection uses the AsyncTransactionDepth::get_transaction_depth() method to handle SqliteConnection, and in the dyn AnyConnectionBackend implementation, it dispatches to the respective connection's implementation.

Concerns

  • Since the method get_transaction_depth() is used for both traits, there is a potential for import collisions between the two traits. However, it is unlikely that both traits would be imported simultaneously, so this should not pose a significant issue.
  • Regarding the SqliteConnection implementation, I'm unsure if this approach is optimal. Specifically, is it acceptable to acquire a lock just to retrieve the transaction nesting level? Additionally, my understanding is that the lock is automatically released when it goes out of scope—is this correct?

Additional Context

I am relatively new to using Rust professionally, having only recently started working with it. If there are any issues with my approach or if I am violating any best practices, please feel free to point them out.

@mpyw mpyw force-pushed the feat/get-transaction-depth branch 2 times, most recently from e924285 to a1e92a6 Compare August 11, 2024 16:07
Copy link
Collaborator

@abonander abonander left a comment

Choose a reason for hiding this comment

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

I'm not a fan of the trait split. The SQLite driver can be refactored a bit to track the transaction depth in the connection struct itself (or the ConnectionWorker struct), we've just got to be more careful of keeping the value in-sync.


Instead of a bespoke trait, since you're only implementing it directly for the connection types anyway, the method can just go on the Connection trait.

It can be a provided method that forwards to a method on TransactionManager. It's not a breaking change to add a required method to the latter because it's #[doc(hidden)] and thus exempt from our semver guarantees.


Since most users would likely just do conn.get_transaction_depth() != 0 to check if the connection is in a transaction, I would also add a provided method like

#[inline]
fn is_in_transaction(&self) -> bool {
    self.get_transaction_depth() != 0
}

No emoji in the commit messages, please. It's not necessary, and who knows what it might break.

@mpyw mpyw force-pushed the feat/get-transaction-depth branch from a1e92a6 to 4f9d3f9 Compare August 12, 2024 04:11
@mpyw

This comment was marked as outdated.

@mpyw
Copy link
Author

mpyw commented Aug 12, 2024

I now understand what you mean by provided method. It means "default implementation" in other languages.

@mpyw mpyw force-pushed the feat/get-transaction-depth branch 9 times, most recently from 6ab39ce to 48dbba7 Compare August 12, 2024 06:16
SQLite implementation is currently WIP
@mpyw mpyw force-pushed the feat/get-transaction-depth branch from 48dbba7 to 8f45019 Compare August 12, 2024 06:17
@mpyw

This comment was marked as outdated.

@mpyw mpyw requested a review from abonander August 12, 2024 06:31
@mpyw mpyw force-pushed the feat/get-transaction-depth branch 2 times, most recently from 3cdccde to 3453955 Compare August 12, 2024 12:34
@mpyw mpyw force-pushed the feat/get-transaction-depth branch from 3453955 to 10d0aea Compare August 12, 2024 13:37
I have included `AtomicUsize` in `WorkerSharedState`. Ideally, it is not desirable to execute `load` and `fetch_add` in two separate steps, but we decided to allow it here since there is only one thread writing. To prevent writing from other threads, the field itself was made private, and a getter method was provided with `pub(crate)`.
@mpyw
Copy link
Author

mpyw commented Aug 12, 2024

@abonander Strictly speaking, it seems more correct to use RwLock rather than AtomicUsize and to keep it locked during operations. However, since RwLock requires handling errors, I chose this approach for now. If you prefer an implementation using RwLock, please let me know your thoughts.


[Edit]

I am considering adopting RwLock from parking_lot crate.

@mpyw mpyw force-pushed the feat/get-transaction-depth branch from 7356c48 to 54c73b6 Compare August 12, 2024 16:29
@mpyw
Copy link
Author

mpyw commented Aug 13, 2024

@abonander If we were to replace the implementation with parking_lot::RwLock, it would look like this:

diff --git a/sqlx-sqlite/src/connection/worker.rs b/sqlx-sqlite/src/connection/worker.rs
index 6dda814a..d36b1df2 100644
--- a/sqlx-sqlite/src/connection/worker.rs
+++ b/sqlx-sqlite/src/connection/worker.rs
@@ -34,14 +34,14 @@ pub(crate) struct ConnectionWorker {
 }
 
 pub(crate) struct WorkerSharedState {
-    transaction_depth: AtomicUsize,
+    transaction_depth: parking_lot::RwLock<usize>,
     cached_statements_size: AtomicUsize,
     pub(crate) conn: Mutex<ConnectionState>,
 }
 
 impl WorkerSharedState {
     pub(crate) fn get_transaction_depth(&self) -> usize {
-        self.transaction_depth.load(Ordering::Acquire)
+        *self.transaction_depth.read()
     }
 
     pub(crate) fn get_cached_statements_size(&self) -> usize {
@@ -104,7 +104,7 @@ impl ConnectionWorker {
                 };
 
                 let shared = Arc::new(WorkerSharedState {
-                    transaction_depth: AtomicUsize::new(0),
+                    transaction_depth: parking_lot::RwLock::new(0),
                     cached_statements_size: AtomicUsize::new(0),
                     // note: must be fair because in `Command::UnlockDb` we unlock the mutex
                     // and then immediately try to relock it; an unfair mutex would immediately
@@ -193,12 +193,12 @@ impl ConnectionWorker {
                             update_cached_statements_size(&conn, &shared.cached_statements_size);
                         }
                         Command::Begin { tx } => {
-                            let depth = shared.transaction_depth.load(Ordering::Acquire);
+                            let mut depth = shared.transaction_depth.write();
                             let res =
                                 conn.handle
-                                    .exec(begin_ansi_transaction_sql(depth))
+                                    .exec(begin_ansi_transaction_sql(*depth))
                                     .map(|_| {
-                                        shared.transaction_depth.fetch_add(1, Ordering::Release);
+                                        *depth += 1;
                                     });
                             let res_ok = res.is_ok();
 
@@ -209,9 +209,9 @@ impl ConnectionWorker {
                                 // immediately otherwise it would remain started forever.
                                 if let Err(error) = conn
                                     .handle
-                                    .exec(rollback_ansi_transaction_sql(depth + 1))
+                                    .exec(rollback_ansi_transaction_sql(*depth + 1))
                                     .map(|_| {
-                                        shared.transaction_depth.fetch_sub(1, Ordering::Release);
+                                        *depth -= 1;
                                     })
                                 {
                                     // The rollback failed. To prevent leaving the connection
@@ -223,13 +223,13 @@ impl ConnectionWorker {
                             }
                         }
                         Command::Commit { tx } => {
-                            let depth = shared.transaction_depth.load(Ordering::Acquire);
+                            let mut depth = shared.transaction_depth.write();
 
-                            let res = if depth > 0 {
+                            let res = if *depth > 0 {
                                 conn.handle
-                                    .exec(commit_ansi_transaction_sql(depth))
+                                    .exec(commit_ansi_transaction_sql(*depth))
                                     .map(|_| {
-                                        shared.transaction_depth.fetch_sub(1, Ordering::Release);
+                                        *depth -= 1;
                                     })
                             } else {
                                 Ok(())
@@ -249,13 +249,13 @@ impl ConnectionWorker {
                                 continue;
                             }
 
-                            let depth = shared.transaction_depth.load(Ordering::Acquire);
+                            let mut depth = shared.transaction_depth.write();
 
-                            let res = if depth > 0 {
+                            let res = if *depth > 0 {
                                 conn.handle
-                                    .exec(rollback_ansi_transaction_sql(depth))
+                                    .exec(rollback_ansi_transaction_sql(*depth))
                                     .map(|_| {
-                                        shared.transaction_depth.fetch_sub(1, Ordering::Release);
+                                        *depth -= 1;
                                     })
                             } else {
                                 Ok(())

This approach would ensure that operations on transaction_depth are handled atomically as a single unit, avoiding potential race conditions that could occur with separate AtomicUsize operations. If you believe this implementation with parking_lot::RwLock is preferable to the current AtomicUsize approach, we can consider adopting it. What are your thoughts?

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.

Feature Request: Expose transaction_depth with a Read-Only Getter Method
2 participants