Issue: Read-only slave connections are not closed
Author: stas
State: closed
Created: 2015-08-17 14:53:19

Using a read-only pooled database as a slave won't close its connections.

I also found it is pretty hard to manually manage these as it is now suggested in case of connection pools. This is mainly because the read queries are randomly picking connections so it is really hard to track those. We're still exploring some solutions, I will try to report back when we have something working.


@stas -- what do you mean by read-only? You mean the `ReadSlaveModel` from `playhouse.read_slave`?

Can you share some code so I can get a better idea of your configuration, as well as the problem you're experiencing?


> @stas -- what do you mean by read-only? You mean the ReadSlaveModel from playhouse.read_slave?

Yes, sorry I forgot to mention the exact module.

> Can you share some code so I can get a better idea of your configuration, as well as the problem you're experiencing?

There's nothing custom, we just extended our models with the `ReadSlaveModel` and we started getting `ValueError`s with: Exceeded maximum connections.
I started tracking this down and it is generated by the `playhouse.pool._connect` which we use. Going deeper I discovered that the `max_connections` limit is reached because the connections are not returning back to the pool (aka closed). Honestly this makes sense since I couldn't find anywhere where those connections are managed to return back to pool/close.


I'm assuming this is in a web app and that you've got some middleware or something opening connections (or checking them out) at the beginning of the request/response cycle, and closing them (returning them) at the end...

If that's the case, perhaps it would be sufficient in the "close" code (when you're done with a request) to iterate through all databases and do something like:

``` python
databases = [master] + read_slaves
for database in databases:
    if database.is_open():
        database.close()
```


> I'm assuming this is in a web app and that you've got some middleware or something opening connections (or checking them out) at the beginning of the request/response cycle, and closing them (returning them) at the end...

Nope, this is a heavily threaded application.

> If that's the case, perhaps it would be sufficient in the "close" code (when you're done with a request) to iterate through all databases and do something like:

We explored this approach, in fact we were using it until we started adding read-only servers. We found that closing of the database connections performs strange (you really need to be handling this in the same thread and so on...).

Last couple of days I started reading the way database and pooled database classes work and I could not understand some of the aspects. One of the main question is related to the fact that we actually need to manually close all the connections. By looking at the database API, I can see that an implementation where the connections are automatically picked, used and closed/put back is possible, still you decided a manual approach is better. Can I ask you why?


> We found that closing of the database connections performs strange (you really need to be handling this in the same thread and so on...).

Can you explain that more? What database engine are you using?

Actually now that you describe that, I can see how that might arise... Thread1 opens connection, does work, puts it back in pool and goes away. Thread2 checks out connection, does work, puts it back in pool. Pool decides connection is stale, so it closes it...Perhaps that's what happened?

> One of the main question is related to the fact that we actually need to manually close all the connections.

That is the case whether you use a pool or just a regular `Database` subclass. It's written all over the docs, but you need to open the conn when you want to start work, then close it when you're done. For web apps this happens typically in the request/response cycle.

If you're using a pool, the only difference is that the pool manages the connections for you. It's transparent to the application -- the `connect()` and `close()` APIs are the same.

> By looking at the database API, I can see that an implementation where the connections are automatically picked, used and closed/put back is possible, still you decided a manual approach is better. Can I ask you why?

It's true that a connection will be automatically opened if you try to execute a query and no conn has been opened yet. This is for backwards compatibility and to reduce boilerplate when writing simple scripts. There is no way, as far as I know, to automatically close or "put back" connections.


Just out of curiosity, what database are you using? And do you have any more info regarding the error your received when closing connections (the error about "threads")?


Following the docs, we managed to manually close opened connections, so I'm closing this.

> It's true that a connection will be automatically opened if you try to execute a query and no conn has been opened yet. This is for backwards compatibility and to reduce boilerplate when writing simple scripts. There is no way, as far as I know, to automatically close or "put back" connections.

In regards to the question about automatic management of the connections, after looking at the query classes, maybe it's worth exploring the approach where connections are handled inside context managers and are automatically open and closed after the query is done. I tried patching the `Query._execute()` but the way query wrappers handle results (lazily), it didn't work properly.

