Hi!

...

...

...

...

"Oleksandr" == Oleksandr Byelkin writes:

Oleksandr> Hi! Here is version for maria 5.2 Oleksandr> test create_options_example.test is quite strange, test on Oleksandr> having EXAMPLE engine fails (the same for plugin.test and Oleksandr> plugin_load.test, but if remove the "have" test and be sure Oleksandr> that EXAMPLE is built in it will work (I will look at it Oleksandr> later).

=== modified file 'include/my_base.h' --- include/my_base.h 2009-09-07 20:50:10 +0000 +++ include/my_base.h 2009-12-01 20:45:53 +0000 @@ -314,6 +314,8 @@ #define HA_OPTION_RELIES_ON_SQL_LAYER 512 #define HA_OPTION_NULL_FIELDS 1024 #define HA_OPTION_PAGE_CHECKSUM 2048 +/* .frm has extra create options in linked-list format */ +#define HA_OPTION_TEXT_CREATE_OPTIONS (1 << 14)

1 -> 1L (just to be similar as the other ones)

#define HA_OPTION_TEMP_COMPRESS_RECORD (1L << 15) /* set by isamchk */ #define HA_OPTION_READ_ONLY_DATA (1L << 16) /* Set by isamchk */ #define HA_OPTION_NO_CHECKSUM (1L << 17)

<cut>

=== added file 'mysql-test/r/create_options.result' --- mysql-test/r/create_options.result 1970-01-01 00:00:00 +0000 +++ mysql-test/r/create_options.result 2009-12-01 21:47:04 +0000 @@ -0,0 +1,143 @@ +drop table if exists t1; +create table t1 (a int fkey1=v1, key akey (a) kkey1=v1) tkey1=1v1 tkey1=1v2 TKEY1=DEFAULT +tkey2=2v1 tkey3=3v1; +Warnings: +Warning 1648 Unused option 'tkey2' (value '2v1') +Warning 1648 Unused option 'tkey3' (value '3v1') +Warning 1649 Unused option 'fkey1' (value 'v1') of field 'a' +Warning 1650 Unused option 'kkey1' (value 'v1') of key 'akey'

Good warnings, but wonder of it would be more clear to use: 'tkey2' = '2v1' We also need a test where we remove options. Looking at the code, setting a value to DEFAULT or NULL should remove it. Please add test of this to all options!

+++ mysql-test/r/create_options_example.result 2009-12-01 21:48:28 +0000 @@ -0,0 +1,16 @@ +drop table if exists t1; +create table t1 (a int ttt=xxx E=1, key akey (a) kkk=xxx ) E=1 ttt=xxx ttt=yyy TTT=DEFAULT mmm=CCC zzz=MMM; +Warnings: +Warning 1648 Unused option 'E' (value '1') +Warning 1648 Unused option 'mmm' (value 'CCC') +Warning 1648 Unused option 'zzz' (value 'MMM') +Warning 1649 Unused option 'E' (value '1') of field 'a' +Warning 1649 Unused option 'ttt' (value 'xxx') of field 'a' +Warning 1650 Unused option 'kkk' (value 'xxx') of key 'akey' +drop table t1;

Please also do a test by not having TTT last ? For example using: .... TTT=DEFAULT ttt=yyy should set the ttt to yyy

=== modified file 'sql/field.cc'

<cut>

@@ -10291,6 +10294,21 @@

/** + Makes a clone of this object for ALTER/CREATE TABLE + + @param mem_root MEM_ROOT where to clone the field +*/ + +Create_field *Create_field::clone(MEM_ROOT *mem_root) const +{ + Create_field *res= new (mem_root) Create_field(*this); + if (res) + res->create_options= create_options_clone(mem_root, res->create_options); + return res; +}

Add a comment that we need to do the clone of the list because in ALTER TABLE we may change the list for the cloned field.

=== modified file 'sql/handler.h' --- sql/handler.h 2009-11-12 04:31:28 +0000 +++ sql/handler.h 2009-12-01 20:45:53 +0000 @@ -915,6 +915,8 @@ LEX_STRING connect_string; const char *password, *tablespace; LEX_STRING comment; + TABLE_OPTIONS create_table_options_orig; + TABLE_OPTIONS *create_table_options;

We would here need a comment why we need both of the above fields. <cut>

=== modified file 'sql/mysql_priv.h' <cut>

@@ -2596,6 +2598,7 @@ CHARSET_INFO *dflt_cl, CHARSET_INFO **cl);

