Added new flag ``retaining=False`` to the kinterbasdb and fdb dialects.

This controls the value of the ``retaining`` flag sent to the
``commit()`` and ``rollback()`` methods of the DBAPI connection.
Defaults to False.  Also in 0.8.2, where it defaults to True.
[ticket:2763]

7 files changed, 208 insertions, 46 deletions

diff --git a/doc/build/changelog/changelog_08.rst b/doc/build/changelog/changelog_08.rst
index d83d0618e..8b3519572 100644
--- a/doc/build/changelog/changelog_08.rst
+++ b/doc/build/changelog/changelog_08.rst
@@ -7,6 +7,16 @@
     :version: 0.8.2
 
     .. change::
+        :tags: feature, firebird
+        :tickets: 2763
+
+        Added new flag ``retaining=True`` to the kinterbasdb and fdb dialects.
+        This controls the value of the ``retaining`` flag sent to the
+        ``commit()`` and ``rollback()`` methods of the DBAPI connection.
+        Due to historical concerns, this flag defaults to ``True``, however
+        in 0.9 this flag will be defaulted to ``False``.
+
+    .. change::
         :tags: requirements
 
         The Python `mock <https://pypi.python.org/pypi/mock>`_ library
diff --git a/doc/build/changelog/changelog_09.rst b/doc/build/changelog/changelog_09.rst
index 466444278..6909c22e0 100644
--- a/doc/build/changelog/changelog_09.rst
+++ b/doc/build/changelog/changelog_09.rst
@@ -7,6 +7,15 @@
     :version: 0.9.0
 
     .. change::
+        :tags: feature, firebird
+        :tickets: 2763
+
+        Added new flag ``retaining=False`` to the kinterbasdb and fdb dialects.
+        This controls the value of the ``retaining`` flag sent to the
+        ``commit()`` and ``rollback()`` methods of the DBAPI connection.
+        Defaults to False.  Also in 0.8.2, where it defaults to True.
+
+    .. change::
         :tags: requirements
 
         The Python `mock <https://pypi.python.org/pypi/mock>`_ library
diff --git a/doc/build/changelog/migration_09.rst b/doc/build/changelog/migration_09.rst
index 03c8563a9..127b1e697 100644
--- a/doc/build/changelog/migration_09.rst
+++ b/doc/build/changelog/migration_09.rst
@@ -520,4 +520,37 @@ DBAPI which per the Firebird project is now their official Python driver.
 
 :ticket:`2504`
 
+Firebird ``fdb`` and ``kinterbasdb`` set ``retaining=False`` by default
+-----------------------------------------------------------------------
+
+Both the ``fdb`` and ``kinterbasdb`` DBAPIs support a flag ``retaining=True``
+which can be passed to the ``commit()`` and ``rollback()`` methods of its
+connection.  The documented rationale for this flag is so that the DBAPI
+can re-use internal transaction state for subsequent transactions, for the
+purposes of improving performance.   However, newer documentation refers
+to analyses of Firebird's "garbage collection" which expresses that this flag
+can have a negative effect on the database's ability to process cleanup
+tasks, and has been reported as *lowering* performance as a result.
+
+It's not clear how this flag is actually usable given this information,
+and as it appears to be only a performance enhancing feature, it now defaults
+to ``False``.  The value can be controlled by passing the flag ``retaining=True``
+to the :func:`.create_engine` call.  This is a new flag which is added as of
+0.8.2, so applications on 0.8.2 can begin setting this to ``True`` or ``False``
+as desired.
+
+.. seealso::
+
+    :mod:`sqlalchemy.dialects.firebird.fdb`
+
+    :mod:`sqlalchemy.dialects.firebird.kinterbasdb`
+
+    http://pythonhosted.org/fdb/usage-guide.html#retaining-transactions - information
+    on the "retaining" flag.
+
+:ticket:`2763`
+
+
+
+
 
diff --git a/lib/sqlalchemy/dialects/firebird/base.py b/lib/sqlalchemy/dialects/firebird/base.py
index ab832178e..dcaa68f4e 100644
--- a/lib/sqlalchemy/dialects/firebird/base.py
+++ b/lib/sqlalchemy/dialects/firebird/base.py
@@ -717,15 +717,3 @@ class FBDialect(default.DefaultDialect):
 
         return list(indexes.values())
 
-    def do_execute(self, cursor, statement, parameters, context=None):
-        # kinterbase does not accept a None, but wants an empty list
-        # when there are no arguments.
-        cursor.execute(statement, parameters or [])
-
-    def do_rollback(self, dbapi_connection):
-        # Use the retaining feature, that keeps the transaction going
-        dbapi_connection.rollback(True)
-
-    def do_commit(self, dbapi_connection):
-        # Use the retaining feature, that keeps the transaction going
-        dbapi_connection.commit(True)
diff --git a/lib/sqlalchemy/dialects/firebird/fdb.py b/lib/sqlalchemy/dialects/firebird/fdb.py
index 8d0bd3d78..36b424d49 100644
--- a/lib/sqlalchemy/dialects/firebird/fdb.py
+++ b/lib/sqlalchemy/dialects/firebird/fdb.py
@@ -19,7 +19,49 @@
        under the ``firebird://`` URL space, as ``fdb`` is now the official
        Python driver for Firebird.
 
