bugfix: statement prepared in TX is closed with TX

[letFunny]

If a sqlair.Statement is first used inside a transaction, it is prepared in the transaction's dedicated connection. When the transaction is closed, the connection is closed as well, meaning the statement is no longer valid.

Why do we prepare the statement in a new dedicated connection in the first place? Because if the database pool has only one connection and we create a transaction statement in that connection, we create a deadlock. This is because a transaction needs a dedicated connection so, if the pool is configured to have a maximum of one, it will wait indefinitely. Additionally, SQLite will only have one connection if opened with :memory: which is a common source of trouble (ref).

There are several solutions:

1. The first is adding a timeout to the context when preparing the sql.Statement. This will avoid the deadlock and will present the user with the error. However, we have to set an arbitrary timeout which can result in false positives depending on the user's network latency.
2. The second approach is to not care about the deadlock. The sql package documentation clearly states that setting a max number of connections is akin to acquiring locks [1]. If the user sets the number, we can expect him to also use a context with a timeout to prevent deadlocks.
3. The third approach is to bypass the cache preparing and closing the statement directly.
4. The last one is to add the TX as another key to the cache. Where right now we have db and statement, we would have db, TX and statement.

I am more inclined to go for 2. as it seems this is a problem with how the database is configured, or in the case of SQLite a problem with the defaults (it only has one connection); and not something we should worry about.

[1]:

Keep in mind that setting a limit makes database usage similar to acquiring a lock or semaphore, with the result that your application can deadlock waiting for a new database connection.

[letFunny]

As discussed in a meeting today the caching system has some potential issues which require a lot of discussion and testing. Right now, we are not able to commit the time investment, we will focus on delivering the roadmap features (slices, insert, primitive types, etc.) first.

The decision has been to remove the cache from the library and revisit it in the future. We will need to create a through design that addresses all of the issues and that is performant. For the latter, we will need to create a test harness and benchmark the different implementations to confirm our assumptions.

For reference, here is a high level list of the issues:

• Finalizers run serially on a single goroutine which means that code that creates a lot of statements potentially will have some contention in the GC.
• Because of the cache design, when the max number of connections is 1 and a transaction is created, we actually need 2 connections which is not intuitive. Incidentally, this is also because of the sql standard library design which can block when requesting new connections and the only way around it is to use a context with a timeout.
• It is not clear whether preparing in SQLite is an improvement in the performance (SQLite does not do a lot of processing when preparing a statement). On top of that, Juju's workload is mainly creating transactions and reusing statements across them. Because of how preparing on a connection works, we are not getting a lot of benefits. We would need to pool the connections ourselves or be smarter about it.