Code

Opened 8 months ago

Closed 6 months ago

#21453 closed Cleanup/optimization (fixed)

Set autocommit mode before calling init_connection_state

Reported by: aaugustin Owned by: aaugustin
Component: Database layer (models, ORM) Version: 1.6
Severity: Normal Keywords:
Cc: w2lkm2n@… Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

I just helped an user on IRC who attempted a fairly reasonable database backend customization:

from django.db.backends.postgresql_psycopg2 import base
     
class DatabaseWrapper(base.DatabaseWrapper):
    def init_connection_state(self):
        super(DatabaseWrapper, self).init_connection_state()
        self.connection.cursor().execute('SET search_path TO something')

Unfortunately that fails with an ugly ProgrammingError: autocommit cannot be used inside a transaction and the traceback is very confusing.

The root cause of this problem is that the code above starts a transaction, and when Django later tries to switch to autocommit, PostgreSQL complains.

Would it make sense to switch to autocommit before calling init_connection_state? This is just a matter of swapping two lines in BaseDatabaseWrapper.connect, and reviewing carefully the code in init_connection_state to ensure it still works as expected. (Hint: it doesn't, see #21452.)

Currently init_connection_state executes in PEP-249 mode (= implicit transactions = no autocommit), while everything else executes in autocommit mode (unless you set AUTOCOMMIT to False in the database settings, but I'm really mentioning that for the sake of completeness, I don't expect anyone to do this). That's quite surprising and probably a good reason for making this change. Of course, there may be even better reasons for NOT making this change that I haven't thought of.

Attachments (0)

Change History (8)

comment:1 Changed 8 months ago by Walkman

  • Cc w2lkm2n@… added

comment:2 Changed 8 months ago by akaariai

I think we should handle this in #21452. The short version is that after init_connection_state autocommit will always be off, but it does get re-enabled in BaseDatabaseWrapper.connect(). More details in #21452.

comment:3 Changed 8 months ago by aaugustin

I'd rather do it the other way round: decide whether it makes most sense to call init_connection_state in PEP-249 mode or in the mode selected in the database settings, and then write a suitable patch for #21452.

I'm afraid init_connection_state has to be aware of transactions anyway. For instance, for setting the time zone as discussed in #21452:

  • in PEP-249 mode, we must either switch to autocommit temporarily or commit after setting the time zone (the latter seems more straightforward, why don't we do it that way currently?)
  • in the mode selected in the database settings, if that mode is autocommit, we're fine, if it isn't, we have to do the same as above.

Switching makes it easier to override init_connection_state and make it work in autocommit, but harder when autocommit is disabled. Given that very very few people will disable autocommit, it could still be a good tradeoff.

comment:4 Changed 8 months ago by akaariai

Maybe we could even go for always run init_connection_state in autocommit mode, switch to settings mode after init_connection_state is done? This way init_connection_state doesn't need to be aware of transactions.

As for why wasn't commit used for timezone set, I don't recall if that was ever tried.

comment:5 Changed 8 months ago by akaariai

  • Triage Stage changed from Unreviewed to Accepted

Marking this as accepted - this seems like a good idea to me.

comment:6 Changed 6 months ago by aaugustin

  • Owner changed from nobody to aaugustin
  • Status changed from new to assigned

Finally I handled #21452 first because I wanted to backport it to 1.6.

comment:7 Changed 6 months ago by aaugustin

Being aware of transactions means ending init_connection_state with:

        # Ensure all changes are preserved even when AUTOCOMMIT is False.
        if not self.get_autocommit():
            self.commit()

I think that's fine.

I'm going to run init_connection_state in the mode prescribed by the settings because it's the most reasonable expectation.

comment:8 Changed 6 months ago by Aymeric Augustin <aymeric.augustin@…>

  • Resolution set to fixed
  • Status changed from assigned to closed

In fbe1abac4af3a7fc138bd176471e36acb1070a58:

Fixed #21453 -- Enabled autocommit before calling init_connection_state.

Also ensured the transaction state is clean on Oracle while I was there.

This change cannot be backported to 1.6 because it's
backwards-incompatible for custom database backends.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.