search

jeremyevans / sequel

Sequel: The Database Toolkit for Ruby
http://sequel.jeremyevans.net
Other
4.99k stars 1.07k forks source link

Don't use transactions for multi-inserts when not required. #1954

Closed shannoncole closed 2 years ago

shannoncole commented 2 years ago

As per discussion [1] we don't always need to wrap multi-inserts in a transaction. When we use a VALUES clause to bundle the updates into a single-statement, the BEGIN and END are redundant.

[1] https://groups.google.com/g/sequel-talk/c/QrCKmoeF08I/m/W0_SbAQbAQAJ

This change updates the generic-code as well as the code-path for Postgres and MSSQL.

Tests have been updated to match the new behaviour, the core specs pass locally. I was able to run the adapters against mysql & postres: not all the tests passed, but nothing failed in addition to runs against master; let's see what GitHub's CI says...

I haven't added any new integration tests yet as mentioned in the discussion thread. I didn't find an obvious place to add adapter tests which assert on generated SQL statements, but please let me know if I've missed something!

We (stileeducation.com) have been running this in prod for about a day with some success.

jeremyevans commented 2 years ago

Thanks for working on this. I'll try to test and merge this tomorrow.

In terms of adapter tests that assert on generated SQL statements, there are tests in the mock adapter in some cases (mostly for adapter coverage), but deliberately no tests in the actual adapter/integration specs, as those should always test only for behavior, not for specific SQL.

shannoncole commented 2 years ago

Thanks for the speedy review!