+

remove extra empty line

#endif /* MYSQL_SERVER */ extern "C" int test_if_data_home_dir(const char *dir);

=== modified file 'sql/share/errmsg.txt' --- sql/share/errmsg.txt 2009-11-10 02:32:39 +0000 +++ sql/share/errmsg.txt 2009-12-01 20:55:42 +0000 @@ -6232,4 +6232,13 @@ eng "'%s' is not yet supported for computed columns."

ER_CONST_EXPR_IN_VCOL - eng "Constant expression in computed column function is not allowed." + eng "Constant expression in computed column function is not allowed." + +WARN_UNUSED_TABLE_OPTION + eng "Unused option '%-.64s' (value '%-.64s')" + +WARN_UNUSED_FIELD_OPTION + eng "Unused option '%-.64s' (value '%-.64s') of field '%-.64s'" + +WARN_UNUSED_KEY_OPTION + eng "Unused option '%-.64s' (value '%-.64s') of key '%-.64s'"

See earlier comment about using: Unused option '%-.64s = '%-.64s' of key '%-.64s'" etc....

--- sql/sql_class.cc 2009-11-12 04:31:28 +0000 +++ sql/sql_class.cc 2009-12-01 20:45:53 +0000 @@ -108,6 +108,7 @@ generated(rhs.generated) { list_copy_and_replace_each_value(columns, mem_root); + create_options= create_options_clone(mem_root, rhs.create_options); }

At some point we should decide if we should have 'mem_root' as first or last argument (no changes needed now). <cut>

=== added file 'sql/sql_create_options.cc' --- sql/sql_create_options.cc 1970-01-01 00:00:00 +0000 +++ sql/sql_create_options.cc 2009-12-01 21:40:38 +0000 @@ -0,0 +1,601 @@ + +#include "mysql_priv.h" + +static CREATE_OPTION last_option; + +/* Additional length of index for CREATE_OPTION_XXX types */ +static uint create_options_len[3]= {0, 2, 2}; + +/** + Adds new option to this list + + @param options pointer to the list + @param root memroot to allocate option + @param str_key key + @param str_val value + @param changed pointer to variable to report changed data or NULL +

A simpler and faster interface is to always require changed to point to something.

