Code

Opened 8 years ago

Closed 3 years ago

#1946 closed New feature (duplicate)

Allow overriding of default sequence name

Reported by: abuse@… Owned by: shmengie
Component: Database layer (models, ORM) Version:
Severity: Normal Keywords:
Cc: mmoedt@…, nirvdrum@…, treborhudson@…, ernst@…, botondus@…, maxirobaina@… Triage Stage: Accepted
Has patch: yes Needs documentation: yes
Needs tests: yes Patch needs improvement: yes
Easy pickings: UI/UX:

Description

I wanted to be able to use Django's admin interface to edit PostgreSQL inherited tables. The normal use would be to have a single sequence on the parent table and let the children inherit that, so that the IDs across all children are unique. However, Django assumes that the sequence name is tablename_columnname_seq which does not apply in this case.

I have created the folloiwing patch to allow one to override the default sequence name with one of your choosing by adding a seq_name= parameter when defining the field, like this:

class PeerIAX(models.Model):
    class Admin: pass
    id = models.AutoField(primary_key=True, seq_name='asterisk_peer_id_seq')
    description = models.CharField(maxlength=255)
    hostname = models.CharField(maxlength=255)
    username = models.CharField(maxlength=255)
    secret = models.CharField(maxlength=255)
    def __str__(self):
	return description

In case it's not obvious, this simple patch does not add support to Django to actually *create* the tables. I hand-created them like so:

CREATE TABLE asterisk_peer (
	id		SERIAL		NOT NULL PRIMARY KEY,
	description	VARCHAR(255)	NOT NULL
);

CREATE TABLE asterisk_peer_iax (
	hostname	VARCHAR(255)	NOT NULL,
	username	VARCHAR(255)	NOT NULL,
	secret		VARCHAR(255)	NOT NULL
) INHERITS (asterisk_peer);

The patch is below. I abandon copyright on the code and you are welcome to incorporate the patch into Django as you see fit.

diff -rduP ../Django-0.91-py2.3.egg.orig/django/db/backends/ado_mssql/base.py ./django/db/backends/ado_mssql/base.py
--- ../Django-0.91-py2.3.egg.orig/django/db/backends/ado_mssql/base.py	2006-05-15 13:52:49.000000000 +0100
+++ ./django/db/backends/ado_mssql/base.py	2006-05-20 20:09:24.000000000 +0100
@@ -94,7 +94,9 @@
 dictfetchmany = util.dictfetchmany
 dictfetchall  = util.dictfetchall
 
-def get_last_insert_id(cursor, table_name, pk_name):
+def get_last_insert_id(cursor, table_name, pk_name, seq_name):
+    # FIXME: seq_name might be a useful thing to add to backends other than
+    # PostgreSQL
     cursor.execute("SELECT %s FROM %s WHERE %s = @@IDENTITY" % (pk_name, table_name, pk_name))
     return cursor.fetchone()[0]
 
diff -rduP ../Django-0.91-py2.3.egg.orig/django/db/backends/mysql/base.py ./django/db/backends/mysql/base.py
--- ../Django-0.91-py2.3.egg.orig/django/db/backends/mysql/base.py	2006-05-15 13:52:49.000000000 +0100
+++ ./django/db/backends/mysql/base.py	2006-05-20 20:10:01.000000000 +0100
@@ -117,7 +117,9 @@
 dictfetchmany = util.dictfetchmany
 dictfetchall  = util.dictfetchall
 
-def get_last_insert_id(cursor, table_name, pk_name):
+def get_last_insert_id(cursor, table_name, pk_name, seq_name):
+    # FIXME: seq_name might be a useful thing to add to backends other than
+    # PostgreSQL
     return cursor.lastrowid
 
 def get_date_extract_sql(lookup_type, table_name):
diff -rduP ../Django-0.91-py2.3.egg.orig/django/db/backends/postgresql/base.py ./django/db/backends/postgresql/base.py
--- ../Django-0.91-py2.3.egg.orig/django/db/backends/postgresql/base.py	2006-05-15 13:52:49.000000000 +0100
+++ ./django/db/backends/postgresql/base.py	2006-05-20 19:40:12.000000000 +0100
@@ -75,8 +75,10 @@
     "Returns all rows from a cursor as a dict"
     return cursor.dictfetchall()
 
-def get_last_insert_id(cursor, table_name, pk_name):
-    cursor.execute("SELECT CURRVAL('\"%s_%s_seq\"')" % (table_name, pk_name))
+def get_last_insert_id(cursor, table_name, pk_name, seq_name):
+    if seq_name is None:
+        seq_name = "%s_%s_seq" % (table_name, pk_name)
+    cursor.execute("SELECT CURRVAL('\"%s\"')" % seq_name)
     return cursor.fetchone()[0]
 
 def get_date_extract_sql(lookup_type, table_name):