> Just out of curiosity, what database are you using? And do you have any more info regarding the error your received when closing connections (the error about "threads")?

We're using an extended postgres adapter, and the problem with that is, especially in a threaded environment, is that having a similar to the request/response approach is not necessarily a common case. Every-time we need to make changes to connection management or integrate our peewee models, we need to find a way how we are going to handle the connections management. You can imagine it is pretty frustrating :(

There's a lot of complexity required to provide the current connection state (`thread.local` and the whole execution context stack), so I started asking myself if there's no better way to handle this. Previously I had experience with working with the psycopg2 modules, and there are a couple of things that might help with this (consider using psycopg2.pool and it's API for managing cursors). I'm seriously thinking to take a shot on making the database adapter use coroutines or at least leverage the `psycopg2.pool` API.

Thanks a lot for helping with answers.


I am not sure about using a connection-per-query. If you wanted to be very granular you could do a connection per transaction, that is not too uncommon, and in fact peewee's [ExecutionContext](http://docs.peewee-orm.com/en/latest/peewee/database.html#advanced-connection-management) ([api](http://docs.peewee-orm.com/en/latest/peewee/api.html#ExecutionContext)) does just this.

I've used huey in a multi-threaded, long-running script -- managing the connections wasn't too difficult since peewee stores a connection-per-thread by default.

The postgresql pool is implemented very similar to the peewee pool as far as I can tell. Peewee goes a step further and adds the concept of an expiration time, but other than that they are quite similar. Peewee simply abstracts away the pool implementation details from the Database API so apps can go on using `connect()` and `close()` without being aware the conns are being pooled.


> I am not sure about using a connection-per-query. If you wanted to be very granular you could do a connection per transaction, that is not too uncommon, and in fact peewee's ExecutionContext (api) does just this.

The issue is a little bit more complex. I can't speak for other databases, but postgres adapter doesn't provide an API to tell if the connection is still open or closed (see psycopg/psycopg2#263). The approach with keeping a connection around in the hope it will stay alive by the time a thread is done using it will not work unless you set a very low `stale_timeout`, and it will still not guarantee if the connection is still alive.

We're having issues with this exact aspect of the psycopg2 and peewee in some long-running multi threaded application (I'm talking about weeks of run-time here, ideally we would like to have it running for months like that, but it doesn't) and using the Redshift database (a postgres flavor). If we lower the `stale_timeout`, it will pretty-much work as a connection-per-query set-up.

If we look at the bigger picture here, the situation with the read-write queries split across different servers is another case where this doesn't fit well in the current approach the connections are handled (we have some workers running for hours, it might be that the read-only queries are just happening 10% of the time, why keeping the connection around all the time?).

What I'm trying to say, is that the current adapter (talking about postgres here) could use some improvements to handle the connections in a better way and, why not, auto-close them without having the users to figure out when a connection is stalled and when it should be closed.

> The postgresql pool is implemented very similar to the peewee pool as far as I can tell. Peewee goes a step further and adds the concept of an expiration time, but other than that they are quite similar. Peewee simply abstracts away the pool implementation details from the Database API so apps can go on using connect() and close() without being aware the conns are being pooled.

That is why I wonder why it was not just extended in peewee. Btw, looking at the persistent connection pool from psycogp2, I can spot the similarities with the current peewee's pool. It is also deprecated now.
http://initd.org/psycopg/docs/pool.html#psycopg2.pool.PersistentConnectionPool


I'm pretty sure that `psycopg2` provides a `closed` boolean attribute on the `Connection` object:

http://initd.org/psycopg/docs/connection.html#connection.closed

The peewee connection pool uses this attribute to check whether a connection is closed when the application checks it out. So the pool will:
1. Check that the conn wasn't open more than `stale_timeout` seconds ago.
2. Check that `conn.closed` is not True.
3. Release the connection to your app

> we have some workers running for hours, it might be that the read-only queries are just happening 10% of the time, why keeping the connection around all the time?

Then could you use a pool for the writers and a simple `PostgresqlDatabase` for the readers?

Regarding the psycopg2 pool, I actually did not look at it's code until you mentioned it, as I didn't know it existed...I guess there's only so many ways to implement a simple pool, haha.