maria-developers team mailing list archive
-
maria-developers team
-
Mailing list archive
-
Message #01954
Re: Added create options for table fields and keys (MWL#43).
Hi!
>>>>> "Oleksandr" == Oleksandr Byelkin <sanja@xxxxxxxxxxxx> 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<char *>(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_field> &create_fields,
> + TABLE_OPTIONS *create_table_options)
> +{
> + List_iterator<Create_field> 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
Follow ups
References
