Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#28451 closed Bug (fixed)

Change in Oracle sequence name truncation causes regression when updating existing database

Reported by: Kevin Grinberg Owned by: Kevin Grinberg
Component: Database layer (models, ORM) Version: 1.11
Severity: Release blocker Keywords: oracle
Cc: Mariusz Felisiak, Shai Berger, Kevin Grinberg Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no


Summary: a change introduced in 1.11 in how the Oracle backend truncates sequence names introduces persistent "ORA-02289: sequence does not exist" errors after upgrading to 1.11.

Explanation (as far best as I can tell): In the Oracle backend, sequence names are truncated to 30 characters.

In 1.10.7 (and 1.9.13 & 1.8.17) the method to do that in is as follows:

def _get_sequence_name(self, table):
    name_length = self.max_name_length() - 3
    return '%s_SQ' % truncate_name(table, name_length).upper()

In 1.11.3 it's:

def _get_sequence_name(self, table):
    name_length = self.max_name_length() - 3
    sequence_name = '%s_SQ' % strip_quotes(table)
    return truncate_name(sequence_name, name_length).upper()

Note the subtle change - in 1.10.7 the return value always ends with '_SQ'; in 1.11.3 the '_SQ' is part of what gets truncated. (For context, truncate_name basically takes a hash of the string and appends the last few digits to the name of the table - so you end up with e.g. 'PATIENTS_PATIENTDOCTORRELA8026', to fit in the 30-character limit).

The consequence of this is that after upgrading an Oracle-backed app to 1.11, inserts start failing (because last_insert_id is looking for the sequence name 'PATIENTS_PATIENTDOCTORR36D1', whereas the actual sequence name is 'PATIENTS_PATIENTDOCTORRC0BD_SQ' - because that's what was generated in the prior version; or, to be precise, whenever the table was created, several versions before that).

As far as I can tell (though I can't be sure) this change was an inadvertent side effect of 69b7d4b1, which was the fix for #27458. I say 'inadvertent' because it doesn't appear to be the focus of the change, and the tests don't appear to be taking that into account. In general, most tests wouldn't pick up the problem because it only manifests if you have existing sequences - for a fresh database, it's fine (since the sequences will be created with the new naming scheme and everything is hunky-dory).

(NB: the same thing appears to have happened for triggers, though this particular database isn't using triggers so I didn't hit that particular error).

As a quick test, patching Django to use the pre-1.11 version of _get_sequence_name worked correctly, so I'm fairly confident that's the issue (there was another change in it, the strip_quotes bit, so if we go that way for the ultimate fix we'll probably want to keep that rather than just reverting).

I'd be glad to work on a patch but to be honest I'm not clear what direction to take... as I see it, the options are:

1) Revert the behavior - make _get_sequence_name return '%s_SQ' like it did pre-1.11 (but with the strip_quotes fix). This has a bad backcompat issue in that it'll introduce essentially the same problem for sequences created with 1.11.x... so that doesn't really seem like a good idea.

2) Create a helper to rename sequences when a change like this is introduced. In theory this is an implementation detail and Django should be able to tweak the way the truncated names are generated, as long as there's a transition path (though I say that as someone who doesn't use custom sequences much - others may have a different opinion).

So I can imagine a utility of some sort to cross-check sequence names (for autonumber fields and such) with what Django expects, and either interactively or automatically rename them. Perhaps call it out in the release notes?

For completeness, this feels related to #23577 but feels a bit different, and IMO is more dire because it's less obvious and less likely to get noticed right away.

Change History (10)

comment:1 by Mariusz Felisiak, 7 years ago

Severity: NormalRelease blocker
Triage Stage: UnreviewedAccepted

Oracle backend uses last_insert_id only when use_returning_into is disabled manually in database settings (see doc), otherwise _get_sequence_name shouldn't affect insert operation. Nevertheless, I think we should revert _get_sequence_name and _get_trigger_name to the previous behavior (with suffixes) and add a detailed instruction how to change (recreate) objects with wrong names.