+ @retval TRUE error + @retval FALSE OK +*/ + +my_bool create_option_add(CREATE_OPTION **options, MEM_ROOT *root, + const LEX_STRING *str_key, + const LEX_STRING *str_val, + my_bool *changed) +{ + CREATE_OPTION *opt, **i;

Rename i to option and maybe 'opt' to 'cur_option'

+ char *key, *val; + DBUG_ENTER("create_option_add"); + DBUG_PRINT("enter", ("key: '%s' value: '%s'", + str_key->str, str_val->str)); + if (changed) + *changed= FALSE;

Remove test if pointer exists

+ + /* try to find the option first */ + for (i= options; + *i && my_strcasecmp(system_charset_info, str_key->str, (*i)->key.str); + i= &((*i)->next)) ; + if (str_val->str) + { + /* add / replace */ + if (*i) + { + /* replace */ + opt= *i; + if (changed && !(*changed) &&

remove test if 'changed'

+ (opt->val.length != str_val->length || + memcmp(opt->val.str, str_val->str, str_val->length))) + { + *changed= TRUE; + } + }

Above we should also handle setting the option to 'default', which means remove it.

+ else + { + /* add */

Here we should ignore setting values to default.

+ if (!(opt= (CREATE_OPTION *)alloc_root(root, sizeof(CREATE_OPTION)))) + DBUG_RETURN(TRUE); + opt->next= *options; + *options= opt; + if (changed) + *changed= TRUE;

Hm... Above we are adding new options first to the option list, while more naturally for the user would be to add them last. After all, i* points to the last element, so it's as easy to add things last! This would make the above code: if (!(opt= (CREATE_OPTION *)alloc_root(root, sizeof(CREATE_OPTION)))) DBUG_RETURN(TRUE); *i= opt; /* add last */ opt->next= 0; *changed= TRUE;

+ if (!changed || *changed) + { + if (!multi_alloc_root(root, &key, str_key->length + 1, + &val, str_val->length + 1, NULL)) + DBUG_RETURN(TRUE); + opt->key.str= (char *)memcpy(key, str_key->str, + (opt->key.length= str_key->length)); + key[str_key->length]= '\0'; + opt->val.str= (char *)memcpy(val, str_val->str, + (opt->val.length= str_val->length)); + val[str_val->length]= '\0'; + opt->used= FALSE; + opt->owner= NULL; + }

I think we should also do the above in case we found a match. Otherwise a change in case for an option would not be stored in the .frm file. Like: CREATE TABLE t1 (a int) new_option=1; alter table t1 NEW_OPTION=1; I think we should register 'NEW_OPTION', but we should not mark the option as changed (and thus force a full alter table).

+ } + else + { + /* remove */ + if (*i) + { + *i= (*i)->next; + if (changed) + *changed= TRUE;

So here we remove options that are empty. We should also remove them that are set to default.

+ } + } + DBUG_RETURN(FALSE); +}

+ +/** + Creates empty fields/keys array for table create options structure + + @param root memroot where to allocate memory for this structure + @param n number of fields/keys + + @return pointer to arrayor NULL in case of error. +*/ + +CREATE_OPTION **create_create_options_array(MEM_ROOT *root, uint n) +{ + DBUG_ENTER("create_create_options_array"); + DBUG_PRINT("enter", ("Number: %u", n)); + + CREATE_OPTION **res= + (CREATE_OPTION **) alloc_root(root, + sizeof(CREATE_OPTION *) * (n + 1)); + if (!res) + DBUG_RETURN(NULL); + bzero(res, sizeof(CREATE_OPTION *) * n); + res[n]= &last_option;

We should also look at adding calloc_root() as a convience function.

+ DBUG_RETURN(res); +}

<cut>

+ + +/** + Reads options from this buffer + + @param buffer the buffer to read from + @param mem_root memroot for allocating + @param opt parametes to write to + + @retval TRUE Error + @retval FALSE OK +*/ + +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 *) alloc_root(root, sizeof(CREATE_OPTION)); + CREATE_OPTION_TYPES type; + uint index= 0; + if (!option) + DBUG_RETURN(TRUE);

Please move the allocation and test together (makes code easier to read) as allocation and test is close together: 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]; + type= (CREATE_OPTION_TYPES)buff[3];

You could add here: buff+= 4; Would make the next code a bit easier to read (and would in theory allow us to change the prefix without having to change the next part)

+ switch (type) { + case CREATE_OPTION_FIELD: + index= uint2korr(buff + 4); + buff+= 6; + option->next= opt->field_opt[index]; + opt->field_opt[index]= option;

Please don't create invers list (even if it's harder)

+ break; + case CREATE_OPTION_KEY: + index= uint2korr(buff + 4); + buff+= 6; + option->next= opt->key_opt[index]; + opt->key_opt[index]= option; + break;

Please don't create invers list (even if it's harder)

+ case CREATE_OPTION_TABLE: + /* table */ + buff+= 4; + option->next= opt->table_opt; + opt->table_opt= option;

Please don't create invers list <cut>

+void create_options_check_unused(THD *thd, + TABLE_OPTIONS *options) +{ + CREATE_OPTION *opt; + CREATE_OPTION **i;

Change 'i' to something a bit more descriptive, like option or make it at least a local variable in each section.

+ uint index; + DBUG_ENTER("create_options_check_unused"); + + if (!options) + DBUG_VOID_RETURN; +

Add comment: /* Check option usage for table */

+ for (opt= options->table_opt; opt != NULL; opt= opt->next) + { + if (!opt->used) + { + push_warning_printf(thd, + MYSQL_ERROR::WARN_LEVEL_WARN, + WARN_UNUSED_TABLE_OPTION, + ER(WARN_UNUSED_TABLE_OPTION), + (const char *) opt->key.str, + (const char *) opt->val.str); + + } + }

Add comment: /* Check option usage for fields */

+ if (options->field_opt) + { + for (i= options->field_opt, index= 0; *i != &last_option; i++, index++) + { + for (opt= i[0]; opt != NULL; opt= opt->next) + { + if (!opt->used) + { + Field *field= (Field *) opt->owner; + push_warning_printf(thd, + MYSQL_ERROR::WARN_LEVEL_WARN, + WARN_UNUSED_FIELD_OPTION, + ER(WARN_UNUSED_FIELD_OPTION), + (const char *) opt->key.str, + (const char *) opt->val.str, + field->field_name); + + } + } + } + }

Add comment: /* Check option usage for keys */

+ if (options->key_opt) + { + for (i= options->key_opt, index= 0; *i != &last_option; i++, index++) + { + for (opt= i[0]; opt != NULL; opt= opt->next) + { + if (!opt->used) + { + KEY *key= (KEY *) opt->owner; + push_warning_printf(thd, + MYSQL_ERROR::WARN_LEVEL_WARN, + WARN_UNUSED_KEY_OPTION, + ER(WARN_UNUSED_KEY_OPTION), + (const char *) opt->key.str, + (const char *) opt->val.str, + key->name); + + } + } + } + } + + DBUG_VOID_RETURN; +}

