C:\src\mantis>cvs diff -ub -r 1.141 core\filter_api.php
Empty password used - try 'cvs login' with a real password
Index: core/filter_api.php

Maybe we need to have a flag that indicates if the used DBMS supports nested queries or not, if so, then we use sub-queries to improve performance. I wonder if such flag is already supported in ADODB library. If so, then we should definately use it.

I haven't review the above patch in details, but the main concept in as far as I quickly see is the use of nested queries.

Some info for MySQL:

http://dev.mysql.com/doc/refman/5.0/en/subquery-restrictions.html
http://dev.mysql.com/doc/refman/5.0/en/ansi-diff-subqueries.html

Newer versions of ADODB; ie 4.93, have introduced a _nestedSQL property. It does not exist in the MySQL driver (yet?).

This fix causes http://bugs.mantisbt.org/view.php?id=7933.

Do you have any other fixes to fix both.

Thanks

At first I couldn't reproduce the issue on our company Mantis, but now I see it.

It seems related to the handling of joins in the subquery- I'm not sure if it is a bug in SQL2000 or Mantis, I have to really look at the query more. I'll also try it on a SQL2005 machine when I have a chance. Mixing ANSI-style joins and non-ANSI style joins is bound to eventually cause issues - either with the DBMS or the sanity of the programmer. :-)

You can fix it by changing the lines above the problem change to "LEFT" instead of "INNER" joins.
$t_id_join = $t_id_join . " LEFT JOIN $t_bugnote_table ON $t_bugnote_table.bug_id = $t_bug_table.id";
$t_id_join = $t_id_join . " LEFT JOIN $t_bugnote_text_table ON $t_bugnote_text_table.id = $t_bugnote_table.bugnote_text_id";

Ok, I see how the code works. We do 2 passes- one with the join to the bugnotes and one without, and then we eliminate the duplicates in PHP. My optimization only picks up the second of the 2 queries, and therefore misses any cases of text searches where there are no bugnotes.

Making it a left join and skipping the second query in the loop is probably the correct solution, since the whole "run it twice" thing seems hacky, especially because you're returning the same results from both queries most of the time.

That whole block of code, IMHO, should look like this. Much more readable:

    # Possibly do two passes. First time, grab the IDs of issues that match the filters. Second time, grab the IDs of issues that
    # have bugnotes that match the text search if necessary.
    $t_id_array = array();
        $t_id_where = $t_where;
        $t_id_join = $t_join;
        if ( !is_blank( $t_textsearch_wherejoin_clause ) ) {
            $t_id_where = $t_id_where . ' AND ' . $t_textsearch_wherejoin_clause;
            $t_id_join = $t_id_join . " LEFT JOIN $t_bugnote_table ON $t_bugnote_table.bug_id = $t_bug_table.id";
            $t_id_join = $t_id_join . " LEFT JOIN $t_bugnote_text_table ON $t_bugnote_text_table.id = $t_bugnote_table.bugnote_text_id";
        }
        $query  = "SELECT DISTINCT $t_bug_table.id AS id
                    $t_from
                    $t_id_join
                    $t_id_where";
            $result = db_query( $query );
            $row_count = db_num_rows( $result );

            for ( $j=0; $j < $row_count; $j++ ) {
                $row = db_fetch_array( $result );
                $t_id_array[] = db_prepare_int ( $row['id'] );
            }

    # Get the total number of bugs that meet the criteria.
    $bug_count = count( $t_id_array );

    $rows = array();

    if ( $bug_count > 0 ) {

#We've already built this query, so use it as a subquery
$t_where = "WHERE $t_bug_table.id in (" .$query . ")";
} else {
return $rows;
}

So, nested queries are now OK to use and I'd like to fix this for 1.1

Could you please attach a patch in a proper format for me to review? I see a lot of code but it is not clear what I should review/apply to CVS.

I'd really appreciate if you could follow the directions here:

http://www.mantisbt.org/wiki/doku.php/mantisbt:howto_submit_patches

and submit a proper patch

I've attached this as a patch. I do warn that this needs to be tested, both for performance and functionality, with mysql. I expect that it will work fine. It works fine for mssql, and I can pull up the "My View" screen for 0006844:0018000 issues in under 2 seconds, and a full text search (for all statuses) in about 13s.

Great.
I am going to review the code and test it with MySQL, so we can fix this in time for 1.1

Some forum thread relating to performance issues:
http://www.mantisbt.org/forums/viewtopic.php?p=10270

Another related forum thread:
http://www.mantisbt.org/forums/viewtopic.php?p=10273

Yet another thread:
http://www.mantisbt.org/forums/viewtopic.php?p=10274

