Fixed #23074 -- Avoided leaking savepoints in atomic.

Thanks Chow Loong Jin for the report and the initial patch.

6 files changed, 35 insertions, 0 deletions

diff --git a/django/db/backends/__init__.py b/django/db/backends/__init__.py
index 9e223c9cbb..5577d3b46e 100644
--- a/django/db/backends/__init__.py
+++ b/django/db/backends/__init__.py
@@ -497,6 +497,7 @@ class BaseDatabaseFeatures(object):
     can_return_id_from_insert = False
     has_bulk_insert = False
     uses_savepoints = False
+    can_release_savepoints = False
     can_combine_inserts_with_and_without_auto_increment_pk = False
 
     # If True, don't use integer foreign keys referring to, e.g., positive
diff --git a/django/db/backends/mysql/base.py b/django/db/backends/mysql/base.py
index 77369dcd8b..5aa17466ac 100644
--- a/django/db/backends/mysql/base.py
+++ b/django/db/backends/mysql/base.py
@@ -182,6 +182,7 @@ class DatabaseFeatures(BaseDatabaseFeatures):
     requires_explicit_null_ordering_when_grouping = True
     allows_auto_pk_0 = False
     uses_savepoints = True
+    can_release_savepoints = True
     atomic_transactions = False
     supports_column_check_constraints = False
 
diff --git a/django/db/backends/postgresql_psycopg2/base.py b/django/db/backends/postgresql_psycopg2/base.py
index 7b2cdc0aa2..4cd327a22b 100644
--- a/django/db/backends/postgresql_psycopg2/base.py
+++ b/django/db/backends/postgresql_psycopg2/base.py
@@ -50,6 +50,7 @@ class DatabaseFeatures(BaseDatabaseFeatures):
     has_select_for_update_nowait = True
     has_bulk_insert = True
     uses_savepoints = True
+    can_release_savepoints = True
     supports_tablespaces = True
     supports_transactions = True
     can_introspect_ip_address_field = True
diff --git a/django/db/backends/sqlite3/base.py b/django/db/backends/sqlite3/base.py
index 551c60759f..3b61a8a1e5 100644
--- a/django/db/backends/sqlite3/base.py
+++ b/django/db/backends/sqlite3/base.py
@@ -119,6 +119,10 @@ class DatabaseFeatures(BaseDatabaseFeatures):
         return Database.sqlite_version_info >= (3, 6, 8)
 
     @cached_property
+    def can_release_savepoints(self):
+        return self.uses_savepoints
+
+    @cached_property
     def supports_stddev(self):
         """Confirm support for STDDEV and related stats functions
 
diff --git a/django/db/transaction.py b/django/db/transaction.py
index 786bc53581..4fb58d6dd8 100644
--- a/django/db/transaction.py
+++ b/django/db/transaction.py
@@ -219,6 +219,9 @@ class Atomic(object):
                         except DatabaseError:
                             try:
                                 connection.savepoint_rollback(sid)
+                                # The savepoint won't be reused. Release it to
+                                # minimize overhead for the database server.
+                                connection.savepoint_commit(sid)
                             except Error:
                                 # If rolling back to a savepoint fails, mark for
                                 # rollback at a higher level and avoid shadowing
@@ -249,6 +252,9 @@ class Atomic(object):
                     else:
                         try:
                             connection.savepoint_rollback(sid)
+                            # The savepoint won't be reused. Release it to
+                            # minimize overhead for the database server.
+                            connection.savepoint_commit(sid)
                         except Error:
                             # If rolling back to a savepoint fails, mark for
                             # rollback at a higher level and avoid shadowing
diff --git a/tests/transactions/tests.py b/tests/transactions/tests.py
index 72e600d41d..005df0ba91 100644
--- a/tests/transactions/tests.py
+++ b/tests/transactions/tests.py
@@ -403,3 +403,25 @@ class AtomicMiscTests(TransactionTestCase):
                 pass
         # Must not raise an exception
         transaction.atomic(Callable())
+
+    @skipUnlessDBFeature('can_release_savepoints')
+    def test_atomic_does_not_leak_savepoints_on_failure(self):
+        # Regression test for #23074
+
+        # Expect an error when rolling back a savepoint that doesn't exist.
+        # Done outside of the transaction block to ensure proper recovery.
+        with self.assertRaises(Error):
+
+            # Start a plain transaction.
+            with transaction.atomic():
+
+                # Swallow the intentional error raised in the sub-transaction.
+                with six.assertRaisesRegex(self, Exception, "Oops"):
+
+                    # Start a sub-transaction with a savepoint.
+                    with transaction.atomic():
+                        sid = connection.savepoint_ids[-1]
+                        raise Exception("Oops")
+
+                # This is expected to fail because the savepoint no longer exists.
+                connection.savepoint_rollback(sid)