286235 - Implicit joins should be replaced by explicit joins - installment A

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, defect)

Description

[Tomas Kopal]

After removing implicit joins using the comma operator from Search.pm, there is
still a lot of places in Bugzilla where implicit joins are used. Postgres choke
up on many of them, so probably the cleanest solution is not to use them at all.
It would improve readibility of the code and avoid problems where the implicit
join can do different thing on different DBs (MySQL understands it as INNER
JOIN, Postgres as CROSS JOIN).

Comment 1

[Tomas Kopal]

Attachment: V1

Sorry that the patch is such a big chunk, but most of the changes are simple
replacement of the comma join with inner join, so hopefully it shouldn't be too
dificult to go through.
I have also done some formatting changes (before I knew the patch will be so
big and I didn't want to spend more time un-doing them later), if that should
be a problem, I don't mind removing them from the patch.
This should hopefully be the last big change neccessary to get Postgres support
(except for the case-sensitivity problem at bug 285695).
It's tested on MySQL (although I admit I for sure didn't test all the changed
queries, there are simply too many of them, I tried to excercise the more
complex ones).

Comment 2

[Max Kanat-Alexander]

Fred, if you want to do some more advanced testing on this patch, it would of
course be appreciated. :-)

Comment 3

[Tomas Kopal]

Attachment: V1.1

Unbitrotted, no other change.

Comment 4

[Joel Peshkin]

         q{SELECT DISTINCT groups.name, groups.id
-            FROM groups, user_group_map, group_group_map AS ggm
+            FROM groups
+            LEFT JOIN user_group_map
+              ON groups.id = user_group_map.group_id
+            LEFT JOIN group_group_map AS ggm
+              ON groups.id = ggm.grantor_id
            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 = 1)
+                  OR (ggm.grant_type = } . GROUP_BLESS . q{ 
+                      AND user_group_map.group_id = ggm.member_id
                       AND user_group_map.isbless = 0))},
          { Columns=>[1,2] }, $self->{id});

Why are you changing this to a LEFT JOIN instead of an INNER JOIN?

Also, if you have to edit this for any reason, please try breaking it up into
about 5 smaller bugs.  This is a lot to review all at once and there is no
reason that any file's changes have to land at the same time as any other's.

Aside from that, I'm half-way through (up to editcomponents) so far and it looks
good so far.

Comment 5

[Tomas Kopal]

(In reply to comment #4)
> 
>          q{SELECT DISTINCT groups.name, groups.id
> -            FROM groups, user_group_map, group_group_map AS ggm
> +            FROM groups
> +            LEFT JOIN user_group_map
> +              ON groups.id = user_group_map.group_id
> +            LEFT JOIN group_group_map AS ggm
> +              ON groups.id = ggm.grantor_id
>             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 = 1)
> +                  OR (ggm.grant_type = } . GROUP_BLESS . q{ 
> +                      AND user_group_map.group_id = ggm.member_id
>                        AND user_group_map.isbless = 0))},
>           { Columns=>[1,2] }, $self->{id});
> 
> Why are you changing this to a LEFT JOIN instead of an INNER JOIN?

Because the parts I took from the original WHERE condition were connected by OR,
if I do both INNER JOINs, its AND.
How does MySQL handle the comma operator? You can't do INNER JOIN without
anything to join on, is it really INNER JOIN? Can't it be a CROSS JOIN? MySQL
documentation is a bit vague on this :-(.
If it is a CROSS JOIN, then we would have all combinations of rows and the WHERE
would limit the result (which is what I think is actually happening). As the
conditions are OR-ed, we'll get even rows when one of the maps is NULL.
I think that in order to get the same behaviour with explicit JOINs, we need
LEFT JOINs. Do I miss something?

> 
> Also, if you have to edit this for any reason, please try breaking it up into
> about 5 smaller bugs.  This is a lot to review all at once and there is no
> reason that any file's changes have to land at the same time as any other's.

Yeah, I was thinking about that before, but could not find any rule how to break
it. I will just split it to even parts next time. It's really a huge patch,
sorry :-).

> 
> Aside from that, I'm half-way through (up to editcomponents) so far and it looks
> good so far.
> 

Ahh, excellent. I was a bit afraid you'll run away screaming when you see the
size of the patch :-).

Comment 6

[Max Kanat-Alexander]

(In reply to comment #4)
>          q{SELECT DISTINCT groups.name, groups.id
> -            FROM groups, user_group_map, group_group_map AS ggm
> +            FROM groups
> +            LEFT JOIN user_group_map
> +              ON groups.id = user_group_map.group_id
> +            LEFT JOIN group_group_map AS ggm
> +              ON groups.id = ggm.grantor_id
>             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 = 1)
> +                  OR (ggm.grant_type = } . GROUP_BLESS . q{ 
> +                      AND user_group_map.group_id = ggm.member_id
>                        AND user_group_map.isbless = 0))},
>           { Columns=>[1,2] }, $self->{id});

  Yeah, I wrote this, and this change does not look correct to me.

  I am fairly certain that you need to move a lot of those WHERE conditions up
into the JOINs, particularly the isbless ones.

