maria-developers team mailing list archive

[Michael Widenius <monty@xxxxxxxxxxxx>]

Hi!

>>>>> "Alex" == Alex Budovski <abudovski@xxxxxxxxx> writes:

Alex> Hi,
Alex> I've implemented a few things for the Windows port of SHOW PROFILE (
Alex> IO read/writes, user/kernel times, page faults) which you may want to
Alex> consider looking at. There's also a few miscellaneous fixes in there.
Alex> The commit log has the details.

Alex> See:  https://launchpad.net/~abudovski/maria/robust

Thanks, I am now doing a full review for the whole branch as of today.

I did a review based on the diff against the lastest 5.1 and your
branch:

bzr merge --preview lp:~abudovski/maria/robust 

=== modified file '.bzrignore'
--- .bzrignore	2010-01-29 10:45:46 +0000
+++ .bzrignore	2010-01-29 14:32:36 +0000

ok.

> === modified file 'client/mysqlslap.c'
> --- client/mysqlslap.c	2010-01-29 10:45:51 +0000
> +++ client/mysqlslap.c	2010-01-29 14:32:36 +0000
> @@ -142,7 +142,8 @@
>  const char *auto_generate_sql_type= "mixed";
>  
>  static unsigned long connect_flags= CLIENT_MULTI_RESULTS |
> -                                    CLIENT_MULTI_STATEMENTS;
> +                                    CLIENT_MULTI_STATEMENTS |
> +                                    CLIENT_REMEMBER_OPTIONS;
> 

I don't like this change:

In the code we have:

if (!(mysql_real_connect(&mysql, host, user, opt_password,
                             NULL, opt_mysql_port,
                             opt_mysql_unix_port, connect_flags)))
    {
      fprintf(stderr,"%s: Error when connecting to server: %s\n",
              my_progname,mysql_error(&mysql));
      free_defaults(defaults_argv);
      my_end(0);
      exit(1);
    }

Without the CLIENT_REMEMBER_OPTIONS, we get a leak (that will be
noticed by for example valgrind) as the options allocated earlier in
the code will not be freed.

I looked at the original bug report and can't see how this bug fix
have anything to the with the real problem.

I have now done a proper fix for this. Can you please test that things
reported in http://bugs.mysql.com/bug.php?id=31173 now works
on windows ?


> === modified file 'mysys/my_thr_init.c'
> --- mysys/my_thr_init.c	2010-01-29 10:45:51 +0000
> +++ mysys/my_thr_init.c	2010-01-29 14:32:36 +0000
> @@ -317,7 +317,7 @@
>    /*
>      Skip initialization if the thread specific variable is already initialized
>    */
> -  if (THR_KEY_mysys.id)
> +  if (THR_KEY_mysys.init)
>      goto end;
>    tmp= &THR_KEY_mysys;
>  #endif

I don't see checking the id would not work, as the id is guaranteed to
always be > 0

The only difference I see is that if you call my_thread_end() then
init will be reset so it will be safe to call my_thread_init() again.

I will do the change based on this assumption.

If there is another reason for this change, please let me know.

<cut>

All other things looked good.

I am now applying all your changes except the above, as a diff but
with your comments, to MariaDB 5.1.  I will push it later today.

Thanks for all the help!

Regards,
Monty