Merge lp:~cedric-lebrouster/ocb-server/ocb-7.0-bug-1322191-db_maxconn into lp:ocb-server

Proposed by Cedric Le Brouster(OpenFire)
Status: Rejected
Rejected by: Holger Brunn (Therp)
Proposed branch: lp:~cedric-lebrouster/ocb-server/ocb-7.0-bug-1322191-db_maxconn
Merge into: lp:ocb-server
Diff against target: 12 lines (+2/-0)
1 file modified
openerp/ (+2/-0)
To merge this branch: bzr merge lp:~cedric-lebrouster/ocb-server/ocb-7.0-bug-1322191-db_maxconn
Reviewer Review Type Date Requested Status
Holger Brunn (Therp) Disapprove
Alexandre Fayolle - camptocamp code review, sometest without being able to exercise that code Approve
Pedro Manuel Baeza code review Approve
Review via email:

Description of the change

Close database connections before removing them.
Fixes bug lp:1322191

To post a comment you must log in.
Revision history for this message
Pedro Manuel Baeza (pedro.baeza) wrote :

LGTM. Thanks for the patch.


review: Approve (code review)
Revision history for this message
Alexandre Fayolle - camptocamp (alexandre-fayolle-c2c) wrote :

LGTM : the connection must be closed.


I cannot find by reading the code a case where this branch is executed (i.e. a logical path in which we get len(self._connections) >= self._maxconn so I'm wondering if we are not improving dead code here.

Do you have a scenario at hand?

review: Approve (code review, sometest without being able to exercise that code)
Revision history for this message
Cedric Le Brouster(OpenFire) (cedric-lebrouster) wrote :

Thanks for review.

What I understand by reading the code is that this function has 4 parts.
First it removes closed or leaked connections - none of our business here
Then it tries to re-use an iddle connection previously opened for the same database
If no iddle connection is found for this database, and if db_maxconn is reached, it will free any other iddle connection, and here was the bug
Finally, it will open a new connection

So the bug needs to have several databases opened to occur.

How to test :
I check number of postgres connections on linux with the psql command:
psql -c "SELECT datid,datname,procpid,usesysid,usename,current_query FROM pg_stat_activity WHERE application_name!='psql';" postgres

Set in config file db_maxconn = 4 (must be higher than max_cron_threads if I remember correct)
Run openerp
Login to a database
You should see 3 or 4 postgres connections for your database.

Now, without loging out, login to another database (loging out would free connections)
You should now see more than 4 used databases. If not, click on a menu on the 2 databases with a low time interval. For whatever reason connections are correctly freed or their database switched sometimes, but I don't know why or where in the code.

Revision history for this message
Alexandre Fayolle - camptocamp (alexandre-fayolle-c2c) wrote :

thanks for the precision.

Revision history for this message
Holger Brunn (Therp) (hbrunn) wrote :

Development for 7.0 has moved to github on - please move your merge proposal there if it is still valid.

(I close and reject this in order to have a cleaner overview for 6.1 MPs which indeed have to be done on launchpad)

review: Disapprove

Unmerged revisions

5343. By Cedric Le Brouster(OpenFire)

[FIX] sql_db: close db connexions before removing them. bug-1322191

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'openerp/'
2--- openerp/ 2014-04-09 15:16:59 +0000
3+++ openerp/ 2014-05-22 14:50:38 +0000
4@@ -430,6 +430,8 @@
5 for i, (cnx, used) in enumerate(self._connections):
6 if not used:
7 self._connections.pop(i)
8+ if not cnx.closed:
9+ cnx.close()
10 self._debug('Removing old connection at index %d: %r', i, cnx.dsn)
11 break
12 else: