maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Alexey!

Thanks! See below

On Jul 31, Alexey Botchkov wrote:
> revision-id: f89d1f55576136fb229392cb9a2997b9c0131241 (mariadb-10.2.1-9-gf89d1f5)
> parent(s): 08683a726773f8cdf16a4a3dfb3920e5f7842481
> committer: Alexey Botchkov
> timestamp: 2016-07-31 14:04:52 +0400
> message:
> 
> MDEV-10421 duplicate CHECK CONSTRAINTs.
> 
>         mysql_prepare_create_table fixed so it doesn't let duplicating
>         constraint names. Syntax for CONSTRAINT IF NOT EXISTS added
>         and handled in mysql_alter_table.
> 
> diff --git a/mysql-test/r/alter_table.result b/mysql-test/r/alter_table.result
> index 3461038..5c1da48 100644
> --- a/mysql-test/r/alter_table.result
> +++ b/mysql-test/r/alter_table.result
> @@ -2057,3 +2057,23 @@ t1	CREATE TABLE `t1` (
>    KEY `i1` (`a`) COMMENT 'comment2'
>  ) ENGINE=MyISAM DEFAULT CHARSET=latin1
>  DROP TABLE t1;
> +#
> +# MDEV-10421 duplicate CHECK CONSTRAINTs
> +#
> +CREATE TABLE t1 (a INT, b INT) engine=myisam;
> +ALTER TABLE t1 ADD CONSTRAINT IF NOT EXISTS `min` CHECK (a+b > 100);
> +ALTER TABLE t1 ADD CONSTRAINT `min` CHECK (a+b > 100);
> +ERROR HY000: Duplicate constraint name 'min'
> +ALTER TABLE t1 ADD CONSTRAINT IF NOT EXISTS `min` CHECK (a+b > 100);
> +Warnings:
> +Note	4027	Duplicate constraint name 'min'
> +ALTER TABLE t1 ADD CONSTRAINT `mini` CHECK (a+b > 100);

please also add a test like

  create table (a int, b int, constraint min check (a>5),
                              constraint min check (b>5));

> +SHOW CREATE TABLE t1;
> +Table	Create Table
> +t1	CREATE TABLE `t1` (
> +  `a` int(11) DEFAULT NULL,
> +  `b` int(11) DEFAULT NULL,
> +  CONSTRAINT `min` CHECK (a+b > 100),
> +  CONSTRAINT `mini` CHECK (a+b > 100)
> +) ENGINE=MyISAM DEFAULT CHARSET=latin1
> +DROP TABLE t1;
> diff --git a/sql/field.h b/sql/field.h
> index 13a0e91..7b271b0 100644
> --- a/sql/field.h
> +++ b/sql/field.h
> @@ -606,6 +606,7 @@ class Virtual_column_info: public Sql_alloc
>    /* Flag indicating  that the field is physically stored in the database */
>    bool stored_in_db;
>    bool utf8;                                    /* Already in utf8 */
> +  bool create_if_not_exists;

Hmm, I don't think it's a good idea. Virtual_column_info is used
for storing run-time information about generated columns, defaults, and
check constraints. It's kind of wrong to put the a parser-time
check-constraint-only flag.

>    /* The expression to compute the value of the virtual column */
>    Item *expr_item;
>    /* Text representation of the defining expression */
> diff --git a/sql/share/errmsg-utf8.txt b/sql/share/errmsg-utf8.txt
> index 8dfa519..261021b 100644
> --- a/sql/share/errmsg-utf8.txt
> +++ b/sql/share/errmsg-utf8.txt
> @@ -7214,3 +7214,5 @@ ER_CALCULATING_DEFAULT_VALUE
>          eng "Got an error when calculating default value for %`s"
>  ER_EXPRESSION_REFERS_TO_UNINIT_FIELD 01000
>          eng "Expression for field %`-.64s is refering to uninitialized field %`s"
> +ER_DUP_CONSTRAINT_NAME
> +        eng "Duplicate constraint name '%-.192s'"

Say "Duplicate CHECK constraint" here, because there is already
ER_FK_DUP_NAME with the text "Duplicate foreign key constraint name"

Alternatively, (better, I think) generalize ER_FK_DUP_NAME to say

  Duplicate %s constraint name: %`s

(note correct quoting %` and no need to limit the length, it's an
identifier anyway, it cannot be arbitrarily long).

Then it can be used like

  my_error(ER_DUP_CONSTRAINT_NAME, MYF(0), "FOREIGN KEY", name);
  my_error(ER_DUP_CONSTRAINT_NAME, MYF(0), "CHECK", name);

That also means renaming the error from ER_FK_DUP_NAME to
ER_DUP_CONSTRAINT_NAME, and putting somewhere a compatibility define

#define ER_FK_DUP_NAME ER_DUP_CONSTRAINT_NAME

> diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy
> index 9d7e735..71f3692 100644
> --- a/sql/sql_yacc.yy
> +++ b/sql/sql_yacc.yy
> @@ -6070,7 +6070,11 @@ key_def:
>  constraint_def:
>           opt_constraint check_constraint
>           {
> -           Lex->add_constraint(&$1, $2);
> +           Lex->add_constraint(&$1, $2, FALSE);
> +         }
> +        | CONSTRAINT IF_SYM not EXISTS field_ident check_constraint
> +         {
> +           Lex->add_constraint(&$5, $6, TRUE);

Eh, not here please. IF NOT EXIST should not be recognized inside CREATE
TABLE :)

>           }
>         ;
>  
> _______________________________________________
> commits mailing list
> commits@xxxxxxxxxxx
> https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits
Regards,
Sergei
Chief Architect MariaDB
and security@xxxxxxxxxxx