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

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

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

Unbitrotted, no other change.

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.

(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 :-).

(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.

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.

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

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)

(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 :-).

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

Comment on attachment 177666 [details] [diff] [review] V1.1 Based on the last few comments, I presume a new version is coming

(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.

Here is an updated version, split into smaller pieces.

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 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 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 on attachment 178346 [details] [diff] [review] V1.2 part 4 disregard comment 20... got it r=joel

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 on attachment 178343 [details] [diff] [review] V1.2 part 1 cancelling extra review request... will land in stages

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

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