maria-developers team mailing list archive

[Vicențiu Ciorbaru <vicentiu@xxxxxxxxxxx>]

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@xxxxxxxxxxx>
> 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