Hi Marc,
1) I have the impression that all m_flags class members are unused
except in some DBUG_ASSERT(), is that true? If yes, they just add
complexity for little value. Can they be removed? PSI_FLAG_GLOBAL is unused?
2) Looking at new files in mysql-test/, I see only tests which verify
that P_S table exist, that forbidden operations are really forbidden
(like deletions in certain P_S tables), but nothing which tests that P_S
gives correct statistical numbers. How about testing the instrumentation
of key objects, for example:
- create a MyISAM table and verify that P_S reports that
THR_LOCK_myisam, LOCK_mdl, etc were taken the expected number of times
- same with conditions, tables, threads...
?
3) About your recent fix with HA_FAST_KEY_READ. As you wrote, P_S is
fast to read keys in random order, so in all logic it should have this
flag, but for other reasons (consistency) we rather have to rely on a
trick (don't use HA_FAST_KEY_READ and rows will be cached). As the
behaviour isn't guaranteed to stay (I don't think Optimizer folks will
remember that they MUST keep caching on for the sake of P_S), could you
please write a .test, so that if the behaviour is later broken this is
caught?
I understand that this involves concurrency, but this is testable with
"debug sync points" (they exist in 6.0 and are used to test Online
Backup); here's how: I think you need a second thread to do something
while a first thread is between rnd_next() and rnd_pos(); then you would
put a debug sync point between those two calls; the test would make the
first thread start its SELECT statement (which reads P_S tables if I
understood correctly), pause at the sync point, then the test would let
the second thread run which overwrites data in P_S tables, and the
trouble should come when the test does "reap" in the first thread. See
example in suite/backup/t/backup_lock_myisam.test. Test should fail when
HA_FAST_KEY_READ is used, blah blah you know the thing. If you have
trouble with setting up your first sync points, just ask.
It's ok to not do it right now, but I hope you could put this in your
short-term todo (before push to main).
In Maria, sometimes for certain fixes of concurrency bugs I haven't
written a .test, but that was only because I knew that those bugs are
automatically always tested by the Random Query Generator in pushbuild2
for every push.
4) In what user-visible view do the *_lock_stat counters
(read_lock_stat, write_lock_stat etc) end up?
5) I'm looking at end_mutex_wait_v1(). There seems to be a nice logic
with statistics rolling up to their aggregates rolling up to their
aggregates (aggregate_single_stat_chain()) but close to this, a
hard-coded rolling up:
if (flag_events_waits_summary_by_thread_by_event_name).
Calls to aggregate_single_stat() (not the _chain() version) are always
tied to flag_events_waits_summary_by_thread_by_event_name. Is that
summary table "breaking your nice model" somehow?
Thanks.
--
Mr. Guilhem Bichot <guilhem@stripped>
Sun Microsystems / MySQL, Lead Software Engineer
Bordeaux, France
www.sun.com / www.mysql.com