+ +/** + clones options + + @param root mem_root where to clone the options + @param opt options list to clone + + @return cloned list +*/ + +CREATE_OPTION* create_options_clone(MEM_ROOT *root, CREATE_OPTION *opt) +{ + CREATE_OPTION *res= NULL, *clone; + char *key, *val; + DBUG_ENTER("create_options_clone"); + + for (; opt != NULL; opt= opt->next) + { + if (!multi_alloc_root(root, &clone, sizeof(CREATE_OPTION), + &key, opt->key.length + 1, + &val, opt->val.length + 1, NULL)) + DBUG_RETURN(NULL); + clone->key.str= (char *)memcpy(key, opt->key.str, + (clone->key.length= opt->key.length) + 1); + clone->val.str= (char *)memcpy(val, opt->val.str, + (clone->val.length= opt->val.length) + 1); + clone->used= opt->used; + clone->owner= opt->owner; + clone->next= res; + res= clone;

Here you invers the list at clone. Better to not do it. Note that you can avoid some code, if you do after multi_alloc_root: *clone= *opt;

+ } + DBUG_RETURN(res); +} + + +/** + Merges source and changes lists checking for real changes + + @param source source list to merge + @param changes changes in the list + @param root memroot to allocate option + @param changed pointer to variable to report changed data or NULL

Assume changed is always set to point to something.

+ + @return merged list +*/ + +CREATE_OPTION *create_table_list_merge(CREATE_OPTION *source, + CREATE_OPTION *changes, + MEM_ROOT *root, + my_bool *changed) +{ + my_bool chng= FALSE;

Move to inner block

+ DBUG_ENTER("create_table_list_merge"); + + for(; changes; changes= changes->next) + { + if (create_option_add(&source, root, &changes->key, &changes->val, + (chng ? NULL : &chng)))

Instead of test, send &chng to function and do: (*changed)|= chng;

+ DBUG_RETURN(NULL); + } + + if (changed) + *changed= chng;

Remove above (after prev change)

+ + DBUG_RETURN(source); +} +

<cut>

=== modified file 'sql/sql_show.cc' --- sql/sql_show.cc 2009-11-12 04:31:28 +0000 +++ sql/sql_show.cc 2009-12-01 20:45:53 +0000 @@ -1067,6 +1067,28 @@ return has_default; }

+ +/** + Appends list of options to string + + @param thd thread handler + @param packet string to append + @param opt list of options +*/ + +static void append_create_options(THD *thd, String *packet, CREATE_OPTION *opt) +{ + bool first= TRUE; + for(; opt; opt= opt->next)

Add space after 'for' (Same for earlier cases in patch)

+ { + packet->append(' '); + append_identifier(thd, packet, opt->key.str, opt->key.length); + packet->append('='); + append_unescaped(packet, opt->val.str, opt->val.length);

+ first= FALSE; + }

I can't figure out the purpose of 'first'. Can you ? ;)

@@ -1479,6 +1505,9 @@ packet->append(STRING_WITH_LEN(" CONNECTION=")); append_unescaped(packet, share->connect_string.str, share->connect_string.length); }

Add comment when 'create_table_options' would not be defined?

+ if (share->create_table_options && share->create_table_options->table_opt) + append_create_options(thd, packet, + share->create_table_options->table_opt); append_directory(thd, packet, "DATA", create_info.data_file_name); append_directory(thd, packet, "INDEX", create_info.index_file_name); }

