maria-developers team mailing list archive

[Anel Husakovic <anelhusakovic88@xxxxxxxxx>]

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