Implicit joins should be replaced by explicit joins - installment B

RESOLVED FIXED in Bugzilla 2.20

Status

()

RESOLVED FIXED
14 years ago
14 years ago

People

(Reporter: Tomas.Kopal, Assigned: mkanat)

Tracking

2.19.2
Bugzilla 2.20
Bug Flags:
approval +

Details

Attachments

(1 attachment, 2 obsolete attachments)

4.09 KB, patch
mkanat
: review+
LpSolit
: review+
Details | Diff | Splinter Review
(Reporter)

Description

14 years ago
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).
(Reporter)

Comment 1

14 years ago
Posted patch V2 (obsolete) — Splinter Review
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.
Attachment #179641 - Flags: review?(bugreport)
(Reporter)

Updated

14 years ago
Status: NEW → ASSIGNED

Comment 2

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


(Assignee)

Comment 3

14 years ago
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.
(Reporter)

Comment 4

14 years ago
(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 ;-).
(Reporter)

Comment 5

14 years ago
(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.
(Assignee)

Comment 6

14 years ago
(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.
(Reporter)

Updated

14 years ago
Attachment #179641 - Flags: review?(bugreport)
(Reporter)

Comment 7

14 years ago
Posted patch V2.1 (obsolete) — Splinter Review
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 :-).
Attachment #179641 - Attachment is obsolete: true
Attachment #179904 - Flags: review?(bugreport)

Comment 8

14 years ago
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.
Attachment #179904 - Flags: review?(bugreport) → review+
(Assignee)

Comment 9

14 years ago
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.
Attachment #179904 - Flags: review?(wicked)
(Assignee)

Updated

14 years ago
Attachment #179904 - Flags: review?(wicked) → review?(LpSolit)

Comment 10

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

Bitrotten due to the rewrite of FlagType::get_clusions().
Attachment #179904 - Flags: review?(LpSolit) → review-
(Assignee)

Comment 11

14 years ago
Posted patch v2.2Splinter Review
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.
Assignee: Tomas.Kopal → mkanat
Attachment #179904 - Attachment is obsolete: true
Attachment #187859 - Flags: review+
(Assignee)

Comment 12

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

LpSolit, could you test it?
Attachment #187859 - Flags: review?(LpSolit)
(Assignee)

Updated

14 years ago
Target Milestone: --- → Bugzilla 2.20
Version: unspecified → 2.19.2

Comment 13

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

tested, works fine. r=LpSolit
Attachment #187859 - Flags: review?(LpSolit) → review+

Updated

14 years ago
Flags: approval?
Flags: approval? → approval+
(Assignee)

Comment 14

14 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.