Code

Opened 18 months ago

Closed 17 months ago

Last modified 17 months ago

#19274 closed Cleanup/optimization (fixed)

Separate DB connection creation and session state initialization

Reported by: akaariai Owned by: nobody
Component: Database layer (models, ORM) Version: master
Severity: Normal Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Currently when creating a new connection we create the connection and initialize it in one go (in _cursor()). Even if a backend splits getting a new connection and initializing the session variable this is not done consistently between backends.

The reason for this split is that this gives nice access point for external pooling implementations. A pool can wrap the backend and the methods "get_new_connection()" and "close()". Instead of actually creating new connections and closing the connection we can just get a connection from the pool and initialize the state of it. The real backend doesn't need to know anything about the pool wrapper.

The pool wrapper implementation would be something like this:

def get_new_connection():
    if pooled_connections_available():
        return connection from pool
    else:
        return super().get_new_connection()

def init_connection():
    do possible pool specific initialization
    super().init_connection()

def close():
    put the connection back to pool

I think this would allow connection pools for Django but with no need to implement them in core. In addition this should add readability of ._cursor() implementations.

Attachments (0)

Change History (5)

comment:1 Changed 18 months ago by Alex

  • Triage Stage changed from Unreviewed to Accepted

Sounds good to me.

comment:2 Changed 17 months ago by akaariai

I have now a working implementation in: https://github.com/akaariai/django/compare/ticket_19274

The work divides into three parts:

  • Backend conversion as mentioned in the ticket description
  • Removing the assumption that connection's self.settings_dict changes will be reflected in the global settings
  • Random test fixed (one Oracle specific fix, some fixes into how to create additional connections for testing and some fixes to accessing the connection.connection variable)

After the fixes the work in https://github.com/akaariai/django_pooled/ passes on all core backends. The pooling implementation in django_pooled is somewhere between ugly and hideous. What is interesting is that django_pooled shows that one can have generic pooling implementation outside core, and that it actually works.

The separation of self.settings_dict from global settings is needed for the above pooling implementation (and I believe any pooling implementation wants that). The basic problem is that you need to tell Django to use the pool in ENGINE, and then you need to have the real engine somewhere (OPTIONS -> WRAPS in django_pooled). You need to alter the settings before passing to the real backend, but you can't do that to the global settings, otherwise the information is lost for next connection creation.

Gis backends aren't converted yet.

comment:3 Changed 17 months ago by akaariai

  • Has patch set
  • Triage Stage changed from Accepted to Ready for checkin

I have update the patch series at https://github.com/akaariai/django/compare/ticket_19274

Gis isn't yet updated, though it seems the only change needed is in spatialite. In addition I would like to remove some checks done against the ENGINE string, this isn't nice for subclassers.

The full test suite does pass using https://github.com/akaariai/django_pooled/. The implementation in django_pooled isn't meant for any kind of production use, but it does prove that creating a connection pool outside core is possible.

I am planning to push the patch series as-is. The changes are mostly mechanical - moving code from ._cursor() to the new methods. Still, it would be great if somebody had the time to double-check my changes, I am pretty good at making stupid little mistakes in this sort of thing...

comment:4 Changed 17 months ago by Anssi Kääriäinen <akaariai@…>

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

In 1893467784deb6cd8a493997e8bac933cc2e4af9:

Fixed #19274 -- Made db connection creation overridable in subclasses

Connection creation was done in db backend ._cursor() call. This
included taking a new connection if needed, initializing the session
state for the new connection and finally creating the connection.

To allow easier modifying of these steps in subclasses (for example to
support connection pools) the _cursor() now calls get_new_connection()
and init_connection_state() if there isn't an existing connection. This
was done for all non-gis core backends. In addition the parameters used
for taking a connection are now created by get_connection_params().

We should also do the same for gis backends and encourage 3rd party
backends to use the same pattern. The pattern is not enforced in code,
and as the backends are private API this will not be required by
documentation either.

comment:5 Changed 17 months ago by Anssi Kääriäinen <akaariai@…>

In 86644e065fbde410aa32e052e206d1c53a916fd3:

Refactored gis/spatialite connection initialization

The connection state is now initialized in get_new_connection().
Refs #19274.

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.