Closed Bug 228917 Opened 21 years ago Closed 21 years ago

Flags uses DB-dependent SQL


(Bugzilla :: Attachments & Requests, enhancement)

Not set



Bugzilla 2.18


(Reporter: mkanat, Assigned: mkanat)




(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?

Attached patch Small fixes for Flags (obsolete) — 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.

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.


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, which I presume was written by Myk:

    # In case the bug's product/component has changed, clear flags that are
    # no longer valid.
        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?
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

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:

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.


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
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.
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).

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?

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/;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/,v  <--
new revision: 1.15; previous revision: 1.14
Checking in Bugzilla/;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/,v  <--
new revision: 1.6; previous revision: 1.5
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.