maria-developers team mailing list archive

[MARK CALLAGHAN <mdcallag@xxxxxxxxx>]

Pardon my manual cut & paste, gmail didn't format the reply cleanly:
>>>
>From Monty
I just finished applying / modifying / rewriting the userstats patch
to MariaDB 5.2 and will commit this in a moment.

As far as I know, the patch was originally made by Mark and his team,
then addopted by Percona and fixed/updated by Arjen & Weldon.

I send this patch in advance here, so that people that has been
involved with the patch can comment on what/if still needs to be
done and what would be the next steps.

I will write extensive comments in the commit message, but here are
the highlights:

- Almost all counters are done through 'thd->status_var' (no separate
 counters used). This was done by creating a copy of thd->status_var
 at start of execution and adding the difference to user stats.

 The benefit of the above is:
 - Less code
 - No double counters (faster execution)
 - Trivial to add new counters to the statistics tables
   (There is a lot of counters already that can be used)
 - No need to reset counters
 - If 'userstat' is not set, very little overhead.

- Changed all of MariaBB code to use handler::ha_read... instead if
 handler::read.  This allowed me to have all counters in the
 ha_... wrapper

 The benefit are:
  - No need to change engine code
  - All engines are now measured

- Formatted code to 'MariaDB' style.
- Removed not called functions.
- A lot of small speed improvements
 - Optimized hash keys to not have to do strlen().
 - Store unique keys in 'keyinfo' to not have to generate
   keys on the fly.
 - Replaced 'if's with DBUG_ASSERT for things that I thought was
   impossible.

- Change naming of some variables to be more consistent
 - userstat_running -> user_stat
 - Rows_fetched -> Rows changed

- Added statistics variables (for user and client statistics)
 Rows_updated, Rows_deleted, Rows_inserted

- Changed busy and cpu timing to double (as in Weldon's patch)

- Changed position of a few columns in the information schemas to
 group things in a more logical way.

Things that I would like to have comments one from the original
authors are:

- I did not remove from sql_parse.cc:

 if (options & REFRESH_USER_RESOURCES)
   reset_mqh((LEX_USER *) NULL, 0);             /* purecov: inspected */

Don't understand why this should be removed.

- Don't call 'set_concurrent_connections_stats()' (as per Weldon's
 patch) to collect information from the running threads as this leads
 will lead to wrong counting as all variables are not updated until
 the whole command is run.

Here follows the patch (without the new test cases, the test cases
will be in the commit).
Note that I am now about to do a 'bzr gcommit'
and I may fix some minor issues while I do that.

You should be able to apply this directly to MariaDB 5.1 or you can
pull MariaDB 5.2 later today if you want to test this.

(MariaDB 5.2 is basicly MariaDB 5.1 + stable patches, so it's should
be safe to use).
>>>>

Thanks for cleaning this up.

The original implementations of SHOW USER_STATS and SHOW TABLE_STATS
had performance problems. SHOW TABLE_STATS was worse, it created
severe mutex contention on LOCK_open. Fixes were published in the v3,
v4 Google patches for table and user stats and in the Facebook patch
for table stats.

The fix is to cache pointers into the global hash tables used to store
stats to reduce the duration for locked mutexes (search for
global_table_stats_version, global_user_stats_version in the v4 Google
patch or global_table_stats_version in the Facebook patch). The fix
for table_stats is a bit more complex. I changed code to update stats
before getting LOCK_open when possible. That change is easy to see in
the Facebook patch at
http://bazaar.launchpad.net/~mysqlatfacebook/mysqlatfacebook/trunk/revision/53

In their current form, the performance overhead was small. Although I
will quantify that again soon using the Facebook patch. I don't think
it is worth the effort to make the overhead even lower, but that could
be done by using the concurrent hash table added for the performance
schema or by extending the existing shared user/account object to
store statistics and by adding a shared table object to which stats
can be aggregated.

I don't know whether the userstats patch from Percona ever picked up
the fixes or whether it had the problems. But from the patch you have
inlined, I did not see global_table_stats_version and I will guess
that your patch has performance problems.

When did the move to ha_ functions get done so that I can add code
there and avoid changing each engine?

I also think that you should commit support for SHOW TABLE_STATS and
SHOW USER_STATS separately.

The v3/v4 Google patch doesn't have set_concurrent_connections_stats()
and I think the problem that Weldon referenced has been fixed. But the
requirement to compute concurrent connections per account made the
implementation for SHOW USER_STATS much more complex. Do you want to
continue to support that?

-- 
Mark Callaghan
mdcallag@xxxxxxxxx