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

Schema sqlalchemy engine caching is per-instance #280

Closed
eito-fis opened this issue Jun 24, 2021 · 3 comments
Closed

Schema sqlalchemy engine caching is per-instance #280

eito-fis opened this issue Jun 24, 2021 · 3 comments
Labels
affects: architecture Improvements or additions to architecture affects: technical debt Improves the state of the codebase ready Ready for implementation type: bug Something isn't working work: backend Related to Python, Django, and simple SQL

Comments

@eito-fis
Copy link
Contributor

Describe the bug
The _sa_engine property of the Schema model is cached using the @cached_property decorator. @cached_property caches on a per-instance basis, which means the engine is not cached across different instances of the same schema object, leading to the engine being created many times.

For example, each table must access it's schema's engine which is then cached on that table's instance of the schema. However, as the engine is only cached on that instance, another table trying to access the same schema's engine will also create a new engine. As a result, something like the table list endpoint will create an engine for every table in the database.

Expected behavior
We should cache the schema's engine object at a higher level so it persists across instances.

Additional context
Issues with too many connections, specifically the too many clients error, is blocking backend work that adds additional tests (see #265, #268). A naive caching implementation:

_engine = [None]
class Schema(DatabaseObject):
    database = models.CharField(max_length=128)

    @property
    def _sa_engine(self):
        if _engine[0] is None:
            _engine[0] = create_mathesar_engine(self.database)
        return _engine[0]

does resolve the error on #265. Unfortunately, I haven't been able to consistently reproduce the error as a single test.

I have also talked to @mathemancer about opening a broader issue on the where engines should be created. If this overlaps with that topic, I'm happy to close.

@eito-fis eito-fis added the type: bug Something isn't working label Jun 24, 2021
@kgodey kgodey added this to the 2. Tables from File Import milestone Jun 24, 2021
@kgodey kgodey added ready Ready for implementation work: backend Related to Python, Django, and simple SQL labels Jun 24, 2021
@kgodey
Copy link
Contributor

kgodey commented Jun 24, 2021

Added this to milestone 2 because it seems important to resolve ASAP.

@eito-fis
Copy link
Contributor Author

Note: We have a temporary fix in place as of #265 which should be removed when this issue is handled.

@kgodey kgodey added affects: architecture Improvements or additions to architecture affects: technical debt Improves the state of the codebase labels Aug 22, 2021
@kgodey kgodey modified the milestones: 2021-09 improvements, 06. 2021-09 Stability Aug 31, 2021
@kgodey kgodey self-assigned this Aug 31, 2021
@kgodey kgodey added status: started and removed ready Ready for implementation labels Oct 14, 2021
@kgodey kgodey modified the milestones: [06] 2021-10 Stability, [06A] 2021-10 improvements Oct 14, 2021
@kgodey kgodey added ready Ready for implementation and removed status: started labels Oct 19, 2021
@kgodey kgodey removed their assignment Oct 19, 2021
@dmos62
Copy link
Contributor

dmos62 commented May 16, 2022

As @eito-fis mentioned, this has been solved, and the solution has proven somewhat stable. There was a bug, where a cached engine might unexpectedly break when dropping and recreating a database, but that's fixed in #1222.

@dmos62 dmos62 closed this as completed May 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects: architecture Improvements or additions to architecture affects: technical debt Improves the state of the codebase ready Ready for implementation type: bug Something isn't working work: backend Related to Python, Django, and simple SQL
Projects
No open projects
Development

No branches or pull requests

4 participants