-The dialect currently accepts the same arguments as the Kinterbasdb driver.
+Arguments
+----------
+
+The ``fdb`` dialect is based on the :mod:`sqlalchemy.dialects.firebird.kinterbasdb`
+dialect, however does not accept every argument that Kinterbasdb does.
+
+* ``enable_rowcount`` - True by default, setting this to False disables
+  the usage of "cursor.rowcount" with the
+  Kinterbasdb dialect, which SQLAlchemy ordinarily calls upon automatically
+  after any UPDATE or DELETE statement.   When disabled, SQLAlchemy's
+  ResultProxy will return -1 for result.rowcount.   The rationale here is
+  that Kinterbasdb requires a second round trip to the database when
+  .rowcount is called -  since SQLA's resultproxy automatically closes
+  the cursor after a non-result-returning statement, rowcount must be
+  called, if at all, before the result object is returned.   Additionally,
+  cursor.rowcount may not return correct results with older versions
+  of Firebird, and setting this flag to False will also cause the
+  SQLAlchemy ORM to ignore its usage. The behavior can also be controlled on a
+  per-execution basis using the ``enable_rowcount`` option with
+  :meth:`.Connection.execution_options`::
+
+      conn = engine.connect().execution_options(enable_rowcount=True)
+      r = conn.execute(stmt)
+      print r.rowcount
+
+* ``retaining`` - False by default.   Setting this to True will pass the
+  ``retaining=True`` keyword argument to the ``.commit()`` and ``.rollback()``
+  methods of the DBAPI connection, which can improve performance in some
+  situations, but apparently with significant caveats.
+  Please read the fdb and/or kinterbasdb DBAPI documentation in order to
+  understand the implications of this flag.
+
+  .. versionadded:: 0.8.2 - ``retaining`` keyword argument specifying
+     transaction retaining behavior - in 0.8 it defaults to ``True``
+     for backwards compatibility.
+
+  .. versionchanged:: 0.9.0 - the ``retaining`` flag defaults to ``False``.
+     In 0.8 it defaulted to ``True``.
+
+  .. seealso::
+
+    http://pythonhosted.org/fdb/usage-guide.html#retaining-transactions - information
+    on the "retaining" flag.
 
 """
 
@@ -29,6 +71,12 @@ from ... import util
 
 class FBDialect_fdb(FBDialect_kinterbasdb):
 
+    def __init__(self, enable_rowcount=True,
+                            retaining=False, **kwargs):
+        super(FBDialect_fdb, self).__init__(
+                            enable_rowcount=enable_rowcount,
+                            retaining=retaining, **kwargs)
+
     @classmethod
     def dbapi(cls):
         return  __import__('fdb')
diff --git a/lib/sqlalchemy/dialects/firebird/kinterbasdb.py b/lib/sqlalchemy/dialects/firebird/kinterbasdb.py
index d581f799a..c8d8e986f 100644
--- a/lib/sqlalchemy/dialects/firebird/kinterbasdb.py
+++ b/lib/sqlalchemy/dialects/firebird/kinterbasdb.py
@@ -14,38 +14,26 @@
 Arguments
 ----------
 
-Kinterbasedb backend specific keyword arguments are:
-
-* type_conv - select the kind of mapping done on the types: by default
-  SQLAlchemy uses 200 with Unicode, datetime and decimal support (see
-  details__).
-
-* concurrency_level - set the backend policy with regards to threading
-  issues: by default SQLAlchemy uses policy 1 (see details__).
-
-* enable_rowcount - True by default, setting this to False disables
-  the usage of "cursor.rowcount" with the
-  Kinterbasdb dialect, which SQLAlchemy ordinarily calls upon automatically
-  after any UPDATE or DELETE statement.   When disabled, SQLAlchemy's
-  ResultProxy will return -1 for result.rowcount.   The rationale here is
-  that Kinterbasdb requires a second round trip to the database when
-  .rowcount is called -  since SQLA's resultproxy automatically closes
-  the cursor after a non-result-returning statement, rowcount must be
-  called, if at all, before the result object is returned.   Additionally,
-  cursor.rowcount may not return correct results with older versions
-  of Firebird, and setting this flag to False will also cause the
-  SQLAlchemy ORM to ignore its usage. The behavior can also be controlled on a
-  per-execution basis using the `enable_rowcount` option with
-  :meth:`execution_options()`::
-
-      conn = engine.connect().execution_options(enable_rowcount=True)
-      r = conn.execute(stmt)
-      print r.rowcount
-
-__ http://sourceforge.net/projects/kinterbasdb
-__ http://firebirdsql.org/index.php?op=devel&sub=python
-__ http://kinterbasdb.sourceforge.net/dist_docs/usage.html#adv_param_conv_dynamic_type_translation
-__ http://kinterbasdb.sourceforge.net/dist_docs/usage.html#special_issue_concurrency
+The Kinterbasdb backend accepts the ``enable_rowcount`` and ``retaining``
+arguments accepted by the :mod:`sqlalchemy.dialects.firebird.fdb` dialect.   In addition, it
+also accepts the following:
+
+* ``type_conv`` - select the kind of mapping done on the types: by default
+  SQLAlchemy uses 200 with Unicode, datetime and decimal support.  See
+  the linked documents below for further information.
+
+* ``concurrency_level`` - set the backend policy with regards to threading
+  issues: by default SQLAlchemy uses policy 1.  See the linked documents
+  below for futher information.
+
+.. seealso::
+
+    http://sourceforge.net/projects/kinterbasdb
+
+    http://kinterbasdb.sourceforge.net/dist_docs/usage.html#adv_param_conv_dynamic_type_translation
+
+    http://kinterbasdb.sourceforge.net/dist_docs/usage.html#special_issue_concurrency
+
 """
 
 from .base import FBDialect, FBExecutionContext
