Repository: diesel-rs/diesel (language: rust)

Issue:
pack order into BoxableExpression
### Discussed in https://github.com/diesel-rs/diesel/discussions/3411

<div type='discussions-op-text'>

<sup>Originally posted by **mlesin** November 15, 2022</sup>
I'm following documentation of BoxableExpression trait, trying to pack dynamically chosen order into BoxableExpression:

```rust
    let order_field: Box<dyn BoxableExpression<tasks::table, Pg, SqlType = NotSelectable>> =
        match order.field {
            TasksOrderField::Id => {
                if order.ascending {
                    Box::new(tasks::id.asc())
                } else {
                    Box::new(tasks::id.desc())
                }
            }
            TasksOrderField::Label => {
                if order.ascending {
                    Box::new(tasks::label.asc())
                } else {
                    Box::new(tasks::label.desc())
                }
            }
        };
    query = query.order(order_field);
```
But this is fails to compile with the following message:
```
   |
74 |     query = query.order(order_field);
   |                   ^^^^^ the trait `AppearsOnTable<FromClause<tasks::table>>` is not implemented for `dyn BoxableExpression<tasks::table, Pg, SqlType = NotSelectable>`
```

And I have no idea how to satisfy AppearsOnTable<FromClause<..>> trait because FromClause trait is private.
What am I doing wrong?</div>


Discussion / hints:

Fix patch:
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
index 7202323cb4a7..8e4d0ee29761 100644
--- a/.github/workflows/ci.yml
+++ b/.github/workflows/ci.yml
@@ -51,13 +51,6 @@ jobs:
           echo "RUSTFLAGS=--cfg doc_cfg" >> $GITHUB_ENV
           echo "RUSTDOCFLAGS=--cfg doc_cfg" >> $GITHUB_ENV
 
-      - name: Set environment variables
-        shell: bash
-        if: matrix.rust != 'nightly'
-        run: |
-          echo "RUSTFLAGS=-D warnings" >> $GITHUB_ENV
-          echo "RUSTDOCFLAGS=-D warnings" >> $GITHUB_ENV
-
       - name: Install postgres (Linux)
         if: runner.os == 'Linux' && matrix.backend == 'postgres'
         run: |
@@ -130,14 +123,12 @@ jobs:
       - name: Install sqlite (MacOS)
         if: runner.os == 'macOS' && matrix.backend == 'sqlite'
         run: |
-          brew update
           brew install sqlite
           echo "SQLITE_DATABASE_URL=/tmp/test.db" >> $GITHUB_ENV
 
       - name: Install mysql (MacOS)
         if: runner.os == 'macOS' && matrix.backend == 'mysql'
         run: |
-          brew update
           brew install mariadb@10.5
           /usr/local/opt/mariadb@10.5/bin/mysql_install_db
           /usr/local/opt/mariadb@10.5/bin/mysql.server start
@@ -333,11 +324,6 @@ jobs:
         run: |
           sudo apt-get update
           sudo apt-get -y install libsqlite3-dev libpq-dev libmysqlclient-dev
-      - name: Set environment variables
-        shell: bash
-        run: |
-          echo "RUSTFLAGS=-D warnings" >> $GITHUB_ENV
-          echo "RUSTDOCFLAGS=-D warnings" >> $GITHUB_ENV
 
       - name: Remove potential newer clippy.toml from dependencies
         run: |
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 1a0d72448567..870da02e6fac 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -10,6 +10,13 @@ Increasing the minimal supported Rust version will always be coupled at least wi
 
 ## Unreleased 
 
+## [2.0.3] 2023-01-24
+
+## Fixed
+
+* Fixed a bug with our transaction manager implementation that caused by marking transactions as broken which could be recovered.
+* Fixed an issue with the combination of `BoxableExpression` and order clauses
+
 ## [2.0.2] 2022-10-11
 
 ### Fixed
diff --git a/diesel/Cargo.toml b/diesel/Cargo.toml
index a01c25f7df89..66b21b973f87 100644
--- a/diesel/Cargo.toml
+++ b/diesel/Cargo.toml
@@ -1,6 +1,6 @@
 [package]
 name = "diesel"
-version = "2.0.2"
+version = "2.0.3"
 license = "MIT OR Apache-2.0"
 description = "A safe, extensible ORM and Query Builder for PostgreSQL, SQLite, and MySQL"
 readme = "README.md"
diff --git a/diesel/src/connection/transaction_manager.rs b/diesel/src/connection/transaction_manager.rs
index cc3c6883c78c..74a00bb56012 100644
--- a/diesel/src/connection/transaction_manager.rs
+++ b/diesel/src/connection/transaction_manager.rs
@@ -135,13 +135,11 @@ impl TransactionManagerStatus {
         }
     }
 
-    #[cfg(any(
-        feature = "i-implement-a-third-party-backend-and-opt-into-breaking-changes",
-        feature = "postgres",
-    ))]
+    #[cfg(feature = "i-implement-a-third-party-backend-and-opt-into-breaking-changes")]
     #[diesel_derives::__diesel_public_if(
         feature = "i-implement-a-third-party-backend-and-opt-into-breaking-changes"
     )]
+    #[deprecated(note = "Removed without replacement")]
     /// Whether we may be interested in calling
     /// `set_top_level_transaction_requires_rollback_if_not_broken`
     ///
@@ -153,6 +151,17 @@ impl TransactionManagerStatus {
         }
     }
 
