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

fix: Use one database connection rather than one per query #126

Merged
merged 2 commits into from
Nov 10, 2024

Conversation

mbhutton
Copy link
Contributor

@mbhutton mbhutton commented Oct 27, 2024

This PR is staged as 2 commits, with the first being a bug fix, and the second being a suggested performance improvement:

  • Close database connection after each query (to avoid leaking connections which causes large queries to fail)
  • Use a single shared database connection rather than a new one per query, to cut execution time in half

This is to address #125

The first change is to allow queries such as things-cli todos on a DB with 2500 tasks.
The second change is to speed up such queries, reducing execution time from ~0.230 seconds to ~0.120 seconds.

@mbhutton mbhutton changed the title fix: Close database connection after each query fix: Using one database connection rather than one per query Oct 27, 2024
@mbhutton mbhutton changed the title fix: Using one database connection rather than one per query fix: Use one database connection rather than one per query Oct 27, 2024
@mbhutton mbhutton force-pushed the main branch 3 times, most recently from 7a91d5e to 11de118 Compare October 27, 2024 06:01
Each query creates a new DB connection.
This change is to close each new connection rather than leaving
them open, to allow for runs which involve large numbers of queries.
Use one connection for the lifetime of the process rather
than creating a new connection for each query.

This cuts execution time roughtly in half for queries
across large Things databases.
Copy link

@AlexanderWillner AlexanderWillner merged commit 0ed2f9d into thingsapi:main Nov 10, 2024
1 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants