MySQL Lists are EOL. Please join:

List:Commits« Previous MessageNext Message »
From:Antony T Curtis Date:March 14 2008 10:56pm
Subject:Re: bk commit into 6.0 tree (antony:1.2573) WL#3771
View as plain text  
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
Thread
bk commit into 6.0 tree (antony:1.2573) WL#3771antony22 Feb
  • Re: bk commit into 6.0 tree (antony:1.2573) WL#3771Sergei Golubchik27 Feb
    • Re: bk commit into 6.0 tree (antony:1.2573) WL#3771Antony T Curtis29 Feb
      • Re: bk commit into 6.0 tree (antony:1.2573) WL#3771Sergei Golubchik14 Mar
        • Re: bk commit into 6.0 tree (antony:1.2573) WL#3771Antony T Curtis14 Mar
          • Re: bk commit into 6.0 tree (antony:1.2573) WL#3771Sergei Golubchik17 Mar