@@ -91,11 +79,13 @@ class FBDialect_kinterbasdb(FBDialect):
     )
 
     def __init__(self, type_conv=200, concurrency_level=1,
-                            enable_rowcount=True, **kwargs):
+                            enable_rowcount=True,
+                            retaining=False, **kwargs):
         super(FBDialect_kinterbasdb, self).__init__(**kwargs)
         self.enable_rowcount = enable_rowcount
         self.type_conv = type_conv
         self.concurrency_level = concurrency_level
+        self.retaining = retaining
         if enable_rowcount:
             self.supports_sane_rowcount = True
 
@@ -103,6 +93,17 @@ class FBDialect_kinterbasdb(FBDialect):
     def dbapi(cls):
         return __import__('kinterbasdb')
 
+    def do_execute(self, cursor, statement, parameters, context=None):
+        # kinterbase does not accept a None, but wants an empty list
+        # when there are no arguments.
+        cursor.execute(statement, parameters or [])
+
+    def do_rollback(self, dbapi_connection):
+        dbapi_connection.rollback(self.retaining)
+
+    def do_commit(self, dbapi_connection):
+        dbapi_connection.commit(self.retaining)
+
     def create_connect_args(self, url):
         opts = url.translate_connect_args(username='user')
         if opts.get('port'):
diff --git a/test/dialect/test_firebird.py b/test/dialect/test_firebird.py
index 6019dc8f9..4a71b7d05 100644
--- a/test/dialect/test_firebird.py
+++ b/test/dialect/test_firebird.py
@@ -444,3 +444,76 @@ class MiscTest(fixtures.TestBase):
             (text("select 'hello % world' from rdb$database"),
              'hello % world'):
             eq_(testing.db.scalar(expr), result)
+
+from sqlalchemy.testing.mock import Mock, call
+
+
+class ArgumentTest(fixtures.TestBase):
+    def _dbapi(self):
+        return Mock(
+                    paramstyle='qmark',
+                    connect=Mock(
+                            return_value=Mock(
+                                server_version="UI-V6.3.2.18118 Firebird 2.1",
+                                cursor=Mock(return_value=Mock())
+                            )
+                    )
+                )
+
+    def _engine(self, type_, **kw):
+        dbapi = self._dbapi()
+        kw.update(
+            dict(
+                    module=dbapi,
+                    _initialize=False
+            )
+        )
+        engine = engines.testing_engine("firebird+%s://" % type_,
+                                options=kw)
+        return engine
+
+    def test_retaining_flag_default_kinterbasdb(self):
+        engine = self._engine("kinterbasdb")
+        self._assert_retaining(engine, False)
+
+    def test_retaining_flag_true_kinterbasdb(self):
+        engine = self._engine("kinterbasdb", retaining=True)
+        self._assert_retaining(engine, True)
+
+    def test_retaining_flag_false_kinterbasdb(self):
+        engine = self._engine("kinterbasdb", retaining=False)
+        self._assert_retaining(engine, False)
+
+    def test_retaining_flag_default_fdb(self):
+        engine = self._engine("fdb")
+        self._assert_retaining(engine, False)
+
+    def test_retaining_flag_true_fdb(self):
+        engine = self._engine("fdb", retaining=True)
+        self._assert_retaining(engine, True)
+
+    def test_retaining_flag_false_fdb(self):
+        engine = self._engine("fdb", retaining=False)
+        self._assert_retaining(engine, False)
+
+
+    def _assert_retaining(self, engine, flag):
+        conn = engine.connect()
+        trans = conn.begin()
+        trans.commit()
+        eq_(
+            engine.dialect.dbapi.connect.return_value.commit.mock_calls,
+            [call(flag)]
+        )
+
+        trans = conn.begin()
+        trans.rollback()
+        eq_(
+            engine.dialect.dbapi.connect.return_value.rollback.mock_calls,
+            [call(flag)]
+        )
+
+
+
+
+