diff -rduP ../Django-0.91-py2.3.egg.orig/django/db/backends/sqlite3/base.py ./django/db/backends/sqlite3/base.py
--- ../Django-0.91-py2.3.egg.orig/django/db/backends/sqlite3/base.py	2006-05-15 13:52:49.000000000 +0100
+++ ./django/db/backends/sqlite3/base.py	2006-05-20 20:10:11.000000000 +0100
@@ -90,7 +90,9 @@
 dictfetchmany = util.dictfetchmany
 dictfetchall  = util.dictfetchall
 
-def get_last_insert_id(cursor, table_name, pk_name):
+def get_last_insert_id(cursor, table_name, pk_name, seq_name):
+    # FIXME: seq_name might be a useful thing to add to backends other than
+    # PostgreSQL
     return cursor.lastrowid
 
 def get_date_extract_sql(lookup_type, table_name):
diff -rduP ../Django-0.91-py2.3.egg.orig/django/db/models/base.py ./django/db/models/base.py
--- ../Django-0.91-py2.3.egg.orig/django/db/models/base.py	2006-05-15 13:52:49.000000000 +0100
+++ ./django/db/models/base.py	2006-05-20 19:55:52.000000000 +0100
@@ -187,7 +187,7 @@
                 (backend.quote_name(self._meta.db_table), ','.join(field_names),
                 ','.join(placeholders)), db_values)
             if self._meta.has_auto_field and not pk_set:
-                setattr(self, self._meta.pk.attname, backend.get_last_insert_id(cursor, self._meta.db_table, self._meta.pk.column))
+                setattr(self, self._meta.pk.attname, backend.get_last_insert_id(cursor, self._meta.db_table, self._meta.pk.column, self._meta.pk.seq_name))
         transaction.commit_unless_managed()
 
         # Run any post-save hooks.
diff -rduP ../Django-0.91-py2.3.egg.orig/django/db/models/fields/__init__.py ./django/db/models/fields/__init__.py
--- ../Django-0.91-py2.3.egg.orig/django/db/models/fields/__init__.py	2006-05-15 13:52:49.000000000 +0100
+++ ./django/db/models/fields/__init__.py	2006-05-20 19:53:28.000000000 +0100
@@ -68,7 +68,7 @@
         core=False, rel=None, default=NOT_PROVIDED, editable=True,
         prepopulate_from=None, unique_for_date=None, unique_for_month=None,
         unique_for_year=None, validator_list=None, choices=None, radio_admin=None,
-        help_text='', db_column=None):
+        help_text='', db_column=None,seq_name=None):
         self.name = name
         self.verbose_name = verbose_name
         self.primary_key = primary_key
@@ -84,6 +84,7 @@
         self.radio_admin = radio_admin
         self.help_text = help_text
         self.db_column = db_column
+        self.seq_name = seq_name
 
         # Set db_index to True if the field has a relationship and doesn't explicitly set db_index.
         self.db_index = db_index

Attachments (2)

source-patch (4.3 KB) - added by hanne.moa@… 6 years ago.
Custom sequence-name patch for trunk (v7156)
test-patch (2.2 KB) - added by hanne.moa@… 6 years ago.
Tests for patch (or a good start anyway)

Download all attachments as: .zip

Change History (25)

comment:1 Changed 8 years ago by nirvdrum

Another use case is integrating with legacy databases. I just found out I can't really use auto-increment IDs with django until something like this patch gets integrated because the legacy sequence names are not what django expects.

comment:2 Changed 7 years ago by adrian

  • Summary changed from [PATCH] to allow overriding of default sequence name to [patch] to allow overriding of default sequence name

comment:3 Changed 7 years ago by anonymous

  • Cc nirvdrum@… added

comment:4 Changed 7 years ago by SmileyChris

  • Triage Stage changed from Unreviewed to Design decision needed

comment:5 Changed 7 years ago by mtredinnick

  • Needs documentation set
  • Needs tests set
  • Patch needs improvement set
  • Triage Stage changed from Design decision needed to Accepted

I'm a pretty big fan of helping out legacy database support where we can do it without too much trouble (not so sold on the initial reasons in the ticket, but if it works for that, too... great). I think the idea is not unreasonable, although the patch needs some work. A couple of things that jump out at me:

  1. get_last_insert_id() should take seq_name=None as a default parameter, rather than hoping the callers will know that None is the "do nothing" default. That also makes it more backwards compatible if anybody is already calling that function from other places.
  2. django/core/management.py needs to be fixed to create the sequence correctly. We don't want something that works except for the fact that you have to manually create it: that would be unusual behaviour for Django.
  3. I don't like the "FIXME" comment that has "maybe" in it: sounds like putting a design query in the code. We just document the seq_name attribute as being only applicable to PostgreSQL for now (and add the others to the docs if/when they are implemented).

