View Issue Details

IDProjectCategoryView StatusLast Update
0017609phplist applicationMessage Send Processpublic06-07-15 18:04
Reporterduncanc 
PrioritynormalSeverityminorReproducibilityalways
Status resolvedResolutionfixed 
Product Version3.0.12 
Target Version3.0.XFixed in Version3.2.0 
Summary0017609: Use of join without an ON clause
DescriptionThere is an sql query that uses JOIN without an ON clause, which means that it is joining every row from one table with every row from the second table. The use of DISTINCT hides this problem.

See file actions/processqueue.php line 795

    $query
    = ' select distinct u.id'
    . ' from %s as listuser'
    . ' cross join %s as u'
    . ' cross join %s as listmessage'
    . ' left join %s as um'
    . ' on (um.messageid = ? and um.userid = listuser.userid)'
    . ' where true'
    . ' and listmessage.messageid = ?'
    . ' and listmessage.listid = listuser.listid'
    . ' and u.id = listuser.userid'
    . ' and um.userid IS NULL'
    . ' and u.confirmed and !u.blacklisted and !u.disabled'
    . ' %s %s';

Also, it would be clearer to use INNER JOIN instead of CROSS JOIN. Although these are equivalent in mysql, in other sql they have different meanings.


TagsNo tags attached.

Activities

support

14-02-15 22:42

administrator   ~0055931


interesting. Have you done any tests to see if there's a speed difference?

support

14-02-15 22:43

administrator   ~0055932

hang on, there is an ON in that query, isn't there?

duncanc

15-02-15 12:23

developer   ~0055933

There are three joins, the last of which has an ON condition, the first two do not. But you have put the ON conditions in the WHERE clause of the query and it looks like mysql uses those to avoid the cartesian product that I was thinking about.

But it is a really confusing approach and likely to be error prone by not putting the conditions in the JOIN itself

I suggest moving conditions to the joins

join %s as u on listuser.userid = u.id
join %s as listmessage on listuser.listid = listmessage.listid

michiel

27-02-15 09:38

manager   ~0055950

https://github.com/michield/phplist/commit/ba5a576c7d6e0406b6f5d69d067505e406d30be5