PR https://github.com/mantisbt/mantisbt/pull/1898

@dregad I noticed that our "Manage User" page is slow.
I suspect it's caused by the user_is_email_unique calls you introduced to show the icon for non-unique emails.
You might want to remove it again, as there is not much benefit for installations with thousands of users.

I'll have a look if it can be optimized and will revert if not.

I ran quick local test with a dump of mantisbt.org tracker's user table (44423 rows):

• master (0553a42): Page execution time: 0.6156 seconds Memory usage: 3,999 KiB Total queries executed: 66 Total query execution time: 0.3678 seconds
• commit d6681928: Page execution time: 0.6105 seconds Memory usage: 4,081 KiB Total queries executed: 16 Total query execution time: 0.3592 seconds

So it does execute 50 additional queries (as expected for the uniqueness check), but the overall performance is nearly the same.

On mantisbt.org on the other hand, the same page executes in approx. 3 seconds, vs 0.2 seconds with commit d66819280.

So it seems environment specific, maybe a missing or corrupt index on the DB ?

Git bisect identifies MantisBT master bf7a3c22 as the offending commit (no surprise).

I tried optimizing and repairing the table, but that did not change anything. I don't understand why I'm not seeing the performance degradation locally. Any ideas ?

I don't understand why I'm not seeing the performance degradation locally

I remember you are using some script to change the original email addresses for local testing purposes.
Maybe you change all addresses to the same one or clear them?

[EDIT] Forget about that, the script adds the user name to the addresses.
https://github.com/mantisbt/mantisbt-tools/blob/master/db-anonymize.sql

Maybe replacing the ILIKE by comparing with UPPER(email) works better for some databases.

I remember you are using some script to change the original email addresses for local testing purposes.

I did not use it in this specific case, to make sure I had a test case as close as possible to the original. My local database is an exact copy.

Maybe replacing the ILIKE by comparing with UPPER(email) works better for some databases

It could be, but in this specific case I don't think that's the issue as both environments are running MySQL (8.0 on the server, 8.1 on my laptop).

PR https://github.com/mantisbt/mantisbt/pull/1919 improves the performance issue with manage_user_page.php reported by @atrol in 0032787:0068077.

With this, the page loads under 0.5 seconds on this tracker, vs approx. 3 seconds before this change. This is still about twice as slow as before MantisBT master d6681928, but I believe that's acceptable for an admin page.

@dregad your changes to add admin check to detect duplicate e-mails introduces a side effect.
If $g_email_ensure_unique is enabled and duplicate emails are found (e. g. about 1000 in our own tracker), the check fails (check_print_test_row), so that the complete check process stops and you can't run the rest of the checks.

As a quick solution, you might want to change from check_print_test_row to check_print_test_warn_row.

The mid-term solution would be to enhance the concept of $g_failed_test (stop running tests after the first one failed).
Of course, some failing tests are blocking and must be fixed to run further tests, but not all of them.

It's not really a side-effect, it's totally by design...

The rationale is that if you configured the system for unique e-mail addresses and your system does not comply then you have a real error condition and you need to fix the problem as it could cause problems. I don't agree that this should be downgraded to a warning.

We do have a specific problem in this tracker due to the large number of non-compliant of legacy accounts and the effort it would take to fix them.

So indeed the real problem is the way our admin test handle the failures. Maybe we should introduce a concept of "blocking" errors that cause the checks to abort, vs non-critical failures that lets them continue.

I don't agree that this should be downgraded to a warning.

Right, I should have written "temporary workaround" instead of "temporary solution".

We do have a specific problem in this tracker due to the large number of non-compliant of legacy accounts and the effort it would take to fix them.

I suspect we are not alone.
Be aware that option $g_email_ensure_unique was introduced with questionable default settng ON in version 1.3, so that installations that started before 1.3 are also affected.

Yes I know, this is exactly why I added the warning in the Admin Guide (see MantisBT master 18ea9a43).

