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 <[email protected]> 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 <[email protected]> > > Date: Thu Jan 24 03:06:56 2019 -0800 > > > > diff --git a/mysql-test/suite/funcs_1/t/is_check_constraint.test > b/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 > 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) > 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 > the else statement here. The DBUG_RETURN from above covers it. > > { > > - 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 > 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 > >
_______________________________________________ Mailing list: https://launchpad.net/~maria-developers Post to : [email protected] Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
