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)

2.17.3
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Bugzilla 2.18

People

(Reporter: justdave, Assigned: dkl)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 3 obsolete files)

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

patch to follow shortly
Attached 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-
Attached 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
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.
Attached 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 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+
Attachment #152450 - Attachment is obsolete: true
Assignee: nobody → dkl
Flags: approval?
Status: NEW → ASSIGNED
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
Closed: 20 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.

Attachment

General

Created:
Updated:
Size: