Opened 14 years ago
Closed 13 years ago
Last modified 22 months ago
#8575 closed defect (fixed)
IntegrityError: duplicate key violates unique constraint
|Reported by:||Owned by:|
This is not a duplicate of #7007 or #3563. It may however, be a part of the cause of the many bugs posted with this title.
This may be a postgres specific issue, we have not tested with sqlite or mysql.
There are kind of two issues here depending on the table structure. One is for tables that use serialized (auto-increment) id columns, the other is for tables like the session table which has a primary key which is not auto incremented. I believe the second is not an issue rlated to this bug, but other regular bugs such as what was fixed in #3563. So far, I've only identified this in tables with primary keys on auto increment fields.
This is caused (or most easily reproduced) by how upgrades to database tables happens in trac+plugins. The problem occurs when a plugin changes a table schema. The typical formula is to move the current table to a temp table, create the new table, and then copy the data from the temp to the new table. If the table has primary keys that are sequences, and those keys are inserted into the new table, the problem will occur. Here is any easy (directly in sql) reproduction:
First, create the table as a plugin would initially do it:
set search_path to public; CREATE TABLE test_keys ( id serial NOT NULL, foo text, CONSTRAINT id_pk PRIMARY KEY (id) );
Next, add data:
set search_path to public; INSERT INTO test_keys (foo) VALUES('test');
Now, later the plugin needs an upgrade, so we recreate that:
set search_path to public; CREATE TEMPORARY TABLE test_keys_old AS SELECT * FROM test_keys; DROP TABLE test_keys; CREATE TABLE test_keys ( id serial NOT NULL, foo text, bar text, CONSTRAINT id_pk PRIMARY KEY (id) ); INSERT INTO test_keys (id, foo) SELECT id, foo FROM test_keys_old; DROP TABLE test_keys_old;
Alls good, right? Wrong.
set search_path to public; INSERT INTO test_keys (foo, bar) VALUES('test2', 'this stinks');
You now get the error:
ERROR: duplicate key value violates unique constraint "id_pk"
And here's the fix:
SELECT setval('test_keys_id_seq', (SELECT MAX(id) FROM test_keys)+1)
What happened? During the upgrade we copied the rows from the temp table to the new table, including the ID column. Since we inserted the id, the new sequence (test_keys_id_seq) is not updated correctly. Any time you manually add a value to an auto sequenced column, you must manually update the sequence value. We're unaware of any postgres setting that would "fix" this on the postgres side.
Change History (14)
comment:1 by , 14 years ago
comment:2 by , 14 years ago
comment:3 by , 14 years ago
comment:4 by , 14 years ago
See also #6466.
comment:5 by , 14 years ago
Even better would be to copy the old value of the sequence, instead of using
MAX(id) + 1 (which should actually be
MAX(id), as the sequence will be incremented before returning the next value).
comment:6 by , 13 years ago
|Milestone:||0.12 → next-minor-0.12.x|
We don't have any DB upgrades to auto-increment tables in 0.12 or multirepos, so this is not absolutely necessary for 0.12. I'd still like to fix this in the 0.12.x cycle.
comment:7 by , 13 years ago
I think that when doing a db schema upgrade you should actually ALTER instead of DROP a table.
comment:8 by , 13 years ago
ALTER isn't well supported on SQLite, unfortunately.
by , 13 years ago
A patch to add update_sequence to the database backends
comment:9 by , 13 years ago
I am attaching a patch that operates a proposed. I tested against sqlite and postgresql (which are really the only two code paths) manually. I also added a unit test and tried to run it. I don't think I got an error, but I am not really sure how to interpret the results.
Either way I hope this helps, and if you need me to do anything else to help this get accepted, I can try to assist.
comment:10 by , 13 years ago
Also, this patch was suggested by CBoos.
I dont want this comment to get tagged as spam, which it is, so I am hoping that adding some more content will help ease it through. It would be nice to allow a spam exception for linking to the mailing lists.
Please go through with this extra bit, Russ Tyndall
comment:11 by , 13 years ago
|API Changes:||modified (diff)|
|Status:||new → closed|
Patch applied in , thanks! I have changed the unit test to actually add a new row after the call to
update_sequence() and check that the row id is correct.
comment:12 by , 13 years ago
Assigning the ticket to Shane, as he provided the implementation of
comment:13 by , 13 years ago
|Milestone:||next-minor-0.12.x → 0.12.1|
I would like to propose the following as an idea for addressing this. We add a update_sequence method to the db backend classes, which is much like the get_last_id method. The plugins would be responsible for calling it, but at least there would be a db independent method for it.