Re: [Maria-developers] [Commits] Rev 4275: MDEV-5867 ALTER TABLE t1 ENGINE=InnoDB keeps bad options when t1 ENGINE is CONNECT in lp:~maria-captains/maria/10.0
Hi Sergei, I believe options handling is way too confusing in general: - CREATE TABLE warns/errors about unknown options - ALTER TABLE ... ENGINE doesn't - ALTER TABLE ... UNKNOWN_OPTION=1 does - CREATE TABLE ... LIKE doesn't - different engines may assign different meaning to the same option - IGNORE_BAD_TABLE_OPTIONS doesn't actually _ignore_ _bad_ options but rather _accepts_ _unkown_ options But the question is if we should show unknown options by default. I have no opinion: both cases are equally wrong to me. On the one hand we get unusable CREATE TABLE statement, on the other hand hidden metadata which may affect further statements. I'm fine with hidding unknown options that were accepted due to IGNORE_BAD_TABLE_OPTIONS, but hidding things that were accepted by fully successful ALTER TABLE statement sounds confusing. Regards, Sergey On Sat, Jul 05, 2014 at 11:42:25PM +0200, Sergei Golubchik wrote:
At lp:~maria-captains/maria/10.0
------------------------------------------------------------ revno: 4275 revision-id: sergii@pisem.net-20140705214225-p0k45asdaq27xocr parent: sergii@pisem.net-20140705214216-npqtc7tz854xqjxo fixes bug: https://mariadb.atlassian.net/browse/MDEV-5867 committer: Sergei Golubchik <sergii@pisem.net> branch nick: 10.0 timestamp: Sat 2014-07-05 23:42:25 +0200 message: MDEV-5867 ALTER TABLE t1 ENGINE=InnoDB keeps bad options when t1 ENGINE is CONNECT
don't print unknown options in SHOW CREATE TABLE unless IGNORE_BAD_TABLE_OPTIONS is used === added file 'mysql-test/r/table_options-5867.result' --- a/mysql-test/r/table_options-5867.result 1970-01-01 00:00:00 +0000 +++ b/mysql-test/r/table_options-5867.result 2014-07-05 21:42:25 +0000 @@ -0,0 +1,37 @@ +install soname 'ha_example'; +set sql_mode='ignore_bad_table_options'; +create table t1 ( +a int complex='c,f,f,f' invalid=3 +) engine=example ull=10000 str='dskj' one_or_two='one' yesno=0 +foobar=barfoo; +Warnings: +Warning 1911 Unknown option 'invalid' +Warning 1911 Unknown option 'foobar' +create table t2 (a int, key (a) some_option=2014); +Warnings: +Warning 1911 Unknown option 'some_option' +show create table t1; +Table Create Table +t1 CREATE TABLE `t1` ( + `a` int(11) DEFAULT NULL `complex`='c,f,f,f' `invalid`=3 +) ENGINE=EXAMPLE DEFAULT CHARSET=latin1 `ull`=10000 `str`='dskj' `one_or_two`='one' `yesno`=0 `foobar`=barfoo `VAROPT`='5' +show create table t2; +Table Create Table +t2 CREATE TABLE `t2` ( + `a` int(11) DEFAULT NULL, + KEY `a` (`a`) `some_option`=2014 +) ENGINE=MyISAM DEFAULT CHARSET=latin1 +set sql_mode=''; +show create table t1; +Table Create Table +t1 CREATE TABLE `t1` ( + `a` int(11) DEFAULT NULL `complex`='c,f,f,f' +) ENGINE=EXAMPLE DEFAULT CHARSET=latin1 `ull`=10000 `str`='dskj' `one_or_two`='one' `yesno`=0 `VAROPT`='5' +show create table t2; +Table Create Table +t2 CREATE TABLE `t2` ( + `a` int(11) DEFAULT NULL, + KEY `a` (`a`) +) ENGINE=MyISAM DEFAULT CHARSET=latin1 +drop table t1, t2; +uninstall soname 'ha_example';
=== modified file 'mysql-test/suite/rpl/r/rpl_table_options.result' --- a/mysql-test/suite/rpl/r/rpl_table_options.result 2013-09-14 01:09:36 +0000 +++ b/mysql-test/suite/rpl/r/rpl_table_options.result 2014-07-05 21:42:25 +0000 @@ -12,6 +12,12 @@ show create table t1; Table Create Table t1 CREATE TABLE `t1` ( `a` int(11) NOT NULL +) ENGINE=MyISAM DEFAULT CHARSET=latin1 +set sql_mode=ignore_bad_table_options; +show create table t1; +Table Create Table +t1 CREATE TABLE `t1` ( + `a` int(11) NOT NULL ) ENGINE=MyISAM DEFAULT CHARSET=latin1 `ull`=12340 drop table t1; set storage_engine=default;
=== modified file 'mysql-test/suite/rpl/t/rpl_table_options.test' --- a/mysql-test/suite/rpl/t/rpl_table_options.test 2011-10-19 19:45:18 +0000 +++ b/mysql-test/suite/rpl/t/rpl_table_options.test 2014-07-05 21:42:25 +0000 @@ -23,6 +23,8 @@ show create table t1; sync_slave_with_master; connection slave; show create table t1; +set sql_mode=ignore_bad_table_options; +show create table t1;
connection master; drop table t1;
=== added file 'mysql-test/t/table_options-5867.test' --- a/mysql-test/t/table_options-5867.test 1970-01-01 00:00:00 +0000 +++ b/mysql-test/t/table_options-5867.test 2014-07-05 21:42:25 +0000 @@ -0,0 +1,30 @@ +# +# MDEV-5867 ALTER TABLE t1 ENGINE=InnoDB keeps bad options when t1 ENGINE is CONNECT +# +# verify that SHOW CREATE TABLE hides unknown options when IGNORE_BAD_TABLE_OPTIONS is not set + +--source include/have_example_plugin.inc +--source include/not_embedded.inc + +install soname 'ha_example'; + +set sql_mode='ignore_bad_table_options'; +create table t1 ( + a int complex='c,f,f,f' invalid=3 +) engine=example ull=10000 str='dskj' one_or_two='one' yesno=0 + foobar=barfoo; + +create table t2 (a int, key (a) some_option=2014); + +show create table t1; +show create table t2; + +set sql_mode=''; + +show create table t1; +show create table t2; + +drop table t1, t2; + +uninstall soname 'ha_example'; +
=== modified file 'sql/create_options.cc' --- a/sql/create_options.cc 2014-03-26 08:33:03 +0000 +++ b/sql/create_options.cc 2014-07-05 21:42:25 +0000 @@ -775,3 +775,20 @@ engine_option_value *merge_engine_table_ &first, &end); DBUG_RETURN(first); } + +bool is_engine_option_known(engine_option_value *opt, + ha_create_table_option *rules) +{ + if (!rules) + return false; + + for (; rules->name; rules++) + { + if (!my_strnncoll(system_charset_info, + (uchar*)rules->name, rules->name_length, + (uchar*)opt->name.str, opt->name.length)) + return true; + } + return false; +} +
=== modified file 'sql/create_options.h' --- a/sql/create_options.h 2013-11-20 11:05:39 +0000 +++ b/sql/create_options.h 2014-07-05 21:42:25 +0000 @@ -99,4 +99,6 @@ uchar *engine_table_options_frm_image(uc
bool engine_options_differ(void *old_struct, void *new_struct, ha_create_table_option *rules); +bool is_engine_option_known(engine_option_value *opt, + ha_create_table_option *rules); #endif
=== modified file 'sql/sql_show.cc' --- a/sql/sql_show.cc 2014-07-05 21:42:16 +0000 +++ b/sql/sql_show.cc 2014-07-05 21:42:25 +0000 @@ -1519,13 +1519,20 @@ static bool get_field_default_value(THD @param thd thread handler @param packet string to append @param opt list of options + @param check_options only print known options + @param rules list of known options */
static void append_create_options(THD *thd, String *packet, - engine_option_value *opt) + engine_option_value *opt, + bool check_options, + ha_create_table_option *rules) { for(; opt; opt= opt->next) { + if (check_options && !is_engine_option_known(opt, rules)) + continue; + DBUG_ASSERT(opt->value.str); packet->append(' '); append_identifier(thd, packet, opt->name.str, opt->name.length); @@ -1584,6 +1591,8 @@ int show_create_table(THD *thd, TABLE_LI MODE_MAXDB | MODE_ANSI); bool limited_mysql_mode= sql_mode & (MODE_NO_FIELD_OPTIONS | MODE_MYSQL323 | MODE_MYSQL40); + bool check_options= !(sql_mode & MODE_IGNORE_BAD_TABLE_OPTIONS) && + !create_info_arg; handlerton *hton; my_bitmap_map *old_map; int error= 0; @@ -1733,7 +1742,8 @@ int show_create_table(THD *thd, TABLE_LI packet->append(STRING_WITH_LEN(" COMMENT ")); append_unescaped(packet, field->comment.str, field->comment.length); } - append_create_options(thd, packet, field->option_list); + append_create_options(thd, packet, field->option_list, check_options, + hton->field_options); }
key_info= table->key_info; @@ -1800,7 +1810,8 @@ int show_create_table(THD *thd, TABLE_LI append_identifier(thd, packet, parser_name->str, parser_name->length); packet->append(STRING_WITH_LEN(" */ ")); } - append_create_options(thd, packet, key_info->option_list); + append_create_options(thd, packet, key_info->option_list, check_options, + hton->index_options); }
/* @@ -1953,7 +1964,8 @@ int show_create_table(THD *thd, TABLE_LI packet->append(STRING_WITH_LEN(" CONNECTION=")); append_unescaped(packet, share->connect_string.str, share->connect_string.length); } - append_create_options(thd, packet, share->option_list); + append_create_options(thd, packet, share->option_list, check_options, + hton->table_options); append_directory(thd, packet, "DATA", create_info.data_file_name); append_directory(thd, packet, "INDEX", create_info.index_file_name); } @@ -5130,7 +5142,7 @@ static int get_schema_tables_record(THD str.qs_append(STRING_WITH_LEN(" transactional=")); str.qs_append(ha_choice_values[(uint) share->transactional]); } - append_create_options(thd, &str, share->option_list); + append_create_options(thd, &str, share->option_list, false, 0);
if (str.length()) table->field[19]->store(str.ptr()+1, str.length()-1, cs);
=== modified file 'storage/connect/mysql-test/connect/r/alter.result' --- a/storage/connect/mysql-test/connect/r/alter.result 2014-03-23 14:50:39 +0000 +++ b/storage/connect/mysql-test/connect/r/alter.result 2014-07-05 21:42:25 +0000 @@ -218,13 +218,22 @@ Three 3 # Changing to another engine is Ok # However, the data file is not deleted. # -ALTER TABLE t1 ENGINE=MARIA; +ALTER TABLE t1 ENGINE=ARIA; +SHOW CREATE TABLE t1; +Table Create Table +t1 CREATE TABLE `t1` ( + `d` char(10) NOT NULL, + `c` int(11) NOT NULL +) ENGINE=Aria DEFAULT CHARSET=latin1 PAGE_CHECKSUM=1 +set @old_sql_mode=@@sql_mode; +set sql_mode=ignore_bad_table_options; SHOW CREATE TABLE t1; Table Create Table t1 CREATE TABLE `t1` ( `d` char(10) NOT NULL `FLAG`=11, `c` int(11) NOT NULL `FLAG`=0 ) ENGINE=Aria DEFAULT CHARSET=latin1 PAGE_CHECKSUM=1 `TABLE_TYPE`=fix `FILE_NAME`='tf1.txt' `ENDING`=1 +set sql_mode=@old_sql_mode; SELECT * from t1; d c One 1
=== modified file 'storage/connect/mysql-test/connect/t/alter.test' --- a/storage/connect/mysql-test/connect/t/alter.test 2014-03-23 14:50:39 +0000 +++ b/storage/connect/mysql-test/connect/t/alter.test 2014-07-05 21:42:25 +0000 @@ -105,8 +105,12 @@ SELECT * FROM t1; --echo # Changing to another engine is Ok --echo # However, the data file is not deleted. --echo # -ALTER TABLE t1 ENGINE=MARIA; +ALTER TABLE t1 ENGINE=ARIA; SHOW CREATE TABLE t1; +set @old_sql_mode=@@sql_mode; +set sql_mode=ignore_bad_table_options; +SHOW CREATE TABLE t1; +set sql_mode=@old_sql_mode; SELECT * from t1; SELECT * from t2;
_______________________________________________ commits mailing list commits@mariadb.org https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits
Hi, Sergey! On Jul 08, Sergey Vojtovich wrote:
Hi Sergei,
I believe options handling is way too confusing in general: - CREATE TABLE warns/errors about unknown options - ALTER TABLE ... ENGINE doesn't - ALTER TABLE ... UNKNOWN_OPTION=1 does - CREATE TABLE ... LIKE doesn't
This is consistent. You get a warning or an error about options that you explicitly specify, but not about options that are implicitly ihnerited.
- different engines may assign different meaning to the same option - IGNORE_BAD_TABLE_OPTIONS doesn't actually _ignore_ _bad_ options but rather _accepts_ _unkown_ options
Yes, I know :( That's why I tried to say in the manual that SHOW CREATE TABLE *filters out* invalid options, and IGNORE_BAD_TABLE_OPTIONS disables this filtering, they are ignored and not filtered out.
But the question is if we should show unknown options by default. I have no opinion: both cases are equally wrong to me.
On the one hand we get unusable CREATE TABLE statement, on the other hand hidden metadata which may affect further statements.
I'm fine with hidding unknown options that were accepted due to IGNORE_BAD_TABLE_OPTIONS, but hidding things that were accepted by fully successful ALTER TABLE statement sounds confusing.
Do you have a better idea? My logic was - SHOW CREATE TABLE should generate a valid CREATE TABLE statement. Regards, Sergei
Hi Sergei, frankly speaking I have no better idea. Let's give your patch a try. Regards, Sergey On Tue, Jul 08, 2014 at 10:43:24AM +0200, Sergei Golubchik wrote:
Hi, Sergey!
On Jul 08, Sergey Vojtovich wrote:
Hi Sergei,
I believe options handling is way too confusing in general: - CREATE TABLE warns/errors about unknown options - ALTER TABLE ... ENGINE doesn't - ALTER TABLE ... UNKNOWN_OPTION=1 does - CREATE TABLE ... LIKE doesn't
This is consistent. You get a warning or an error about options that you explicitly specify, but not about options that are implicitly ihnerited.
- different engines may assign different meaning to the same option - IGNORE_BAD_TABLE_OPTIONS doesn't actually _ignore_ _bad_ options but rather _accepts_ _unkown_ options
Yes, I know :(
That's why I tried to say in the manual that SHOW CREATE TABLE *filters out* invalid options, and IGNORE_BAD_TABLE_OPTIONS disables this filtering, they are ignored and not filtered out.
But the question is if we should show unknown options by default. I have no opinion: both cases are equally wrong to me.
On the one hand we get unusable CREATE TABLE statement, on the other hand hidden metadata which may affect further statements.
I'm fine with hidding unknown options that were accepted due to IGNORE_BAD_TABLE_OPTIONS, but hidding things that were accepted by fully successful ALTER TABLE statement sounds confusing.
Do you have a better idea?
My logic was - SHOW CREATE TABLE should generate a valid CREATE TABLE statement.
Regards, Sergei
Hi, Why not displaying the ignored options as comments ? Jocelyn Le 08/07/2014 11:34, Sergey Vojtovich a écrit :
Hi Sergei,
frankly speaking I have no better idea. Let's give your patch a try.
Regards, Sergey
On Tue, Jul 08, 2014 at 10:43:24AM +0200, Sergei Golubchik wrote:
Hi, Sergey!
On Jul 08, Sergey Vojtovich wrote:
Hi Sergei,
I believe options handling is way too confusing in general: - CREATE TABLE warns/errors about unknown options - ALTER TABLE ... ENGINE doesn't - ALTER TABLE ... UNKNOWN_OPTION=1 does - CREATE TABLE ... LIKE doesn't
This is consistent. You get a warning or an error about options that you explicitly specify, but not about options that are implicitly ihnerited.
- different engines may assign different meaning to the same option - IGNORE_BAD_TABLE_OPTIONS doesn't actually _ignore_ _bad_ options but rather _accepts_ _unkown_ options
Yes, I know :(
That's why I tried to say in the manual that SHOW CREATE TABLE *filters out* invalid options, and IGNORE_BAD_TABLE_OPTIONS disables this filtering, they are ignored and not filtered out.
But the question is if we should show unknown options by default. I have no opinion: both cases are equally wrong to me.
On the one hand we get unusable CREATE TABLE statement, on the other hand hidden metadata which may affect further statements.
I'm fine with hidding unknown options that were accepted due to IGNORE_BAD_TABLE_OPTIONS, but hidding things that were accepted by fully successful ALTER TABLE statement sounds confusing.
Do you have a better idea?
My logic was - SHOW CREATE TABLE should generate a valid CREATE TABLE statement.
Regards, Sergei
_______________________________________________ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Hi, Jocelyn! On Jul 08, Jocelyn Fournier wrote:
Hi,
Why not displaying the ignored options as comments ?
Jocelyn
Good question. This is easy to do, I've just tried. It might look confusing, though, the table from connect.alter test would then be: SHOW CREATE TABLE t1; Table Create Table t1 CREATE TABLE `t1` ( `d` char(10) NOT NULL /* `FLAG`=11 */, `c` int(11) NOT NULL /* `FLAG`=0 */ ) ENGINE=Aria DEFAULT CHARSET=latin1 PAGE_CHECKSUM=1 /* `TABLE_TYPE`=fix `FILE_NAME`='tf1.txt' `ENDING`=1 */ without comments it'd be a pure Aria table of SHOW CREATE TABLE t1; Table Create Table t1 CREATE TABLE `t1` ( `d` char(10) NOT NULL, `c` int(11) NOT NULL ) ENGINE=Aria DEFAULT CHARSET=latin1 PAGE_CHECKSUM=1 I cannot really decide which one is better :) So, let's others voice their opinions. I can push this change any minute. Regards, Sergei
Hi Sergei, Le 08/07/2014 12:25, Sergei Golubchik a écrit :
Hi, Jocelyn!
On Jul 08, Jocelyn Fournier wrote:
Hi,
Why not displaying the ignored options as comments ?
Jocelyn
Good question. This is easy to do, I've just tried.
It might look confusing, though, the table from connect.alter test would then be:
SHOW CREATE TABLE t1; Table Create Table t1 CREATE TABLE `t1` ( `d` char(10) NOT NULL /* `FLAG`=11 */, `c` int(11) NOT NULL /* `FLAG`=0 */ ) ENGINE=Aria DEFAULT CHARSET=latin1 PAGE_CHECKSUM=1 /* `TABLE_TYPE`=fix `FILE_NAME`='tf1.txt' `ENDING`=1 */
Perhaps something like : SHOW CREATE TABLE t1; Table Create Table t1 CREATE TABLE `t1` ( `d` char(10) NOT NULL /* `FLAG`=11 */, `c` int(11) NOT NULL /* `FLAG`=0 */ ) ENGINE=Aria DEFAULT CHARSET=latin1 PAGE_CHECKSUM=1 /* Ignored Options : `TABLE_TYPE`=fix `FILE_NAME`='tf1.txt' `ENDING`=1 */ would be clearer ? Jocelyn
I like Jocelyn's solution. The whole situation is quite messy with all sorts of edge cases, but this seems to make the best of it. THE SHOW CREATE statement will be functional, and the metadata will be displayed in a readable form. On 08/07/2014 14:21, Jocelyn Fournier wrote:
This is easy to do, I've just tried.
It might look confusing, though, the table from connect.alter test would then be:
SHOW CREATE TABLE t1; Table Create Table t1 CREATE TABLE `t1` ( `d` char(10) NOT NULL /* `FLAG`=11 */, `c` int(11) NOT NULL /* `FLAG`=0 */ ) ENGINE=Aria DEFAULT CHARSET=latin1 PAGE_CHECKSUM=1 /* `TABLE_TYPE`=fix `FILE_NAME`='tf1.txt' `ENDING`=1 */
Perhaps something like :
SHOW CREATE TABLE t1; Table Create Table t1 CREATE TABLE `t1` ( `d` char(10) NOT NULL /* `FLAG`=11 */, `c` int(11) NOT NULL /* `FLAG`=0 */ ) ENGINE=Aria DEFAULT CHARSET=latin1 PAGE_CHECKSUM=1 /* Ignored Options : `TABLE_TYPE`=fix `FILE_NAME`='tf1.txt' `ENDING`=1 */
would be clearer ?
Jocelyn
Hi, Ian! On Jul 08, Ian Gilfillan wrote:
I like Jocelyn's solution. The whole situation is quite messy with all sorts of edge cases, but this seems to make the best of it. THE SHOW CREATE statement will be functional, and the metadata will be displayed in a readable form.
Okay, then. I'll put ignored options in a comment. Regards, Sergei
participants (4)
-
Ian Gilfillan -
Jocelyn Fournier -
Sergei Golubchik -
Sergey Vojtovich