+    #[cfg(feature = "i-implement-a-third-party-backend-and-opt-into-breaking-changes")]
+    #[diesel_derives::__diesel_public_if(
+        feature = "i-implement-a-third-party-backend-and-opt-into-breaking-changes"
+    )]
+    #[deprecated(note = "Use `set_requires_rollback_maybe_up_to_top_level` instead")]
+    /// If in transaction and transaction manager is not broken, registers that the
+    /// connection can not be used anymore until top-level transaction is rolled back
+    pub(crate) fn set_top_level_transaction_requires_rollback(&mut self) {
+        self.set_requires_rollback_maybe_up_to_top_level(true);
+    }
+
     #[cfg(any(
         feature = "i-implement-a-third-party-backend-and-opt-into-breaking-changes",
         feature = "postgres",
@@ -162,18 +171,21 @@ impl TransactionManagerStatus {
     #[diesel_derives::__diesel_public_if(
         feature = "i-implement-a-third-party-backend-and-opt-into-breaking-changes"
     )]
-    /// If in transaction and transaction manager is not broken, registers that the
-    /// connection can not be used anymore until top-level transaction is rolled back
-    pub(crate) fn set_top_level_transaction_requires_rollback(&mut self) {
+    /// If in transaction and transaction manager is not broken, registers that it's possible that
+    /// the connection can not be used anymore until top-level transaction is rolled back.
+    ///
+    /// If that is registered, savepoints rollbacks will still be attempted, but failure to do so
+    /// will not result in an error. (Some may succeed, some may not.)
+    pub(crate) fn set_requires_rollback_maybe_up_to_top_level(&mut self, to: bool) {
         if let TransactionManagerStatus::Valid(ValidTransactionManagerStatus {
             in_transaction:
                 Some(InTransactionStatus {
-                    top_level_transaction_requires_rollback,
+                    requires_rollback_maybe_up_to_top_level,
                     ..
                 }),
         }) = self
         {
-            *top_level_transaction_requires_rollback = true;
+            *requires_rollback_maybe_up_to_top_level = to;
         }
     }
 
@@ -213,7 +225,9 @@ pub struct ValidTransactionManagerStatus {
 #[derive(Debug)]
 struct InTransactionStatus {
     transaction_depth: NonZeroU32,
-    top_level_transaction_requires_rollback: bool,
+    /// If that is registered, savepoints rollbacks will still be attempted, but failure to do so
+    /// will not result in an error. (Some may succeed, some may not.)
+    requires_rollback_maybe_up_to_top_level: bool,
     test_transaction: bool,
 }
 
@@ -252,7 +266,7 @@ impl ValidTransactionManagerStatus {
             (None, TransactionDepthChange::IncreaseDepth) => {
                 self.in_transaction = Some(InTransactionStatus {
                     transaction_depth: NonZeroU32::new(1).expect("1 is non-zero"),
-                    top_level_transaction_requires_rollback: false,
+                    requires_rollback_maybe_up_to_top_level: false,
                     test_transaction: false,
                 });
                 Ok(())
@@ -332,40 +346,38 @@ where
     fn rollback_transaction(conn: &mut Conn) -> QueryResult<()> {
         let transaction_state = Self::get_transaction_state(conn)?;
 
-        let rollback_sql = match transaction_state.in_transaction {
-            Some(ref mut in_transaction) => {
+        let (
+            (rollback_sql, rolling_back_top_level),
+            requires_rollback_maybe_up_to_top_level_before_execute,
+        ) = match transaction_state.in_transaction {
+            Some(ref in_transaction) => (
                 match in_transaction.transaction_depth.get() {
-                    1 => Cow::Borrowed("ROLLBACK"),
-                    depth_gt1 => {
-                        if in_transaction.top_level_transaction_requires_rollback {
-                            // There's no point in *actually* rolling back this one
-                            // because we won't be able to do anything until top-level
-                            // is rolled back.
-
-                            // To make it easier on the user (that they don't have to really look
-                            // at actual transaction depth and can just rely on the number of
-                            // times they have called begin/commit/rollback) we don't mark the
-                            // transaction manager as out of the savepoints as soon as we
-                            // realize there is that issue, but instead we still decrement here:
-                            in_transaction.transaction_depth = NonZeroU32::new(depth_gt1 - 1)
-                                .expect("Depth was checked to be > 1");
-                            return Ok(());
-                        } else {
-                            Cow::Owned(format!(
-                                "ROLLBACK TO SAVEPOINT diesel_savepoint_{}",
-                                depth_gt1 - 1
-                            ))
-                        }
-                    }
-                }
-            }
+                    1 => (Cow::Borrowed("ROLLBACK"), true),
+                    depth_gt1 => (
+                        Cow::Owned(format!(
+                            "ROLLBACK TO SAVEPOINT diesel_savepoint_{}",
+                            depth_gt1 - 1
+                        )),
+                        false,
+                    ),
+                },
+                in_transaction.requires_rollback_maybe_up_to_top_level,
+            ),
             None => return Err(Error::NotInTransaction),
         };
 
         match conn.batch_execute(&*rollback_sql) {
             Ok(()) => {
-                Self::get_transaction_state(conn)?
-                    .change_transaction_depth(TransactionDepthChange::DecreaseDepth)?;
+                match Self::get_transaction_state(conn)?
+                    .change_transaction_depth(TransactionDepthChange::DecreaseDepth)
+                {
+                    Ok(()) => {}
+                    Err(Error::NotInTransaction) if rolling_back_top_level => {
+                        // Transaction exit may have already been detected by connection
+                        // implementation. It's fine.
+                    }
+                    Err(e) => return Err(e),
+                }
                 Ok(())
             }
             Err(rollback_error) => {
@@ -375,17 +387,33 @@ where
                         in_transaction:
                             Some(InTransactionStatus {
                                 transaction_depth,
-                                top_level_transaction_requires_rollback,
+                                requires_rollback_maybe_up_to_top_level,
                                 ..
                             }),
-                    }) if transaction_depth.get() > 1
-                        && !*top_level_transaction_requires_rollback =>
-                    {
+                    }) if transaction_depth.get() > 1 => {
                         // A savepoint failed to rollback - we may still attempt to repair
-                        // the connection by rolling back top-level transaction.
+                        // the connection by rolling back higher levels.
+
+                        // To make it easier on the user (that they don't have to really
+                        // look at actual transaction depth and can just rely on the number
+                        // of times they have called begin/commit/rollback) we still
+                        // decrement here:
                         *transaction_depth = NonZeroU32::new(transaction_depth.get() - 1)
                             .expect("Depth was checked to be > 1");
-                        *top_level_transaction_requires_rollback = true;
+                        *requires_rollback_maybe_up_to_top_level = true;
+                        if requires_rollback_maybe_up_to_top_level_before_execute {
+                            // In that case, we tolerate that savepoint releases fail
+                            // -> we should ignore errors
+                            return Ok(());
+                        }
+                    }
+                    TransactionManagerStatus::Valid(ValidTransactionManagerStatus {
+                        in_transaction: None,
+                    }) => {
+                        // we would have returned `NotInTransaction` if that was already the state
+                        // before we made our call
+                        // => Transaction manager status has been fixed by the underlying connection
+                        // so we don't need to set_in_error
                     }
                     _ => tm_status.set_in_error(),
                 }
@@ -402,53 +430,50 @@ where
     fn commit_transaction(conn: &mut Conn) -> QueryResult<()> {
         let transaction_state = Self::get_transaction_state(conn)?;
         let transaction_depth = transaction_state.transaction_depth();
-        let commit_sql = match transaction_depth {
+        let (commit_sql, committing_top_level) = match transaction_depth {
             None => return Err(Error::NotInTransaction),
-            Some(transaction_depth) if transaction_depth.get() == 1 => Cow::Borrowed("COMMIT"),
-            Some(transaction_depth) => Cow::Owned(format!(
-                "RELEASE SAVEPOINT diesel_savepoint_{}",
-                transaction_depth.get() - 1
-            )),
+            Some(transaction_depth) if transaction_depth.get() == 1 => {
+                (Cow::Borrowed("COMMIT"), true)
+            }
+            Some(transaction_depth) => (
+                Cow::Owned(format!(
+                    "RELEASE SAVEPOINT diesel_savepoint_{}",
+                    transaction_depth.get() - 1
+                )),
+                false,
+            ),
         };
         match conn.batch_execute(&*commit_sql) {
             Ok(()) => {
-                Self::get_transaction_state(conn)?
-                    .change_transaction_depth(TransactionDepthChange::DecreaseDepth)?;
+                match Self::get_transaction_state(conn)?
+                    .change_transaction_depth(TransactionDepthChange::DecreaseDepth)
+                {
+                    Ok(()) => {}
+                    Err(Error::NotInTransaction) if committing_top_level => {
+                        // Transaction exit may have already been detected by connection.
+                        // It's fine
+                    }
+                    Err(e) => return Err(e),
+                }
                 Ok(())
             }
             Err(commit_error) => {
                 if let TransactionManagerStatus::Valid(ValidTransactionManagerStatus {
                     in_transaction:
                         Some(InTransactionStatus {
-                            ref mut transaction_depth,
-                            top_level_transaction_requires_rollback: true,
+                            requires_rollback_maybe_up_to_top_level: true,
                             ..
                         }),
                 }) = conn.transaction_state().status
                 {
-                    match transaction_depth.get() {
-                        1 => match Self::rollback_transaction(conn) {
-                            Ok(()) => {}
-                            Err(rollback_error) => {
-                                conn.transaction_state().status.set_in_error();
-                                return Err(Error::RollbackErrorOnCommit {
-                                    rollback_error: Box::new(rollback_error),
-                                    commit_error: Box::new(commit_error),
-                                });
-                            }
-                        },
-                        depth_gt1 => {
-                            // There's no point in *actually* rolling back this one
-                            // because we won't be able to do anything until top-level
-                            // is rolled back.
-
-                            // To make it easier on the user (that they don't have to really look
-                            // at actual transaction depth and can just rely on the number of
-                            // times they have called begin/commit/rollback) we don't mark the
-                            // transaction manager as out of the savepoints as soon as we
-                            // realize there is that issue, but instead we still decrement here:
-                            *transaction_depth = NonZeroU32::new(depth_gt1 - 1)
-                                .expect("Depth was checked to be > 1");
+                    match Self::rollback_transaction(conn) {
+                        Ok(()) => {}
+                        Err(rollback_error) => {
+                            conn.transaction_state().status.set_in_error();
+                            return Err(Error::RollbackErrorOnCommit {
+                                rollback_error: Box::new(rollback_error),
+                                commit_error: Box::new(commit_error),
+                            });
                         }
                     }
                 }
@@ -490,7 +515,7 @@ mod test {
                 if self.top_level_requires_rollback_after_next_batch_execute {
                     self.transaction_state
                         .status
-                        .set_top_level_transaction_requires_rollback();
+                        .set_requires_rollback_maybe_up_to_top_level(true);
                 }
                 res
             }
@@ -1010,4 +1035,181 @@ mod test {
             ).transaction_depth().expect("Transaction depth")
         );
     }
+
+    // regression test for #3470
+    // crates.io depends on this behaviour
+    #[test]
+    #[cfg(feature = "postgres")]
+    fn some_libpq_failures_are_recoverable_by_rolling_back_the_savepoint_only() {
+        use crate::connection::{AnsiTransactionManager, TransactionManager};
+        use crate::prelude::*;
+        use crate::sql_query;
+
+        crate::table! {
+            rollback_test (id) {
+                id -> Int4,
+                value -> Int4,
+            }
+        }
+
+        let conn = &mut crate::test_helpers::pg_connection_no_transaction();
+        assert_eq!(
+            None,
+            <AnsiTransactionManager as TransactionManager<PgConnection>>::transaction_manager_status_mut(
+                conn
+            ).transaction_depth().expect("Transaction depth")
+        );
+
+        let res = conn.transaction(|conn| {
+            sql_query(
+                "CREATE TABLE IF NOT EXISTS rollback_test (id INT PRIMARY KEY, value INT NOT NULL)",
+            )
+            .execute(conn)?;
+            conn.transaction(|conn| {
+                sql_query("SET TRANSACTION READ ONLY").execute(conn)?;
+                crate::update(rollback_test::table)
+                    .set(rollback_test::value.eq(0))
+                    .execute(conn)
+            })
+            .map(|_| {
+                panic!("Should use the `or_else` branch");
+            })
+            .or_else(|_| sql_query("SELECT 1").execute(conn))
+            .map(|_| ())
+        });
+        assert!(res.is_ok());
+
+        assert_eq!(
+            None,
+            <AnsiTransactionManager as TransactionManager<PgConnection>>::transaction_manager_status_mut(
+                conn
+            ).transaction_depth().expect("Transaction depth")
+        );
+    }
+
+    #[test]
+    #[cfg(feature = "postgres")]
+    fn other_libpq_failures_are_not_recoverable_by_rolling_back_the_savepoint_only() {
+        use crate::connection::{AnsiTransactionManager, TransactionManager};
+        use crate::prelude::*;
+        use crate::sql_query;
+        use std::num::NonZeroU32;
+        use std::sync::{Arc, Barrier};
+
+        crate::table! {
+            rollback_test2 (id) {
+                id -> Int4,
+                value -> Int4,
+            }
+        }
+        let conn = &mut crate::test_helpers::pg_connection_no_transaction();
+
+        sql_query(
+            "CREATE TABLE IF NOT EXISTS rollback_test2 (id INT PRIMARY KEY, value INT NOT NULL)",
+        )
+        .execute(conn)
+        .unwrap();
+
+        let start_barrier = Arc::new(Barrier::new(2));
+        let commit_barrier = Arc::new(Barrier::new(2));
+
+        let other_start_barrier = start_barrier.clone();
+        let other_commit_barrier = commit_barrier.clone();
+
+        let t1 = std::thread::spawn(move || {
+            let conn = &mut crate::test_helpers::pg_connection_no_transaction();
+            assert_eq!(
+                None,
+                <AnsiTransactionManager as TransactionManager<PgConnection>>::transaction_manager_status_mut(
+                    conn
+                ).transaction_depth().expect("Transaction depth")
+            );
+            let r = conn.build_transaction().serializable().run::<_, crate::result::Error, _>(|conn| {
+                assert_eq!(
+                    NonZeroU32::new(1),
+                    <AnsiTransactionManager as TransactionManager<PgConnection>>::transaction_manager_status_mut(
+                        conn
+                    ).transaction_depth().expect("Transaction depth")
+                );
+                rollback_test2::table.load::<(i32, i32)>(conn)?;
+                crate::insert_into(rollback_test2::table)
+                    .values((rollback_test2::id.eq(1), rollback_test2::value.eq(42)))
+                    .execute(conn)?;
+                let r = conn.transaction(|conn| {
+                    assert_eq!(
+                        NonZeroU32::new(2),
+                        <AnsiTransactionManager as TransactionManager<PgConnection>>::transaction_manager_status_mut(
+                            conn
+                        ).transaction_depth().expect("Transaction depth")
+                    );
+                    start_barrier.wait();
+                    commit_barrier.wait();
+                    let r = rollback_test2::table.load::<(i32, i32)>(conn);
+                    assert!(r.is_err());
+                    Err::<(), _>(crate::result::Error::RollbackTransaction)
+                });
+                assert_eq!(
+                    NonZeroU32::new(1),
+                    <AnsiTransactionManager as TransactionManager<PgConnection>>::transaction_manager_status_mut(
+                        conn
+                    ).transaction_depth().expect("Transaction depth")
+                );
+                assert!(
+                    matches!(r, Err(crate::result::Error::RollbackTransaction)),
+                    "rollback failed (such errors should be ignored by transaction manager): {}",
+                    r.unwrap_err()
+                );
+                let r = rollback_test2::table.load::<(i32, i32)>(conn);
+                assert!(r.is_err());
+                // fun fact: if hitting "commit" after receiving a serialization failure, PG
+                // returns that the commit has succeeded, but in fact it was actually rolled back.
+                // soo.. one should avoid doing that
+                r
+            });
+            assert!(r.is_err());
+            assert_eq!(
+                None,
+                <AnsiTransactionManager as TransactionManager<PgConnection>>::transaction_manager_status_mut(
+                    conn
+                ).transaction_depth().expect("Transaction depth")
+            );
+        });
+
+        let t2 = std::thread::spawn(move || {
+            other_start_barrier.wait();
+            let conn = &mut crate::test_helpers::pg_connection_no_transaction();
+            assert_eq!(
+                None,
+                <AnsiTransactionManager as TransactionManager<PgConnection>>::transaction_manager_status_mut(
+                    conn
+                ).transaction_depth().expect("Transaction depth")
+            );
+            let r = conn.build_transaction().serializable().run::<_, crate::result::Error, _>(|conn| {
+                assert_eq!(
+                    NonZeroU32::new(1),
+                    <AnsiTransactionManager as TransactionManager<PgConnection>>::transaction_manager_status_mut(
+                        conn
+                    ).transaction_depth().expect("Transaction depth")
+                );
+                let _ = rollback_test2::table.load::<(i32, i32)>(conn)?;
+                crate::insert_into(rollback_test2::table)
+                    .values((rollback_test2::id.eq(23), rollback_test2::value.eq(42)))
+                    .execute(conn)?;
+                Ok(())
+            });
+            other_commit_barrier.wait();
+            assert!(r.is_ok(), "{:?}", r.unwrap_err());
+            assert_eq!(
+                None,
+                <AnsiTransactionManager as TransactionManager<PgConnection>>::transaction_manager_status_mut(
+                    conn
+                ).transaction_depth().expect("Transaction depth")
+            );
+        });
+        crate::sql_query("DELETE FROM rollback_test2")
+            .execute(conn)
+            .unwrap();
+        t1.join().unwrap();
+        t2.join().unwrap();
+    }
 }
diff --git a/diesel/src/mysql/connection/mod.rs b/diesel/src/mysql/connection/mod.rs
index 00cec1d77f78..7b520cca9571 100644
--- a/diesel/src/mysql/connection/mod.rs
+++ b/diesel/src/mysql/connection/mod.rs
@@ -171,7 +171,7 @@ fn update_transaction_manager_status<T>(
     if let Err(Error::DatabaseError(DatabaseErrorKind::SerializationFailure, _)) = query_result {
         transaction_manager
             .status
-            .set_top_level_transaction_requires_rollback()
+            .set_requires_rollback_maybe_up_to_top_level(true)
     }
     query_result
 }
diff --git a/diesel/src/pg/connection/mod.rs b/diesel/src/pg/connection/mod.rs
index fd2130da9aff..d1f7cfc7446d 100644
--- a/diesel/src/pg/connection/mod.rs
+++ b/diesel/src/pg/connection/mod.rs
@@ -239,37 +239,54 @@ fn update_transaction_manager_status<T>(
     query_result: QueryResult<T>,
     conn: &mut ConnectionAndTransactionManager,
 ) -> QueryResult<T> {
-    if let Err(Error::DatabaseError { .. }) = query_result {
-        /// avoid monomorphizing for every result type - this part will not be inlined
-        fn non_generic_inner(conn: &mut ConnectionAndTransactionManager) {
-            let raw_conn: &mut RawConnection = &mut conn.raw_connection;
-            let tm: &mut AnsiTransactionManager = &mut conn.transaction_state;
-            if tm.status.is_not_broken_and_in_transaction() {
-                // libpq keeps track of the transaction status internally, and that is accessible
-                // via `transaction_status`. We can use that to update the AnsiTransactionManager
-                // status
-                match raw_conn.transaction_status() {
-                    PgTransactionStatus::InError => {
-                        tm.status.set_top_level_transaction_requires_rollback()
-                    }
-                    PgTransactionStatus::Unknown => tm.status.set_in_error(),
-                    PgTransactionStatus::Idle => {
-                        // This may repair the transaction manager
-                        tm.status = TransactionManagerStatus::Valid(Default::default())
-                    }
-                    PgTransactionStatus::InTransaction => {
-                        let transaction_status = &mut tm.status;
-                        if !matches!(transaction_status, TransactionManagerStatus::Valid(valid_tm) if valid_tm.transaction_depth().is_some())
-                        {
-                            transaction_status.set_in_error()
-                        }
+    /// avoid monomorphizing for every result type - this part will not be inlined
+    fn non_generic_inner(conn: &mut ConnectionAndTransactionManager, is_err: bool) {
+        let raw_conn: &mut RawConnection = &mut conn.raw_connection;
+        let tm: &mut AnsiTransactionManager = &mut conn.transaction_state;
+        // libpq keeps track of the transaction status internally, and that is accessible
+        // via `transaction_status`. We can use that to update the AnsiTransactionManager
+        // status
+        match raw_conn.transaction_status() {
+            PgTransactionStatus::InError => {
+                tm.status.set_requires_rollback_maybe_up_to_top_level(true)
+            }
+            PgTransactionStatus::Unknown => tm.status.set_in_error(),
+            PgTransactionStatus::Idle => {
+                // This is useful in particular for commit attempts (even
+                // if `COMMIT` errors it still exits transaction)
+
+                // This may repair the transaction manager
+                tm.status = TransactionManagerStatus::Valid(Default::default())
+            }
+            PgTransactionStatus::InTransaction => {
+                let transaction_status = &mut tm.status;
+                // If we weren't an error, it is possible that we were a transaction start
+                // -> we should tolerate any state
+                if is_err {
+                    // An error may not have us enter a transaction, so if we weren't in one
+                    // we may not be in one now
+                    if !matches!(transaction_status, TransactionManagerStatus::Valid(valid_tm) if valid_tm.transaction_depth().is_some())
+                    {
+                        // -> transaction manager is broken
+                        transaction_status.set_in_error()
                     }
-                    PgTransactionStatus::Active => {}
+                } else {
+                    // If transaction was InError before, but now it's not (because we attempted
+                    // a rollback), we may pretend it's fixed because
+                    // if it isn't Postgres *will* tell us again.
+
+                    // Fun fact: if we have not received an `InTransaction` status however,
+                    // postgres will *not* warn us that transaction is broken when attempting to
+                    // commit, so we may think that commit has succeeded but in fact it hasn't.
+                    tm.status.set_requires_rollback_maybe_up_to_top_level(false)
                 }
             }
+            PgTransactionStatus::Active => {
+                // This is a transient state for libpq - nothing we can deduce here.
+            }
         }
-        non_generic_inner(conn)
     }
+    non_generic_inner(conn, query_result.is_err());
     query_result
 }
 
diff --git a/diesel/src/pg/connection/raw.rs b/diesel/src/pg/connection/raw.rs
index a7483cfa9bac..3696230daa4e 100644
--- a/diesel/src/pg/connection/raw.rs
+++ b/diesel/src/pg/connection/raw.rs
@@ -108,6 +108,8 @@ impl RawConnection {
         RawResult::new(ptr, self)
     }
 
+    /// This is reasonably inexpensive as it just accesses variables internal to the connection
+    /// that are kept up to date by the `ReadyForQuery` messages from the PG server
     pub(super) fn transaction_status(&self) -> PgTransactionStatus {
         unsafe { PQtransactionStatus(self.internal_connection.as_ptr()) }.into()
     }
diff --git a/diesel/src/query_builder/select_statement/boxed.rs b/diesel/src/query_builder/select_statement/boxed.rs
index 5dc2835d6385..c6935c9fbe1d 100644
--- a/diesel/src/query_builder/select_statement/boxed.rs
+++ b/diesel/src/query_builder/select_statement/boxed.rs
@@ -389,9 +389,12 @@ where
     }
 }
 
-impl<'a, ST, QS, DB, Order, GB> OrderDsl<Order> for BoxedSelectStatement<'a, ST, QS, DB, GB>
+// no impls for `NoFromClause` here because order is not really supported there yet
+impl<'a, ST, QS, DB, Order, GB> OrderDsl<Order>
+    for BoxedSelectStatement<'a, ST, FromClause<QS>, DB, GB>
 where
     DB: Backend,
+    QS: QuerySource,
     Order: QueryFragment<DB> + AppearsOnTable<QS> + Send + 'a,
 {
     type Output = Self;
@@ -402,9 +405,11 @@ where
     }
 }
 
-impl<'a, ST, QS, DB, Order, GB> ThenOrderDsl<Order> for BoxedSelectStatement<'a, ST, QS, DB, GB>
+impl<'a, ST, QS, DB, Order, GB> ThenOrderDsl<Order>
+    for BoxedSelectStatement<'a, ST, FromClause<QS>, DB, GB>
 where
     DB: Backend + 'a,
+    QS: QuerySource,
     Order: QueryFragment<DB> + AppearsOnTable<QS> + Send + 'a,
 {
     type Output = Self;
diff --git a/diesel/src/query_builder/select_statement/dsl_impls.rs b/diesel/src/query_builder/select_statement/dsl_impls.rs
index 579f46426040..8d475dca7aa7 100644
--- a/diesel/src/query_builder/select_statement/dsl_impls.rs
+++ b/diesel/src/query_builder/select_statement/dsl_impls.rs
@@ -261,15 +261,18 @@ where
     }
 }
 
+// no impls for `NoFromClause` here because order is not really supported there yet
 impl<ST, F, S, D, W, O, LOf, G, H, LC, Expr> OrderDsl<Expr>
-    for SelectStatement<F, S, D, W, O, LOf, G, H, LC>
+    for SelectStatement<FromClause<F>, S, D, W, O, LOf, G, H, LC>
 where
+    F: QuerySource,
     Expr: AppearsOnTable<F>,
     Self: SelectQuery<SqlType = ST>,
-    SelectStatement<F, S, D, W, OrderClause<Expr>, LOf, G, H, LC>: SelectQuery<SqlType = ST>,
+    SelectStatement<FromClause<F>, S, D, W, OrderClause<Expr>, LOf, G, H, LC>:
+        SelectQuery<SqlType = ST>,
     OrderClause<Expr>: ValidOrderingForDistinct<D>,
 {
-    type Output = SelectStatement<F, S, D, W, OrderClause<Expr>, LOf, G, H, LC>;
+    type Output = SelectStatement<FromClause<F>, S, D, W, OrderClause<Expr>, LOf, G, H, LC>;
 
     fn order(self, expr: Expr) -> Self::Output {
         let order = OrderClause(expr);
@@ -288,11 +291,12 @@ where
 }
 
 impl<F, S, D, W, O, LOf, G, H, LC, Expr> ThenOrderDsl<Expr>
-    for SelectStatement<F, S, D, W, OrderClause<O>, LOf, G, H, LC>
+    for SelectStatement<FromClause<F>, S, D, W, OrderClause<O>, LOf, G, H, LC>
 where
+    F: QuerySource,
     Expr: AppearsOnTable<F>,
 {
-    type Output = SelectStatement<F, S, D, W, OrderClause<(O, Expr)>, LOf, G, H, LC>;
+    type Output = SelectStatement<FromClause<F>, S, D, W, OrderClause<(O, Expr)>, LOf, G, H, LC>;
 
     fn then_order_by(self, expr: Expr) -> Self::Output {
         SelectStatement::new(
diff --git a/diesel/src/result.rs b/diesel/src/result.rs
index ce8b1ad00f9b..2a9ff4e0979f 100644
--- a/diesel/src/result.rs
+++ b/diesel/src/result.rs
@@ -73,8 +73,7 @@ pub enum Error {
     RollbackErrorOnCommit {
         /// The error that was encountered when attempting the rollback
         rollback_error: Box<Error>,
-        /// If the rollback attempt resulted from a failed attempt to commit the transaction,
-        /// you will find the related error here.
+        /// The error that was encountered during the failed commit attempt
         commit_error: Box<Error>,
     },
 
diff --git a/diesel_bench/Cargo.toml b/diesel_bench/Cargo.toml
index 971f2b9a9565..795696a79ee5 100644
--- a/diesel_bench/Cargo.toml
+++ b/diesel_bench/Cargo.toml
@@ -15,7 +15,7 @@ sqlx = {version = "0.6", features = ["runtime-tokio-rustls"], optional = true}
 tokio = {version = "1", optional = true}
 rusqlite = {version = "0.27", optional = true}
 rust_postgres = {version = "0.19", optional = true, package = "postgres"}
-rust_mysql = {version = "22.1", optional = true, package = "mysql"}
+rust_mysql = {version = "23.0", optional = true, package = "mysql"}
 rustorm = {version = "0.20", optional = true}
 rustorm_dao = {version = "0.20", optional = true}
 quaint = {version = "=0.2.0-alpha.13", optional = true}


Test patch:
diff --git a/diesel_compile_tests/Cargo.lock b/diesel_compile_tests/Cargo.lock
index 0da3f61c940d..ee009f56d994 100644
--- a/diesel_compile_tests/Cargo.lock
+++ b/diesel_compile_tests/Cargo.lock
@@ -58,7 +58,7 @@ dependencies = [
 
 [[package]]
 name = "diesel"
-version = "2.0.0"
+version = "2.0.2"
 dependencies = [
  "bigdecimal",
  "bitflags",
@@ -92,7 +92,7 @@ dependencies = [
 
 [[package]]
 name = "diesel_derives"
-version = "2.0.0"
+version = "2.0.1"
 dependencies = [
  "proc-macro-error",
  "proc-macro2",
diff --git a/diesel_compile_tests/tests/fail/boxed_queries_require_selectable_expression_for_order.stderr b/diesel_compile_tests/tests/fail/boxed_queries_require_selectable_expression_for_order.stderr
index a6f50f5bdce1..c5d6cd20d9f0 100644
--- a/diesel_compile_tests/tests/fail/boxed_queries_require_selectable_expression_for_order.stderr
+++ b/diesel_compile_tests/tests/fail/boxed_queries_require_selectable_expression_for_order.stderr
@@ -1,11 +1,10 @@
-error[E0277]: the trait bound `FromClause<users::table>: AppearsInFromClause<posts::table>` is not satisfied
+error[E0271]: type mismatch resolving `<users::table as AppearsInFromClause<posts::table>>::Count == diesel::query_source::Once`
   --> tests/fail/boxed_queries_require_selectable_expression_for_order.rs:21:37
    |
 21 |     users::table.into_boxed::<Pg>().order(posts::title.desc());
-   |                                     ^^^^^ the trait `AppearsInFromClause<posts::table>` is not implemented for `FromClause<users::table>`
+   |                                     ^^^^^ expected struct `diesel::query_source::Never`, found struct `diesel::query_source::Once`
    |
-   = help: the trait `AppearsInFromClause<QS1>` is implemented for `FromClause<QS2>`
-note: required because of the requirements on the impl of `AppearsOnTable<FromClause<users::table>>` for `posts::columns::title`
+note: required because of the requirements on the impl of `AppearsOnTable<users::table>` for `posts::columns::title`
   --> tests/fail/boxed_queries_require_selectable_expression_for_order.rs:13:1
    |
 13 | / table! {
@@ -16,6 +15,35 @@ note: required because of the requirements on the impl of `AppearsOnTable<FromCl
 18 | | }
    | |_^
    = note: 1 redundant requirement hidden
-   = note: required because of the requirements on the impl of `AppearsOnTable<FromClause<users::table>>` for `diesel::expression::operators::Desc<posts::columns::title>`
+   = note: required because of the requirements on the impl of `AppearsOnTable<users::table>` for `diesel::expression::operators::Desc<posts::columns::title>`
+   = note: required because of the requirements on the impl of `OrderDsl<diesel::expression::operators::Desc<posts::columns::title>>` for `BoxedSelectStatement<'_, (diesel::sql_types::Integer, diesel::sql_types::Text), FromClause<users::table>, Pg>`
+   = note: this error originates in the macro `$crate::__diesel_column` which comes from the expansion of the macro `table` (in Nightly builds, run with -Z macro-backtrace for more info)
+
+error[E0277]: the trait bound `users::table: TableNotEqual<posts::table>` is not satisfied
+  --> tests/fail/boxed_queries_require_selectable_expression_for_order.rs:21:37
+   |
+21 |     users::table.into_boxed::<Pg>().order(posts::title.desc());
+   |                                     ^^^^^ the trait `TableNotEqual<posts::table>` is not implemented for `users::table`
+   |
+   = help: the following other types implement trait `TableNotEqual<T>`:
+             <Only<pg::metadata_lookup::pg_namespace::table> as TableNotEqual<pg::metadata_lookup::pg_type::table>>
+             <Only<pg::metadata_lookup::pg_type::table> as TableNotEqual<pg::metadata_lookup::pg_namespace::table>>
+             <pg::metadata_lookup::pg_namespace::table as TableNotEqual<Only<pg::metadata_lookup::pg_type::table>>>
+             <pg::metadata_lookup::pg_namespace::table as TableNotEqual<pg::metadata_lookup::pg_type::table>>
+             <pg::metadata_lookup::pg_type::table as TableNotEqual<Only<pg::metadata_lookup::pg_namespace::table>>>
+             <pg::metadata_lookup::pg_type::table as TableNotEqual<pg::metadata_lookup::pg_namespace::table>>
+   = note: required because of the requirements on the impl of `AppearsInFromClause<posts::table>` for `users::table`
+note: required because of the requirements on the impl of `AppearsOnTable<users::table>` for `posts::columns::title`
+  --> tests/fail/boxed_queries_require_selectable_expression_for_order.rs:13:1
+   |
+13 | / table! {
+14 | |     posts {
+15 | |         id -> Integer,
+16 | |         title -> VarChar,
+17 | |     }
+18 | | }
+   | |_^
+   = note: 1 redundant requirement hidden
+   = note: required because of the requirements on the impl of `AppearsOnTable<users::table>` for `diesel::expression::operators::Desc<posts::columns::title>`
    = note: required because of the requirements on the impl of `OrderDsl<diesel::expression::operators::Desc<posts::columns::title>>` for `BoxedSelectStatement<'_, (diesel::sql_types::Integer, diesel::sql_types::Text), FromClause<users::table>, Pg>`
    = note: this error originates in the macro `$crate::__diesel_column` which comes from the expansion of the macro `table` (in Nightly builds, run with -Z macro-backtrace for more info)
diff --git a/diesel_compile_tests/tests/fail/order_requires_column_from_same_table.stderr b/diesel_compile_tests/tests/fail/order_requires_column_from_same_table.stderr
index 0d1cf5376115..ee30be6dea37 100644
--- a/diesel_compile_tests/tests/fail/order_requires_column_from_same_table.stderr
+++ b/diesel_compile_tests/tests/fail/order_requires_column_from_same_table.stderr
@@ -1,11 +1,36 @@
-error[E0277]: the trait bound `FromClause<users::table>: AppearsInFromClause<posts::table>` is not satisfied
+error[E0271]: type mismatch resolving `<users::table as AppearsInFromClause<posts::table>>::Count == diesel::query_source::Once`
   --> tests/fail/order_requires_column_from_same_table.rs:18:31
    |
 18 |     let source = users::table.order(posts::id);
-   |                               ^^^^^ the trait `AppearsInFromClause<posts::table>` is not implemented for `FromClause<users::table>`
+   |                               ^^^^^ expected struct `diesel::query_source::Never`, found struct `diesel::query_source::Once`
    |
-   = help: the trait `AppearsInFromClause<QS1>` is implemented for `FromClause<QS2>`
-note: required because of the requirements on the impl of `AppearsOnTable<FromClause<users::table>>` for `posts::columns::id`
+note: required because of the requirements on the impl of `AppearsOnTable<users::table>` for `posts::columns::id`
+  --> tests/fail/order_requires_column_from_same_table.rs:11:1
+   |
+11 | / table! {
+12 | |     posts {
+13 | |         id -> Integer,
+14 | |     }
+15 | | }
+   | |_^
+   = note: required because of the requirements on the impl of `OrderDsl<posts::columns::id>` for `SelectStatement<FromClause<users::table>>`
+   = note: this error originates in the macro `$crate::__diesel_column` which comes from the expansion of the macro `table` (in Nightly builds, run with -Z macro-backtrace for more info)
+
+error[E0277]: the trait bound `users::table: TableNotEqual<posts::table>` is not satisfied
+  --> tests/fail/order_requires_column_from_same_table.rs:18:31
+   |
+18 |     let source = users::table.order(posts::id);
+   |                               ^^^^^ the trait `TableNotEqual<posts::table>` is not implemented for `users::table`
+   |
+   = help: the following other types implement trait `TableNotEqual<T>`:
+             <Only<pg::metadata_lookup::pg_namespace::table> as TableNotEqual<pg::metadata_lookup::pg_type::table>>
+             <Only<pg::metadata_lookup::pg_type::table> as TableNotEqual<pg::metadata_lookup::pg_namespace::table>>
+             <pg::metadata_lookup::pg_namespace::table as TableNotEqual<Only<pg::metadata_lookup::pg_type::table>>>
+             <pg::metadata_lookup::pg_namespace::table as TableNotEqual<pg::metadata_lookup::pg_type::table>>
+             <pg::metadata_lookup::pg_type::table as TableNotEqual<Only<pg::metadata_lookup::pg_namespace::table>>>
+             <pg::metadata_lookup::pg_type::table as TableNotEqual<pg::metadata_lookup::pg_namespace::table>>
+   = note: required because of the requirements on the impl of `AppearsInFromClause<posts::table>` for `users::table`
+note: required because of the requirements on the impl of `AppearsOnTable<users::table>` for `posts::columns::id`
   --> tests/fail/order_requires_column_from_same_table.rs:11:1
    |
 11 | / table! {
diff --git a/diesel_tests/tests/order.rs b/diesel_tests/tests/order.rs
index a65f257f4059..9addd2641540 100644
--- a/diesel_tests/tests/order.rs
+++ b/diesel_tests/tests/order.rs
@@ -78,3 +78,54 @@ fn order_by_descending_column() {
     let data: Vec<_> = users.order(name.desc()).load(conn).unwrap();
     assert_eq!(expected_data, data);
 }
+
+// regression test for #3412
+#[test]
+fn dynamic_order() {
+    use crate::schema::users;
+    use diesel::expression::expression_types::NotSelectable;
+
+    let conn = &mut connection_with_sean_and_tess_in_users_table();
+    let expected = &["Tess", "Sean"] as &[_];
+
+    let order_field: Box<dyn BoxableExpression<users::table, _, SqlType = NotSelectable>> =
+        Box::new(users::id.desc());
+
+    let result = users::table
+        .select(users::name)
+        .order(order_field)
+        .load::<String>(conn)
+        .unwrap();
+    assert_eq!(expected, &result);
+
+    let order_field: Box<dyn BoxableExpression<users::table, _, SqlType = NotSelectable>> =
+        Box::new(users::id.desc());
+
+    let result = users::table
+        .select(users::name)
+        .then_order_by(order_field)
+        .load::<String>(conn)
+        .unwrap();
+    assert_eq!(expected, &result);
+
+    let order_field: Box<dyn BoxableExpression<users::table, _, SqlType = NotSelectable>> =
+        Box::new(users::id.desc());
+    let result = users::table
+        .select(users::name)
+        .into_boxed()
+        .order(order_field)
+        .load::<String>(conn)
+        .unwrap();
+    assert_eq!(expected, &result);
+
+    let order_field: Box<dyn BoxableExpression<users::table, _, SqlType = NotSelectable>> =
+        Box::new(users::id.desc());
+
+    let result = users::table
+        .select(users::name)
+        .into_boxed()
+        .then_order_by(order_field)
+        .load::<String>(conn)
+        .unwrap();
+    assert_eq!(expected, &result);
+}