IntegrityError: duplicate key violates unique constraint

[shanec@…]

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.

[Shane Caraveo <shanec@…>]

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.

For postgres:

def update_sequence(self, cursor, table, column='id'):
    sql = "SELECT setval('%s_%s_seq', (SELECT MAX(id) FROM %s)+1)" % (table, column, table)
    cursor.execute(sql)

[Christian Boos]

See also #6466.

[Remy Blank]

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).

[Remy Blank]

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.

[Carsten Klein <carsten.klein@…>]

I think that when doing a db schema upgrade you should actually ALTER instead of DROP a table.

[Remy Blank]

ALTER isn't well supported on SQLite, unfortunately.

[bobbysmith007@…]

A patch to add update_sequence to the database backends

[bobbysmith007@…]

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.

Russ Tyndall

[bobbysmith007@…]

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

[Remy Blank]

Patch applied in [10114], 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.

[Remy Blank]

Assigning the ticket to Shane, as he provided the implementation of update_sequence().