This should be testable and needs a docs patch as well.

comment:6 Changed 7 years ago by nirvdrum@…

I'm willing to step up to the plate with regards to the docs patch. I'm not sure I have the requisite knowledge to provide an adequate patch for testing. If no one else can step up to the plate, I'll try to, but no guarantees I'll get anywhere with it.

comment:7 Changed 6 years ago by hanne.moa@…

Another case of global sequence in an existing db here so I also need this (and it is a showstopper). Where exactly should this be in the documentation, since it is (currently) only needed for postgres? It might be *useful* for other dbs also, but...

Changed 6 years ago by hanne.moa@…

Custom sequence-name patch for trunk (v7156)

Changed 6 years ago by hanne.moa@…

Tests for patch (or a good start anyway)

comment:8 Changed 6 years ago by hanne.moa@…

I noticed that the oracle backend currently uses get_sequence_name() in autoinc_sql() as well as changing last_insert_id(), this might in fact be a better solution as it is possible to ask postgres which sequence a field uses, which is a lot more DRY. Might be slower, though. A combined solution would be to set/cache a sequence_name at instantiation of the model then use that all other places, making adding an attribute to AutoField's init unneccessary. Things would just work and no new docs would be needed... The test I attached is still relevant though.

Get sequence-name from postgres:

<database>=# SELECT * FROM pg_get_serial_sequence('<tablename>', '<fieldname>');
 pg_get_serial_sequence
------------------------
 public.<sequence_name>
(1 row)

Unfortunately, in the case of inherited tables (see Table-inheritance in postgresql)you need to check the parent-table, not the child. This is worth testing for if the above sql returns NULL.

comment:9 follow-up: Changed 6 years ago by anonymous

I subclassed a database-backend while waiting for a solution to this ticket to wind up in trunk, as according to http://www.djangoproject.com/documentation/settings/#database-engine this should be possible:

In the Django development version, you can use a database backend that doesn’t 
ship with Django by setting DATABASE_ENGINE to a fully-qualified path (i.e. 
mypackage.backends.whatever). Writing a whole new database backend from
scratch is left as an exercise to the reader; see the other backends for examples.

However as of trunk v. 7167, this is not possible, failing with:

  ..
  File "/usr/lib/python2.5/site-packages/django/db/__init__.py", line 32, in <module>
    (settings.DATABASE_ENGINE, ", ".join(map(repr, available_backends)))
django.core.exceptions.ImproperlyConfigured: 'extra.backends.psycopg2_sequencesafe' isn't 
an available database backend. Available options are: 'ado_mssql', 'dummy', 'mysql', 
'mysql_old', 'oracle', 'postgresql', 'postgresql_psycopg2', 'sqlite3'

comment:10 in reply to: ↑ 9 Changed 6 years ago by ramiro

Replying to anonymous:

I subclassed a database-backend while waiting for a solution to this ticket to wind up in trunk, as according to http://www.djangoproject.com/documentation/settings/#database-engine this should be possible:

Make sure you are really doing what the documentations says, see http://code.djangoproject.com/ticket/6676#comment:1. Please don't reply here, take it to django-users if you need further help.

comment:11 Changed 5 years ago by robhudson

  • Cc treborhudson@… added

comment:12 Changed 5 years ago by mmoedt

  • Cc mmoedt@… added

comment:13 Changed 5 years ago by mtredinnick

For anybody working to bring this to conclusion, a few notes from some discussions Rob Hudson and I have had about this:

There are three separate sub-problems that probably need to be addressed.

  1. Manually specifying the sequence name to use.
  2. Specifying that ModelA should use the same sequence name as ModelB
  3. Perhaps: somehow creating the sequence for case 1.

It's not necessarily compulsory that all are solved, and the original problem description only looks at situation 1, from what I understand. The difficulty there is when does this sequence we are referring to by name get created? It can't be as part of "initial SQL" processing or post_syncdb signal handling, since that's after the table has been created and, presumably, the sequence has to exist before you can refer to it in a table definition. Maybe that's completely out of scope (quite possibly). If not, it's situation 3. Solving that certainly makes testing a lot easier (might well move it from "impossible" into the "possible" category).

The second point, above, seems to be something that might crop up quite naturally here. If you want to share a sequence for some reason, you need to indicate that, but you don't actually know the name of the sequence. Again, maybe out of scope, although it would be nice if it were possible.

comment:14 Changed 5 years ago by anonymous

Is there any chance this patch can be integrated into the core?

comment:15 Changed 5 years ago by 1st2be@…

I chose to take a slightly different approach to this issue, which seems a less intrusive alternative. It limited to one sequence per table, but I doubt that's an issue.

In db/models/options.py:
Added property 'sequence_name'

In postgresql/operations.py

def sequence_currval(self, cursor, sequence):

