maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Jacob!

On Apr 03, jacob.mathew@xxxxxxxxxxx wrote:
> revision-id: b7aa15458db95149af4f473a90009844a16e9db8 (mariadb-10.2.4-103-gb7aa15458db)
> parent(s): 3c422e60bbee79bb636e65910c26ac193de70a84
> author: Jacob Mathew
> committer: Jacob Mathew
> timestamp: 2017-04-03 17:54:32 -0700
> message:
> 
> MDEV-11117 CHECK constraint fails on intermediate step of ALTER
> 
> Fixed the bug by failing the statement with an error message that explains
> that an auto-increment column may not be used in an expression for a
> check constraint, a default value or a generated column.
> Added a test case.
> 
> new file mode 100644
> index 00000000000..e192fd16677
> --- /dev/null
> +++ b/mysql-test/t/check_constraint_autoinc.test
> @@ -0,0 +1,7 @@
> +#
> +# An auto-increment column may not be used in an expression for
> +# a check constraint: MDEV-11117
> +#
> +--echo # AUTO_INCREMENT IN VCOL
> +-- error ER_AUTOINC_IN_VIRTUAL_COLUMN_FUNCTION
> +create or replace table t1( c1 int auto_increment primary key, check( c1 > 0 or c1 is null ) );

you could've added this 2-line test to the existing file
check_constraint.test

No strong arguments either way, though.

Pro: mysqltest is executed for every test file, it's faster to have less
     test files. Don't know if the difference is big, though.
Contra: it's easier to see what has failed with one test per file.
     It doesn't matter now, but might matter in the future, when we'll
     have more automatic failure analysys.

> diff --git a/sql/field.h b/sql/field.h
> index 50dcb397616..4bd43551648 100644
> --- a/sql/field.h
> +++ b/sql/field.h
> @@ -591,7 +591,8 @@ static inline const char *vcol_type_name(enum_vcol_info_type type)
>  #define VCOL_NON_DETERMINISTIC 2
>  #define VCOL_SESSION_FUNC      4  /* uses session data, e.g. USER or DAYNAME */
>  #define VCOL_TIME_FUNC         8
> -#define VCOL_IMPOSSIBLE       16
> +#define VCOL_AUTO_INC         16
> +#define VCOL_IMPOSSIBLE       32
>  
>  #define VCOL_NOT_STRICTLY_DETERMINISTIC                       \
>    (VCOL_NON_DETERMINISTIC | VCOL_TIME_FUNC | VCOL_SESSION_FUNC)
> diff --git a/sql/share/errmsg-utf8.txt b/sql/share/errmsg-utf8.txt
> index 930cd2ceaa2..a78e97c2577 100644
> --- a/sql/share/errmsg-utf8.txt
> +++ b/sql/share/errmsg-utf8.txt
> @@ -7142,6 +7142,8 @@ ER_NO_EIS_FOR_FIELD
>  ER_WARN_AGGFUNC_DEPENDENCE
>          eng "Aggregate function '%-.192s)' of SELECT #%d belongs to SELECT #%d"
>          ukr "Агрегатна функція '%-.192s)' з SELECTу #%d належить до SELECTу #%d"
> +ER_AUTOINC_IN_VIRTUAL_COLUMN_FUNCTION
> +        eng "Auto-increment column %`s may not be used in an expression for a check constraint, a default value or a generated column."

First, please add error messages at the very end of the file.
Second, there's an existing error

ER_GENERATED_COLUMN_FUNCTION_IS_NOT_ALLOWED
  eng "Function or expression '%s' cannot be used in the %s clause of %`s"

which is generally used like

  my_error(ER_GENERATED_COLUMN_FUNCTION_IS_NOT_ALLOWED, MYF(0),
           "NOW()", "CHECK", field->name);

or, for subqueries

  my_error(ER_GENERATED_COLUMN_FUNCTION_IS_NOT_ALLOWED, MYF(0),
           "SELECT ...", "DEFAULT", field->name);

It'd be best if you could use it. If not, at least rephrase your error
message to use

   ... in the %s clause of %`s

pattern, instead of enumerating all possible places where autoincrement
is not allowed.

> diff --git a/sql/table.cc b/sql/table.cc
> index 6b15c06cb91..d1772933190 100644
> --- a/sql/table.cc
> +++ b/sql/table.cc
> @@ -2811,6 +2811,19 @@ static bool fix_and_check_vcol_expr(THD *thd, TABLE *table,
>               "???", "?????");
>      DBUG_RETURN(1);
>    }
> +  else if (res.errors & VCOL_AUTO_INC)
> +  {
> +    /*
> +      An auto_increment field may not be used in an expression for
> +      a check constraint, a default value or a generated column
> +
> +      Note that this error condition is not detected during parsing
> +      of the statement because the field item does not have a field
> +      pointer at that time
> +    */
> +    my_error(ER_AUTOINC_IN_VIRTUAL_COLUMN_FUNCTION, MYF(0), res.name);
> +    DBUG_RETURN(1);
> +  }
>    vcol->flags= res.errors;

There is already a check for auto-increment in generated columns.
I've added it in commit a418c9920047d5222a0d065343347312127b780f
(see the change in item.cc).

But the error message is somewhat confusing. And very confusing when
applied to CHECK and DEFAULT. Yours is better, so please remove my
check, no need to do it twice. Don't forget to re-run tests after that
(at least vcol and gcol suites) as some error messages could change.

Regards,
Sergei
Chief Architect MariaDB
and security@xxxxxxxxxxx