Concurrent sandbox support with begin concurrent #99

Activity

1. warmwaffles commented on Jan 20, 2023

   

   warmwaffles

   on Jan 20, 2023

   Member

   More actions

   I wonder what the drawback is for using BEGIN CONCURRENT all the time is.
2. josevalim commented on Jan 20, 2023

   

   josevalim

   on Jan 20, 2023

   Author

   More actions

   A concurrent transaction can fail to commit as a whole, so I think outside of concurrent tests, it should certainly be opt-in behavior.
3. ruslandoga commented on Jan 24, 2023

   

   ruslandoga

   on Jan 24, 2023

   Last edited by ruslandoga

   Contributor

   More actions

   I think BEGIN CONCURRENT requires a special build from begin-concurrent branch

   This branch contains the "BEGIN CONCURRENT" patch. Trunk changes are periodically merged directly into this branch.

   In the stock version it fails during parsing:

   sqlite> BEGIN CONCURRENT;
   Parse error: near "CONCURRENT": syntax error
     BEGIN CONCURRENT;
           ^--- error here
4. warmwaffles commented on Jan 24, 2023

   

   warmwaffles

   on Jan 24, 2023

   Member

   More actions

   It's a shame that BEGIN CONCURRENT isn't implied because the WAL mode is enabled signalling that it would be a potentially concurrent database.
5. ruslandoga commented on Jan 24, 2023

   

   ruslandoga

   on Jan 24, 2023

   Last edited by ruslandoga

   Contributor

   More actions

   However, it seems like that custom build + sandbox work :)

   1. begin-concurrent amalgamation + mode: :concurrent in handle_begin and co.
   2. Ecto.Adapters.SQL.Sandbox copy-past that uses mode: :concurrent
   3. demo project using SQLite3 and async: true
6. josevalim commented on Jan 24, 2023

   

   josevalim

   on Jan 24, 2023

   Author

   More actions

   Given this project ships with sqlite3, maybe we can ship with the begin concurrent version?
7. ruslandoga commented on Jan 24, 2023

   

   ruslandoga

   on Jan 24, 2023

   Last edited by ruslandoga

   Contributor

   More actions

   Personally, I'd prefer running the official releases. Also begin-concurrent isn't merged with all released versions, e.g. it doesn't have a commit corresponding to 3.40.1.

   Maybe we'd get BEGIN CONCURRENT if or once HC-tree gets more popular...

   Is there maybe another way to enable concurrent tests?

   Like starting and migrating pool_size in-memory databases with after_connect

   Sync:

   ..............................................................................................................................................
   Finished in 0.8 seconds (0.3s async, 0.5s sync)
   142 tests, 0 failures
   

   Async:

   ..............................................................................................................................................
   Finished in 0.6 seconds (0.6s async, 0.00s sync)
   142 tests, 0 failures
   
8. warmwaffles commented on Jan 24, 2023

   

   warmwaffles

   on Jan 24, 2023

   Member

   More actions

   Personally, I'd prefer running the official releases.

   This is generally my feelings as well. I haven't followed this feature too closely and don't know if it is going to be mainlined.

   Given this project ships with sqlite3, maybe we can ship with the begin concurrent version?

   I believe we'd need to change a few things in order to take that amalgamation work. It would need to be automated here and the following would cause tests to break:

   • https://github.com/elixir-sqlite/exqlite/blob/bd7d4ca8e085c0fa2cb81a4a2cdb62cec6a2b8ca/mix.exs#L5
   • https://github.com/elixir-sqlite/exqlite/blob/bd7d4ca8e085c0fa2cb81a4a2cdb62cec6a2b8ca/mix.exs#L39
   • https://github.com/elixir-sqlite/exqlite/blob/bd7d4ca8e085c0fa2cb81a4a2cdb62cec6a2b8ca/lib/mix/tasks/test_sqlite_version.ex
   • https://github.com/elixir-sqlite/exqlite/blob/bd7d4ca8e085c0fa2cb81a4a2cdb62cec6a2b8ca/.github/workflows/ci.yml#L38
9. kevinlang commented on Feb 16, 2023

   

   kevinlang

   on Feb 16, 2023

   Last edited by kevinlang

   Member

   More actions

   If the begin-concurrent stuff was in trunk, I think that would definitely be the way to go, as it aligns more with how other Ecto adapters interface with the Sandbox. Unfortunately, it is not, yet - and I don't see any immediate plans to merge it :(

   I like the idea of testing against an in-memory database, and I think I explored that approach at one point. However, it has some friction with how the existing Sandbox adapter works and ends up feeling hacky.

   To expand, the current adapter has normal connections, but proxies those connections and does the necessary transaction wrapping statements on checkout/checkin. This allows e.g., Ecto migrations to run as expected, as this proxying is only enabled once the test_helper.exs enables manual mode.

   If we follow a similar mechanism, what we want need to do in our custom Sandbox adapter is to, on checkout when manual mode is enabled, "convert" those connections into in-memory ones (via the serialization/deserialization interface), and on checkin "convert" them back into normal connections (where we get rid of the in-memory connection and re-connect to the real database), which feels a bit hacky. Hacky in that this approach modifies the actual connection itself and isn't a proxy in the true sense.
10. ruslandoga commented on Feb 16, 2023

    

    ruslandoga

    on Feb 16, 2023

    Last edited by ruslandoga

    Contributor

    More actions

    Why not use the in-memory databases from the start? It seems like I didn't encounter any problems with the sandbox using that approach.
11. warmwaffles commented on Feb 16, 2023

    

    warmwaffles

    on Feb 16, 2023

    Member

    More actions

    I would love to use memory for the database, but with how the connections are pooled, that is impossible to do. Once the connection goes idle, it closes the database and that in memory database is lost.

    It's currently being discussed here elixir-sqlite/exqlite#192
12. ruslandoga commented on Feb 16, 2023

    

    ruslandoga

    on Feb 16, 2023

    Last edited by ruslandoga

    Contributor

    More actions

    Once the connection goes idle, it closes the database and that in memory database is lost.

    Even if so, on re-open, after_connect would be executed again (running the migrations) putting the database in a clean state for the next test. Note that I'm only suggesting using in memory databases for tests: ruslandoga/sqlite3-memory-async@master...async

    By idle here, what do you mean? (I think the issue link is wrong)
13. warmwaffles commented on Feb 16, 2023

    

    warmwaffles

    on Feb 16, 2023

    Member

    More actions

    Even if so, on re-open, after_connect would be executed again (running the migrations)

    There will be people who use a structure.sql as well and don't build from scratch completely. I know this because I am one of them 😞.

    By idle here, what do you mean?

    I don't remember the details, but I was running into issues where the connection pool had 1 connection during a test and the in memory database just disappeared. I chalked that up to the connection going "idle" and closing the database. I threw some debugging in and saw that Exqlite.Connection.disconnect was called.
14. josevalim commented on Apr 16, 2023

    

    josevalim

    on Apr 16, 2023

    Author

    More actions

    Is begin concurrent an extension? If so, could this be used to install it? https://observablehq.com/@asg017/making-sqlite-extensions-npm-installable-and-deno
15. ruslandoga commented on Apr 16, 2023

    

    ruslandoga

    on Apr 16, 2023

    Contributor

    More actions

    It doesn't seem to be an extension, but rather a fork.