=== modified file 'sql/sql_table.cc' --- sql/sql_table.cc 2009-11-12 04:31:28 +0000 +++ sql/sql_table.cc 2009-12-01 20:45:54 +0000 @@ -3034,6 +3034,7 @@ key_info->key_part=key_part_info; key_info->usable_key_parts= key_number; key_info->algorithm= key->key_create_info.algorithm; + key_info->create_options= key->create_options;

if (key->type == Key::FULLTEXT) { @@ -5713,7 +5714,8 @@ create_info->used_fields & HA_CREATE_USED_TRANSACTIONAL || create_info->used_fields & HA_CREATE_USED_PACK_KEYS || create_info->used_fields & HA_CREATE_USED_MAX_ROWS || - (alter_info->flags & (ALTER_RECREATE | ALTER_FOREIGN_KEY)) || + (alter_info->flags & (ALTER_RECREATE | ALTER_FOREIGN_KEY | + ALTER_CREATE_OPT)) || order_num || !table->s->mysql_version || (table->s->frm_version < FRM_VER_TRUE_VARCHAR && varchar)) @@ -5766,6 +5768,15 @@ DBUG_RETURN(0); }

<cut>

char *tablespace= static_cast(thd->alloc(FN_LEN + 1)); /* Regular alter table of disk stored table (no tablespace/storage change) - Copy tablespace name + C&opy tablespace name

Revert

*/ if (tablespace && (table->file->get_tablespace_name(thd, tablespace, FN_LEN))) @@ -6089,6 +6102,30 @@ } restore_record(table, s->default_values); // Empty record for DEFAULT

