maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxxx>]

Hi, Michael!

On Jan 18, Michael Widenius wrote:
> >>>>> "Sergei" == Sergei Golubchik <serg@xxxxxxxxxxxx> writes:
> 
> >> === modified file 'client/completion_hash.cc'
> >> --- client/completion_hash.cc	2011-06-30 15:46:53 +0000
> >> +++ client/completion_hash.cc	2013-01-15 21:00:33 +0000
> >> @@ -49,7 +49,7 @@ int completion_hash_init(HashTable *ht, ht-> initialized = 0;
> >> return FAILURE;
> >> }
> >> -  init_alloc_root(&ht->mem_root, 8192, 0);
> >> +  init_alloc_root(&ht->mem_root, 8192, 0, 0);
> 
> > very often you write 0 or MY_THREAD_SPECIFIC instead of
> > MYF(0) and MYF(MY_THREAD_SPECIFIC)
> 
> Yes. MYF() was important in C, but not that critical anymore in C++ I
> can change all MY_THREAD_SPECIFIC to MYF(MY_THREAD_SPECIFIC) if you
> want.

Why was it important in C?
I never could find a reason why it might be needed because of a language
or a specific compiler requirements.

I always saw it as a visual help to attribute the last 0 in the function
argument list to mysys flags. Like, by convention many functions accept
'flags' argument, and one can easily see that this function takes flags,
because the flag is always MYF(0) or MYF(something). This reason is true
both for C and C++.

> >> === modified file 'include/mysql.h'
> >> --- include/mysql.h	2012-11-20 14:24:39 +0000
> >> +++ include/mysql.h	2013-01-15 21:00:33 +0000
> >> @@ -167,7 +167,7 @@ enum mysql_option
> >> MYSQL_OPT_GUESS_CONNECTION, MYSQL_SET_CLIENT_IP, MYSQL_SECURE_AUTH,
> >> MYSQL_REPORT_DATA_TRUNCATION, MYSQL_OPT_RECONNECT,
> >> MYSQL_OPT_SSL_VERIFY_SERVER_CERT, MYSQL_PLUGIN_DIR, MYSQL_DEFAULT_AUTH,
> >> -  MYSQL_PROGRESS_CALLBACK,
> >> +  MYSQL_PROGRESS_CALLBACK, MYSQL_THREAD_SPECIFIC_MALLOC,
> 
> > I don't think you need this on the client side.
> 
> > (the only use case is federated, and I don't think you need to extend
> > the client API specifically for it, federated can set whatever it needs
> > directliy).
> 
> I think it's better to use a clear interface for that.
> CONNECTDB will need to also use this.

Yes, but unlike all other client options - which are truly client
options - this one is only for "clients linked into the server".

Which is a bit special use case, and I'd suggest to have a special
interface for that, instead of making it every client aware of it.

> >> === modified file 'sql/sql_error.cc'
> >> --- sql/sql_error.cc	2012-01-13 14:50:02 +0000
> >> +++ sql/sql_error.cc	2013-01-15 21:00:33 +0000
> >> @@ -457,25 +457,37 @@ Diagnostics_area::disable_status()
> >> m_status= DA_DISABLED;
> >> }
> >> 
> >> -Warning_info::Warning_info(ulonglong warn_id_arg, bool allow_unlimited_warnings)
> >> +Warning_info::Warning_info(ulonglong warn_id_arg,
> >> +                           bool allow_unlimited_warnings, bool initialize)
> 
> > I'd rather add a special constructor for Warning_info
> > that THD would use. Because there's only one place where you need
> > initialize=false, and all other "normal" uses, should not suffer (be
> > changed) because of that.
> 
> I can do that. thanks for the idea.

Later you said on irc:

> hm, don't like how much code I have to repeat just to add an creator
> function for Warning_info::Warning_info(ulonglong warn_id_arg, bool
> allow_unlimited_warnings, bool initialize)
> don't really know how to do this nicely. have to leave this for now.
> Don't want to duplicate a lot of code as this is a risk for errors

Ok. I don't quite understand it, but please push either way.
And I'll see if I could do with with little code duplication.

Regards,
Sergei