Re: [Maria-developers] options for CREATE TABLE (MWL#43)

Hi!

...

...

...

...

"Sergei" == Sergei Golubchik 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= ¬_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.

...

...

...

+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><table option>

Sergei> make_create_options_array ? Sergei> construct_create_options_array ? construct_create_options_array sounds nice to me. 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> Sergei> Sergei> Sergei> Sergei> ... Sergei> Sergei> Sergei> Sergei> 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