Closed
Bug 190432
Opened 22 years ago
Closed 20 years ago
saving a named query uses REPLACE INTO (not ANSI)
Categories
(Bugzilla :: Query/Bug List, defect)
Tracking
()
RESOLVED
FIXED
Bugzilla 2.18
People
(Reporter: justdave, Assigned: dkl)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 3 obsolete files)
4.15 KB,
patch
|
goobix
:
review+
|
Details | Diff | Splinter Review |
saving a named query uses REPLACE INTO, which Sybase doesn't like (and I assume Postgres doesn't either). patch to follow shortly
Reporter | ||
Comment 1•22 years ago
|
||
Reporter | ||
Updated•22 years ago
|
Attachment #112501 -
Flags: review?(bbaetz)
Reporter | ||
Updated•22 years ago
|
Attachment #112501 -
Flags: review?(dkl)
Comment 2•22 years ago
|
||
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-
Reporter | ||
Comment 3•22 years ago
|
||
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
Reporter | ||
Updated•22 years ago
|
Attachment #112501 -
Flags: review?(dkl)
Comment 4•21 years ago
|
||
Comment on attachment 112579 [details] [diff] [review] patch v2 This is still racy. REPLACE is atomic; UPDATE/INSERT aren't.
Attachment #112579 -
Flags: review-
Reporter | ||
Updated•21 years ago
|
Assignee: endico → nobody
The solution is to DELETE (ignore errors) and then INSERT. Preferably inside a transaction.
Comment 6•20 years ago
|
||
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.
Assignee | ||
Comment 7•20 years ago
|
||
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 8•20 years ago
|
||
Comment on attachment 152450 [details] [diff] [review] patch v3 The LOCK alternative would make sense to me.
Attachment #152450 -
Flags: review-
Assignee | ||
Comment 9•20 years ago
|
||
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 10•20 years ago
|
||
Comment on attachment 152519 [details] [diff] [review] patch with table locking v4 r=vladd
Attachment #152519 -
Flags: review+
Updated•20 years ago
|
Attachment #152450 -
Attachment is obsolete: true
Updated•20 years ago
|
Assignee: nobody → dkl
Flags: approval?
Updated•20 years ago
|
Status: NEW → ASSIGNED
Reporter | ||
Comment 11•20 years ago
|
||
Looks good to me, too. a= justdave
Flags: approval? → approval+
Target Milestone: --- → Bugzilla 2.18
Assignee | ||
Comment 12•20 years ago
|
||
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
Closed: 20 years ago
Resolution: --- → FIXED
Updated•11 years ago
|
QA Contact: matty_is_a_geek → default-qa
You need to log in
before you can comment on or make changes to this bug.
Description
•