[mgaffigan]

In load testing, there are many random failures that can be root-caused to:

• A transaction failed, but was handed back to the pool without rollback - giving some succedent caller a nasty surprise
• Commands and result sets are leaked, which gives slow exhaustion

Both of these are easy to avoid in current best practice of "use hikari" and "use try-with-resources". This PR mostly removes code in implementation of those premises.

Impact:

• The database pool option is dead following this PR - warns on boot if it is set to anything other than hikari.
• Performance is neutral to positive. All numbers are workload sensitive - see Add load testing to CI #525 for methodology. 10 runs each configuration for Postgres & Derby. Other servers were 3 runs.
  • Postgres increased message throughput ~5%
  • Derby decreased ~3%
  • MySQL increased by ~8%
  • SQL Server increased ~2%
  • Oracle increased ~6%

Advice for review:

• The noisy try-with-resources edits were made in a separate commit. Make sure to ignore whitespace on that since so many indentions changed.
• MySQL and Postgres are still quite flaky under load - Several runs are often required to get one without errors.

[github-actions]

Test Results

129 files   - 1  129 suites   - 1   2m 58s ⏱️ -4s
789 tests ±0  789 ✅ ±0  0 💤 ±0  0 ❌ ±0 
915 runs  ±0  909 ✅ ±0  6 💤 ±0  0 ❌ ±0 

Results for commit 5960ad2. ± Comparison against base commit a403039.

This pull request removes 2 and adds 2 tests. Note that renamed tests count towards both.

com.mirth.connect.donkey.server.data.jdbc.OracleJdbcDaoTest ‑ testBatchInsertMessageContentKeepsStatementOpenUntilExecuted
com.mirth.connect.donkey.server.data.jdbc.OracleJdbcDaoTest ‑ testFailedBatchInsertClearsAndClosesTheStatement

com.mirth.connect.donkey.server.data.jdbc.JdbcDaoTest ‑ insertMessageContentBatchBatchesEveryRowThenExecutesOnce
com.mirth.connect.donkey.server.data.jdbc.JdbcDaoTest ‑ insertMessageContentBatchClosesTheStatementWhenBindingFails

♻️ This comment has been updated with latest results.

[NicoPiel]

1. No statement cache anymore: the app-level cache is removed, and nothing turns on the drivers' own caches for Oracle, MySQL or SQL Server. Every query on the message path is prepared again each time. The benchmark gains may come from fixing the leaks and hide this cost.
2. Pool warning fires for hikari too: it warns whenever database.pool is set to anything, including hikari. The PR description says it only warns for values other than hikari.
3. Public API breaks: DonkeyDao, ConnectionPool, the JdbcDao constructor and createConnectionPool all change. Any third-party extension that implements or subclasses them will fail at runtime after upgrading. This needs a release note.
4. A few result sets are left out: five methods still don't put their result set in try-with-resources. It's harmless today, since closing the statement closes it, but it's inconsistent with the rest.
5. Derby cache setting only in the launcher options: derby.language.statementCacheSize is set in base_includes.vmoptions, so Docker images, service wrappers and tests that don't read that file won't get it.
6. Leftover dbcp2 references: gradle/vendored-layout.json still lists commons-dbcp2.
7. Dead code in tests: a line that sets the DAO factory to itself (setDaoFactory(getDaoFactory())) and empty @Before methods were left in four test classes.

[mgaffigan]

1. No statement cache anymore: the app-level cache is removed, and nothing turns on the drivers' own caches for Oracle, MySQL or SQL Server. Every query on the message path is prepared again each time. The benchmark gains may come from fixing the leaks and hide this cost.

I think there's more nuance than that:

• Derby and Postgres cache by default. The derby setting is to adjust the cache size - not to enable it.
• Oracle and SQL Server use server-side plan caching. Client side caching is often negligible.
• MySQL has never had a prepared statements enabled (useServerPrepStmts=false by default historically and currently), so no loss of caching there. A bit more GC pressure, but no change on the wire.

Any changes would require additional performance testing to identify cost/benefit. It's worth perf-testing on all backends - I am often wrong.

2. Pool warning fires for hikari too: it warns whenever database.pool is set to anything, including hikari. The PR description says it only warns for values other than hikari.

Fixed.

3. Public API breaks: DonkeyDao, ConnectionPool, the JdbcDao constructor and createConnectionPool all change. Any third-party extension that implements or subclasses them will fail at runtime after upgrading. This needs a release note.

That's true - but I'd be shocked by third-party implementations. Maybe a release-notes section for plugin authors, but otherwise not worried about it.

4. A few result sets are left out: five methods still don't put their result set in try-with-resources. It's harmless today, since closing the statement closes it, but it's inconsistent with the rest.

Fixed.

5. Derby cache setting only in the launcher options: derby.language.statementCacheSize is set in base_includes.vmoptions, so Docker images, service wrappers and tests that don't read that file won't get it.

I don't follow. As far as I'm aware, this is the vmoptions for all consumers.

6. Leftover dbcp2 references: gradle/vendored-layout.json still lists commons-dbcp2.

Fixed.

7. Dead code in tests: a line that sets the DAO factory to itself (setDaoFactory(getDaoFactory())) and empty @Before methods were left in four test classes.

Fixed.