Edgewall Software
Modify

Opened 15 years ago

Closed 14 years ago

Last modified 3 years ago

#8575 closed defect (fixed)

IntegrityError: duplicate key violates unique constraint

Reported by: shanec@… Owned by: shanec@…
Priority: normal Milestone: 0.12.1
Component: database backend Version: none
Severity: normal Keywords:
Cc: shanec@…, dfraser Branch:
Release Notes:
API Changes:

trac.db: Added a method update_sequence() to database connections to allow updating table sequences during schema updates. [10114]

Internal Changes:

Description

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.

Attachments (1)

update_sequence.patch (2.4 KB ) - added by bobbysmith007@… 14 years ago.
A patch to add update_sequence to the database backends

Download all attachments as: .zip

Change History (14)

comment:1 by Shane Caraveo <shanec@…>, 15 years ago

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)

comment:2 by Shane Caraveo <shanec@…>, 15 years ago

Cc: shanec@… added

comment:3 by dfraser, 15 years ago

Cc: dfraser added

comment:4 by Christian Boos, 14 years ago

Milestone: 0.12

See also #6466.

comment:5 by Remy Blank, 14 years ago

Owner: set to 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).

comment:6 by Remy Blank, 14 years ago

Milestone: 0.12next-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 Carsten Klein <carsten.klein@…>, 14 years ago

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

comment:8 by Remy Blank, 14 years ago

ALTER isn't well supported on SQLite, unfortunately.

by bobbysmith007@…, 14 years ago

Attachment: update_sequence.patch added

A patch to add update_sequence to the database backends

comment:9 by bobbysmith007@…, 14 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.

Russ Tyndall

comment:10 by bobbysmith007@…, 14 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 Remy Blank, 14 years ago

API Changes: modified (diff)
Resolution: fixed
Status: newclosed

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.

comment:12 by Remy Blank, 14 years ago

Owner: changed from Remy Blank to shanec@…

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

comment:13 by Remy Blank, 14 years ago

Milestone: next-minor-0.12.x0.12.1

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain shanec@….
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from shanec@… to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.