Roundup Tracker - Issues

Issue 1876683

classification
Race condition in issue creation may lead to db corruption
Type: Severity: normal
Components: Database Versions:
process
Status: closed accepted
:
: richard : richard, stefan
Priority: urgent :

Created on 2008-01-21 19:05 by stefan, last changed 2008-02-07 01:09 by richard.

Messages
msg2502 Author: [hidden] (stefan) Date: 2008-01-21 19:05
The rdbms_common.Class.create_inner code now creates a new issue, after an initial check that an issue with the requested key doesn't exist.

However, if two such requests arrive simultaneously, a race condition may result where both sessions report the key as not existing, and then both continue creating the item.

Some backends lock the database during issue creation, but not all.
We are using the mysql backend, and we just found multiple user items with the same username. This is a critical problem.

Thanks,
msg2503 Author: [hidden] (anonymous) Date: 2008-01-22 21:16
Logged In: NO 

Please suggest a solution - I know very little about MySQL (only that I prefer not to use it :)
msg2504 Author: [hidden] (stefan) Date: 2008-01-22 21:25
Instead of letting Roundup handle this uniqueness constraint, I think the SQL backends are a far better place to do this, i.e. keys should map to unique indices in the backends. (Of course, this means the change affects all backends, not just the backend_mysql.py.
The proposed solution is:

1. Adopt the convention that retired nodes use the nodeid for the value
of the __retired__ column.  So, if the node with id 7 is retired, the
value of __retired__ for that node is "7", not "1".  (There is never a
node with id zero, so this is a robust encoding.)  The "id" column is
also an INTEGER, so there is no problem with widths.

2. Change all code in the relational database backends that does
"__retired__ <> 1" to instead do "__retired__ = 0".  The former
comparison will no longer work with the new encoding.

3. For all tables with a key property ($key), run:

CREATE UNIQUE INDEX $name on $table ($key, __retired__)

This statement will make MySQL check that all records in the table have
a unique combination of the $key column and the __retired__ column.
That will cause an exception in Roundup at the point that the erroneous
commit is attempted.  We can trap this and try to give a sensible error,
but even if we fail to give a good error message, we will at least avoid
database corruption.

Does this sound sensible ?
msg2505 Author: [hidden] (stefan) Date: 2008-01-24 18:03
Do you have an opinion on the suggested solution ? This is a critical bug which we need to fix ASAP (we now have multiple users 'sharing' the same username !). As the suggested solution is fairly radical, I'd like to make sure such a change wouldn't diverge from mainline, were we to patch our local copy of Roundup.

Thanks !
msg2506 Author: [hidden] (richard) Date: 2008-01-26 22:37
Your proposed solution seems reasonable. I will not be in a position to produce a fix until the middle of next week.
msg2507 Author: [hidden] (richard) Date: 2008-02-05 09:22
I've still not found time to devote to this issue, sorry!
msg2508 Author: [hidden] (richard) Date: 2008-02-07 01:09
It seems to me that there's no value in retaining a retired item in the database if a new item with the same id is created.

Such a create operation would actually edit the existing retired item (and unretire it).

Thus a unique index on id would be appropriate.
msg2509 Author: [hidden] (richard) Date: 2008-02-07 01:21
Sorry, ignore that last comment.
msg2510 Author: [hidden] (richard) Date: 2008-02-07 01:32
Your proposed solution seems reasonable. I have begun implementation.
msg2511 Author: [hidden] (richard) Date: 2008-02-07 03:28
OK, I've implemented a fix. It's in CVS and will be released with 1.4.2 once I've done some additional testing.
History
Date User Action Args
2008-01-21 19:05:51stefancreate