Comment 7

[Max Kanat-Alexander]

OK, looking at bug 279693 (where I wrote that statement), and I'm remembering
what the heck this code does. :-)

I think that the LEFT JOINs are *probably* OK, but I'd have to test with bless
group that was only inherited to be certain.

Note the "user_group_map.group_id = ggm.member_id" that I have in there, also.
Should that be in the join? I think with that change it makes the joins an INNER
JOIN, although I'm not certain.

Comment 8

[Tomas Kopal]

OK, for that User.pm part, I re-derived it again and this is what I ended up
with. Is this looking more correct than the previous one?

SELECT DISTINCT groups.name, groups.id
FROM      groups
LEFT JOIN user_group_map
       ON (groups.id = user_group_map.group_id
      AND  user_group_map.isbless = 1)
LEFT JOIN group_group_map AS ggm
       ON (groups.id = ggm.grantor_id
      AND  ggm.grant_type = GROUP_BLESS
      AND  user_group_map.group_id = ggm.member_id
      AND  user_group_map.isbless = 0)
WHERE user_group_map.user_id = ?
  AND ((user_group_map.group_id IS NOT NULL) OR (ggm.member_id IS NOT NULL))

Comment 9

[Tomas Kopal]

OK, this *should* be correct and is even simpler to understand :-). Comments?

SELECT DISTINCT groups.name, groups.id
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)
WHERE (user_group_map.isbless = 1) OR (ggm.member_id IS NOT NULL)

Comment 10

[Tomas Kopal]

(In reply to comment #9)
> WHERE (user_group_map.isbless = 1) OR (ggm.member_id IS NOT NULL)

Actually, isbless is a boolean, so we don't even need the ' = 1' part :-).

Comment 11

[Max Kanat-Alexander]

No!! We have no real booleans in PostgreSQL!! We have only smallints. You must
use the = 1.

Comment 12

[Joel Peshkin]

Comment on attachment 177666 [details] [diff] [review]
V1.1

Based on the last few comments, I presume a new version is coming

Comment 13

[Tomas Kopal]

(In reply to comment #12)
> (From update of attachment 177666 [details] [diff] [review] [edit])
> Based on the last few comments, I presume a new version is coming
> 

Actually, I was waiting for you to finish the review and incorporate all your
comments in one shot, so you don't have to go through the whole lot again.
But if you prefer updated version (split into few pieces), I can do the update
first.

Comment 14

[Tomas Kopal]

Attachment: V1.2 part 1

Here is an updated version, split into smaller pieces.

Comment 15

[Tomas Kopal]

Attachment: V1.2 part 2

Comment 16

[Tomas Kopal]

Attachment: V1.2 part 3

Comment 17

[Tomas Kopal]

Attachment: V1.2 part 4

Comment 18

[Joel Peshkin]

Comment on attachment 178343 [details] [diff] [review]
V1.2 part 1

     # for this to function properly in all circumstances.
     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});

This is a logic change, please explain

Comment 19

[Joel Peshkin]

Comment on attachment 178344 [details] [diff] [review]
V1.2 part 2

r=joel with or without a change to the nit below

One of these days (soon) we should expunge the remaining SendSQL calls, but
that is another bug.

nit:  
-	     $dbh->sql_to_days('creation_ts') . " != 'NULL' " .
+	     $dbh->sql_to_days('creation_ts') . " != 'NULL'" .
	     $and_product .
-	     "ORDER BY start " . $dbh->sql_limit(1));

We're in double-quotes here, why not just stay there and then we wont have to
worry about someone forgetting to start $and_product with whitespace.

Comment 20

[Joel Peshkin]

Comment on attachment 178346 [details] [diff] [review]
V1.2 part 4

    # the user should not have access.
-    " FROM	 flags 
-		 LEFT JOIN attachments ON ($attach_join_clause), 
-		 flagtypes, 
-		 profiles AS requesters
-		 LEFT JOIN profiles AS requestees 
-		   ON flags.requestee_id  = requestees.userid, 
-		 bugs 
-		 LEFT JOIN products ON bugs.product_id = products.id
-		 LEFT JOIN components ON bugs.component_id = components.id
-		 LEFT JOIN bug_group_map AS bgmap 
-		   ON bgmap.bug_id = bugs.bug_id 
-		 LEFT JOIN user_group_map AS ugmap 
-		   ON bgmap.group_id = ugmap.group_id 
-		   AND ugmap.user_id = $::userid 
+    " FROM	 flags
+		 LEFT JOIN attachments
+		   ON ($attach_join_clause)
+		 INNER JOIN flagtypes
+		   ON flags.type_id = flagtypes.id
+		 INNER JOIN profiles AS requesters
+		   ON flags.setter_id = requesters.userid
+		 LEFT JOIN profiles AS requestees
+		   ON flags.requestee_id  = requestees.userid
+		 INNER JOIN bugs
+		   ON flags.bug_id = bugs.bug_id
+		 LEFT JOIN products
+		   ON bugs.product_id = products.id
+		 LEFT JOIN components
+		   ON bugs.component_id = components.id
+		 LEFT JOIN bug_group_map AS bgmap
+		   ON bgmap.bug_id = bugs.bug_id
+		 LEFT JOIN user_group_map AS ugmap
+		   ON bgmap.group_id = ugmap.group_id
+		   AND ugmap.user_id = $::userid
		   AND ugmap.isbless = 0
