Fixed idempotence check changing connection characteristics

4 files changed, 48 insertions, 22 deletions

diff --git a/NEWS b/NEWS
index adb5d3a..0fb4119 100644
--- a/NEWS
+++ b/NEWS
@@ -6,6 +6,8 @@ What's new in psycopg 2.7.4
 
 - Fixed Solaris 10 support (:ticket:`#532`).
 - `cursor.mogrify()` can be called on closed cursors (:ticket:`#579`).
+- Fixed setting session characteristics in corner cases on autocommit
+  connections (:ticket:`#580`).
 - Fixed `~psycopg2.extras.MinTimeLoggingCursor` on Python 3 (:ticket:`#609`).
 - Fixed parsing of array of points as floats (:ticket:`#613`).
 - Fixed `~psycopg2.__libpq_version__` building with libpq >= 10.1
diff --git a/psycopg/connection_int.c b/psycopg/connection_int.c
index e8081b9..3ea5ca3 100644
--- a/psycopg/connection_int.c
+++ b/psycopg/connection_int.c
@@ -67,6 +67,9 @@ const char *srv_state_guc[] = {
 };
 
 
+const int SRV_STATE_UNCHANGED = -1;
+
+
 /* Return a new "string" from a char* from the database.
  *
  * On Py2 just get a string, on Py3 decode it in the connection codec.
@@ -1188,6 +1191,8 @@ conn_set_session(connectionObject *self, int autocommit,
     int rv = -1;
     PGresult *pgres = NULL;
     char *error = NULL;
+    int want_autocommit = autocommit == SRV_STATE_UNCHANGED ?
+        self->autocommit : autocommit;
 
     if (deferrable != self->deferrable && self->server_version < 90100) {
         PyErr_SetString(ProgrammingError,
@@ -1209,24 +1214,24 @@ conn_set_session(connectionObject *self, int autocommit,
     Py_BEGIN_ALLOW_THREADS;
     pthread_mutex_lock(&self->lock);
 
-    if (autocommit) {
-        /* we are in autocommit state, so no BEGIN will be issued:
+    if (want_autocommit) {
+        /* we are or are going in autocommit state, so no BEGIN will be issued:
          * configure the session with the characteristics requested */