cursor.execute("SELECT CURRVAL('\"%s\"')" % (sequence,))
return cursor.fetchone()[0]

models/sql/InsertQuery.execute_sql() # last if before catch-all return

if self.model._meta.sequence_name:

return self.connection.ops.sequence_currval(

cursor, self.model._meta.sequence_name)

comment:16 Changed 5 years ago by HM

I wound up making my own postgres-backend (based on psycopg2) that asks postgres itself what the sequence is, recursively:

# introspection.py

from django.db.backends.postgresql_psycopg2.base import DatabaseOperations
from django.db.backends.postgresql_psycopg2.introspection import *
from psycopg2 import ProgrammingError

def quote_name(name):
    if name.startswith('"') and name.endswith('"'):
        return name # Quoting once is enough.
    return '"%s"' % name

def _get_direct_sequence_name(cursor, table_name, pk_name):
    "Fetch the sequence name of an auto-incrementing field, if any"
    table_name = quote_name(table_name)
    cursor.execute('''SELECT * FROM pg_get_serial_sequence(%s, %s);''', (table_name, pk_name))
    return cursor.fetchone()[0]

def get_sequence_name(cursor, table_name, pk_name, level=1):
    "Fetch the sequence name of an auto-incrementing field, if any, also tries parents"
    try:
        seq_name = _get_direct_sequence_name(cursor, table_name, pk_name)
        if not seq_name:
            raise ProgrammingError, "Table '%s' had no direct sequence-name" % table_name
        return seq_name
    except ProgrammingError, e:
        cursor.execute('SELECT parent.relname '
            'FROM pg_inherits i '
            'INNER JOIN pg_class parent ON parent.oid = i.inhparent '
            'INNER JOIN pg_class child ON child.oid = i.inhrelid '
            'WHERE child.relname = %s ORDER BY i.inhseqno;', (table_name,))
        if cursor.rowcount < 1 or cursor.rowcount is None:
            raise ProgrammingError, 'No sequence found for pk %s of table %s or parents of table %s' % (pk_name, table_name, table_name)
        # This is safe as the number of rows returned usually will be very few
        rows = [str(row[0]) for row in cursor.fetchall()]
        for parent in rows:
            seq_name = _get_direct_sequence_name(cursor, parent, pk_name)
            if seq_name:
                return seq_name.split('.')[1]
        # XXX: level (recursion-depth) should be a setting?
        if level < 3:
            return get_sequence_name(cursor, parent, pk_name, level+1)
    return None

I think the below is the minimum needed in base.py:

# base.py

# lots of imports here, look at django.db.backends.postgresql_psycopg2.base

class DatabaseOperations(PostgresqlDatabaseOperations):

    def last_insert_id(self, cursor, table_name, pk_name):
        "Try fetching existing sequence, else generate as per standard django"
        seq_name = get_sequence_name(cursor, table_name, pk_name)
        if seq_name is None:
            seq_name = "%s_%s_seq" % (table_name, pk_name)
        cursor.execute("SELECT CURRVAL(%s)", (seq_name,))
        return cursor.fetchone()[0]

class DatabaseWrapper(BaseDatabaseWrapper):
    def __init__(self, *args, **kwargs):
        super(DatabaseWrapper, self).__init__(*args, **kwargs)
        self.ops = DatabaseOperations()

The annoyances with this solution is of course that it is slower than hardcoded sequence-names and that it has to be deployed with django, as a dir in django/db/backends/.

Now, a system that fetched the sequence-names during model-import and stored them in the models, that would maybe be the best solution (for a legacy database like what I'm working with anyway).

comment:17 Changed 5 years ago by shmengie

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

HM I like the elegance of your solution, but the potential performance repercussion will likely keep it out of the codebase.

The Meta.sequence_name = 'my_cust_seq' has minimal performance impact (one python if statement) per InsertQuery. Legacy integrators will not be concerned with table creation, but will appreciate being able to integrate their schema into django. The Meta.sequence_name would need to be documented.

Pros:

Eases legacy integration
Adds feature custom sequence_name ability

Cons:

Does not address sequence creation issues
minimal performance impact (more a pro than con)
minimal additional code in base




comment:18 Changed 5 years ago by ernst

  • Cc ernst@… added

comment:19 Changed 4 years ago by anonymous

  • Cc botondus@… added

comment:20 Changed 3 years ago by HM

Partially fixed by #8901.

comment:21 Changed 3 years ago by maxi

  • Cc maxirobaina@… added

comment:22 Changed 3 years ago by lrekucki

  • Severity changed from normal to Normal
  • Summary changed from [patch] to allow overriding of default sequence name to Allow overriding of default sequence name
  • Type changed from enhancement to New feature

comment:23 Changed 3 years ago by jacob

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

#13295 has a better patch; marking duplicate of that one.

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.