On 14 Mar, 2008, at 13:02, Sergei Golubchik wrote:
> Hi!
>
> On Feb 29, Antony T Curtis wrote:
>> On 27 Feb, 2008, at 12:52, Sergei Golubchik wrote:
>>
>>>> --- /dev/null Wed Dec 31 16:00:00 196900
>>>> +++ b/include/mysql/plugin_audit.h 2008-02-22 08:56:22 -08:00
>>>> @@ -0,0 +1,78 @@
>>> .....
>>>> +#define MYSQL_AUDIT_CLASS_MASK_SIZE 1
>>>> +typedef unsigned long long
>>>> mysql_audit_mask_t[MYSQL_AUDIT_CLASS_MASK_SIZE];
>>>> +
>>>> +struct st_mysql_audit
>>>> +{
>>>> + int interface_version;
>>>> + const mysql_audit_mask_t *class_mask;
>>>
>>> sorry, I don't understand - a pointer to an array ?
>>> You had a pointer to ulonglong, I suggested to change a pointer to
>>> an array. But now you have a pointer to an array. One dereference
>>> too much.
>>
>> Pointer to an array, same amount of dereferencing as original pointer
>> to ulonglong except that array is now not unbounded.
>
> okay, but why not to have this array in the st_mysql_audit structure ?
future proofing... allows mask to grow without changing structure.
Additional logic can be added in the future so that for audit iface
version X has A class_mask elements etc.
>
>>>> + void (*release_thd)(MYSQL_THD);
>>>> + void (*event_notify)(MYSQL_THD, const struct mysql_event *);
>>>> +};
>>>> +
>>>> diff -Nrup a/sql/mysqld.cc b/sql/mysqld.cc
>>>> --- a/sql/mysqld.cc 2008-02-14 01:00:48 -08:00
>>>> +++ b/sql/mysqld.cc 2008-02-22 08:56:20 -08:00
>>>> @@ -1239,6 +1240,7 @@ extern "C" void unireg_abort(int exit_co
>>>> clean_up(!opt_help && (exit_code || !opt_bootstrap)); /*
>>>> purecov: inspected */
>>>> DBUG_PRINT("quit",("done with cleanup in unireg_abort"));
>>>> wait_for_signal_thread_to_end();
>>>> + mysql_audit_finalize();
>>>
>>> why did you move it from clean_up_mutexes() ?
>>>
>>>> clean_up_mutexes();
>>>> my_end(opt_endinfo ? MY_CHECK_ERROR | MY_GIVE_INFO : 0);
>>>> exit(exit_code); /* purecov: inspected */
>>>> @@ -4313,6 +4325,7 @@ int main(int argc, char **argv)
>>>> #endif
>>>> clean_up(1);
>>>> wait_for_signal_thread_to_end();
>>>> + mysql_audit_finalize();
>>>
>>> Cannot you have mysql_audit_finalize() in one place ?
>>
>> It seemed to interfere with my aesthetic sense having it in
>> clean_up_mutexes() because all the other lines in that function are
>> pthread_mutex_destroy() calls.
>
> okay, I can see that.
>
> Could you try to find a place for this call to have it only once and
> still not interfere with your aesthetic sense ?
ok, I shall investigate suitable locations.
>
>>>> clean_up_mutexes();
>>>> my_end(opt_endinfo ? MY_CHECK_ERROR | MY_GIVE_INFO : 0);
>>>>
>>>> diff -Nrup a/sql/scheduler.cc b/sql/scheduler.cc
>>>> --- a/sql/scheduler.cc 2007-12-13 09:17:25 -08:00
>>>> +++ b/sql/scheduler.cc 2008-02-22 08:56:20 -08:00
>>>> @@ -666,6 +667,8 @@ static bool libevent_needs_immediate_pro
>>>> static void libevent_thd_add(THD* thd)
>>>> {
>>>> char c=0;
>>>> + /* release any audit resources, this thd is going to sleep */
>>>> + mysql_audit_release(thd);
>>>
>>> Why do you need it here ?
>>> 1. Why not to do it in do_command(), at the end of the statement
>>> processing, that is ?
>>> 2. Having it in libevent_thd_add implies that it depends on the
>>> scheduler. We'll need to patch every new scheduler that could be
>>> created, then ?
>>
>> minor optimization, don't bother releasing audit plugins when you're
>> busy executing statements. releasing is deferred until thd is idle.
>> reduces overall overhead, especially in a busy machine.
>
> When will you release audit plugins in the old thread-per-connection
> scheduler ?
Hmm... It should be already there before it blocks on socket. It looks
like it was forgotten when I applied patch to this tree. It should be
in sql_connect.cc, the inner loop of handle_one_connection(), before
the do_command(thd) statement.
>
>>>> pthread_mutex_lock(&LOCK_thd_add);
>>>> /* queue for libevent */
>>>> thds_need_adding= list_add(thds_need_adding,
> &thd->scheduler.list);
>
> Regards / Mit vielen Grüssen,
> Sergei
Regards,
Antony
