Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow prepared_statements_cache=false option to disable prepared statements cache #194

Merged
merged 2 commits into from
Nov 4, 2023

Conversation

bcardiff
Copy link
Member

@bcardiff bcardiff commented Nov 3, 2023

During #178 we came up that it would be nice to be able to tweak whether the prepared statements are cached or not. This option allows the user to opt-out of the statement cache, while still use prepared statements. Some drivers only allow prepared statements.

cc: @luislavena

@bcardiff bcardiff merged commit 285e865 into master Nov 4, 2023
luislavena added a commit to luislavena/test-crystal-sqlite3-db that referenced this pull request Nov 4, 2023
Override crystal-sqlite3 dependency on DB to help test
crystal-lang/crystal-db#194
luislavena added a commit to luislavena/test-crystal-sqlite3-db that referenced this pull request Nov 4, 2023
Allow to define at runtime which database configuration to use but keep
some sensitive defaults.

This helps with testing `prepared_statements_cache=false` functionality
introduced by crystal-lang/crystal-db#194
@straight-shoota straight-shoota deleted the feature/disable-stmt-cache branch November 4, 2023 08:58
@luislavena
Copy link

Hello @bcardiff, while I'm working on some updated benchmarks in https://github.com/luislavena/test-crystal-sqlite3-db, I found that disabling the statement cache breaks proper cleanup of the statements in memory, raising exception when attempt to close the connections:

$ DATABASE_URL="sqlite3:data.db?journal_mode=wal&synchronous=normal&busy_timeout=5000&prepared_statements_cache=false" ./bin/server
Using: 'sqlite3:data.db?journal_mode=wal&synchronous=normal&busy_timeout=5000&prepared_statements_cache=false'
Listening on:
- 0.0.0.0:8080
Use Ctrl-C to stop
^CShutting down.
Shutdown completed.
Unhandled exception: unable to close due to unfinalized statements or unfinished backups (SQLite3::Exception)
  from /app/lib/sqlite3/src/sqlite3/connection.cr:134:5 in 'check'
  from /app/lib/sqlite3/src/sqlite3/connection.cr:78:5 in 'do_close'
  from /app/lib/db/src/db/disposable.cr:11:7 in 'close'
  from /app/lib/db/src/db/pool.cr:91:21 in 'close'
  from /app/lib/db/src/db/database.cr:83:7 in 'close'
  from /app/src/server.cr:53:1 in '__crystal_main'
  from /usr/local/share/crystal/src/crystal/main.cr:129:5 in 'main_user_code'
  from /usr/local/share/crystal/src/crystal/main.cr:115:7 in 'main'
  from /usr/local/share/crystal/src/crystal/main.cr:141:3 in 'main'
  from src/env/__libc_start_main.c:95:2 in 'libc_start_main_stage2'

This happens on both single-thread and preview_mt compilations.

Let me know if this is not correct as I understand that prepared_statements_cache=false disabled the shared cache between connections and we cannot disable prepared statements entirely in SQLite3 since doesn't support unprepared statements.

Thank you.
❤️ ❤️ ❤️

@bcardiff
Copy link
Member Author

bcardiff commented Nov 4, 2023

Oh. Disabling it for the pool, the last commit might have been to much.

When using the pool, and not s connection directly, there is always 2 prepared. The first one checks out a connection to validate the statement can be prepared. And then when executing the statement another might be used.

By disabling the cache in the pool I think we prevent the cleanup. Will check if there is way around that or if the cache should not be disabled at the pool.

@bcardiff
Copy link
Member Author

bcardiff commented Nov 4, 2023

Ok, it's not just the pool statement. We need to release every prepared statement after using it if we are not caching it. The cache served also for tracking those resources and cleaning it up before the connection where closed.

Would be better to eager release right after using them or delaying that until the connection is no longer needed? I guess eager, but either can work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants