289042 - Implicit joins should be replaced by explicit joins - installment B

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, defect)

Description

[Tomas Kopal]

This is a followup to bug 286235, dealing with the more joins that needs to be
converted (aka part 1 of the patch from bug 286235).

Comment 1

[Tomas Kopal]

Attachment: V2

This is just un-bitrotted part 1 from bug 286235.

For the explanation of the logic change asked for at bug 286235, comment #18,
please see comments #5-#9 there.

Comment 2

[Joel Peshkin]

Most of this is fine....  I am still unconvinced about the logic change.

     my $bless_groups = $dbh->selectcol_arrayref(
         q{SELECT DISTINCT groups.name, groups.id
-            FROM groups, user_group_map, group_group_map AS ggm
-           WHERE user_group_map.user_id = ?
-             AND ((user_group_map.isbless = 1
-                   AND groups.id=user_group_map.group_id)
-                  OR (groups.id = ggm.grantor_id
-                      AND ggm.grant_type = } . GROUP_BLESS .
-                   q{ AND user_group_map.group_id = ggm.member_id
-                      AND user_group_map.isbless = 0))},
+            FROM groups
+      INNER JOIN user_group_map
+              ON groups.id = user_group_map.group_id
+             AND user_group_map.user_id = ?
+       LEFT JOIN group_group_map AS ggm
+              ON user_group_map.group_id = ggm.member_id
+             AND groups.id = ggm.grantor_id
+             AND ggm.grant_type = } . GROUP_BLESS . q{
+           WHERE user_group_map.isbless = 1
+              OR ggm.member_id IS NOT NULL},
          { Columns=>[1,2] }, $self->{id});

If a user is a can bless group A
But that user is not a member of group A
And group A can bless group B
The user can bless group B

The user could never do this before.  While the use could have added himself to
group A, then gone back and blessed group B, this is not the prior behavior and
does make it a bit easier to make administrative blunders.

Comment 3

[Max Kanat-Alexander]

For SQL statements that don't currently mix implicit joins with explicit joins,
I'd like to leave them as they are. PostgreSQL has no problems with such
implicit joins, and I believe that the join ordering will be faster on 7.3 with
the implicit joins than with the explicit joins.

It's also less code change, which would make me happier.

Comment 4

[Tomas Kopal]

(In reply to comment #3)
> For SQL statements that don't currently mix implicit joins with explicit joins,
> I'd like to leave them as they are. PostgreSQL has no problems with such
> implicit joins, and I believe that the join ordering will be faster on 7.3 with
> the implicit joins than with the explicit joins.

Well, I believe it's quite the opositte. In postgres, comma translates to CROSS
JOIN (according to pg 7.3 docs, see
http://www.postgresql.org/docs/7.3/interactive/queries-table-expressions.html#QUERIES-FROM),
which means that we are getting huge numbers of rows (for two tables with N and
M rows, we are getting N*M rows, for more tables it gets even worse) and then
stripping unneeded data in the WHERE clause. I doubt it would be faster than
INNER JOIN where you get only rows you really want.
Moreover, this behaviour differs between MySQL and Postgres. Although I can't
think of anything from the top of my head, I am sure this can cause problems
when the WHERE part will not weed out all the rubbish properly, causing
different results on Postgres than on MySQL.
And as a cherry on top, explicit join is much more readable in the code ;-).

Comment 5

[Tomas Kopal]

(In reply to comment #4)
> Moreover, this behaviour differs between MySQL and Postgres. Although I can't
> think of anything from the top of my head, I am sure this can cause problems
> when the WHERE part will not weed out all the rubbish properly, causing
> different results on Postgres than on MySQL.

Well, after reading MySQL docs again, this is not true, both MySQL and Postgres
are doing CROSS JOIN when tables are separated by comma. All other points should
be still valid though.

Comment 6

[Max Kanat-Alexander]

(In reply to comment #5)
> Well, after reading MySQL docs again, this is not true, both MySQL and Postgres
> are doing CROSS JOIN when tables are separated by comma. All other points should
> be still valid though.

  If the query planner behaves well on PostgreSQL in such a situation as it does
for MySQL, then there should be no problem keeping them as they are, for now.

Comment 7

[Tomas Kopal]

Attachment: V2.1

As Max requested due to the fact we are in freeze, I have removed all changes
from implicit to explicit JOINs which are currently purely implicit. The patch
is much smaller now and it will (hopefully) still work on postgres.
Just for the record, I still think that converting all implicit JOINs to
explicit ones is a good think to do and I promise I will try again after the
freeze :-).

Comment 8

[Joel Peshkin]

Comment on attachment 179904 [details] [diff] [review]
V2.1

r=joel
I inspected and gave a quick test.  I would like some assurance of a very
thorough test before you land.

Comment 9

[Max Kanat-Alexander]

Comment on attachment 179904 [details] [diff] [review]
V2.1

Hey wicked, do you want to test this out? If you don't have time, feel free to
just clear the request, and I'd imagine that Tomas will have some time later to
do more testing.

Comment 10

[Frédéric Buclin]

Comment on attachment 179904 [details] [diff] [review]
V2.1

Bitrotten due to the rewrite of FlagType::get_clusions().

Comment 11

[Max Kanat-Alexander]

Attachment: v2.2

OK, I've un-bitrotted the patch and removed parts that were irrelevant to this
bug. Some large sections were only whitespace changes.

Basically, though, it's an identical patch -- the part that bitrotted no longer
needs to be applied, since that section of the code already had its joins
fixed.

Comment 12

[Max Kanat-Alexander]

Comment on attachment 187859 [details] [diff] [review]
v2.2

LpSolit, could you test it?

Comment 13

[Frédéric Buclin]

Comment on attachment 187859 [details] [diff] [review]
v2.2

tested, works fine. r=LpSolit

Comment 14

[Max Kanat-Alexander]

Checking in Bugzilla/Bug.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Bug.pm,v  <--  Bug.pm
new revision: 1.79; previous revision: 1.78
done
Checking in Bugzilla/Flag.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Flag.pm,v  <--  Flag.pm
new revision: 1.43; previous revision: 1.42
done
Checking in Bugzilla/FlagType.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/FlagType.pm,v  <--  FlagType.pm
new revision: 1.18; previous revision: 1.17
done