Use psycopg2 connection pools #145
Conversation
Simplify our code a bit by using psycopg2's built-in connection pools
|
The diff is rather large here because I deleted a bunch of code from the |
|
|
||
| register_json(conn, loads=json.loads, globally=True) |
There was a problem hiding this comment.
A subtle, notable difference here is that I'm passing in ujson's loads for psycopg2 to use when parsing the data from PostgreSQL.
There was a problem hiding this comment.
(This should make things way faster.)
| self.sql_conn_pool = DBAffinityConnectionsNoLimit( | ||
| self.dbnames, n_conn, self.conn_info) | ||
| self.sql_conn_pool = DatabaseCycleConnectionPool( | ||
| n_conn, 100, self.dbnames, self.conn_info) |
There was a problem hiding this comment.
I have a hard-coded 100 connection maximum here but it should probably be a configurable thing. The only time this gets checked is when we're requesting a new connection out of the pool. The connection pools will only maintain n_conn connections per dbname otherwise they'll get closed whenever the connection is returned to the pool.
There was a problem hiding this comment.
How many active connections do we maintain per DB now?
How many are typically from a single tileserver or tilequeue instance / worker?
There was a problem hiding this comment.
I wouldn't expect us to ever need to go over the minimum. What we can use for the maximum is the conservative number where we calculate how many io threads we'll need:
tilequeue/tilequeue/command.py
Line 528 in 738038d
We can just pass that on through to the datafetcher constructor.
| for conn in conns: | ||
| pool = self._conns_to_pool.pop(id(conn), None) | ||
| if pool: | ||
| pool.putconn(conn) |
There was a problem hiding this comment.
I recognize that this case should never happen, but should we have an else clause where we try to close the connection? Actually an assert might make more sense because this means a caller tried to close a connection that it didn't get from a pool.
There was a problem hiding this comment.
Looking closer: I didn't use an assert because I didn't want to dump out of the loop and leave the rest of the connections in the conns list open.
There was a problem hiding this comment.
… but I'll switch to assert so we just dump out of here in that case. It's not particularly recoverable with existing code anyway.
There was a problem hiding this comment.
I see what you're saying. If we cared, we could try to close all the connections that were valid before raising, but I agree that this should trigger a failure case anyway.
Connects to #141
Simplify our code a bit by using psycopg2's built-in connection pools.