+ if (create_info->create_table_options_orig.table_opt) + { + my_bool changed= FALSE; + create_info->create_table_options_orig.table_opt=

Can't we ensure that table->s->create_table_options always exists? It would remove a lot of if's in your code.

+ create_table_list_merge((table->s->create_table_options? + table->s->create_table_options->table_opt: + NULL), + create_info->create_table_options_orig.table_opt, + thd->mem_root, + &changed);

<cut>

--- sql/sql_yacc.yy 2009-11-12 04:31:28 +0000

<cut>

+ | IDENT_sys equal plugin_option_value + { + LEX *lex= Lex; + create_option_add(&(lex-> + create_info. + create_table_options_orig.table_opt), + YYTHD->mem_root, &$1, &$3, + NULL);

I assume it's ok that we here collect the options in inverse order, as long as we revert this when we add it to the table, key and field objects. <cut>

+plugin_option_value: + DEFAULT + { + $$.str= NULL; /* We are going to remove the option */ + $$.length= 0; + } + | NULL_SYM + { + $$.str= NULL; /* We are going to remove the option */ + $$.length= 0; + }

I didn't see you test for = NULL in your test results. Please add this! <cut>

+++ sql/table.cc 2009-12-01 21:38:32 +0000 @@ -670,12 +670,13 @@ uint db_create_options, keys, key_parts, n_length; uint key_info_length, com_length, null_bit_pos; uint vcol_screen_length; - uint extra_rec_buf_length; + uint extra_rec_buf_length, options_len; uint i,j; bool use_hash; char *keynames, *names, *comment_pos, *vcol_screen_pos; uchar *record; - uchar *disk_buff, *strpos, *null_flags, *null_pos; + uchar *disk_buff, *strpos, *null_flags, *null_pos, *options; + uchar *buff= 0; ulong pos, record_offset, *rec_per_key, rec_buff_length; handler *handler_file= 0; KEY *keyinfo; @@ -791,7 +792,8 @@

for (i=0 ; i < keys ; i++, keyinfo++) { - keyinfo->table= 0; // Updated in open_frm + keyinfo->table= 0; // Updated in open_frm + keyinfo->create_options= NULL; // Updated in create_options_bindings

Both of above can be removed (as we reset keyinfo with bzero() just above)

if (new_frm_ver >= 3) { keyinfo->flags= (uint) uint2korr(strpos) ^ HA_NOSAME; @@ -861,15 +863,14 @@ if ((n_length= uint4korr(head+55))) { /* Read extra data segment */ - uchar *buff, *next_chunk, *buff_end; + uchar *next_chunk, *buff_end; DBUG_PRINT("info", ("extra segment size is %u bytes", n_length)); if (!(next_chunk= buff= (uchar*) my_malloc(n_length, MYF(MY_WME)))) goto err; if (my_pread(file, buff, n_length, record_offset + share->reclength, MYF(MY_NABP))) { - my_free(buff, MYF(0)); - goto err; + goto free_and_err; } share->connect_string.length= uint2korr(buff); if (!(share->connect_string.str= strmake_root(&share->mem_root, @@ -877,8 +878,7 @@ share->connect_string. length))) { - my_free(buff, MYF(0)); - goto err; + goto free_and_err; } next_chunk+= share->connect_string.length + 2; buff_end= buff + n_length; @@ -898,8 +898,7 @@ plugin_data(tmp_plugin, handlerton *))) { /* bad file, legacy_db_type did not match the name */ - my_free(buff, MYF(0)); - goto err; + goto free_and_err; } /* tmp_plugin is locked with a local lock. @@ -928,8 +927,7 @@ error= 8; my_error(ER_OPTION_PREVENTS_STATEMENT, MYF(0), "--skip-partition"); - my_free(buff, MYF(0)); - goto err; + goto free_and_err; } plugin_unlock(NULL, share->db_plugin); share->db_plugin= ha_lock_engine(NULL, partition_hton); @@ -943,8 +941,7 @@ /* purecov: begin inspected */ error= 8; my_error(ER_UNKNOWN_STORAGE_ENGINE, MYF(0), name.str); - my_free(buff, MYF(0)); - goto err; + goto free_and_err; /* purecov: end */ } next_chunk+= str_db_type_length + 2; @@ -960,16 +957,14 @@ memdup_root(&share->mem_root, next_chunk + 4, partition_info_len + 1))) { - my_free(buff, MYF(0)); - goto err; + goto free_and_err; } } #else if (partition_info_len) { DBUG_PRINT("info", ("WITH_PARTITION_STORAGE_ENGINE is not defined")); - my_free(buff, MYF(0)); - goto err; + goto free_and_err; } #endif next_chunk+= 5 + partition_info_len; @@ -995,6 +990,17 @@ #endif next_chunk++; } + if (share->db_create_options & HA_OPTION_TEXT_CREATE_OPTIONS) + { + /* + store options position, but skip till the time we will + know number of fields + */ + options_len= uint4korr(next_chunk); + options= next_chunk + 4; + next_chunk+= options_len; + options_len-= 4; + } keyinfo= share->key_info; for (i= 0; i < keys; i++, keyinfo++) { @@ -1005,8 +1011,7 @@ { DBUG_PRINT("error", ("fulltext key uses parser that is not defined in .frm")); - my_free(buff, MYF(0)); - goto err; + goto free_and_err; } parser_name.str= (char*) next_chunk; parser_name.length= strlen((char*) next_chunk); @@ -1016,12 +1021,10 @@ if (! keyinfo->parser) { my_error(ER_PLUGIN_IS_NOT_LOADED, MYF(0), parser_name.str); - my_free(buff, MYF(0)); - goto err; + goto free_and_err; } } } - my_free(buff, MYF(0)); } share->key_block_size= uint2korr(head+62);

@@ -1031,21 +1034,21 @@ share->rec_buff_length= rec_buff_length; if (!(record= (uchar *) alloc_root(&share->mem_root, rec_buff_length))) - goto err; /* purecov: inspected */ + goto free_and_err; /* purecov: inspected */ share->default_values= record; if (my_pread(file, record, (size_t) share->reclength, record_offset, MYF(MY_NABP))) - goto err; /* purecov: inspected */ + goto free_and_err; /* purecov: inspected */

VOID(my_seek(file,pos,MY_SEEK_SET,MYF(0))); if (my_read(file, head,288,MYF(MY_NABP))) - goto err; + goto free_and_err; #ifdef HAVE_CRYPTED_FRM if (crypted) { crypted->decode((char*) head+256,288-256); if (sint2korr(head+284) != 0) // Should be 0 - goto err; // Wrong password + goto free_and_err; // Wrong password } #endif

@@ -1065,6 +1068,20 @@ share->comment.length);

