Edgewall Software
Modify

Opened 5 years ago

Closed 4 years ago

Last modified 4 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
Release Notes:
API Changes:

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

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@… 4 years ago.
A patch to add update_sequence to the database backends

Download all attachments as: .zip

Change History (14)

comment:1 Changed 5 years ago by 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)

comment:2 Changed 5 years ago by Shane Caraveo <shanec@…>

  • Cc shanec@… added

comment:3 Changed 5 years ago by dfraser

  • Cc dfraser added

comment:4 Changed 4 years ago by cboos

  • Milestone set to 0.12

See also #6466.

comment:5 Changed 4 years ago by rblank

  • Owner set to rblank

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 Changed 4 years ago by rblank

  • Milestone changed from 0.12 to 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 Changed 4 years ago by Carsten Klein <carsten.klein@…>

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

comment:8 Changed 4 years ago by rblank

ALTER isn't well supported on SQLite, unfortunately.

Changed 4 years ago by bobbysmith007@…

A patch to add update_sequence to the database backends

comment:9 Changed 4 years ago by 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

comment:10 Changed 4 years ago by 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

comment:11 Changed 4 years ago by rblank

  • API Changes modified (diff)
  • Resolution set to fixed
  • Status changed from new to closed

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 Changed 4 years ago by rblank

  • Owner changed from rblank to shanec@…

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

comment:13 Changed 4 years ago by rblank

  • Milestone changed from next-minor-0.12.x to 0.12.1

Add Comment

Modify Ticket

Change Properties
<Author field>
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.
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.