[Maria-developers] Review for follow up fix of MDEV-17323: Backport INFORMATION_SCHEMA.CHECK_CONSTRAINTS to 10.2
Hi Anel! This is not a terribly bad patch, however you have forgotten the following things: You removed the misspelled is_check_constraint.test file, but forgot to remove the is_check_constraint.result file. Now we would be left with a dangling result file. Please be more careful next time. You modified the logic of is_check_constraints.test file from the one in 10.3 I suggest you use git cherry-pick next time when backporting a patch to get the correct changes. This also helps with merges later, as a change in 10.2 will be easily reconcileable in 10.3. Moving onwards with the patch, you will find other comments inline.
commit c94f53a34e0b6bf313ecb36a13d5d138150a8a8d Author: Anel Husakovic <anel@mariadb.org> Date: Thu Jan 24 03:06:56 2019 -0800
diff --git a/mysql-test/suite/funcs_1/t/is_check_constraint.test
deleted file mode 100644 index 30a72d02b34..00000000000 --- a/mysql-test/suite/funcs_1/t/is_check_constraint.test +++ /dev/null @@ -1,92 +0,0 @@ ---source include/have_innodb.inc ---source include/not_embedded.inc ---echo # ---echo # MDEV-17323: Backport INFORMATION_SCHEMA.CHECK_CONSTRAINTS to 10.2 ---echo # -CREATE user boo1; -GRANT select,create,alter,drop on foo.* to boo1; -SHOW GRANTS for boo1; -CREATE user boo2; -create database foo; -# Connect with user boo1 -CONNECT(con1,localhost, boo1,, foo); - -SET check_constraint_checks=1; -CREATE TABLE t0 -( - t int, check (t>32) # table constraint -) ENGINE=myisam; ---sorted_result -SELECT * from information_schema.check_constraints; - -ALTER TABLE t0 -ADD CONSTRAINT CHK_t0_t CHECK(t<100); ---sorted_result -SELECT * from information_schema.check_constraints; - -ALTER TABLE t0 -DROP CONSTRAINT CHK_t0_t; ---sorted_result -SELECT * from information_schema.check_constraints; - -ALTER TABLE t0 -ADD CONSTRAINT CHECK(t<50); ---sorted_result -SELECT * from information_schema.check_constraints; - -CREATE TABLE t1 -( t int CHECK(t>2), # field constraint - tt int, - CONSTRAINT CHECK (tt > 32), CONSTRAINT CHECK (tt <50),# autogenerated names table constraints - CONSTRAINT CHK_tt CHECK(tt<100) # named table constraint -) ENGINE=InnoDB; - --sorted_result -SELECT * from information_schema.check_constraints; - -ALTER TABLE t1 -DROP CONSTRAINT CHK_tt; ---sorted_result -SELECT * from information_schema.check_constraints; - -CREATE TABLE t2 -( -name VARCHAR(30) CHECK(CHAR_LENGTH(name)>2), #field constraint -start_date DATE, -end_date DATE, -CONSTRAINT CHK_dates CHECK(start_date IS NULL) #table constraint -)ENGINE=Innodb; - --sorted_result -SELECT * from information_schema.check_constraints; - -ALTER TABLE t1 -ADD CONSTRAINT CHK_new_ CHECK(t>tt); ---sorted_result -SELECT * from information_schema.check_constraints; - -# Create table with same field and table check constraint name -CREATE TABLE t3 -( -a int, -b int check (b>0), # field constraint named 'b' -CONSTRAINT b check (b>10) # table constraint -) ENGINE=InnoDB; - --sorted_result -SELECT * from information_schema.check_constraints; - -DISCONNECT con1; -CONNECT(con2, localhost, boo2,, test); - --sorted_result -SELECT * from information_schema.check_constraints; - -DISCONNECT con2; -CONNECT(con1, localhost, boo1,,foo); -DROP TABLE t0; -DROP TABLE t1; -DROP TABLE t2; -DROP TABLE t3; -DROP DATABASE foo; - -DISCONNECT con1; ---CONNECTION default -DROP USER boo1; -DROP USER boo2; This updated version of a test covers less functionality than the previous version, I don't like that. Please keep the old version as is, just rename it
b/mysql-test/suite/funcs_1/t/is_check_constraint.test properly. Serg has already added the include to skip embedded tests in the most up-to-date 10.2. Please rebase on top of that version before you perform the rename.
diff --git a/sql/sql_show.cc b/sql/sql_show.cc index 39763451dfb..e951c69d009 100644 --- a/sql/sql_show.cc +++ b/sql/sql_show.cc @@ -6474,33 +6474,35 @@ static int get_check_constraints_record(THD
LEX_STRING *table_name) { DBUG_ENTER("get_check_constraints_record"); - if(res) + if (res) Nice catch with the missing whitespace. { if (thd->is_error()) push_warning(thd, Sql_condition::WARN_LEVEL_WARN, thd->get_stmt_da()->sql_errno(), thd->get_stmt_da()->message()); - thd->clear_error(); - DBUG_RETURN(0); + thd->clear_error(); + DBUG_RETURN(0); Great change, making sure the code is indented appropriately. } - if(!tables->view) + else if (!tables->view) Again, nice catch with the missing whitespace, but there is no need for
{ - StringBuffer<MAX_FIELD_WIDTH> str(system_charset_info); - for (uint i= 0; i < tables->table->s->table_check_constraints; i++) + if (tables->table->s->table_check_constraints) There is no need for the if statement here. The for loop would already catch
*thd, TABLE_LIST *tables, the else statement here. The DBUG_RETURN from above covers it. the condition and not execute any bit. There is also a subtle difference with initialising the StringBuffer object only once, instead of once every loop. "Smart" compilers might be good enough to realise that the object should be reused, but I don't think that is the general case. Lets make the compiler's job easier and do things properly. Keep the StringBuffer initialisation before the for loop and just reset it's length. If you think that str.length(0) is confusing (I agree that the method name reflects a getter, not a setter), put a comment after it saying something like: "Clear the string." Somehow I vaguely remember we discussed this before, but somehow the wrong version ended up in 10.3. We will change that too in a subsequent patch. Also, ouch, I missed during the review that you switched the columns around compared to 10.3. The current version in 10.2 seems like the correct one for me. Which one is the correct version? I remember we discussed this but I can't find the logs. Furthermore, when we do such changes, please explain *everything you modify* in the commit message, don't make me hunt for differences.
{ - Virtual_column_info *check= tables->table->check_constraints[i]; - table->field[0]->store(STRING_WITH_LEN("def"),
system_charset_info);
- table->field[3]->store(check->name.str, check->name.length, - system_charset_info); - str.length(0); - check->print(&str); - table->field[4]->store(str.ptr(), str.length(), system_charset_info); - if (schema_table_store_record(thd, table)) - DBUG_RETURN(1); + for (uint i= 0; i < tables->table->s->table_check_constraints; i++) + { + StringBuffer<MAX_FIELD_WIDTH> str(system_charset_info); + Virtual_column_info *check= tables->table->check_constraints[i]; + restore_record(table, s->default_values); + table->field[0]->store(STRING_WITH_LEN("def"), system_charset_info); + table->field[1]->store(db_name->str, db_name->length, system_charset_info); + table->field[2]->store(check->name.str, check->name.length, system_charset_info); + table->field[3]->store(table_name->str, table_name->length, system_charset_info); + check->print(&str); + table->field[4]->store(str.ptr(), str.length(), system_charset_info); + schema_table_store_record(thd, table); + } } } - - DBUG_RETURN(0); + DBUG_RETURN(res); }
static int get_schema_constraints_record(THD *thd, TABLE_LIST *tables, @@ -9342,8 +9346,8 @@ ST_SCHEMA_TABLE schema_tables[]= fill_schema_applicable_roles, 0, 0, -1, -1, 0, 0}, {"CHARACTER_SETS", charsets_fields_info, 0, fill_schema_charsets, make_character_sets_old_format, 0, -1, -1, 0, 0}, - {"CHECK_CONSTRAINTS", check_constraints_fields_info, 0, get_all_tables, 0, - get_check_constraints_record, 1, 2, 0, OPTIMIZE_I_S_TABLE|OPEN_TABLE_ONLY}, + {"CHECK_CONSTRAINTS", check_constraints_fields_info, 0, + get_all_tables, 0, get_check_constraints_record, 1, 2, 0, OPTIMIZE_I_S_TABLE|OPEN_TABLE_ONLY}, I don't think this particular change was necessary. {"COLLATIONS", collation_fields_info, 0, fill_schema_collation, make_old_format, 0, -1, -1, 0, 0}, {"COLLATION_CHARACTER_SET_APPLICABILITY",
coll_charset_app_fields_info,
Vicențiu
Hi Vicentiu, the patch is updated and notes are in comment: https://github.com/MariaDB/server/pull/1127/ 1. Should I support embedded server in this case, since it is supported in `10.3`? What else could be done => According to the comment in this task https://jira.mariadb.org/browse/MDEV-14474?focusedCommentId=120838&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-120838 I investigated and found out that in `sql/field.cc` indeed in ``` int Field_varstring::store(const char *from,uint length,CHARSET_INFO *cs) { ASSERT_COLUMN_MARKED_FOR_WRITE_OR_COMPUTED; uint copy_length; String_copier copier; copy_length= copier.well_formed_copy(field_charset, (char*) ptr + length_bytes, field_length, cs, from, length, field_length / field_charset->mbmaxlen); ``` value ` field_length / field_charset->mbmaxlen` is set on max of 64 bytes used later for `well_formed_copy()`where the `field_length = 192` and `field_charset->mbmaxlen=3`. 2. Should this be limitation for `CHECK_CLAUSE` field in `Information_schema.check_constraints` table or that we change this also ? Thanks, Anel On Thu, Jan 31, 2019 at 12:05 PM Vicențiu Ciorbaru <vicentiu@mariadb.org> wrote:
Hi Anel! This is not a terribly bad patch, however you have forgotten the following things:
You removed the misspelled is_check_constraint.test file, but forgot to remove the is_check_constraint.result file. Now we would be left with a dangling result file. Please be more careful next time.
You modified the logic of is_check_constraints.test file from the one in 10.3
I suggest you use git cherry-pick next time when backporting a patch to get the correct changes. This also helps with merges later, as a change in 10.2 will be easily reconcileable in 10.3.
Moving onwards with the patch, you will find other comments inline.
commit c94f53a34e0b6bf313ecb36a13d5d138150a8a8d Author: Anel Husakovic <anel@mariadb.org> Date: Thu Jan 24 03:06:56 2019 -0800
diff --git a/mysql-test/suite/funcs_1/t/is_check_constraint.test
deleted file mode 100644 index 30a72d02b34..00000000000 --- a/mysql-test/suite/funcs_1/t/is_check_constraint.test +++ /dev/null @@ -1,92 +0,0 @@ ---source include/have_innodb.inc ---source include/not_embedded.inc ---echo # ---echo # MDEV-17323: Backport INFORMATION_SCHEMA.CHECK_CONSTRAINTS to 10.2 ---echo # -CREATE user boo1; -GRANT select,create,alter,drop on foo.* to boo1; -SHOW GRANTS for boo1; -CREATE user boo2; -create database foo; -# Connect with user boo1 -CONNECT(con1,localhost, boo1,, foo); - -SET check_constraint_checks=1; -CREATE TABLE t0 -( - t int, check (t>32) # table constraint -) ENGINE=myisam; ---sorted_result -SELECT * from information_schema.check_constraints; - -ALTER TABLE t0 -ADD CONSTRAINT CHK_t0_t CHECK(t<100); ---sorted_result -SELECT * from information_schema.check_constraints; - -ALTER TABLE t0 -DROP CONSTRAINT CHK_t0_t; ---sorted_result -SELECT * from information_schema.check_constraints; - -ALTER TABLE t0 -ADD CONSTRAINT CHECK(t<50); ---sorted_result -SELECT * from information_schema.check_constraints; - -CREATE TABLE t1 -( t int CHECK(t>2), # field constraint - tt int, - CONSTRAINT CHECK (tt > 32), CONSTRAINT CHECK (tt <50),# autogenerated names table constraints - CONSTRAINT CHK_tt CHECK(tt<100) # named table constraint -) ENGINE=InnoDB; - --sorted_result -SELECT * from information_schema.check_constraints; - -ALTER TABLE t1 -DROP CONSTRAINT CHK_tt; ---sorted_result -SELECT * from information_schema.check_constraints; - -CREATE TABLE t2 -( -name VARCHAR(30) CHECK(CHAR_LENGTH(name)>2), #field constraint -start_date DATE, -end_date DATE, -CONSTRAINT CHK_dates CHECK(start_date IS NULL) #table constraint -)ENGINE=Innodb; - --sorted_result -SELECT * from information_schema.check_constraints; - -ALTER TABLE t1 -ADD CONSTRAINT CHK_new_ CHECK(t>tt); ---sorted_result -SELECT * from information_schema.check_constraints; - -# Create table with same field and table check constraint name -CREATE TABLE t3 -( -a int, -b int check (b>0), # field constraint named 'b' -CONSTRAINT b check (b>10) # table constraint -) ENGINE=InnoDB; - --sorted_result -SELECT * from information_schema.check_constraints; - -DISCONNECT con1; -CONNECT(con2, localhost, boo2,, test); - --sorted_result -SELECT * from information_schema.check_constraints; - -DISCONNECT con2; -CONNECT(con1, localhost, boo1,,foo); -DROP TABLE t0; -DROP TABLE t1; -DROP TABLE t2; -DROP TABLE t3; -DROP DATABASE foo; - -DISCONNECT con1; ---CONNECTION default -DROP USER boo1; -DROP USER boo2; This updated version of a test covers less functionality than the previous version, I don't like that. Please keep the old version as is, just rename it
b/mysql-test/suite/funcs_1/t/is_check_constraint.test properly. Serg has already added the include to skip embedded tests in the most up-to-date 10.2. Please rebase on top of that version before you perform the rename.
diff --git a/sql/sql_show.cc b/sql/sql_show.cc index 39763451dfb..e951c69d009 100644 --- a/sql/sql_show.cc +++ b/sql/sql_show.cc @@ -6474,33 +6474,35 @@ static int get_check_constraints_record(THD
*thd, TABLE_LIST *tables,
LEX_STRING *table_name) { DBUG_ENTER("get_check_constraints_record"); - if(res) + if (res)
{ if (thd->is_error()) push_warning(thd, Sql_condition::WARN_LEVEL_WARN, thd->get_stmt_da()->sql_errno(), thd->get_stmt_da()->message()); - thd->clear_error(); - DBUG_RETURN(0); + thd->clear_error(); + DBUG_RETURN(0); Great change, making sure the code is indented appropriately. } - if(!tables->view) + else if (!tables->view) Again, nice catch with the missing whitespace, but there is no need for
{ - StringBuffer<MAX_FIELD_WIDTH> str(system_charset_info); - for (uint i= 0; i < tables->table->s->table_check_constraints; i++) + if (tables->table->s->table_check_constraints) There is no need for the if statement here. The for loop would already catch
Nice catch with the missing whitespace. the else statement here. The DBUG_RETURN from above covers it. the condition and not execute any bit. There is also a subtle difference with initialising the StringBuffer object only once, instead of once every loop. "Smart" compilers might be good enough to realise that the object should be reused, but I don't think that is the general case. Lets make the compiler's job easier and do things properly. Keep the StringBuffer initialisation before the for loop and just reset it's length. If you think that str.length(0) is confusing (I agree that the method name reflects a getter, not a setter), put a comment after it saying something like: "Clear the string."
Somehow I vaguely remember we discussed this before, but somehow the wrong version ended up in 10.3. We will change that too in a subsequent patch.
Also, ouch, I missed during the review that you switched the columns around compared to 10.3. The current version in 10.2 seems like the correct one for me. Which one is the correct version? I remember we discussed this but I can't find the logs.
Furthermore, when we do such changes, please explain *everything you modify* in the commit message, don't make me hunt for differences.
{ - Virtual_column_info *check= tables->table->check_constraints[i]; - table->field[0]->store(STRING_WITH_LEN("def"),
- table->field[3]->store(check->name.str, check->name.length, - system_charset_info); - str.length(0); - check->print(&str); - table->field[4]->store(str.ptr(), str.length(), system_charset_info); - if (schema_table_store_record(thd, table)) - DBUG_RETURN(1); + for (uint i= 0; i < tables->table->s->table_check_constraints; i++) + { + StringBuffer<MAX_FIELD_WIDTH> str(system_charset_info); + Virtual_column_info *check=
system_charset_info); tables->table->check_constraints[i];
+ restore_record(table, s->default_values); + table->field[0]->store(STRING_WITH_LEN("def"), system_charset_info); + table->field[1]->store(db_name->str, db_name->length, system_charset_info); + table->field[2]->store(check->name.str, check->name.length, system_charset_info); + table->field[3]->store(table_name->str, table_name->length, system_charset_info); + check->print(&str); + table->field[4]->store(str.ptr(), str.length(), system_charset_info); + schema_table_store_record(thd, table); + } } } - - DBUG_RETURN(0); + DBUG_RETURN(res); }
static int get_schema_constraints_record(THD *thd, TABLE_LIST *tables, @@ -9342,8 +9346,8 @@ ST_SCHEMA_TABLE schema_tables[]= fill_schema_applicable_roles, 0, 0, -1, -1, 0, 0}, {"CHARACTER_SETS", charsets_fields_info, 0, fill_schema_charsets, make_character_sets_old_format, 0, -1, -1, 0, 0}, - {"CHECK_CONSTRAINTS", check_constraints_fields_info, 0, get_all_tables, 0, - get_check_constraints_record, 1, 2, 0, OPTIMIZE_I_S_TABLE|OPEN_TABLE_ONLY}, + {"CHECK_CONSTRAINTS", check_constraints_fields_info, 0, + get_all_tables, 0, get_check_constraints_record, 1, 2, 0, OPTIMIZE_I_S_TABLE|OPEN_TABLE_ONLY}, I don't think this particular change was necessary. {"COLLATIONS", collation_fields_info, 0, fill_schema_collation, make_old_format, 0, -1, -1, 0, 0}, {"COLLATION_CHARACTER_SET_APPLICABILITY",
coll_charset_app_fields_info,
Vicențiu
participants (2)
-
Anel Husakovic -
Vicențiu Ciorbaru