Ok. In the revised patch I added some more changes because after applying the proposed patch some part of the code were just useless.

I also made few minor modifications related to code readability.

I tested the code but I will appreciate someone else willing to try the patch before I'll commit in CVS.

Minor possible performance suggestion, use (!$result->EOF) instead of db_num_rows() to know if or how many times to loop.

I've tried the latest patch provided to me by giallu today (28th of September) and for some reason it seems to be slower than the old one, it also runs the same number of queries. I've noticed that we run a query to get the number of elements, then re-run it again as a nested query. I would have thought that we will run this query once as a nested query, hence, we should run 1 less query.

Following are three measurements based on the new and the old filter API.

NEW FILTER_API (37 queries)

1.1877 SQL Queries Total Time
2.4224 Page Request Total Time

1.5427 SQL Queries Total Time
3.3641 Page Request Total Time

1.4332 SQL Queries Total Time
2.567 Page Request Total Time

OLD FILTER_API (37 queries)

0.2486 SQL Queries Total Time
1.6529 Page Request Total Time

0.2755 SQL Queries Total Time
1.993 Page Request Total Time

0.478 SQL Queries Total Time
2.7048 Page Request Total Time

I haven't did some minor testing on the results of the queries (in terms of items returned), but no thorough.

Well, I can't pretend I expected that but...

I noticed in the filter_get_bug_rows function 3 queries are run (this does not change after the patch); the difference is that after the patch the first one is used as subquery for the other two: now, since the first is the "heavy" one, I was wondering why it could be faster than before, since the heavy query is now run 3 times, instead of only once.

Even if we manage to not run the first one (but the num of rows from it is used to calculate the pagination parameters, so we should re-arrange the code a little bit), it will still run 2 heavy queries instead of one.

I should try to further analyze the actual queries, but you know, we are talking about the single 1100 lines filters function... I doubt it will make the cut for rc1

Lets see how we go, if we come up with something we are comfortable with, then fine, otherwise it won't make it in 1.1.0rc1. Understanding filter_api better would be great, since it will help us later to refactor this over complicated part of the code.

The worse performance on MySQL is somewhat expected because of limitations in their query optimizer when dealing with subqueries. From http://dev.mysql.com/doc/refman/5.0/en/subquery-restrictions.html:

"Subquery optimization for IN is not as effective as for the = operator or for IN(value_list) constructs.
[...]
Consider the following statement that uses an uncorrelated subquery:

SELECT ... FROM t1 WHERE t1.a IN (SELECT b FROM t2);

The optimizer rewrites the statement to a correlated subquery:

SELECT ... FROM t1 WHERE EXISTS (SELECT 1 FROM t2 WHERE t2.b = t1.a);

If the inner and outer queries return M and N rows, respectively, the execution time becomes on the order of O(M×N), rather than O(M+N) as it would be for an uncorrelated subquery.

An implication is that an IN subquery can be much slower than a query written using an IN(value_list) construct that lists the same values that the subquery would return."

The MSSQL query optimizer does not have the limitations that MySQL has, and subqueries like these perform just fine. MSSQL does have a limitation on the use of IN(value_list)- specifically if value_list contains more than a couple dozen values, the performance becomes very poor. So a fix for one DBMS will likely impact the other, and vice versa.

hello,

RZG, could you see if the SVN HEAD code improves this issue for you ?

Paul

I believe the issues raised in this are resolved in later releases/SVN TRUNK.

I know i've done some amount of work profiling database queries and php code across the code base.

If issues still remain in SVN trunk, can you log the remaining issues as new bugs.

Thanks
Paul

I can't say for trunk, but as for 1.1.2, my patch is still required to get the database queries to perform reasonably.

(My company mantis is at about 25000 issues spread across about 40 queues, with about 100 users.)

So, this looks anything but fixed ( and thanks to rzg for the great explanation why this is not an easy fix).

Maybe we can consider composing the best query based on the DB backend?

gaillu: as far as i'm concerned, i'm pretty sure it's fixed in 1.2.x

Paul

Well, given that filter api is undergoing a fair amount of changes, I'd close this and move on.

Let's open a new ticket afterwards if the code we will ship in 1.2 still performs badly

MantisBT currently supports Mysql and has support for other database engines.

The support for other databases is known to be problematic.

Having implemented the current database layer into Mantis 10 years ago, I'm currently working on replacing the current layer.

If you are interested in using Mantis with non-mysql databases - for example, Oracle, PGSQL or MSSQL, and would be willing to help out testing the new database layer, please drop me an email at paul@mantisforge.org

In the meantime, I'd advise running Mantis with Mysql Only to avoid issues.

Thanks
Paul