Hi Chuck,
Chuck Bell, 05.02.2009 22:16:
...
>>> --- a/sql/sql_parse.cc 2008-12-04 23:14:30 +0000
>>> +++ b/sql/sql_parse.cc 2009-01-06 20:28:28 +0000
>> ...
>>> @@ -2316,26 +2316,43 @@ mysql_execute_command(THD *thd)
>> ...
>>> + int8 val= (int8)it->val_int();
>>> + /*
>>> + Check options.
>>> + */
>>> + switch (val) {
>>> + /* OVERWRITE option */
>>> + case 1:
>>> + overwrite_restore= true;
>>> + break;
>>> + /* SKIP GAP EVENT option */
>>> + case 2:
>>> + skip_gap_event= true;
>>> + break;
>>> + }
>>
>> Wouldn't it be better to have symbolic names instead of numeric literals
>> for the options?
>>
>> I don't see a good reason for reducing the value from longlong to int8.
>> Using the natural machine size of 'int' seems more appropriate.
>>
>> Hopefully we won't have more than 255 options ever, but...
>
> Just following direction from the replication guys. I think the reason
> is because the code is split between master and slave and it is just
> easier to code and debug this way. If you insist, I will ask the
> replication gurus for their opinions/recommendations.
I don't understand the reason. It doesn't seem to apply to any of the
three questions:
- Why not symbolic names?
- Why int8 (8 bit integer) just for internal use (the switch)?
- Will we never have more than 255 options?
So yes, please ask them for explanations.
>
>>
>> ...
>>> --- a/sql/sql_yacc.yy 2008-12-04 23:14:30 +0000
>>> +++ b/sql/sql_yacc.yy 2009-01-06 20:28:28 +0000
>> ...
>>> @@ -6435,7 +6436,7 @@ restore:
>>> }
>>> FROM
>>> TEXT_STRING_sys - opt_overwrite
>>> + opt_overwrite opt_skip_gap_event
>>
>> This means that the options have to be given is a certain order. Is this
>> intentional?
>
> Yes, it is. Do you feel strongly it should be orderless? I think that
> could be much more work.
I don't feel strongly it should be orderless. But usually we have
orderless options. If there isn't a good reason to enforce an order, I'd
prefer to allow arbitrary order. This should not be a lot of work (see
also opt_create_table_options):
- opt_overwrite
+ opt_restore_options
{
Lex->backup_dir = $4;
}
+opt_restore_options:
+ /*empty*/
+ | restore_options
+ ;
+
+restore_options:
+ restore_option
+ | restore_option restore_options
+ ;
+
+restore_option:
+ opt_overwrite
+ | opt_skip_gap_event
+ ;
...
>>> + +opt_skip_gap_event:
>>> + /* empty */ + { + LEX *lex= Lex;
>>> + Item *it= new Item_int((int8) 0);
>>> +
>>> + lex->value_list.push_back(it);
>>> + }
>>
>> What is the advantage to add a zero option in case the SKIP_GAP_EVENT
>> option is not present? In mysql_execute_command() you ignore it anyway.
>
> This is done so that in case something goes wrong it will not trigger
> the opt_overwrite option by mistake.
What might go _that_ wrong, that it could trigger the opt_overwrite
option by mistake? If you leave away {...} then value_list would only
contain requested options. It could be empty, if no options are
requested. The "while" loop in mysql_execute_command() would do nothing.
To me this feels pretty safe.
BTW, I just noticed that you have the same construct in "opt_overwrite"
too. IMHO this places unnecessary operations in the parser. I really
would like to get rid of them. But since the code is not incorrect, I
can only recommend it.
Regards
Ingo
--
Ingo Strüwing, Database Group
Sun Microsystems GmbH, Sonnenallee 1, D-85551 Kirchheim-Heimstetten
Geschäftsführer: Thomas Schröder, Wolfgang Engels, Dr. Roland Bömer
Vorsitzender des Aufsichtsrates: Martin Häring HRB München 161028
