Closed
Bug 228917
Opened 21 years ago
Closed 21 years ago
Flags uses DB-dependent SQL
Categories
(Bugzilla :: Attachments & Requests, enhancement)
Tracking
()
RESOLVED
FIXED
Bugzilla 2.18
People
(Reporter: mkanat, Assigned: mkanat)
References
Details
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
myk
:
review+
|
Details | Diff | Splinter Review |
While other work is going on to add support for other DBs, I think it could be
useful to make as much Bugzilla SQL cross-database as possible.
Here's a pretty simple one:
If you have this:
FROM flags, bugs
In MySQL that means "INNER JOIN based on WHERE clause"
In some databases, that can mean CROSS JOIN.
In PgSQL, it means nothing at all, and usually throws an error.
The solution is to replace all instances of commas in the FROM clause with the
correct INNER JOIN statements.
I was informed that Oracle doesn't like INNER JOIN, and requires WHERE -- is
this the case?
-M
Assignee | ||
Comment 1•21 years ago
|
||
Here's just part of the fix -- this is my first patch against bugzilla-tip, so
I thought I'd take it small to start.
-M
Comment 2•21 years ago
|
||
The issue here is not that "," isn't supported, it is, and it's the most
fundamental SQL.
The issue is something like:
SELECT * FROM a, b LEFT JOIN c ON a.a = c.a AND b.b = c.b
The problem is there's no guarantee that a is before b/c, and hence that a is
available to test.
In MySQL it presumably works out that it has to be, implicitly.
In PgSQL, you get an error. A simple solution is to change it to:
SELECT * FROM a CROSS JOIN b LEFT JOIN c ON a.a = c.a AND b.b = c.b
which dictates an order, so a is available before c. However, it also dictates
an order between a/b, which is unnecessary.
The PgSQL docs suggest you can use parentheses, but:
SELECT * FROM (a, b) LEFT JOIN c ON a.a = c.a AND b.b = c.b
doesn't seem to work.
However:
SELECT * FROM a LEFT JOIN c ON a.a = c.a RIGHT JOIN b ON b.b = c.b
might be the solution. I'm not sure about RIGHT JOIN support across databases,
however. This solution wouldn't be scalable to three tables, though.
One example where this occured is in Flag.pm, which I presume was written by Myk:
# In case the bug's product/component has changed, clear flags that are
# no longer valid.
&::SendSQL("
SELECT flags.id
FROM flags, bugs LEFT OUTER JOIN flaginclusions i
ON (flags.type_id = i.type_id
AND (bugs.product_id = i.product_id OR i.product_id IS NULL)
AND (bugs.component_id = i.component_id OR i.component_id IS NULL))
WHERE flags.type_id = $target->{'bug'}->{'id'}
AND flags.bug_id = bugs.bug_id
AND i.type_id IS NULL
");
It might be possible to rearchitect the query altogether, but it seems to me
that both bugs and flags must be left of inclusions.
Summary: Commas in the FROM clause are not DB-Independent → Can't JOIN on all tables that were syntactically previous.
Comment 3•21 years ago
|
||
Since we require a new enough mysql version, we can replace , with INNER JOIN.
However, in a lot of these cases what we really want is a subselect....
Comment 4•21 years ago
|
||
Apparently PgSQL 7.4 removes the restrictions on table ordering that INNER JOIN
implies. What that means for whether either , or CROSS/INNER JOIN works I don't
know ... it needs retesting. I would have expected them both to work the same now.
By subselects, do you mean WHERE NOT EXISTS?
Status: NEW → ASSIGNED
Comment 5•21 years ago
|
||
I mean subselcts. For example, to select all the bugs whose product is FooBar, do:
SELECT bugs.* from bugs where product_id=(SELECT id FROM products WHERE
name='FooBar').
That avoids unneeded joins complicationg the optimiser, and also makes it a lot
clearer what we're trying to do.
But MySQL doesn't support those yet, so...
Comment 6•21 years ago
|
||
That sort of subselect is not relevant to this example, I don't think. What
might be, is a NOT EXISTS subselect, which is what I was trying to say:
SELECT * FROM flags WHERE NOT EXISTS (SELECT * FROM flaginclusions WHERE
flags.flag_id = flaginclusions.flag_id);
Assignee | ||
Comment 7•21 years ago
|
||
I think that what PgSQL 7.4 supports might not be relevant, since no
distribution that I know of ships PgSQL 7.4. :-) However, I see the point that
in the future, this won't matter.
Also, what is the real performance impact of INNER JOIN? If INNER JOIN is what
is actually meant, I think it's generally better to use INNER JOIN (for
readability, if nothing else) than FROM a,b WHERE a.field = b.field.
I agree about the sub-selects -- that's what I thought, originally. It's too bad
that MySQL didn't support them until way too recently. :-)
I'm changing the summary slightly to have some terms that people would search
for if they were concerned about the issue.
-M
P.S. -- mattyt, when you set this to ASSIGNED, did you intend to take it? I
originally assigned the bug to myself.
Summary: Can't JOIN on all tables that were syntactically previous. → Can't explicitly JOIN on all tables that were previously implicitly joined by commas (PostgreSQL)
Assignee | ||
Comment 8•21 years ago
|
||
Okay, I'm assuming that this bug is meant to be assigned to me. I'm going to
focus this bug a little more, just on Flags. -M
Component: Bugzilla-General → Administration
OS: Linux → All
Hardware: PC → All
Summary: Can't explicitly JOIN on all tables that were previously implicitly joined by commas (PostgreSQL) → Flags uses DB-dependent SQL
Assignee | ||
Comment 9•21 years ago
|
||
This is actually the minimal fix for this issue -- as long as there aren't
other JOINs, PgSQL can do unordered JOINs. I hear that SyBase can't (or does
unexpected things with them) but I don't think we have SyBase support planned.
-M
Attachment #137685 -
Attachment is obsolete: true
Assignee | ||
Comment 10•21 years ago
|
||
Comment on attachment 140050 [details] [diff] [review]
Minimal fixes against current CVS
Hry myk, both these files are yours. Review? -M
Attachment #140050 -
Flags: review?(myk)
Comment 11•21 years ago
|
||
This is probably the right thing to do.
Has it been tested against MySQL 3.23, 4.0 and PgSQL 7.4? 3.23 wouldn't be
necessary if this waits for 2.19, cause we're dropping that I believe.
There are some inconsistent spaces inside brackets in the second file to fix, if
you get a chance.
Assignee | ||
Comment 12•21 years ago
|
||
Okay, fixed the spacing. -M
Attachment #140050 -
Attachment is obsolete: true
Assignee | ||
Comment 13•21 years ago
|
||
Oh, in response to matty's comment, I only tested it against 3.23. I wouldn't
think that the 4.0 syntax would possibly change that much... but does somebody
have a 4.0 installation they could test it against? -M
Comment 14•21 years ago
|
||
Comment on attachment 140050 [details] [diff] [review]
Minimal fixes against current CVS
Query looks semantically equivalent and "works" on MySQL (although the
functionality doesn't actually work; but that's bug 232554).
r=myk
Attachment #140050 -
Flags: review?(myk) → review+
Updated•21 years ago
|
Flags: approval+
Assignee | ||
Comment 15•21 years ago
|
||
Hooray. :-) Make sure that you actually check in the one below. The only
difference is spacing consistency.
I don't have CVS write access... could somebody check this in?
-M
Assignee | ||
Comment 16•21 years ago
|
||
Oh, and just for charting purposes, I guess this bug should have actually been
moved to "Attachments & Requests" when I changed the focus of it. -M
Component: Administration → Attachments & Requests
Updated•21 years ago
|
Attachment #140050 -
Flags: review+
Updated•21 years ago
|
Attachment #140093 -
Flags: review+
Comment 17•21 years ago
|
||
Checking in Bugzilla/Flag.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Flag.pm,v <-- Flag.pm
new revision: 1.15; previous revision: 1.14
done
Checking in Bugzilla/FlagType.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/FlagType.pm,v <-- FlagType.pm
new revision: 1.6; previous revision: 1.5
done
Status: ASSIGNED → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Target Milestone: --- → Bugzilla 2.18
Updated•12 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
•