-        if (isolevel != self->isolevel) {
+        if (isolevel != SRV_STATE_UNCHANGED) {
             if (0 > pq_set_guc_locked(self,
                     "default_transaction_isolation", srv_isolevels[isolevel],
                     &pgres, &error, &_save)) {
                 goto endlock;
             }
         }
-        if (readonly != self->readonly) {
+        if (readonly != SRV_STATE_UNCHANGED) {
             if (0 > pq_set_guc_locked(self,
                     "default_transaction_read_only", srv_state_guc[readonly],
                     &pgres, &error, &_save)) {
                 goto endlock;
             }
         }
-        if (deferrable != self->deferrable) {
+        if (deferrable != SRV_STATE_UNCHANGED) {
             if (0 > pq_set_guc_locked(self,
                     "default_transaction_deferrable", srv_state_guc[deferrable],
                     &pgres, &error, &_save)) {
@@ -1260,10 +1265,18 @@ conn_set_session(connectionObject *self, int autocommit,
         }
     }
 
-    self->autocommit = autocommit;
-    self->isolevel = isolevel;
-    self->readonly = readonly;
-    self->deferrable = deferrable;
+    if (autocommit != SRV_STATE_UNCHANGED) {
+        self->autocommit = autocommit;
+    }
+    if (isolevel != SRV_STATE_UNCHANGED) {
+        self->isolevel = isolevel;
+    }
+    if (readonly != SRV_STATE_UNCHANGED) {
+        self->readonly = readonly;
+    }
+    if (deferrable != SRV_STATE_UNCHANGED) {
+        self->deferrable = deferrable;
+    }
     rv = 0;
 
 endlock:
diff --git a/psycopg/connection_type.c b/psycopg/connection_type.c
index 8c5085b..6a66d48 100644
--- a/psycopg/connection_type.c
+++ b/psycopg/connection_type.c
@@ -39,6 +39,7 @@
 extern HIDDEN const char *srv_isolevels[];
 extern HIDDEN const char *srv_readonly[];
 extern HIDDEN const char *srv_deferrable[];
+extern HIDDEN const int SRV_STATE_UNCHANGED;
 
 /** DBAPI methods **/
 
@@ -561,10 +562,10 @@ psyco_conn_set_session(connectionObject *self, PyObject *args, PyObject *kwargs)
     PyObject *deferrable = Py_None;
     PyObject *autocommit = Py_None;
 
-    int c_isolevel = self->isolevel;
-    int c_readonly = self->readonly;
-    int c_deferrable = self->deferrable;
-    int c_autocommit = self->autocommit;
+    int c_isolevel = SRV_STATE_UNCHANGED;
+    int c_readonly = SRV_STATE_UNCHANGED;
+    int c_deferrable = SRV_STATE_UNCHANGED;
+    int c_autocommit = SRV_STATE_UNCHANGED;
 
     static char *kwlist[] =
         {"isolation_level", "readonly", "deferrable", "autocommit", NULL};
@@ -637,7 +638,7 @@ psyco_conn_autocommit_set(connectionObject *self, PyObject *pyvalue)
     if (!_psyco_set_session_check_setter_wrapper(self)) { return -1; }
     if (-1 == (value = PyObject_IsTrue(pyvalue))) { return -1; }
     if (0 > conn_set_session(self, value,
-                self->isolevel, self->readonly, self->deferrable)) {
+                SRV_STATE_UNCHANGED, SRV_STATE_UNCHANGED, SRV_STATE_UNCHANGED)) {
         return -1;
     }
 
@@ -668,8 +669,8 @@ psyco_conn_isolation_level_set(connectionObject *self, PyObject *pyvalue)
 
     if (!_psyco_set_session_check_setter_wrapper(self)) { return -1; }
     if (0 > (value = _psyco_conn_parse_isolevel(pyvalue))) { return -1; }
-    if (0 > conn_set_session(self, self->autocommit,
-                value, self->readonly, self->deferrable)) {
+    if (0 > conn_set_session(self, SRV_STATE_UNCHANGED,
+                value, SRV_STATE_UNCHANGED, SRV_STATE_UNCHANGED)) {
         return -1;
     }
 
@@ -715,13 +716,13 @@ psyco_conn_set_isolation_level(connectionObject *self, PyObject *args)
 
     if (level == 0) {
         if (0 > conn_set_session(self, 1,
-                self->isolevel, self->readonly, self->deferrable)) {
+                SRV_STATE_UNCHANGED, SRV_STATE_UNCHANGED, SRV_STATE_UNCHANGED)) {
             return NULL;
         }
     }
     else {
         if (0 > conn_set_session(self, 0,
-                level, self->readonly, self->deferrable)) {
+                level, SRV_STATE_UNCHANGED, SRV_STATE_UNCHANGED)) {
             return NULL;
         }
     }
@@ -767,8 +768,8 @@ psyco_conn_readonly_set(connectionObject *self, PyObject *pyvalue)
 
     if (!_psyco_set_session_check_setter_wrapper(self)) { return -1; }
     if (0 > (value = _psyco_conn_parse_onoff(pyvalue))) { return -1; }
-    if (0 > conn_set_session(self, self->autocommit,
-                self->isolevel, value, self->deferrable)) {
+    if (0 > conn_set_session(self, SRV_STATE_UNCHANGED,
+                SRV_STATE_UNCHANGED, value, SRV_STATE_UNCHANGED)) {
         return -1;
     }
 
@@ -813,8 +814,8 @@ psyco_conn_deferrable_set(connectionObject *self, PyObject *pyvalue)
 
     if (!_psyco_set_session_check_setter_wrapper(self)) { return -1; }
     if (0 > (value = _psyco_conn_parse_onoff(pyvalue))) { return -1; }
-    if (0 > conn_set_session(self, self->autocommit,
-                self->isolevel, self->readonly, value)) {
+    if (0 > conn_set_session(self, SRV_STATE_UNCHANGED,
+                SRV_STATE_UNCHANGED, SRV_STATE_UNCHANGED, value)) {
         return -1;
     }
 
diff --git a/tests/test_connection.py b/tests/test_connection.py
index eff1065..21aec41 100755
--- a/tests/test_connection.py
+++ b/tests/test_connection.py
@@ -1382,6 +1382,16 @@ class TransactionControlTests(ConnectingTestCase):
         cur.execute("SHOW default_transaction_read_only;")
         self.assertEqual(cur.fetchone()[0], 'off')
 
+    def test_idempotence_check(self):
+        self.conn.autocommit = False
+        self.conn.readonly = True
+        self.conn.autocommit = True
+        self.conn.readonly = True
+
+        cur = self.conn.cursor()
+        cur.execute("SHOW transaction_read_only")
+        self.assertEqual(cur.fetchone()[0], 'on')
+
 
 class AutocommitTests(ConnectingTestCase):
     def test_closed(self):