The admin must be aware that these errors must be fixed, particularly when using REST API, as the system expects the email to be unique and if it is not then user_get_id_by_email() may silently return the wrong user, leading to unpredictable and unwanted system behavior (for example: updating an issue's reporter or handler, assigning a user to a project, etc) particularly when the accounts with duplicate emails do not have the same access level.

@atrol, following up on your note 0032787:0068167 and subsequent discussion, I had a closer look yesterday

The mid-term solution would be to enhance the concept of $g_failed_test (stop running tests after the first one failed).
Of course, some failing tests are blocking and must be fixed to run further tests, but not all of them.

Indeed this is the key, and I actually believe it would not be so hard to fix this - we just need a concept of critical failure (meaning that if the test fails, then it is not possible to continue with the Checks), in addition to the existing fail, warn and info checks.

So the only difficulty is to identify those truly critical tests that should cause the Admin Checks to abort. I have not looked in detail yet, but off-hand I think only some failures in the database and paths checks would qualify.

Can you think of others ?

@dregad right, most of the blocking checks are part of check_database_inc.php
In a first step, I would add a new function check_print_test_block_row that identifies the blocking checks in addition to check_print_test_row and check_print_test_warn_row.
After that, check all current checks and change some check_print_test_row to check_print_test_block_row

To start with check_database_inc.php:
Some of the warnings should even no longer be a warning, but critical / blocking in 2.26.0

    check_print_test_warn_row(
        'PHP support for Microsoft SQL Server driver',
        'mssql' != $t_database_type,
        array( false => "'mssql' driver is no longer supported in PHP >= 5.3, please use 'mssqlnative' instead" )
    );

    check_print_test_warn_row(
        'PHP support for MySQL driver',
        'mysql' != $t_database_type,
        array( false => "'mysql' driver is deprecated as of PHP 5.5.0, please use 'mysqli' instead" )
    );

These ones must no longer block further tests, although they can be critical in terms of runtime.
So keep them like they are at the moment, but change the remaining ones to check_print_test_block_row

        check_print_test_row(
            'MySQL version is a General Availability (GA) release',
            $t_mysql_ga_release,
            array(
                true => 'You are using MySQL version ' . htmlentities( $t_db_version ) . '.',
                false => 'The version of MySQL you are using is '
                    . htmlentities( $t_db_version )
                    . '. This is a development or pre-GA version which '
                    . ( $t_versions[$t_db_major_version][0] == 'Discontinued' ? 'has been discontinued and ' : '' )
                    . 'is not recommended for Production use. You should upgrade to a supported GA release.'
            ) );

        # Within lifecycle 'Extended' support
        check_print_test_row(
            'MySQL version is within the ' . $t_support_url . ' period (GA + 8 years)',
            date_create( $t_date_extended_end ) > date_create( 'now' ),
            array(
                true => 'Extended support for MySQL ' . $t_db_major_version . ' series ends on ' . $t_date_extended_end,
                false => 'Support for the release of MySQL you are using ('
                    . htmlentities( $t_db_version )
                    . ') ended on ' . $t_date_extended_end
                    . '. It should not be used, as security flaws discovered in this version will not be fixed.'
            ) );

    check_print_test_row(
        'Version of PostgreSQL is ' . $t_support_url . '',
        date_create( $t_date_eol ) > date_create( 'now' ),
        array(
            false => 'PostgreSQL version ' . htmlentities( $t_db_version )
                . ' is no longer supported and should not be used, as security flaws discovered in this version will not be fixed.'
        ) );

    check_print_test_row(
        'Database default collation is UTF-8',
        check_is_collation_utf8( $t_collation ),
        array( false => 'Database is using '
            . htmlentities( $t_collation )
            . ' collation where UTF-8 collation is required.' )
    );

            check_print_test_row(
                'Table <em>' . htmlentities( $t_row['name'] ) . '</em> is using UTF-8 collation',
                check_is_collation_utf8( $t_row['collation'] ),
                array( false => 'Table ' . htmlentities( $t_row['name'] )
                    . ' is using ' . htmlentities( $t_row['collation'] )
                    . ' collation where UTF-8 collation is required.' )
            );

                check_print_test_row(
                    'Text column <em>' . htmlentities( $t_row['field'] )
                    . '</em> of type <em>' . $t_row['type']
                    . '</em> on table <em>' . htmlentities( $t_table )
                    . '</em> is using UTF-8 collation',
                    check_is_collation_utf8( $t_row['collation'] ),
                    array( false => 'Text column ' . htmlentities( $t_row['field'] )
                        . ' of type ' . $t_row['type']
                        . ' on table ' . htmlentities( $t_table )
                        . ' is using ' . htmlentities( $t_row['collation'] )
                        . ' collation where UTF-8 collation is required.' )
                );

If you agree with the general approach, I could go thru the other check files and identify the checks where a change is needed.

@dregad, as 0032787:0068195 is a bit off-topic to the current issue and will introduce even more delay in getting out 2.26.0:
What about a quick workaround in 2.26.0 but implement the better check approach in 2.27.0? E. g. just move the email tests to the end of the tests?

I have opened 0033012 to track and follow-up on this so we can get it out of the way.

I'll see what's the best workaround - moving email to the end or maybe remove the if( $g_failed_test ) check.

or maybe remove the if( $g_failed_test ) check

I guess you mean remove all if( $g_failed_test ) checks, as removing one single check will not solve the issue.
Another idee for a quick workaround: set $g_failed_test = fallse after include( 'check_email_inc.php' );

Right, it's a typo... I forgot the s, but you got the idea anyway ;-)

PR to allow failing email checks https://github.com/mantisbt/mantisbt/pull/1928