DBUG_PRINT("info",("i_count: %d i_parts: %d index: %d n_length: %d int_length: %d com_length: %d vcol_screen_length: %d", interval_count,interval_parts, share->keys,n_length,int_length, com_length, vcol_screen_length)); + + if (share->db_create_options & HA_OPTION_TEXT_CREATE_OPTIONS) + { + if (!(share->create_table_options= + create_create_options(&share->mem_root, share->fields, keys)) || + create_options_read(options, options_len, &share->mem_root, + share->create_table_options)) + { + goto free_and_err; + } + }

I think it would be good to always create share->create_table_options or have this as a basic struct in share; It makes some of the other code a bit easer.

=== modified file 'sql/table.h' --- sql/table.h 2009-11-12 04:31:28 +0000 +++ sql/table.h 2009-12-01 20:45:54 +0000 @@ -310,6 +310,7 @@ #ifdef NOT_YET struct st_table *open_tables; /* link to open tables */ #endif + TABLE_OPTIONS *create_table_options; /* text options for table */

See comment that we should consider making this a full object, not a pointer)

+++ sql/unireg.cc 2009-12-01 20:45:54 +0000 @@ -75,6 +75,94 @@ return is_handled; }

+ +/** + Collects information about fields text create options into one array + + @param thd Thread handler + @param create_fields List of fields + @param create_table_options Table create options information + structure to store result in it + + @retval FALSE OK + @retval TRUE Error +*/ + +static my_bool +collect_fields_create_options(THD *thd, List &create_fields, + TABLE_OPTIONS *create_table_options) +{ + List_iterator it(create_fields); + Create_field *field; + uint index= 0; + uint fields= create_fields.elements; + bool is_allocated= create_table_options->field_opt; + DBUG_ENTER("collect_fields_create_options"); + + while ((field= it++)) + { + if (field->create_options) + { + if (!is_allocated) + { + if (!(create_table_options->field_opt= + create_create_options_array(thd->mem_root, + fields))) + { + DBUG_RETURN(TRUE); + } + is_allocated= TRUE;

Move directly after test. (Easier to understand and in theory a bit faster as we are changing memory we are looking at)

+ } + create_table_options->field_opt[index]= field->create_options; + } + index++; + }

Move index to part of loop to make code easier to read: for (index= 0 ; (field= it++) ; index++)

+static my_bool +collect_keys_create_options(THD *thd, uint keys, KEY *key_info, + TABLE_OPTIONS *create_table_options) +{ + uint index; + bool is_allocated= create_table_options->key_opt; + DBUG_ENTER("collect_keys_create_options"); + + for(index= 0; index < keys; index++, key_info++)

Better loop here ! Add however space after 'for'.

+ { + if (key_info->create_options) + { + if (!is_allocated) + { + if (!(create_table_options->key_opt= + create_create_options_array(thd->mem_root, + keys))) + { + DBUG_RETURN(TRUE); + } + is_allocated= TRUE;

move row after if (!is_allocated)

+ } + create_table_options->key_opt[index]= key_info->create_options; + key_info->create_options= create_table_options->key_opt[index]; + } + } + DBUG_RETURN(FALSE); +}

<cut>

@@ -124,6 +213,7 @@ DBUG_RETURN(1); DBUG_ASSERT(db_file != NULL);

+ create_info->create_table_options= &create_info->create_table_options_orig; /* If fixed row records, we need one bit to check for deleted rows */ if (!(create_info->table_options & HA_OPTION_PACK_RECORD)) create_info->null_bits++; @@ -183,6 +273,25 @@ create_info->extra_size+= key_info[i].parser_name->length + 1; }

+ if (collect_fields_create_options(thd, create_fields, + create_info->create_table_options) || + collect_keys_create_options(thd, keys, key_info, + create_info->create_table_options)) + { + my_free(screen_buff, MYF(0)); + DBUG_RETURN(1); + }

Is not create_table_options here always guaranteed to be set ?

