228917 - Flags uses DB-dependent SQL

Status: RESOLVED FIXED

(Bugzilla :: Attachments & Requests, enhancement)

Description

[Max Kanat-Alexander]

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

Comment 1

[Max Kanat-Alexander]

Attachment: Small fixes for Flags

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

[Matthew Tuck [:CodeMachine]]

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.

Comment 3

[Bradley Baetz (:bbaetz)]

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

[Matthew Tuck [:CodeMachine]]

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?

Comment 5

[Bradley Baetz (:bbaetz)]

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

[Matthew Tuck [:CodeMachine]]

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

Comment 7

[Max Kanat-Alexander]

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.

Comment 8

[Max Kanat-Alexander]

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

Comment 9

[Max Kanat-Alexander]

Attachment: Minimal fixes against current CVS

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

Comment 10

[Max Kanat-Alexander]

Comment on attachment 140050 [details] [diff] [review]
Minimal fixes against current CVS

Hry myk, both these files are yours. Review? -M

Comment 11

[Matthew Tuck [:CodeMachine]]

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.

Comment 12

[Max Kanat-Alexander]

Attachment: Consistent spacing for parentheses

Okay, fixed the spacing. -M

Comment 13

[Max Kanat-Alexander]

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

[Myk Melez [:myk] [@mykmelez]]

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

Comment 15

[Max Kanat-Alexander]

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

Comment 16

[Max Kanat-Alexander]

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

Comment 17

[Myk Melez [:myk] [@mykmelez]]

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