[Django] #28451: Change in Oracle sequence name truncation causes regression when updating existing database
Django
existing database
-------------------------------------+-------------------------------------
Reporter: Kevin | Owner: nobody
Grinberg |
Type: Bug | Status: new
Component: Database | Version: 1.11
layer (models, ORM) |
Severity: Normal | Keywords: oracle
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
**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
`django.db.backends.oracle.operations` 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
[changeset:"69b7d4b116e3b70b250c77829e11038d5d55c2a8" 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/28451>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
existing database
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: 1.11
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: oracle | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* severity: Normal => Release blocker
* stage: Unreviewed => Accepted
Comment:
Oracle backend uses `last_insert_id` only when `use_returning_into` is
disabled manually in database settings (see
[https://docs.djangoproject.com/en/1.11/ref/databases/#insert-returning-
into 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/28451#comment:1>
Django
existing database
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: 1.11
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: oracle | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: felixxm (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/28451#comment:2>
Django
existing database
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: 1.11
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: oracle | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: Shai Berger (added)
Comment:
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
--
Ticket URL: <https://code.djangoproject.com/ticket/28451#comment:3>
Django
existing database
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: 1.11
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: oracle | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Kevin Grinberg):
Shai/Felix: do you think it's critical to provide a migration script as
part of the fix?
--
Ticket URL: <https://code.djangoproject.com/ticket/28451#comment:4>
Django
existing database
-------------------------------------+-------------------------------------
| Grinberg
Type: Bug | Status: assigned
Component: Database layer | Version: 1.11
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: oracle | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: new => assigned
* cc: Kevin Grinberg (added)
* owner: nobody => Kevin Grinberg
--
Ticket URL: <https://code.djangoproject.com/ticket/28451#comment:5>
Django
existing database
-------------------------------------+-------------------------------------
| Grinberg
Type: Bug | Status: assigned
Component: Database layer | Version: 1.11
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: oracle | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/28451#comment:6>
Django
existing database
-------------------------------------+-------------------------------------
| Grinberg
Type: Bug | Status: assigned
Component: Database layer | Version: 1.11
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: oracle | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Kevin Grinberg):
PRs - all tests pass under Oracle (the changes don't touch anything else).
master: https://github.com/django/django/pull/8898
backport to 1.11.x: https://github.com/django/django/pull/8899
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/28451#comment:7>
Django
existing database
-------------------------------------+-------------------------------------
| Grinberg
Type: Bug | Status: assigned
Component: Database layer | Version: 1.11
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: oracle | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Tim Graham):
We'll put the upgrade script here:
{{{#!python
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)
BEGIN
SELECT "%(sq_name)s".nextval
INTO :new.%(col_name)s FROM dual;
END;
/""" % args
cursor.execute(trigger_sql)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/28451#comment:8>
Django
existing database
-------------------------------------+-------------------------------------
| Grinberg
Component: Database layer | Version: 1.11
(models, ORM) |
Keywords: oracle | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"c6a3546093bebae8225a2c5b7e0836a2b0617ee5" c6a35460]:
{{{
#!CommitTicketReference repository=""
revision="c6a3546093bebae8225a2c5b7e0836a2b0617ee5"
Fixed #28451 -- Restored pre-Django 1.11 Oracle sequence/trigger naming.
Regression in 69b7d4b116e3b70b250c77829e11038d5d55c2a8.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/28451#comment:9>
Django
existing database
-------------------------------------+-------------------------------------
| Grinberg
Type: Bug | Status: closed
Component: Database layer | Version: 1.11
(models, ORM) |
Severity: Release blocker | Resolution: fixed
Keywords: oracle | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Tim Graham <timograham@…>):
In [changeset:"90be8cf2a4d81005c5c35074ba763f5fd3a56bd1" 90be8cf2]:
{{{
#!CommitTicketReference repository=""
revision="90be8cf2a4d81005c5c35074ba763f5fd3a56bd1"
[1.11.x] Fixed #28451 -- Restored pre-Django 1.11 Oracle sequence/trigger
naming.
Regression in 69b7d4b116e3b70b250c77829e11038d5d55c2a8.
Backport of c6a3546093bebae8225a2c5b7e0836a2b0617ee5 from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/28451#comment:10>