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 "
+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