Re: [Maria-developers] options for CREATE TABLE (MWL#43)
11 Mar
2010
11 Mar
'10
10:24 p.m.
Hi!
Some you request contradicts with Mony's ones so I think it should be
discussed somehow.
11 марта 2010, в 12:30, Sergei Golubchik написал(а):
> Hi, Sanja!
>
> Here's the review, below:
>
> Summary:
>
> 1. please, store options together with the objects they describe, not
> separately.
I tried to do so in my very first implementation and IMHO it was my
mistake. The same related to way of storing elements - it should be
the same for parsing and reading. During parsing we one one structures
and classes, for storing in TABLE/TABLE_SHARE others. Moving data
between them (taking into account different memroot where they are
allocated) was quite tricky. I can make it again if you both think
that it is really important.
> 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.
> 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?
> 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.
> 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 TEBLE. Monty's
requirement was do not touch current code. I would be glad if you
discuss it and make some non contradicting requirement.
> 6. parser: use ident, not IDENT_sys
OK
> 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)
Other problem is should we store old options in new way, old way,
both. (I think in this case both).
> 8. few existing options, like row_format, insert_method, checksum,
> delay_key_write, key_block_size, min_rows/max_rows, avg_row_length,
> tablespace, connection, pack_keys could be moved into storage
> engines
> out of the parser.
See above.
> 9. make sure your code works (and tested) with table options specified
> per partition/subpartition
OK.
> 10. misc details, like using 'changed' or unnecessary complex encoding
> of options in the frm file, see below.
>
>> === 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 2010-03-04 20:46:55 +0000
>> @@ -0,0 +1,197 @@
>> +drop table if exists t1;
>> +create table t1 (a int fkey1=v1, key akey (a) kkey1=v1) tkey1=1v1
>> TKEY1=NULL tkey1=1v2 tkey2=2v1 tkey3=3v1;
>> +Warnings:
>> +Warning 1650 Unused option 'tkey1'='1v2'
>> +Warning 1650 Unused option 'tkey2'='2v1'
>> +Warning 1650 Unused option 'tkey3'='3v1'
>> +Warning 1651 Unused option 'fkey1'='v1' of field 'a'
>> +Warning 1652 Unused option 'kkey1'='v1' of key 'akey'
>
> 1. Better "unknown" or "unsupported" e.g.
>
> Unknown option 'tkey1'
> Unsupported option 'fkey1' specified for field 'a'
> Invalid option 'kkey1' used for key 'akey'
>
> no, "invalid" is bad here, scratch that
ok
>
> 2. why there's no warning for TKEY1=NULL ?
Because it means remove option.
>
>> +drop table t1;
>> +create table t1 (a int fkey1=v1, key akey (a) kkey1=v1) tkey1=1v1
>> tkey1=1v2 TKEY1=NULL tkey1=1v1 tkey1=1v2 tkey2=2v1 tkey3=3v1;
>
> I don't understand how this is different from the first test
> (and many of the tests bellow),
> could you please add short one-line comments to the .test file
keys are in different order.
> explaining what you test in each statement ?
OK
>
> also, a thought about "warning vs errors":
> making warnings for typos and unknown options is one of the most
> disliked features in MySQL - judging from the number of bugreports
> (bug reports about USE HASH/BTREE, mind you - only a couple of places
> where MySQL is promiscuous like that, guess what will happen when your
> patch will take it to a whole new level!).
>
> moving engines, and so on, I know - but most users don't care.
>
> STRICT mode is too strict here, I think, it adds too much strictness
> everywhere. What about adding a special mode that's only "strict" in
> create
> table (and alter table - user specified part) ? That should be ON by
> default
> (or, rather, a negative mode should be OFF by default).
>
> In other words - I want the patch to be optimized (performance, and
> user
> experience) for the common case, not to boundary cases. And the common
> case, I believe, is the one when a user does not change engines all
> the
> time. We support the boundary case, yes, but optimize for the common
> one.
You remember that I also was for errors, but MOnty still want
warnings. Also there is problem in implementation of the way we
agreed on (see abopve about ALTER TABLE).
>> +Warnings:
>> +Warning 1650 Unused option 'tkey1'='1v2'
>> +Warning 1650 Unused option 'tkey2'='2v1'
>> +Warning 1650 Unused option 'tkey3'='3v1'
>> +Warning 1651 Unused option 'fkey1'='v1' of field 'a'
>> +Warning 1652 Unused option 'kkey1'='v1' of key 'akey'
>> +drop table t1;
>> +create table t1 (a int fkey1=v1, key akey (a) kkey1=v1) tkey1=1v1
>> tkey1=1v2 TKEY1='NULL' tkey2=2v1 tkey3=3v1;
> ...
>> === added file 'mysql-test/t/create_options_example.test'
>> --- mysql-test/t/create_options_example.test 1970-01-01 00:00:00
>> +0000
>> +++ mysql-test/t/create_options_example.test 2010-03-04 20:46:55
>> +0000
>> @@ -0,0 +1,16 @@
>> +--source include/have_example_plugin.inc
>> +
>> +--disable_warnings
>> +drop table if exists t1;
>> +--enable_warnings
>> +
>> +#All vaues with warnings
>
> this should go into plugin.test or exampledb.test
Why not separate test?
>> +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;
>> +
>> +drop table t1;
>> +
>> +# E=1 accepted by engine
>> +create table t1 (a int ttt=xxx E=1) ENGINE=EXAMPLE E=1 ttt=xxx
>> ttt=yyy TTT=DEFAULT mmm=CCC zzz=MMM;
>> +
>> +drop table t1;
>> +
>> === modified file 'sql/Makefile.am'
>> --- sql/Makefile.am 2010-03-03 14:44:14 +0000
>> +++ sql/Makefile.am 2010-03-04 20:46:55 +0000
>> @@ -124,7 +124,7 @@ mysqld_SOURCES = sql_lex.cc sql_handler.
>> sql_plugin.cc sql_binlog.cc \
>> sql_builtin.cc sql_tablespace.cc
>> partition_info.cc \
>> sql_servers.cc event_parse_data.cc \
>> - opt_table_elimination.cc
>> + opt_table_elimination.cc
>> sql_create_options.cc
>
> please make sure that 'make distcheck' works after your changes
OK
>>
>> nodist_mysqld_SOURCES = mini_client_errors.c pack.c client.c
>> my_time.c my_user.c
>>
>> === modified file 'storage/example/ha_example.cc'
>> --- storage/example/ha_example.cc 2010-03-03 14:44:14 +0000
>> +++ storage/example/ha_example.cc 2010-03-04 20:46:55 +0000
>> @@ -836,11 +836,43 @@ ha_rows ha_example::records_in_range(uin
>> 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*/
>> + if (!create_info->create_table_options)
>> + DBUG_RETURN(0);
>> + for (opt= create_info->create_table_options->table_opt.first;
>> + opt;
>> + opt= opt->next)
>> + {
>> + /* 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; /* tell MariaDB that we used the only legal
>> parameter */
>> + }
>> + /* Example of checking parameters for fields*/
>> + for (Field **field= table_arg->s->field; *field; field++)
>> + {
>> + if ((*field)->create_options.first)
>> + {
>> + for (opt= (*field)->create_options.first; opt; opt= opt->next)
>> + {
>> + /* 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; /* tell MariaDB that we used the only
>> legal parameter */
>> + }
>> + }
>> + }
>
> No, that's way too complex and too much code.
> *every* engine will need to do that, which means - it should be done
> in the
> server for all engines. Why you didn't use my_getopt as we originally
> discussed ?
OK (see above)
>> +
>> DBUG_RETURN(0);
>> }
>>
>> === added file 'sql/sql_create_options.h'
>> --- sql/sql_create_options.h 1970-01-01 00:00:00 +0000
>> +++ sql/sql_create_options.h 2010-03-04 20:46:55 +0000
>> @@ -0,0 +1,102 @@
>> +
>> +#ifndef _SQL_CREATE_OPTIONS_H
>> +#define _SQL_CREATE_OPTIONS_H
>> +
>> +
>> +/* types of cretate options records on disk, also it is length of
>> extra data */
>
> 1. typo: create
> 2. I know what does "length of extra data" mean, but the comment does
> not help to understand it.
I just forgot to change comment after changing parameter (monty wanted
2 bytes for key number also just tu be able to increase number of keys)
>> +typedef enum enum_create_options_type {
>> + CREATE_OPTION_TABLE= 0,
>> + CREATE_OPTION_KEY= 1,
>> + CREATE_OPTION_FIELD= 2
>> +} CREATE_OPTION_TYPES;
>> +
>> +typedef struct st_create_option {
>> + /* pointer to the next option or NULL */
>> + struct st_create_option *next;
>> + /* pointer to Field or KEY or NULL */
>> + void *owner;
>
> 1. better to use union { Field *, KEY *}
> 2. even better - use 'const char* name' as you don't need anything
> else
> from your fields/keys here
> 3. even better remove this 'owner' at all, you don't need it - see
> below,
> if you iterate the list of fields and keys you always know
> what field/key the option belongs to.
OK
>> + /* key and value of the option (\0 terminated)*/
>> + LEX_STRING key, val;
>> + /* used to issue warnings about unused options */
>> + my_bool used;
>> +} CREATE_OPTION;
>> +
>> +struct st_table_options;
>> +
>> +
>> +class st_create_option_list {
>
> why did you need to create your own list implementation instead of
> using
> either one that MySQL already has ?
> (hint: LIST, I_List, List, or even dynamic array)
>
> and -
> why do you need any list at all, if you store options in Fields and
> KEYs
> and simply can use the existing lists of fields and keys ?
Historically. But yes now it would be better to use LIST (if it allow
to insert item at the end)...
>> +public:
>> + /**
>> + pointer on the first list element
>> + */
>> + CREATE_OPTION *first;
>> + /**
>> + pointer on last list '.next' or beginning of the list in case
>> of empty list
>> +
>> + @note:
>> + If it is NULL then it is just sign of array of list end
>> + */
>> +private:
>> + CREATE_OPTION **last;
>> +public:
>> + void empty() {first= NULL; last= &first;}
>> + st_create_option_list() {empty();}
>> + st_create_option_list(const st_create_option_list &o)
>> + {
>> + if ((first= o.first))
>> + last= o.last;
>> + else
>> + last= &first;
>> + }
>> + my_bool last_opt() { return last == NULL; }
>> + friend my_bool create_option_add(st_create_option_list *options,
>> + MEM_ROOT *root,
>> + const LEX_STRING *str_key,
>> + const LEX_STRING *str_val,
>> + my_bool *changed);
>> + friend st_create_option_list
>> *create_create_options_array(MEM_ROOT *root,
>> + uint n);
>> + friend my_bool create_options_read(const uchar *buff, uint length,
>> + MEM_ROOT *root,
>> + st_table_options *opt);
>> + friend my_bool create_options_clone(MEM_ROOT *root,
>> + st_create_option_list *opts);
>> +};
>> +typedef class st_create_option_list CREATE_OPTION_LIST;
>> +
>> +
>> +typedef struct st_table_options {
>> + CREATE_OPTION_LIST table_opt; /* table options list */
>> + CREATE_OPTION_LIST *field_opt; /* fields options array */
>> + CREATE_OPTION_LIST *key_opt; /* keys options array */
>> +} TABLE_OPTIONS;
>> +
>> +CREATE_OPTION_LIST *create_create_options_array(MEM_ROOT *root,
>> uint n);
>> +TABLE_OPTIONS *create_create_options(MEM_ROOT *root, uint fields,
>> uint keys);
>> +
>> +my_bool create_options_read(const uchar *buff, uint length,
>> MEM_ROOT *root,
>> + TABLE_OPTIONS *opt);
>> +
>> +my_bool create_option_add(CREATE_OPTION_LIST *options, MEM_ROOT
>> *root,
>> + const LEX_STRING *k, const LEX_STRING *v,
>> + my_bool *chanes);
>> +
>> +ulong create_options_length(TABLE_OPTIONS *opt);
>> +
>> +void create_options_store(uchar *buff, TABLE_OPTIONS *opt);
>> +
>> +void create_options_check_unused(THD *thd,
>> + TABLE_OPTIONS *options);
>> +
>> +struct st_table_share;
>> +void create_options_binding(struct st_table_share *share);
>> +
>> +my_bool create_options_clone(MEM_ROOT *root, CREATE_OPTION_LIST
>> *opt);
>> +
>> +CREATE_OPTION_LIST *create_table_list_merge(CREATE_OPTION_LIST
>> *source,
>> + CREATE_OPTION_LIST
>> *changes,
>> + MEM_ROOT *root,
>> + my_bool *changed);
>> +my_bool is_equal_create_options(CREATE_OPTION *opt1, CREATE_OPTION
>> *opt2);
>> +
>> +#endif
>> === modified file 'sql/table.h'
>> --- sql/table.h 2010-02-12 08:47:31 +0000
>> +++ sql/table.h 2010-03-04 20:46:55 +0000
>> @@ -340,6 +340,7 @@ typedef struct st_table_share
>> #ifdef NOT_YET
>> struct st_table *open_tables; /* link to open tables */
>> #endif
>> + TABLE_OPTIONS *create_table_options; /* text options for table */
>
> do you need TABLE_OPTIONS - I mean, table, field, and key options -
> here ? TABLE_SHARE has an array of KEYs and KEYs store options
> internally (in KEY::create_options). And exactly the same
> applies to Fields.
I described it in the beginning.
>>
>> /* The following is copied to each TABLE on OPEN */
>> Field **field;
>> === modified file 'sql/structs.h'
>> --- sql/structs.h 2010-02-01 06:14:12 +0000
>> +++ sql/structs.h 2010-03-04 20:46:55 +0000
>> @@ -101,6 +101,8 @@ typedef struct st_key {
>> int bdb_return_if_eq;
>> } handler;
>> struct st_table *table;
>> + /** reference to the list of options or NULL */
>> + CREATE_OPTION_LIST create_options;
>
> eh, strictly speaking 'create_options' is not a pointer and it
> cannot be NULL.
> And it is not a reference in the C++ sense either.
>
> you could've simply said "list of options"
Not fixed comment, sorry.
>> } KEY;
>>
>>
>> === modified file 'sql/handler.h'
>> --- sql/handler.h 2010-02-01 06:14:12 +0000
>> +++ sql/handler.h 2010-03-04 20:46:55 +0000
>> @@ -919,6 +919,12 @@ typedef struct st_ha_create_information
>> LEX_STRING connect_string;
>> const char *password, *tablespace;
>> LEX_STRING comment;
>> + TABLE_OPTIONS create_table_options_orig;
>> + /**
>> + Originally create_table_options points on above field, but
>> during ALTER
>> + TABLE of the options it points on new built parameters
>> + */
>> + TABLE_OPTIONS *create_table_options;
>
> after reading the patch I still don't understand why do you need
> create_table_options_orig
For avoiding allocating it for normal table, it will be changed in
ALTER TABLE process.
>> const char *data_file_name, *index_file_name;
>> const char *alias;
>> ulonglong max_rows,min_rows;
>> === modified file 'sql/sql_class.cc'
>> --- sql/sql_class.cc 2010-02-01 06:14:12 +0000
>> +++ sql/sql_class.cc 2010-03-04 20:46:55 +0000
>> @@ -109,6 +109,8 @@ Key::Key(const Key &rhs, MEM_ROOT *mem_r
>> generated(rhs.generated)
>> {
>> list_copy_and_replace_each_value(columns, mem_root);
>> + create_options= rhs.create_options;
>> + create_options_clone(mem_root, &create_options);
>
> in create_options_clone() you don't need to clone everything,
> this constructor only copies elements that can change during
> execution,
> for example field and key names don't change and don't need to be
> copied. And options don't change either, only their "used" property
> is.
> but it would be best if you would get rid of it and make options
> completely
> immutable.
There was problems like pointer on freed memory which gone after this
I suspect different mem_roots.
>> }
>>
>> /**
>> === modified file 'sql/field.h'
>> --- sql/field.h 2010-02-01 06:14:12 +0000
>> +++ sql/field.h 2010-03-04 20:46:55 +0000
>> @@ -137,6 +137,8 @@ class Field
>> struct st_table *table; // Pointer for table
>> struct st_table *orig_table; // Pointer to original table
>> const char **table_name, *field_name;
>> + /** reference to the list of options or NULL */
>
> this is neither a reference nor it can be NULL
old comment
>> + CREATE_OPTION_LIST create_options;
>> LEX_STRING comment;
>> /* Field is part of the following keys */
>> key_map key_start, part_of_key, part_of_key_not_clustered;
>> === modified file 'sql/field.cc'
>> --- sql/field.cc 2010-02-01 06:14:12 +0000
>> +++ sql/field.cc 2010-03-04 20:46:55 +0000
>> @@ -10220,6 +10225,7 @@ Create_field::Create_field(Field *old_fi
>> decimals= old_field->decimals();
>> vcol_info= old_field->vcol_info;
>> stored_in_db= old_field->stored_in_db;
>> + create_options= old_field->create_options;
>
> explain in a comment please why you don't need to copy the data
> here, and can simply assign pointers
Because copy constructor makes correct list assignment, is it correct
comment?
>
>>
>> /* Fix if the original table had 4 byte pointer blobs */
>> if (flags & BLOB_FLAG)
>> === modified file 'sql/sql_show.cc'
>> --- sql/sql_show.cc 2010-02-01 06:14:12 +0000
>> +++ sql/sql_show.cc 2010-03-04 20:46:55 +0000
>> @@ -1356,6 +1376,8 @@ int store_create_info(THD *thd, TABLE_LI
>> packet->append(STRING_WITH_LEN(" COMMENT "));
>> append_unescaped(packet, field->comment.str, field-
>> >comment.length);
>> }
>> + if (field->create_options.first)
>
> you don't need an if() here and below, append_create_options()
> can handle the case of create_options.first == 0
OK (but i will need change list implementation in any case)
>
>> + append_create_options(thd, packet, field-
>> >create_options.first);
>> }
>>
>> key_info= table->key_info;
>> @@ -1586,6 +1610,11 @@ int store_create_info(THD *thd, TABLE_LI
>> packet->append(STRING_WITH_LEN(" CONNECTION="));
>> append_unescaped(packet, share->connect_string.str, share-
>> >connect_string.length);
>> }
>> + /* create_table_options can be NULL for temporary tables */
>> + if (share->create_table_options &&
>
> why TABLE_SHARE::create_table_options is a pointer to something
> allocared
> on TABLE_SHARE::mem_root ? In Field and KEY it's simply
> a structure - part of the Field/KEY class, why not the same here ?
Most time it is pointer to create_table_options_orig.
It is not the same here because of ALTER TABLE and the way how it
plays with TABLE_SHARE.
>
>> + share->create_table_options->table_opt.first)
>> + append_create_options(thd, packet,
>> + share->create_table_options-
>> >table_opt.first);
>> 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 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)
>> + DBUG_PRINT("info", ("Options difference in field '%s'",
>> + new_field->field_name));
>> + *need_copy_table= ALTER_TABLE_DATA_CHANGED;
>> + DBUG_RETURN(0);
>> + }
>> +
>> /* Don't pack rows in old tables if the user has requested
>> this. */
>> if (create_info->row_type == ROW_TYPE_DYNAMIC ||
>> (tmp_new_field->flags & BLOB_FLAG) ||
>> @@ -6112,6 +6125,41 @@ mysql_prepare_alter_table(THD *thd, TABL
>> }
>> restore_record(table, s->default_values); // Empty record for
>> DEFAULT
>>
>> + if (create_info->create_table_options_orig.table_opt.first)
>> + {
>> + CREATE_OPTION_LIST *res;
>> + my_bool changed= FALSE;
>> + if (!table->s->create_table_options &&
>> + !(table->s->create_table_options=
>> + create_create_options(&table->s->mem_root,
>> + table->s->fields, table->s->keys)))
>> + goto err;
>> +
>> + if (!(res=
>> + create_table_list_merge(&table->s->create_table_options-
>> >table_opt,
>> + &create_info->
>> +
>> create_table_options_orig.table_opt,
>> + thd->mem_root,
>> + &changed)))
>> + goto err;
>> + DBUG_ASSERT(res->first);
>> + create_info->create_table_options_orig.table_opt= *res;
>> +
>> + if (changed)
>> + alter_info->change_level= ALTER_TABLE_DATA_CHANGED;
>> + else
>> + {
>> + alter_info->flags&= ~ALTER_CREATE_OPT;
>> + DBUG_PRINT("info", ("Table options was not changed"));
>> + }
>> + }
>> + else
>> + if (table->s->create_table_options)
>> + create_info->create_table_options_orig.table_opt=
>> + table->s->create_table_options->table_opt;
>
> why don't you set ALTER_TABLE_DATA_CHANGED here ?
it used as flag from parser only.
>
>> + else
>> + create_info->create_table_options_orig.table_opt.empty();
>> +
>> /*
>> First collect all fields from table which isn't in drop_list
>> */
>> === modified file 'sql/sql_yacc.yy'
>> --- sql/sql_yacc.yy 2010-02-01 06:14:12 +0000
>> +++ sql/sql_yacc.yy 2010-03-04 20:46:55 +0000
>> @@ -4714,6 +4718,16 @@ create_table_option:
>> Lex->create_info.used_fields|=
>> HA_CREATE_USED_TRANSACTIONAL;
>> Lex->create_info.transactional= $3;
>> }
>> + | IDENT_sys equal plugin_option_value
>
> 1. why IDENT_sys and not ident ?
OK
> 2. perhaps we should make the equal sign optional ?
> first - that's backward compatible,
> second - that would allow us to simplify the code quite a bit,
> moving existing table and index options onto a new framework
Answered above.
>> + {
>> + LEX *lex= Lex;
>> + create_option_add(&(lex->
>> + create_info.
>> +
>> create_table_options_orig.table_opt),
>> + YYTHD->mem_root, &$1, &$3,
>> + NULL);
>> + lex->alter_info.flags|= ALTER_CREATE_OPT;
>> + }
>> ;
>>
>> default_charset:
>> @@ -13827,6 +13867,32 @@ uninstall:
>> }
>> ;
>>
>> +/
>> **************************************************************************
>> +
>> + Create options
>> +
>> +
>> **************************************************************************/
>> +
>> +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".
>> + {
>> + $$.str= NULL; /* We are going to remove the option */
>> + $$.length= 0;
>> + }
>> + | IDENT_sys { $$ = $1; }
>> + | TEXT_STRING_sys { $$ = $1; }
>> + | DECIMAL_NUM { $$ = $1; }
>> + | FLOAT_NUM { $$ = $1; }
>> + | NUM { $$ = $1; }
>> + | LONG_NUM { $$ = $1; }
>> + | HEX_NUM { $$ = $1; }
>
> looks like you forgot a semicolon here
OK
>> +
>> +
>> /**
>> @} (end of group Parser)
>> */
>>
>> === added file 'sql/sql_create_options.cc'
>> --- sql/sql_create_options.cc 1970-01-01 00:00:00 +0000
>> +++ sql/sql_create_options.cc 2010-03-04 20:46:55 +0000
>> @@ -0,0 +1,646 @@
>> +
>> +#include "mysql_priv.h"
>> +
>> +/* Additional length of index for CREATE_OPTION_XXX types */
>
> the comment is confusing. I could understand from the code what
> create_options_len[] is for, but the comment did not help in the least
"Length of additional data stored for every CREATE_OPTION_XXX types "
Is it OK?
>
>> +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
>> +
>> + @retval TRUE error
>> + @retval FALSE OK
>> +*/
>> +
>> +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.
>> +
>> + /* 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
>> + {
>> + /* add */
>> + if (!(cur_option= (CREATE_OPTION *)alloc_root(root,
>> +
>> sizeof(CREATE_OPTION))))
>> + DBUG_RETURN(TRUE);
>> + bzero(cur_option, sizeof(CREATE_OPTION));
>> + *(options->last)= cur_option;
>> + options->last= &(cur_option->next);
>> + *changed= TRUE;
>> + }
>> + if (changed || replace)
>> + {
>> + /*
>> + In case of replace we use new key in case it differ only
>> in case
>> + like 'key' and 'KEY'
>> + */
>> + if (!multi_alloc_root(root, &key, str_key->length + 1,
>> + &val, str_val->length + 1, NULL))
>> + DBUG_RETURN(TRUE);
>> + cur_option->key.str=
>> + (char *)memcpy(key, str_key->str,
>> + (cur_option->key.length= str_key->length));
>> + key[str_key->length]= '\0';
>> + cur_option->val.str=
>> + (char *)memcpy(val, str_val->str,
>> + (cur_option->val.length= str_val->length));
>> + val[str_val->length]= '\0';
>> + cur_option->used= FALSE;
>> + cur_option->owner= NULL;
>> + }
>> + DBUG_ASSERT(options->first ||
>> + (!options->first && options->last == &options-
>> >first));
>> + }
>> + else
>> + {
>> + /* remove */
>> + if (*option)
>> + {
>> + if (options->last == &((*option)->next))
>> + options->last= option; /* we deleted last option */
>> + *option= (*option)->next;
>> + *changed= TRUE;
>> + DBUG_ASSERT(options->first ||
>> + (!options->first && options->last == &options-
>> >first));
>> + }
>> + }
>> + 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 array or NULL in case of error.
>> +*/
>> +
>> +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.
>
>> +{
>> + uint i;
>> + DBUG_ENTER("create_create_options_array");
>> + DBUG_PRINT("enter", ("Number: %u", n));
>> +
>> + CREATE_OPTION_LIST *res=
>> + (CREATE_OPTION_LIST *) alloc_root(root,
>> + sizeof(CREATE_OPTION_LIST) * (n
>> + 1));
>> + bzero(res, sizeof(CREATE_OPTION_LIST) * (n + 1));
>> + if (!res)
>> + DBUG_RETURN(NULL);
>> + for (i= 0; i < n; i++)
>> + res[i].last= &res[i].first;
>> + /* We do not do above for res[n]. It is sign of array end */
>> + DBUG_RETURN(res);
>> +}
>> +
>> +
>> +/**
>> + 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_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?
>> + index= uint2korr(buff);
>> + buff+= 2;
>> + *(opt->field_opt[index].last)= option;
>> + opt->field_opt[index].last= &option->next;
>> + break;
>> + case CREATE_OPTION_KEY:
>> + index= uint2korr(buff);
>> + buff+= 2;
>> + *(opt->key_opt[index].last)= option;
>> + opt->key_opt[index].last= &option->next;
>> + break;
>> + case CREATE_OPTION_TABLE:
>> + /* table */
>> + *(opt->table_opt.last)= option;
>> + opt->table_opt.last= &option->next;
>> + break;
>> + default:
>> + DBUG_ASSERT(0);
>> + }
>> + if (!(option->key.str= strmake_root(root, (const char*)buff,
>> + option->key.length)))
>> + DBUG_RETURN(TRUE);
>> + buff+= option->key.length;
>> + if (!(option->val.str= strmake_root(root, (const char*)buff,
>> + option->val.length)))
>> + DBUG_RETURN(TRUE);
>> + buff+= option->val.length;
>> + option->used= FALSE;
>> + option->owner= NULL;
>> + DBUG_PRINT("info", ("type: %u index: %u key: '%s' value:
>> '%s'",
>> + (uint) type, (uint) index,
>> + option->key.str, option->val.str));
>> + }
>> + DBUG_RETURN(FALSE);
>> +}
>> +
>> +/**
>> + Calculates length of saved image of the option lists
>> +
>> + @param opt list of options
>> + @param extra_length type of the record
>
> eh, extra_length is not really a "type of the record", is it ?
it was, but you are right it should be fixed.
>> +
>> + @return length
>> +*/
>> +
>> +static ulong create_options_list_length(CREATE_OPTION_LIST *opts,
>> int extra_length)
>> +{
>> + CREATE_OPTION *opt;
>> + ulong res= 0;
>> + DBUG_ENTER("create_options_list_length");
>> + for (opt= opts->first; opt != NULL; opt= opt->next)
>> + {
>> + DBUG_PRINT("info", ("key: '%s' value: '%s'",
>> + (opt->key.str ? opt->key.str : "<NULL>"),
>> + (opt->val.str ? opt->val.str : "<NULL>")));
>> + DBUG_ASSERT(opt->key.length);
>> + /*
>> + length of disk for every record:
>> + 2 bytes - value length
>> + 1 byte - key length
>> + 1 byte - record type
>> + 0/2 bytes - none/key number/field number
>> + */
>> + res+= 2 + 1 + 1 + extra_length + opt->key.length + opt-
>> >val.length;
>> + }
>> + DBUG_RETURN(res);
>> +}
>> +
>> +/**
>> + Calculates length of saved image of the all options of the table
>> +
>> + @param opts table of options
>> +
>> + @return length
>> +*/
>> +
>> +ulong create_options_length(TABLE_OPTIONS *opt)
>> +{
>> + CREATE_OPTION_LIST *opts;
>> + ulong res;
>> + DBUG_ENTER("create_options_length");
>> +
>> + res=
>> + (opt->table_opt.first ?
>> + create_options_list_length(&opt->table_opt,
>> +
>> create_options_len[CREATE_OPTION_TABLE]):
>> + 0);
>> + if (opt->field_opt)
>> + {
>> + for (opts= opt->field_opt; !opts->last_opt(); opts++)
>
> why wouldn't you simply iterate over an array of the fixed length -
> you know how many fields and keys are there. And you wouldn't need
> this "invalid list" array element at the end.
To avoid knowing too much about other structures and classes.
> even better - as I wrote above, keep options together with fields/
> keys only
> and don't maintain a separate array of them.
I explained what problems it brings if you think that it is vitally
important I will make it.
>> + res+=
>> + create_options_list_length(opts,
>> +
>> create_options_len[CREATE_OPTION_FIELD]);
>> + }
>> + if (opt->key_opt)
>> + {
>> + for (opts= opt->key_opt; !opts->last_opt(); opts++)
>> + res+=
>> + create_options_list_length(opts,
>> +
>> create_options_len[CREATE_OPTION_KEY]);
>> + }
>> + DBUG_RETURN(res);
>> +}
>
>
> Regards,
> Sergei
5400
Age (days ago)
5400
Last active (days ago)
0 comments
1 participants
participants (1)
-
Oleksandr Byelkin