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

Revisit SQLALchemy engine cache logic #698

Closed
rsyring opened this issue Mar 14, 2019 · 1 comment
Closed

Revisit SQLALchemy engine cache logic #698

rsyring opened this issue Mar 14, 2019 · 1 comment
Milestone

Comments

@rsyring
Copy link
Contributor

rsyring commented Mar 14, 2019

We currently cache the SQLAlchemy engine in _EngineConnector based on the echo setting. Prior to #684, that was the main engine option we supported. However, with more ways to customize the engine options, do we need to cache the engine differently? What kind of assumptions are we making and what kind of scenarios do we want to support with respect to caching the engine object?

Some considerations:

  • If I understand SQLAlchemy.get_engine() correctly, we are already using state to cache the _EngineConnector instance per application. Since the engine is cached on that instance, we are caching the engine connection per application.
  • _EngineConnector will currently return the existing engine as long as the database URI/binds are the same and the echo setting is the same. So, it seems the goal is to support changing db connections or echo configuration settings after an engine is established and FSA will detect this and create a new engine accordingly.
  • Enhancing this logic to consider all engine options is possible, but it seems like that code path was specifically part of what would get cached. I'm not sure if there are performance issues that could arise if you were calling get_options() every time you wanted to get an engine.

As a result of these considerations, I believe it's best to leave the current caching logic as-is for #684 and revisit this issue if/when other users can provide scenarios where the current behavior is [un]desired.

@davidism
Copy link
Member

Fixed in #1087. There is no longer a separate state object with separate connectors. Instead, the extension is stored directly in app.extensions["sqlalchemy"], and the engines are stored in a weakref dict for each app on the extension. The engines are created when intializing the app, there's no reason to defer creating them because they don't connect until they're actually used. Configuration is now required to be done before calling init_app, it is never read after that.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 3, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

No branches or pull requests

2 participants