comment:2 by Mariusz Felisiak, 7 years ago

Cc: Mariusz Felisiak added

comment:3 by Shai Berger, 7 years ago

Cc: Shai Berger added

I concur. Note that the change makes the names shorter than necessary -- the idea behind using self.max_name_length() - 3 is to save space for the _SQ suffix, but the change now includes that suffix in the truncated text.

I also think we should similarly revert the analogous change in trigger name truncation. I'm not sure we ever use calculated trigger names after the trigger is created, however,

  • if we do, then the same considerations as with sequences apply
  • if we don't, then it's better to have consistent code and use the full available name length

comment:4 by Kevin Grinberg, 7 years ago

Shai/Felix: do you think it's critical to provide a migration script as part of the fix?

comment:5 by Kevin Grinberg, 7 years ago

Cc: Kevin Grinberg added
Owner: changed from nobody to Kevin Grinberg
Status: newassigned

comment:6 by Kevin Grinberg, 7 years ago

Has patch: set

comment:7 by Kevin Grinberg, 7 years ago

PRs - all tests pass under Oracle (the changes don't touch anything else).

backport to 1.11.x:

NB: The migration script in the backport may be a bit unwieldy to put in the release notes, particularly given that it likely affects very few people (basically ONLY users who created projects with 1.11, are on Oracle, and using use_returning_into=False). Is there a better standard place to put these sorts of things?

I also omitted it from the master release notes, since it looks like we drop support for older versions of Oracle and stop relying on triggers altogether.

comment:8 by Tim Graham, 7 years ago

We'll put the upgrade script here:

from django.db import connection
from django.db.backends.utils import strip_quotes, truncate_name

for seq in connection.introspection.sequence_list():
    name_length = 27
    table, column = seq['table'], seq['column']
    if len(table) >= name_length:
        with connection.cursor() as cursor:
            # 1.11.[1-4] format
            old_sq_name = truncate_name('%s_SQ' % strip_quotes(table), name_length).upper()
            # pre-1.11, 1.11.5+
            new_sq_name = '%s_SQ' % truncate_name(strip_quotes(table), name_length).upper()
            cursor.execute('SELECT sequence_name FROM user_sequences WHERE sequence_name=%s', [old_sq_name])
            row = cursor.fetchone()
            if row:
                cursor.execute('RENAME %s TO %s' % (old_sq_name, new_sq_name))
                args = {
                    'new_tr_name': '%s_TR' % truncate_name(strip_quotes(table), name_length).upper(),
                    'tbl_name': table.upper(),
                    'sq_name': new_sq_name,
                    'col_name': column.upper(),
                old_tr_name = truncate_name('%s_TR' % strip_quotes(table), name_length).upper()
                cursor.execute('DROP TRIGGER %s' % old_tr_name)
                trigger_sql = """
                    CREATE OR REPLACE TRIGGER "%(new_tr_name)s"
                    BEFORE INSERT ON %(tbl_name)s
                    FOR EACH ROW
                    WHEN (new.%(col_name)s IS NULL)
                            SELECT "%(sq_name)s".nextval
                            INTO :new.%(col_name)s FROM dual;
                /""" % args

comment:9 by Tim Graham <timograham@…>, 7 years ago

Resolution: fixed
Status: assignedclosed

In c6a35460:

Fixed #28451 -- Restored pre-Django 1.11 Oracle sequence/trigger naming.

Regression in 69b7d4b116e3b70b250c77829e11038d5d55c2a8.

comment:10 by Tim Graham <timograham@…>, 7 years ago

In 90be8cf2:

[1.11.x] Fixed #28451 -- Restored pre-Django 1.11 Oracle sequence/trigger naming.

Regression in 69b7d4b116e3b70b250c77829e11038d5d55c2a8.

Backport of c6a3546093bebae8225a2c5b7e0836a2b0617ee5 from master

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