He Zhenxing wrote:
> Hi Mats
>
> Did you noticed that I included <my_global.h> in plugin.h?
>
> The functions I added in plugin.h used some typedefs defined
> in it, such as uint32, uchar. I am not quite sure other stuffs defined
> in my_global.h should be exported or not.
No, actually I didn't. I'm reluctant to include my_global.h since it
contain a lot of dependencies on other code in the server... wait...
... no, I don't think it is a fundamental problem to include the file.
There are a lot of definitions that are not really needed, but I see no
harm in including it from plugin.h, we just have to make sure that it is
part of the SDK as well.
[snip]
>>> +Trans_delegate transaction_delegate;
>>> +Binlog_storage_delegate binlog_storage_delegate;
>>> +#ifdef HAVE_REPLICATION
>>> +Binlog_transmit_delegate binlog_transmit_delegate;
>>> +Binlog_relay_IO_delegate binlog_relay_io_delegate;
>>> +#endif /* HAVE_REPLICATION */
>> These are declared extern (i.e., have external linkage), but are not
>> used anywhere outside the file (as far as I can tell). I suggest you
>> assign them internal linkage by declaring them static and remove the
>> extern declarations from the header file. It is easy to make a variable
>> with internal linkage have external linkage, but the opposite can be
>> problematic.
>>
>
> They are used externally, I hided it in the following macro :)
>
> #define RUN_HOOK(group, hook, args...) \
> group ##_delegate.hook(args)
OK. :)
[snip]
>>> + user_var_entry *entry=
>>> + (user_var_entry*) hash_search(&mysql_thd_to_thd(thd)->user_vars,
>> (uchar*) name,
>>> + strlen(name));
>>> + if (entry)
>>> + {
>>> + entry->val_str(null_value, &str, precision);
>>> + strncpy(value, str.c_ptr(), len);
>>> + }
>>> + else
>>> + {
>>> + *value= 0;
>>> + }
>>> + return 0;
>>> +}
>> I would suggest to return an error code from the function if the
>> variable is not found. Otherwise, it is impossible to distinguish
>> between a variable that exists but is the empty string, and a variable
>> that does not exist.
>
> Actually the out parameter `null_value' is used for this purpose, maybe
> I should change this to use the return value as an indicator of if the
> variable is defined (return 0 on success, 1 if variable not found). and
> remove `null_value' parameter.
>
> int get_user_var_int(MYSQL_THD thd, const char *name, longlong *value);
> int get_user_var_real(MYSQL_THD thd, const char *name, double *value);
> int get_user_var_str(MYSQL_THD thd, const char *name,
> char *value, size_t len, uint precision);
Although the cases are not distinguishable right now, I think it is
reasonable to distinguish unassigned variables and variables that have
been assigned the value NULL. I can easily see that developers might
want to distinguish the cases programmatically, even though they might
currently not be distinguishable through SQL.
AFAICT from the reading the code, it seems like setting a variable to
NULL does not remove it from the hash, so a variable can actually have
an explicit NULL value.
... but yes, I agree that you can return an error code instead of
explicitly returning it as an out parameter, but make sure to
distinguish between the case "NULL because the value does not exist" and
"NULL because the variable was assigned NULL".
[snip]
>> +int delegates_init()
>> +{
>>
>> Maybe you should call this function "rpl_delegates_init()" (and the
>> "delegates_destroy()" similarly changed) since there conceivably can be
>> other delegates in the future that are not related to replication.
>>
>
> You're right, and event now not all the delegates are related to
> replication, the Transaction_delegate, for example, can be used for
> other purpose, so I omitted the `rpl' prefix intentionally. If we want
> to add new delegates in the future, they should be intialized here too.
>
>>> +template <typename T>
>>> +class Observer_info_ {
>>> +public:
>> Avoid the trailing underscore, they are easy to miss.
>
> OK
>
>> Template classes are not allowed according to the coding standard, with
>> some exceptions for, e.g., the List template class (sorry).
>>
>
> I don't see this on the coding standard:
> http://forge.mysql.com/wiki/MySQL_Internals_Coding_Guidelines
>
> Is there an other coding standard?
I haven't read the guidelines on the forge that carefully actually. I
was handed a document with the internal guidelines when I started, which
included an item that template functions or classes should not be used.
I have also been asked to remove a few that I have added, since I
personally don't see a big problem with them, although one should be a
little careful when using them.
Just my few cents,
Mats Kindahl
--
Mats Kindahl
Lead Software Developer
Replication Team
MySQL AB, www.mysql.com
