Re: Psycopg 2.2.x issue with PgBouncer

This is a multi-part message in MIME format.
--------------080808010104090607080701
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit

On 24/07/10 01:45, Jason Yan wrote:
> I'm seeing an issue with how Psycopg 2.2.x handles connections with
> PgBouncer.

Hi!

Thanks for the report, I think it's a psycopg2 regression. With the
merging of the green branch psycopg stopped issuing a ROLLBACK when the
connection is garbage collected while still open. PFA the patches that
fix this.

> And the small test script that I'm reproducing the issue with:
>   import psycopg2
>   connection = psycopg2.connect("dbname=test user=postgres
> host=localhost port=6432")
>   cursor = connection.cursor()
>   cursor.execute("SELECT 1")
>   connection.close()

As an immediate workaround you can add connection.rollback() or
connection.commit() before your connection.close().

I fixed this problem in my repo, in the rollback_on_exit branch:

http://git.wulczer.org/?p=psycopg2.git;a=shortlog;h=ref/head/rollback_on_exit

However that triggered interesting issues with the green connection
tests. In short, when deallocating a connection, conn_close() is called.
It then tries to execute a ROLLBACK, which in case of green connections
leads to calling the user-defined wait callback. After the callback
returns, as the connection has zero references, it gets deallocated
again, recursively, which leads to all sorts of problems.

I fixed it in two ways. First, the code keeps a flag that tells if
deallocation is underway. If the flag is set, the deallocation function
is a noop (this prevents recursive invocation).

Second, if deallocation is underway, the green execution method is never
used (it always blocks). That's not ideal, but otherwise it would mean
calling the user-defined callback with the connection object as an
argument that has zero refs... Not sure how this could be fixed.

Note that with the second fix the first is not needed, as we bypass
green execution mode and so won't trigger recursive dealloc. I still
think it's useful to have a guard there, in case later on we start doing
someting in the connection_dealloc function that will cause similar issues.

Cheers,
Jan

--------------080808010104090607080701
Content-Type: text/x-diff;
 name="0001-Force-a-rollback-when-closing-the-connection.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename="0001-Force-a-rollback-when-closing-the-connection.patch"

From dfd1ef5104dd366e2c25fde26c28bb15590de74c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jan=20Urba=C5=84ski?= <[email protected]>
Date: Sat, 24 Jul 2010 14:29:33 +0200
Subject: [PATCH 1/3] Force a rollback when closing the connection.

A typo was preventing that, and that broke pgbouncer. Connections were
being closed without an explicit rollback, and pgbouncer was treating
that as unclean exit and shutting down its backend.

Commit 0ec73a18b47040b8fef41a8e546057b767b26e26 moved the block that
sets the connection status to closed which triggered the problem, as
original coding was forcing a rollback if the connection was
closed. It seems more logical to do it only when the connection is
open, and mark it as closed afterward.

Problem report by Jason Yan.
---
 psycopg/connection_int.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/psycopg/connection_int.c b/psycopg/connection_int.c
index 902fdbb..a69ae7c 100644
--- a/psycopg/connection_int.c
+++ b/psycopg/connection_int.c
@@ -745,7 +745,7 @@ conn_close(connectionObject *self)
 
     /* execute a forced rollback on the connection (but don't check the
        result, we're going to close the pq connection anyway */
-    if (self->pgconn && self->closed == 1) {
+    if (self->pgconn && self->closed == 0) {
         PGresult *pgres = NULL;
         char *error = NULL;
 
-- 
1.7.1


--------------080808010104090607080701
Content-Type: text/x-diff;
 name="0002-Keep-a-flag-telling-whether-the-connection-is-being-.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename*0="0002-Keep-a-flag-telling-whether-the-connection-is-being-.pa";
 filename*1="tch"

From e5de0ced2a3381e64fe6586ad2b3c0bc9021e707 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jan=20Urba=C5=84ski?= <[email protected]>
Date: Sat, 24 Jul 2010 15:27:32 +0200
Subject: [PATCH 2/3] Keep a flag telling whether the connection is being deallocated.

When a connection is garbage collected, a rollback can be issued. That
can result in the wait callback being called, if the connection is
green. That in turn leads to recursive calling of the deallocation
function, because it is used as the callback argument and has zero
references.
---
 psycopg/connection.h      |    2 ++
 psycopg/connection_type.c |   16 +++++++++++++++-
 2 files changed, 17 insertions(+), 1 deletions(-)

diff --git a/psycopg/connection.h b/psycopg/connection.h
index 69f1385..eef4275 100644
--- a/psycopg/connection.h
+++ b/psycopg/connection.h
@@ -110,6 +110,8 @@ typedef struct {
     PyObject *binary_types;   /* a set of typecasters for binary types */
 
     int equote;               /* use E''-style quotes for escaped strings */
+
+    int in_dealloc;           /* a flag to prevent recursive deallocation */
 } connectionObject;
 
 /* C-callable functions in connection_int.c and connection_ext.c */
diff --git a/psycopg/connection_type.c b/psycopg/connection_type.c
index 3e4bbdd..afb924e 100644
--- a/psycopg/connection_type.c
+++ b/psycopg/connection_type.c
@@ -600,6 +600,7 @@ connection_setup(connectionObject *self, const char *dsn, long int async)
     self->binary_types = PyDict_New();
     self->notice_pending = NULL;
     self->encoding = NULL;
+    self->in_dealloc = 0;
 
     pthread_mutex_init(&(self->lock), NULL);
 
@@ -629,7 +630,20 @@ static void
 connection_dealloc(PyObject* obj)
 {
     connectionObject *self = (connectionObject *)obj;
-    
+
+    /*
+      If the connection is still open at this point, we will call conn_close,
+      which can end up calling the user-defined wait callback if the connection
+      is green. The callback is called with the connection as an argument, and
+      after the call completes it will try to deallocate it again (because it
+      currently has 0 refs). To prevent such resursive deallocation, keep a
+      flag that tells whether a deallocation is underway.
+    */
+    if (self->in_dealloc)
+        return;
+
+    self->in_dealloc = 1;
+
     PyObject_GC_UnTrack(self);
 
     if (self->closed == 0) conn_close(self);
-- 
1.7.1


--------------080808010104090607080701
Content-Type: text/x-diff;
 name="0003-Do-not-use-green-execution-when-garbage-collecting-t.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename*0="0003-Do-not-use-green-execution-when-garbage-collecting-t.pa";
 filename*1="tch"

From 1daa6564090e30337d04133f0b6765a7269b2630 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jan=20Urba=C5=84ski?= <[email protected]>
Date: Sat, 24 Jul 2010 15:50:06 +0200
Subject: [PATCH 3/3] Do not use green execution when garbage collecting the connection.

The green execution mode calls a user-defined callback with the
connection as an argument and since it's being garbage collected it
shouldn't be passed to user code anymore.
---
 psycopg/pqpath.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/psycopg/pqpath.c b/psycopg/pqpath.c
index 69c7813..e8d31ea 100644
--- a/psycopg/pqpath.c
+++ b/psycopg/pqpath.c
@@ -350,7 +350,10 @@ pq_execute_command_locked(connectionObject *conn, const char *query,
             conn->pgconn, query);
     *error = NULL;
 
-    if (!psyco_green()) {
+    /* if we are grabage collecting the connection we can't use the green mode,
+       because it will call the wait callback and pass the connection being
+       removed as an argument */
+    if (conn->in_dealloc || !psyco_green()) {
         *pgres = PQexec(conn->pgconn, query);
     } else {
         PyEval_RestoreThread(*tstate);
-- 
1.7.1


--------------080808010104090607080701
Content-Type: text/plain; charset="us-ascii"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Content-Disposition: inline

_______________________________________________
Psycopg mailing list
Psycopg-IAPFreCvJWPBWskQ1e/[email protected]
http://lists.initd.org/mailman/listinfo/psycopg

--------------080808010104090607080701--