saving a named query uses REPLACE INTO, which Sybase doesn't like (and I assume Postgres doesn't either). patch to follow shortly
Attachment #112501 - Flags: review?(bbaetz)
Comment on attachment 112501 [details] [diff] [review] patch v1 This has a race condidion between the delete and the insert. If I have two transactions, and both try to add an entry which doesn't exist, then both will delete nothing, then try to insert, which will fail the duplicate key tests. You can either: a) LOCK the table first; or b) UPDATE <if no rows updated> LOCK DELETE INSERT UNLOCK <end if> IOW, have a fast-path, and a slow path. I'm not sure if thats worth it here, though, since people don't replace namdqueries that often, and it is a quick operation. Not to mention that the INSERT is a table locking operation anyway for mysql...
Attachment #112501 - Flags: review?(bbaetz) → review-
one of them was already halfway there, someone just forgot to remove the "REPLACE INTO", which never would have gotten called unless it was an INSERT anyway. Changes the second one to just an INSERT, and adds the if/then to the first one.
Attachment #112501 - Attachment is obsolete: true
Comment on attachment 112579 [details] [diff] [review] patch v2 This is still racy. REPLACE is atomic; UPDATE/INSERT aren't.
Attachment #112579 - Flags: review-
Assignee: endico → nobody
The solution is to DELETE (ignore errors) and then INSERT. Preferably inside a transaction.
A more performant way to deal with this is to deal with both error codes you can get: UPDATE IF no rows THEN INSERT IF duplicate key THEN UPDATE This removes the need to do ugly things like lock tables. It's also probably the most database agnostic way to do this, since Oracle and PostgreSQL handle locking comelpetely differently than other databases. Note that the drawback to this is that it doesn't protect against deletes.
New patch that I have also in my PostgreSQL patch. I am trying to break things out into smaller chunks for easier commiting of the PostgreSQL changes later. This patch also adds a couple of other fixes for query.cgi and checksetup.pl. Was a descision ever made whether just performing a LOCK around this section is the way to go or to do a DELETE/INSERT pair?
Attachment #112579 - Attachment is obsolete: true
Comment on attachment 152450 [details] [diff] [review] patch v3 The LOCK alternative would make sense to me.
Attachment #152450 - Flags: review-
Added locks to namedqueries table around UPDATE/INSERT operations. I have tested with adding/removing/changing stored queries and seems to work for me. Dave
Comment on attachment 152519 [details] [diff] [review] patch with table locking v4 r=vladd
Attachment #152519 - Flags: review+
Looks good to me, too. a= justdave
Flags: approval? → approval+
Target Milestone: --- → Bugzilla 2.18
Checking in buglist.cgi; /cvsroot/mozilla/webtools/bugzilla/buglist.cgi,v <-- buglist.cgi new revision: 1.254; previous revision: 1.253 done Checking in checksetup.pl; /cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v <-- checksetup.pl new revision: 1.287; previous revision: 1.286 done Checking in query.cgi; /cvsroot/mozilla/webtools/bugzilla/query.cgi,v <-- query.cgi new revision: 1.126; previous revision: 1.125 done
Status: ASSIGNED → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.