If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Flags uses DB-dependent SQL

RESOLVED FIXED in Bugzilla 2.18

Status

()

Bugzilla
Attachments & Requests
--
enhancement
RESOLVED FIXED
14 years ago
5 years ago

People

(Reporter: Max Kanat-Alexander, Assigned: Max Kanat-Alexander)

Tracking

2.17.6
Bugzilla 2.18
Bug Flags:
approval +

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

14 years ago
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

14 years ago
Created attachment 137685 [details] [diff] [review]
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
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);
(Assignee)

Comment 7

14 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

14 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

14 years ago
Created attachment 140050 [details] [diff] [review]
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
Attachment #137685 - Attachment is obsolete: true
(Assignee)

Comment 10

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

14 years ago
Created attachment 140093 [details] [diff] [review]
Consistent spacing for parentheses

Okay, fixed the spacing. -M
Attachment #140050 - Attachment is obsolete: true
(Assignee)

Comment 13

14 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 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+
(Assignee)

Comment 15

14 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

14 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
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
Last Resolved: 14 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.