seancorfield / next-jdbc

A modern low-level Clojure wrapper for JDBC-based access to databases.
https://cljdoc.org/d/com.github.seancorfield/next.jdbc/
Eclipse Public License 1.0
768 stars 90 forks source link

Nested transactions detection assumes there is only one DB #175

Closed mrkam2 closed 3 years ago

mrkam2 commented 3 years ago

Describe the bug Nested transactions detection checks for *active-tx* variable that is not specific to the DB or connection.

To Reproduce

  1. Set *nested-tx* to prohibit.
  2. Open two connections to different databases.
  3. Create a transaction with the first database.
  4. Inside the transaction, create a transaction with the second database. The operation above will fail although these transactions are not nested.

Expected behavior "Nested" transactions over different DBs should be allowed.

seancorfield commented 3 years ago

This is why the default behavior is to assume the developer knows what they are doing. If you are using multiple databases, within a single TX, you can't use this feature to trap unintentional nesting. I'll update the documentation to clarify that.

mrkam2 commented 3 years ago

This is why the default behavior is to assume the developer knows what they are doing. If you are using multiple databases, within a single TX, you can't use this feature to trap unintentional nesting. I'll update the documentation to clarify that.

Is old-autocommit = false not a good indicator of already being inside a transaction?

seancorfield commented 3 years ago

@mrkam2 Nope. There are other reasons for disabling auto-commit (some DBs require auto-commit is disabled for certain DDL operations, for example). JDBC is pretty nasty in this respect because a "transaction" is really just a convention for calling .commit or .rollback -- or using savepoints. The *nested-tx* option was only added about a year ago and I originally didn't plan to document it, except as a potential aid to migration. The default behavior of trusting the developer has been the same since next.jdbc was released, over a year prior to that.