maria-developers team mailing list archive

[Michael Widenius <monty@xxxxxxxxxxxx>]

Hi!

>>>>> "Sergei" == Sergei Golubchik <serg@xxxxxxxxxxxx> writes:

<cut>

>>> 2. Unknown option should be an error by default.
>> 
>> OK. The only problem is that it is contradict to Monty requirements.
>> Our initial decision was issue error if option was added explicitly.
>> The only problem is that it is very difficult to implement - we write
>> options to .frm first then read them and pass to engine. I have no
>> idea how to pass this information via/over frm.

Sergei> I hope you've seen my reasoning below about optimizing for a common
Sergei> case. Monty wants boundary cases to work - like changing engines back
Sergei> and forth and replication. I am saying that by default unknown options
Sergei> should be an error, but one should be able to disable that.

Sergei> "An error if opion as added explicitly" does not solve all boundary
Sergei> cases, for example, restoring a dump into a different engine.
Sergei> Monty would probably want to cover that too.

As almost all options are just 'extra information', I prefer that by
default one doesn't get an error if the engine doesn't recognize the
option.

This is otherwise it's hell for automatic create table tools to work.
It's much easier if one can just choose engine and then different
options, some which are supported and others that may not be supported.

Otherwise each tool would need to have a list of all existing engines
and what options each support, which would be real hell.

>>> 3. use something my_getopt-like as we discussed, don't force every
>>> engine to parse its options
>> 
>> I can add such function for users to use, but it will be thier choice
>> use it or do not, is it OK?

Sergei> What was the problem with doing it automatically ?

Beccause the engine will still needs to do a switch over all options it
supports, so it's hard to do it automatically.

>>> 4. make options immutable to avoid copying them in ::clone
>> 
>> I do not know way to do it if they should be allocated in different
>> mem_roots.

Sergei> Example ? Where are they allocated in different memroots ?

This should work if we create the new table and reopen it before the
old table is closed (which should be the case).

>>> 5. don't check for changed options in alter table with your
>>> check_if_incompatible_data. let the engine do that.
>> 
>> This and 8 require big changes engine and ALTER TABLE. Monty's
>> requirement was do not touch current code. I would be glad if you
>> discuss it and make some non contradicting requirement.

