Closed Bug 228917 Opened 21 years ago Closed 21 years ago

Flags uses DB-dependent SQL

Categories

(Bugzilla :: Attachments & Requests, enhancement)

2.17.6
enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
Bugzilla 2.18

People

(Reporter: mkanat, Assigned: mkanat)

References

Details

Attachments

(1 file, 2 obsolete files)

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
Attached patch Small fixes for Flags (obsolete) (deleted) — Splinter Review
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
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.
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....
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
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...
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);
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)
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
Attached patch Minimal fixes against current CVS (obsolete) (deleted) — Splinter Review
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
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)
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.
Okay, fixed the spacing. -M
Attachment #140050 - Attachment is obsolete: true
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 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+
Flags: approval+
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
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
Attachment #140050 - Flags: review+
Attachment #140093 - Flags: review+
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
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: