Question about the `transact` mechanism

[jivanic-demystdata]

Hi @AugustNagro,

Thanks for this great lib 🙂

I was reading the code of the transact function and was surprised you don't restore setAutoCommit value of the connection at the end of the transaction:

Current code is:

def transact[T](dataSource: DataSource)(f: DbTx ?=> T): T =
  Using.resource(dataSource.getConnection)(con =>
    con.setAutoCommit(false)
    try
      val res = f(using DbTx(con))
      con.commit()
      res
    catch
      case t =>
        con.rollback()
        throw t
  )

Restoring the value would give something like:

def transact[T](dataSource: DataSource)(f: DbTx ?=> T): T =
  Using.resource(dataSource.getConnection)(con =>
    val previous = con.getAutoCommit
    con.setAutoCommit(false)
    try
      val res = f(using DbTx(con))
      con.commit()
      res
    catch
      case t =>
        con.rollback()
        throw t
   finally
     con.setAutoCommit(previous)
  )

That's what Quill seems to be doing, for example. See https://github.com/zio/zio-protoquill/blob/806867078dfb0e99ed9a5a43663a55e65b9fbad6/quill-jdbc/src/main/scala/io/getquill/context/jdbc/JdbcContext.scala#L93-L104

Why don't you need to restore the previous value? 🤔

[Nexus6]

I would think that restoring the setAutoCommit previous value would be superfluous when the resource manager code (Using.resource) will always be releasing the connection at the end of the code block that it encloses. This will close the db connection or, if you're using a pooling datasource, return it to the pool. I don't know, but I would expect, that most/all connection pooling packages (e.g. HikariCP) are always going to reset connections to a default state when they're added or released to their pool, or perhaps just before they're handed-out to a client.

The above would apply to that protoquill code too - I don't know why they're bothering to save+restore the autocommit state on connections that they're allocating in transact(), unless the authors don't trust a pooling datasource to properly manage the state of pooled connections.

[jivanic-demystdata]

Would be interesting to know what HikariCP is doing indeed because if it doesn't reset the connection settings, this might be an issue for Magnum users

[Nexus6]

There are quite a few Connection settings that could be tweaked in addition to AutoCommit. HikariCP specifically tracks whether or not some of those settings get changed by its clients, and resets them to default (configurable) values when the client returns the Connections to the pool via close(). You can see some of that code here. AutoCommit is included in the list of tracked parameters.

One would think that the default settings for pooled connection objects would be defined by the JDBC spec but if so I haven't found it. "By default, new connections are in auto-commit mode." is all I've found. I can only speculate that the Protoquill authors might have had some adverse experiences w/various connection pooling packages and decided to play it safe by ensuring that at minimum, transact() wouldn't have any side-effects on the connection's AutoCommit state.

[jivanic-demystdata]

Shouldn't Magnum play it safe too? It's not like it's a complicated or expensive thing to do 🤔

[AugustNagro]

Thanks for sharing Jules.

I'm happy to accept this change if we can figure out why Quill introduced it (specifically which connection pool).

On Mon, Aug 19, 2024, 10:48 PM Jules Ivanic @.***> wrote:

Shouldn't Magnum play it safe too? It's not like it's a complicated or expensive thing to do 🤔

— Reply to this email directly, view it on GitHub https://github.com/AugustNagro/magnum/issues/40#issuecomment-2298016627, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABQOKNRP2FHT7Z6PNSUMUKDZSLKBVAVCNFSM6AAAAABMWZ5XVWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEOJYGAYTMNRSG4 . You are receiving this because you were mentioned.Message ID: @.***>

[jivanic-demystdata]

I didn't know it was safe to not re-initialize the Connection when using a connection pool. I guess we can leave it like this then. I'm not sure we'll be able to know why Quill did it this way.