Opened 4 years ago

Closed 4 years ago

#32527 closed Bug (duplicate)

needs_rollback flag issue with implementation of backend that does not support savepoint

Reported by: Hemant Bhanawat Owned by: nobody
Component: Database layer (models, ORM) Version: dev
Severity: Normal Keywords: savepoint needs_rollback database backend
Cc: Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by Mariusz Felisiak)

I am implementing a backend for Yugabyte. Yugabyte currently doesn't support savepoints. Hence, in the backend, I have set uses_savepoints= False

However, while running the test. I hit the following issue.

The error: "TransactionManagementError: An error occurred in the current transaction. You can't execute queries until the end of the 'atomic' block."

On some investigation, I found that this exception is thrown if self.needs_rollback is True. The first test that uses savepoint passes successfully. However, the second test has needs_rollback set as true and hits this exception.

The problem stems from the following code of enter function in transaction.py

            if self.savepoint and not connection.needs_rollback:
                sid = connection.savepoint()
                connection.savepoint_ids.append(sid)
            else:
                connection.savepoint_ids.append(None)

Basically, a None is inserted in the savepoint_ids when the savepoints are not supported. And the following code in exit function of transaction.py, it sets needs_rollback as true:

                    if sid is None:
                        connection.needs_rollback = True

Can someone please clarify if this is an issue with the code or there is something else that we can do at our layer. For the time being, we have overridden the function savepoint of DatabaseWrapper to return 1. And, we ask the users of our backend to not set savepoint=true in parameters of transaction.atomic().

    def savepoint(self):
             return 1 

Change History (6)

comment:1 by Mariusz Felisiak, 4 years ago

Description: modified (diff)
Resolution: needsinfo
Status: newclosed

Thanks for this report, however I'm not able to confirm (with the given details) that it is an issue in Django. As far as I'm aware it should work properly. In Django < 2.0 we supported SQLite < 3.7.5 with the same configuration (see 27193aea0088b238e3ee0f0f235364a34a09265c) and it worked fine. You can try to create a topic on the https://forum.djangoproject.com or a discussion on DevelopersMailingList.

Feel-free to reopen the ticket when you can provide more details and prove it's an issue in Django.

comment:2 by Hemant Bhanawat, 4 years ago

Resolution: needsinfo
Status: closednew

Before Django 2.0, sqlite had different handling.

if not connection.get_autocommit():
                # Some database adapters (namely sqlite3) don't handle
                # transactions and savepoints properly when autocommit is off.
                # Turning autocommit back on isn't an option; it would trigger
                # a premature commit. Give up if that happens.
                if connection.features.autocommits_when_autocommit_is_off:
                    raise TransactionManagementError(
                        "Your database backend doesn't behave properly when "
                        "autocommit is off. Turn it on before using 'atomic'.")

Please create a dummy backend with the following two settings or may be try to change an existing one and add these two settings and you will hit this issue.

    uses_savepoints= False
    can_release_savepoints = False
Last edited 4 years ago by Hemant Bhanawat (previous) (diff)

comment:3 by Mariusz Felisiak, 4 years ago

Resolution: needsinfo
Status: newclosed

You didn't mention autocommit before, I have a feeling that more bits are missing here. Can you provide a small project illustrating this issue?

comment:4 by Hemant Bhanawat, 4 years ago

On master branch of django, apply this patch:

diff --git a/django/db/backends/postgresql/features.py b/django/db/backends/postgresql/features.py
index 722bfe0475..7365b57b54 100644
--- a/django/db/backends/postgresql/features.py
+++ b/django/db/backends/postgresql/features.py
@@ -6,6 +6,8 @@ from django.utils.functional import cached_property
 
 
 class DatabaseFeatures(BaseDatabaseFeatures):
+    uses_savepoints= False
+    can_release_savepoints = False
     allows_group_by_selected_pks = True
     can_return_columns_from_insert = True
     can_return_rows_from_bulk_insert = True

The run the following command to run the tests:

./runtests.py --settings=postgres_settings  -v 3  admin_changelist  --failfast

The second test will fail because needs_rollback is set as true.

Version 1, edited 4 years ago by Hemant Bhanawat (previous) (next) (diff)

comment:5 by Hemant Bhanawat, 4 years ago

Resolution: needsinfo
Status: closednew

comment:6 by Tim Graham, 4 years ago

Resolution: duplicate
Status: newclosed

Duplicate of #28263.

Note: See TracTickets for help on using tickets.
Back to Top