-		 LEFT JOIN cc AS ccmap 
-		 ON ccmap.who = $::userid AND ccmap.bug_id = bugs.bug_id 
-    " . 
-    # All of these are inner join clauses.  Actual match criteria are added
-    # in the code below.
-    " WHERE	 flags.type_id	     = flagtypes.id
-      AND	 flags.setter_id     = requesters.userid
-      AND	 flags.bug_id	     = bugs.bug_id
+		 LEFT JOIN cc AS ccmap
+		   ON ccmap.who = $::userid
+		   AND ccmap.bug_id = bugs.bug_id
     ";

please explain logic change

Aside from that, the rest of this patch is OK.

Comment 21

[Joel Peshkin]

Comment on attachment 178346 [details] [diff] [review]
V1.2 part 4

disregard comment 20...  got it
r=joel

Comment 22

[Joel Peshkin]

Thomas - I suggest you land part 2 and 4 and then try for part 1 and 3 under
another bug.  I started to review part 3 and saw a bunch of apparrent logic
changes.  I'll leave those for another time after we finish part 1.

Comment 23

[Joel Peshkin]

Comment on attachment 178343 [details] [diff] [review]
V1.2 part 1

cancelling extra review request...  will land in stages

Comment 24

[Joel Peshkin]

I'm requesting approval for parts 2 and 4 only.  

Comment 25

[Max Kanat-Alexander]

Checking in CGI.pl;
/cvsroot/mozilla/webtools/bugzilla/CGI.pl,v  <--  CGI.pl
new revision: 1.236; previous revision: 1.235
done
Checking in attachment.cgi;
/cvsroot/mozilla/webtools/bugzilla/attachment.cgi,v  <--  attachment.cgi
new revision: 1.79; previous revision: 1.78
done
Checking in buglist.cgi;
/cvsroot/mozilla/webtools/bugzilla/buglist.cgi,v  <--  buglist.cgi
new revision: 1.289; previous revision: 1.288
done
Checking in checksetup.pl;
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.383; previous revision: 1.382
done
Checking in collectstats.pl;
/cvsroot/mozilla/webtools/bugzilla/collectstats.pl,v  <--  collectstats.pl
new revision: 1.43; previous revision: 1.42
done
Checking in editclassifications.cgi;
/cvsroot/mozilla/webtools/bugzilla/editclassifications.cgi,v  <-- 
editclassifications.cgi
new revision: 1.11; previous revision: 1.10
done
Checking in editcomponents.cgi;
/cvsroot/mozilla/webtools/bugzilla/editcomponents.cgi,v  <--  editcomponents.cgi
new revision: 1.53; previous revision: 1.52
done
Checking in editflagtypes.cgi;
/cvsroot/mozilla/webtools/bugzilla/editflagtypes.cgi,v  <--  editflagtypes.cgi
new revision: 1.17; previous revision: 1.16
done
Checking in editgroups.cgi;
/cvsroot/mozilla/webtools/bugzilla/editgroups.cgi,v  <--  editgroups.cgi
new revision: 1.52; previous revision: 1.51
done
Checking in editmilestones.cgi;
/cvsroot/mozilla/webtools/bugzilla/editmilestones.cgi,v  <--  editmilestones.cgi
new revision: 1.36; previous revision: 1.35
done
Checking in process_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v  <--  process_bug.cgi
new revision: 1.245; previous revision: 1.244
done
Checking in request.cgi;
/cvsroot/mozilla/webtools/bugzilla/request.cgi,v  <--  request.cgi
new revision: 1.21; previous revision: 1.20
done
Checking in sanitycheck.cgi;
/cvsroot/mozilla/webtools/bugzilla/sanitycheck.cgi,v  <--  sanitycheck.cgi
new revision: 1.92; previous revision: 1.91
done
Checking in showdependencytree.cgi;
/cvsroot/mozilla/webtools/bugzilla/showdependencytree.cgi,v  <-- 
showdependencytree.cgi
new revision: 1.32; previous revision: 1.31
done
Checking in summarize_time.cgi;
/cvsroot/mozilla/webtools/bugzilla/summarize_time.cgi,v  <--  summarize_time.cgi
new revision: 1.6; previous revision: 1.5
done
Checking in userprefs.cgi;
/cvsroot/mozilla/webtools/bugzilla/userprefs.cgi,v  <--  userprefs.cgi
new revision: 1.73; previous revision: 1.72
done
Checking in votes.cgi;
/cvsroot/mozilla/webtools/bugzilla/votes.cgi,v  <--  votes.cgi
new revision: 1.28; previous revision: 1.27
done
Checking in whineatnews.pl;
/cvsroot/mozilla/webtools/bugzilla/whineatnews.pl,v  <--  whineatnews.pl
new revision: 1.17; previous revision: 1.16
done