No comments, but I think this is easier to do on the top level than in
the engine (but I don't remember Sanjas code exactly regarding this).

>>> 7. parser: make the equal sign optional
>> 
>> I have some doubts that it is doable
>> 
>> DATA DIRECTORY TEST VALUE ...
>> 
>> Does it mean:
>> 
>> DATA = DIRECTORY TEST = VALUE ...
>> 
>> or
>> 
>> DATA DIRECTORY = TEST VALUE ... ? - error
>> (ALTER TABLE uses create_table_options_space_separated list of options)

Sergei> did you try the code from my previous email ?

Agree with sanja that not having = can lead to parse problems.
Also using = is more readable so I would prefer to over time start
deprecate space between keyword and value.

<cut>

>>>> === modified file 'sql/sql_table.cc'
>>>> --- sql/sql_table.cc	2010-02-12 08:47:31 +0000
>>>> +++ sql/sql_table.cc	2010-03-04 20:46:55 +0000
>>>> @@ -5789,6 +5791,15 @@ compare_tables(TABLE *table,
>>>> DBUG_RETURN(0);
>>>> }
>>>> 
>>>> +    if (!is_equal_create_options(tmp_new_field->create_options.first,
>>>> +                                 field->create_options.first))
>>>> +    {
>>> 
>>> I am not sure this should be checked on MySQL level, we don't know the
>>> semantics of options. I'd say this check belong to
>>> handler::check_if_incompatible_data() and should be implemented in the
>>> storage engine internally.
>> 
>> Monty even requested me to recreate .frm even if case of KEY was chenged 
>> (which is clear do not chengr semantic) - i.e. any change == rewriting 
>> .frm. So your requests contradict here it should be discussed (I do not see 
>> sens nor harm in such rewriting policy)

Sergei> recreating frm is one thing, doing a full alter with copying the data is
Sergei> another. I'm saying that it's not MySQL that should decide what change
Sergei> in table options requires copy_data_between_tables - but the engine
Sergei> itself.

Agree that it's only the engine that knows if we need to copy the data
or not.

>>>> +plugin_option_value:
>>>> +  DEFAULT
>>>> +    {
>>>> +      $$.str= NULL; /* We are going to remove the option */
>>>> +      $$.length= 0;
>>>> +    }
>>>> +  | NULL_SYM
>>> 
>>> I don't like this trick.
>>> If you don't support NULLs, dont't allow users to specify them
>> 
>> how it can be stored as parameter value? Such semantic prevent users of 
>> thinking that assigning NULL will make it really NULL not "NULL".

Sergei> It won't be "NULL", IDENT_sys that you use in plugin_option_value
Sergei> will not treat NULL as an ident. I think if you simply remove
Sergei> NULL alternative from the plugin_option_value rule, you'll end up
Sergei> having a syntax error for option=NULL, which is better than what you
Sergei> have now.

Ok with me that we delete the =NULL syntax to remove options.


>>>> +++ sql/sql_create_options.cc	2010-03-04 20:46:55 +0000
>>>> +my_bool create_option_add(CREATE_OPTION_LIST *options, MEM_ROOT *root,
>>>> +                          const LEX_STRING *str_key,
>>>> +                          const LEX_STRING *str_val,
>>>> +                          my_bool *changed)
>>>> +{
>>>> +  CREATE_OPTION *cur_option, **option;
>>>> +  char *key, *val;
>>>> +  my_bool not_used;
>>>> +  my_bool copy= FALSE;
>>>> +  my_bool replace= FALSE;
>>>> +  DBUG_ENTER("create_option_add");
>>>> +  DBUG_PRINT("enter", ("key: '%s'  value: '%s'",
>>>> +                       str_key->str, str_val->str));
>>>> +  if (changed)
>>>> +    copy= TRUE;
>>>> +  else
>>>> +    changed= &not_used;
>>>> +
>>>> +  DBUG_ASSERT(options->first ||
>>>> +              (!options->first && options->last == &options->first));
>>>> +  *changed= FALSE;
>>> 
>>> Hmm, strange. From the way you use 'changed' I thought it should 
>>> accumulate
>>> the results - I mean, it's one variable that is passed into
>>> create_option_add() for all options. Apparently at the end it should be
>>> true if *any* of the options has changed.
>>> 
>>> But then, why do you set it to false inside create_option_add() ?
>> 
>> It was special case for call from ALTER TABLE and from parser. Only ALTER 
>> TABLE was interested in changes and so required copying parameters.

Sergei> I don't understand.

I also in my review thought it would be much more logical if 'changed'
would be reset (if needed) on the outer level, not in the function.

>>>> +
>>>> +  /* try to find the option first */
>>>> +  for (option= &(options->first);
>>>> +       *option && my_strcasecmp(system_charset_info,
>>>> +                                str_key->str, (*option)->key.str);
>>>> +       option= &((*option)->next)) ;
>>>> +  if (str_val->str)
>>>> +  {
>>>> +    /* add / replace */
>>>> +    if (*option)
>>>> +    {
>>>> +      /* replace */
>>>> +      cur_option= *option;
>>>> +      if (!(*changed) &&
>>>> +          (cur_option->val.length != str_val->length ||
>>>> +           memcmp(cur_option->val.str, str_val->str, str_val->length)))
>>>> +      {
>>>> +        *changed= TRUE;
>>>> +      }
>>>> +      replace= TRUE;
>>>> +    }
>>>> +    else
>>>> +    {
Sergei> ...
>>>> +CREATE_OPTION_LIST *create_create_options_array(MEM_ROOT *root, uint n)
>>> 
>>> "create_create" is not a good name :(
>> 
>> I did not found better but open for suggestion.

Sergei> make_create_options_array ?
Sergei> construct_create_options_array ?

construct_create_options_array sounds nice to me.

>>>> +my_bool create_options_read(const uchar *buff, uint length, MEM_ROOT 
>>>> *root,
>>>> +                            TABLE_OPTIONS *opt)
>>>> +{
>>>> +  const uchar *buff_end= buff + length;
>>>> +  DBUG_ENTER("create_options_read");
>>>> +  while (buff < buff_end)
>>>> +  {
>>>> +    CREATE_OPTION *option;
>>>> +    CREATE_OPTION_TYPES type;
>>>> +    uint index= 0;
>>>> +
>>>> +    if (!(option= (CREATE_OPTION *) alloc_root(root, 
>>>> sizeof(CREATE_OPTION))))
>>>> +      DBUG_RETURN(TRUE);
>>>> +
>>>> +    DBUG_ASSERT(buff + 4 <= buff_end);
>>>> +    option->val.length= uint2korr(buff);
>>>> +    option->key.length= buff[2];
>>>> +    option->next= NULL;
>>>> +    type= (CREATE_OPTION_TYPES)buff[3];
>>>> +    buff+= 4;
>>>> +    switch (type) {
>>>> +    case CREATE_OPTION_FIELD:
>>> 
>>> interesting encoding. so basically you support the case when field,
>>> key, and table options are all written interleaved:
>>> 
>>> <table option><key 1 option><field 5 option><table option><field 3 
option> <key 4 option>...
>>> 
>>> why the heck do you want to support it ?
>> 
>> Could you propose other encoding taking into account that some fields, keys 
>> and tables do not have parameters and some has several ones?

Sergei> Sure. Many :)
Sergei> For example

Sergei> <number of table options>
Sergei>   <length-encoded strings for table options>
Sergei> <number of field 1 options>
Sergei>   <length-encoded strings for field 1 options>
Sergei> <number of field 2 options>
Sergei>   <length-encoded strings for field 2 options>
Sergei> ...
Sergei> <number of key 1 options>
Sergei>   <length-encoded strings for key 1 options>
Sergei> <number of key 2 options>
Sergei>   <length-encoded strings for key 2 options>

Sergei> Assuming a table with three fields and two keys that would be

Sergei> 0x02 0x05 "topt1" 0x03 "val" 0x03 "to2" 0x04 "val2"
Sergei> 0x00
Sergei> 0x01 0x04 "fil1" 0x01 "1"
Sergei> 0x03 0x01 "A" 0x02 "bb" 0x01 "B" 0x02 "CC" 0x02 "de" 0x01 "0"
Sergei> 0x01 0x06 "packed" 0x03 "yes"
Sergei> 0x00

I also originally thought about this (I would probably have stored
things the above way if I would have coded this).

However, I am not sure that the code would be shorter than Sanjas
code.  The fact that the code can handle cases that never happens in
reality didn't bother me.

Regards,
Monty