saving a named query uses REPLACE INTO (not ANSI)

RESOLVED FIXED in Bugzilla 2.18

Status

()

RESOLVED FIXED
16 years ago
6 years ago

People

(Reporter: justdave, Assigned: dkl)

Tracking

(Blocks: 1 bug)

2.17.3
Bugzilla 2.18
Dependency tree / graph
Bug Flags:
approval +

Details

Attachments

(1 attachment, 3 obsolete attachments)

saving a named query uses REPLACE INTO, which Sybase doesn't like (and I assume
Postgres doesn't either).

patch to follow shortly
Posted patch patch v1 (obsolete) — Splinter Review
Attachment #112501 - Flags: review?(bbaetz)
Attachment #112501 - Flags: review?(dkl)
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-
Posted patch patch v2 (obsolete) — Splinter 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
Attachment #112501 - Flags: review?(dkl)
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

Comment 5

15 years ago
The solution is to DELETE (ignore errors) and then INSERT. Preferably inside a transaction.

Comment 6

15 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

15 years ago
Posted patch patch v3 (obsolete) — Splinter Review
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

15 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

15 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

15 years ago
Comment on attachment 152519 [details] [diff] [review]
patch with table locking v4

r=vladd
Attachment #152519 - Flags: review+

Updated

15 years ago
Attachment #152450 - Attachment is obsolete: true

Updated

15 years ago
Assignee: nobody → dkl
Flags: approval?

Updated

15 years ago
Status: NEW → ASSIGNED
Looks good to me, too.
a= justdave
Flags: approval? → approval+
Target Milestone: --- → Bugzilla 2.18
(Assignee)

Comment 12

15 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
Last Resolved: 15 years ago
Resolution: --- → FIXED
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.