AugustNagro / magnum

A 'new look' for database access in Scala
Apache License 2.0
153 stars 10 forks source link

findAll returns zero results from H2 #17

Closed objektwerks closed 11 months ago

objektwerks commented 1 year ago

Magnum findAll returns zero results from H2 - when it should return a result of 1.

The following project - https://github.com/objektwerks/magnum - can be used via - sbt clean test - to reproduce this issue. Read the test console output for details. The project code will provide additional details.

AugustNagro commented 1 year ago

We have good coverage of H2 including findAll: https://github.com/AugustNagro/magnum/blob/master/magnum/src/test/scala/H2Tests.scala

I think the problem in your JMH test is that you need a

@Setup
def setup(): Unit = ???

Method that populates the database rows. The rows inserted from

  @Benchmark
  def addTodo(): Todo =

Will not be visible from the other benchmarks; the in-memory H2 instance is fresh.

Please share the results when done, I'd like to see them. I expect Magnum to perform well, given its low abstraction & code size.

objektwerks commented 1 year ago

In addition to the Github documentation, I referenced H2Tests extensively to build my project. So I was quite surprised to see findAll fail in my simple StoreTest. :)

Did you run - sbt clean test and read the test console output?

The issue is with findAll failing to return a Vector of length == 1. ...

At this time, my issue is not with sbt:jmh and Performance.

I have a Quill project ( https://github.com/objektwerks/quill ) that mimics my Magnum project; and for both projects - running sbt:jmh fails with this issue: https://github.com/sbt/sbt-jmh/issues/192 But I will look into your setup idea.

As I recall, sbt:jmh used to work with Java 8. :) It works just fine in my non-jdbc projects.

The Quill project StoreTest, though, passes; where the Magnum StoreTest fails.

AugustNagro commented 1 year ago

The problem in StoreTest is that the runscript is dropping table Todo every time a new connection is made.

You can change url to url = "jdbc:h2:mem:test;DB_CLOSE_DELAY=-1", and then initialize the db once:

  Using.Manager(use =>
    val con = use(ds.getConnection)
    val stmt = use(con.createStatement)
    val sql = Files.readString(Path.of("ddl.sql"))
    println("executing \n" + sql)
    stmt.execute(sql)
  ).get

Quill maintains the single connection, whereas Magnum creates a new connection every connect/transact call.

Note that you should be calling transact at a higher scope and then share the connection.

So for example, call transact in StoreTest, make each Store method require the implicit:

  def count(): Long =
    transact(ds, withRepeatableRead):
      repo.count

Becomes

  def count(using DbTx): Long = repo.count

This will then match Quill's behavior. You should also replace transact with connect, because I don't think Quill is running with transactions.

objektwerks commented 1 year ago

Brilliant! I refactored accordingly, and everything works as expected. Thanks! And apologies for failing to understand the finer points of Magnum. :)

Currently, I use transact for writes and connect for reads. ...

After my initial euphoria wore off a bit, I noticed having Magnum code in Performance and StoreTest ( user code ) was not ideal. So I added Delegate ( a level of indirection ) to Store, allowing me to remove all Magnum code from my user code.

See: https://github.com/objektwerks/magnum/blob/main/src/main/scala/objektwerks/Store.scala for details.

I think the code is cleaner. But, perhaps, there's an even better way? :)

objektwerks commented 11 months ago

Closed. ...

With the recent release of JDK 21 and sbt-jmh 46, Magnum and Quill jmh benchmarks still fail.

Only my ScalikeJdbc jmh benchmark runs.

Question: What is ScalikeJdbc ** doing that Magnum and Quill aren't doing?

** https://github.com/objektwerks/scalikejdbc ...

Apologies for all the edits. Just trying to get it right for reader(s). :)

AugustNagro commented 11 months ago

Glad it's working now.

Currently, I use transact for writes and connect for reads.

Why? If you have this running in parallel:

myWrite()
myRead
myWrite2()

You will easily have dirty reads: https://www.postgresql.org/docs/current/transaction-iso.html

To use transactions safely you would use DbTx and DbCon: https://github.com/AugustNagro/magnum#type-safe-transaction--connection-management

After my initial euphoria wore off a bit, I noticed having Magnum code in Performance and StoreTest ( user code ) was not ideal. So I added Delegate ( a level of indirection ) to Store, allowing me to remove all Magnum code from my user code.

There's no reason to use transactions for methods that only execute one sql statement. The whole point of transactions is that they roll-back if your code throws an exception. But that won't be possible using your Store class.

Question: What is ScalikeJdbc ** doing that Magnum and Quill aren't doing?

Did you try the fix suggested in https://github.com/AugustNagro/magnum/issues/17#issuecomment-1711139093 ?

objektwerks commented 11 months ago

In Store and Delegate, I use transact/DbTx and connect/DbCon, for add/update and list, respectively.

Note, ScalikeJdbc has DB localTx and DB readOnly, while Quill has transaction and query. Other Jdbc libraries function similarly.

On an insert or update, I want a transaction/rollback capability for 1 or more sql statements. On a database select, I don't want a transaction - just a read only capability. I assumed I had those capabilities with transact/DbTx and connect/DbCon, respectively.

With all other Scala Jdbc libraries, I can readily converge Store and Delegate. Magnum, as you explain above, requires me to separate transact/connect from DbTx/DbCon in an effort to keep Magnum code out of my user code.

I don't know how else to explain it. :) Naturally, remote communications has it challenges. ;) Thanks for your help! ...

I tried your setup suggestion with Performance; but I still get a java.lang.ClassNotFoundException: javax/sql/DataSource error.

I suspect a class loader or module issue. Not sure, though. And adding .jvmopts with "--add-modules java.sql" fails to resolve the issue.

objektwerks commented 5 months ago

Please see below @AugustNagro Cheers! :)

Using magnum 1.1.1, jmh 0.4.7 and JDK 22, my benchmark runs finally work as of 2024.3.21 - 25!

Magnum:

OpenJDK Runtime Environment Zulu22.28+91-CA (build 22+36), Scala 3.4.1-RC2, Apple M1
1. addTodo - 4.455
2. updateTodo - 3.648
3. listTodos - 1.901
Total time: 604 s (10:04), 10 warmups, 10 iterations, average time in microseconds, completed 2024.3.21

Jdbc:

OpenJDK Runtime Environment Zulu22.28+91-CA (build 22+36), Scala 3.4.1-RC2, Apple M1
addTodo - 2.879
updateTodo - 2.183
listTodos - 1.733
Total time: 602 s (10:02), 10 warmups, 10 iterations, average time in microseconds, completed 2024.3.25

Quill:

OpenJDK Runtime Environment Zulu22.28+91-CA (build 22+36), Scala 3.4.1-RC2, Apple M1
addTodo - 17.064
updateTodo - 4.594
listTodos - 2.803
Total time: 602 s (10:02), 10 warmups, 10 iterations, average time in microseconds, completed 2024.3.22

ScalikeJdbc:

OpenJDK Runtime Environment Zulu22.28+91-CA (build 22+36), Scala 3.4.1-RC2, Apple M1
addTodo - 32.528
updateTodo - 30.165
listTodos - 25.936
Total time: 602 s (10:02), 10 warmups, 10 iterations, average time in microseconds, completed 2024.3.22

Slick benchmark results will be included once I adapt my Slick work to version 3.5.0, which looks to be quite painful. :)