+ if (create_info->create_table_options && + (create_info->create_table_options->table_opt || + create_info->create_table_options->field_opt || + create_info->create_table_options->key_opt)) + { + create_info->table_options|= HA_OPTION_TEXT_CREATE_OPTIONS;

+ if (options_len) + { + uchar *optbuff= (uchar *)my_malloc(options_len, MYF(0)); + DBUG_PRINT("info", ("Create options length: %u", options_len)); + if (!optbuff) + goto err; + int4store(optbuff, options_len); + create_options_store(optbuff + 4, create_info->create_table_options); + if (my_write(file, optbuff, options_len, MYF_RW)) + { + my_free(optbuff, MYF(0)); + goto err; + } + my_free(optbuff, MYF(0));

A little bit cleaner with: error= my_write(file, optbuff, options_len, MYF_RW); my_free(optbuff, MYF(0)); if (error) goto err; <cut>

=== modified file 'storage/example/ha_example.cc' --- storage/example/ha_example.cc 2008-02-24 13:12:17 +0000 +++ storage/example/ha_example.cc 2009-12-01 20:45:54 +0000 @@ -836,11 +836,39 @@ int ha_example::create(const char *name, TABLE *table_arg, HA_CREATE_INFO *create_info) { + CREATE_OPTION *opt; DBUG_ENTER("ha_example::create"); /* This is not implemented but we want someone to be able to see that it works. */ + /* Example of checking parameters for table*/ + for(opt= create_info->create_table_options->table_opt; opt; opt= opt->next)

space after for

+ { + /* check for legal options and its legal values */ + if (opt->key.length == 1 && + (opt->key.str[0] == 'e' || opt->key.str[0] == 'E') && + opt->val.length == 1 && + opt->val.str[0] == '1') + opt->used= 1; /* suppose that we used the only legal parameter */

suppose -> Tell MySQL

+ } + /* Example of checking parameters for fields*/ + for (Field **field= table_arg->s->field; *field; field++) + { + if ((*field)->create_options) + { + for(opt= (*field)->create_options; opt; opt= opt->next)

space after for

+ { + /* check for legal options and its legal values */ + if (opt->key.length == 1 && + (opt->key.str[0] == 'e' || opt->key.str[0] == 'E') && + opt->val.length == 1 && + opt->val.str[0] == '1') + opt->used= 1; /* suppose that we used the only legal parameter */

suppose -> Tell MySQL

+ } + } + } + DBUG_RETURN(0); }

Regards, Monty

Hi!

...

...

...

...

"Oleksandr" == Oleksandr Byelkin writes:

Oleksandr> Michael Widenius wrote: Oleksandr> [skip]

...

...

+ DBUG_ENTER("create_table_list_merge"); + + for(; changes; changes= changes->next) + { + if (create_option_add(&source, root, &changes->key, &changes->val, + (chng ? NULL : &chng)))

Instead of test, send &chng to function and do: (*changed)|= chng;

Oleksandr> It will make create_option_add more complicated, because it need info Oleksandr> was that change in this list.

Sorry, but don't understand the above. I didn't notice before that the create_option_add() function does things differently if changed is set or not. I looked at the function create_option_list() but don't understand why you copy all other arguments after you have found the first change, even if the value didn't change. The function prototype of create_option_add() also doesn't tell anything about this strange usage of 'changed'. So the question is: Why does create_option_add() work differently if changed is set or not? Oleksandr> [skip]

...

...

*/ if (tablespace && (table->file->get_tablespace_name(thd, tablespace, FN_LEN))) @@ -6089,6 +6102,30 @@ } restore_record(table, s->default_values); // Empty record for DEFAULT

+ if (create_info->create_table_options_orig.table_opt) + { + my_bool changed= FALSE; + create_info->create_table_options_orig.table_opt=

Can't we ensure that table->s->create_table_options always exists? It would remove a lot of if's in your code.

Oleksandr> It is difficult because temporary tables involved. As temporary tables is only created in one place it would be trivial to ensure that the pointer always exits. Oleksandr> [skip]

...

...

=== modified file 'sql/table.h' --- sql/table.h 2009-11-12 04:31:28 +0000 +++ sql/table.h 2009-12-01 20:45:54 +0000 @@ -310,6 +310,7 @@ #ifdef NOT_YET struct st_table *open_tables; /* link to open tables */ #endif + TABLE_OPTIONS *create_table_options; /* text options for table */

See comment that we should consider making this a full object, not a pointer

Oleksandr> We have plugins on C and I think it will be inconvenient to convert Oleksandr> object to something C can read.

There is no difference for C if the above variable is a TABLE_OPTIONS or TABLE_OPTIONS *; We can always send the pointer to the struct for any function that would need it. Regards, Monty