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

Database connections are never closed #340

Open
Zaczero opened this issue Jan 23, 2025 · 4 comments
Open

Database connections are never closed #340

Zaczero opened this issue Jan 23, 2025 · 4 comments

Comments

@Zaczero
Copy link

Zaczero commented Jan 23, 2025

PgstacDB maintains a persistent connection to the database through self.connection = pool.getconn(). According to the getconn() documentation, this connection will not be automatically returned to the pool, and therefore won't be closed when idle. An explicit putconn() call is required. Currently, db_max_idle is ignored, which leads to database timeouts during long idle periods. I suggest not implementing a custom self.connection singleton pooling logic, but rather relying on the psycopg_pool implementation. Otherwise, you would need to implement the same feature set as psycopg_pool, including idle handling, which adds unnecessary complexity.

@Zaczero
Copy link
Author

Zaczero commented Jan 31, 2025

I could prepare a PR dropping the singleton caching and moving all caching logic to the psycopg_pool (as it should imo).

@bitner
Copy link
Collaborator

bitner commented Feb 10, 2025

PgstacDB is set up with a disconnect method that is called both when using as a context manager (via the exit() method) as well as by trapping it using atexit.register(self.disconnect) to ensure that connections are automatically returned to the pool.

@Zaczero
Copy link
Author

Zaczero commented Feb 13, 2025

I'm confused about the purpose of using ConnectionPool and having the db_max_idle setting if they're not meant to be used. If users are expected to initialize PgstacDB via a context manager each time, creating a new pool just to get a single connection and then closing everything afterward, what's the benefit? The typical use case in SQLAlchemy/Redis/Valkey is to maintain the same connection pool throughout the program's lifecycle - that's where settings like db_max_idle make practical sense.

What you describe would also result in a constant memory leak due to repeated atexit.register calls that prevent the entire PgstacDB instance from ever being freed from memory.

All of these seem like unusual design decisions that cause more issues than they solve. I thought PostgreSQL connection pooling was a solved problem.

@drnextgis
Copy link
Collaborator

I've been reviewing the PgstacDB class, and here's what doesn't seem right to me: we return the connection to the pool

self.pool.putconn(self.connection)
but just a few lines later, we set the pool to None
self.pool = None

As a result, the pool is never reused across different context managers:

>>> pgdb = PgstacDB()
>>> with pgdb as ctx:
...     pgdb.pool is None
... 
False
>>> pgdb.pool is None
True

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

No branches or pull requests

3 participants