maria-developers team mailing list archive

[Alexander Barkov <bar@xxxxxxxxxxx>]

  Hello Sergei,

> commit 5a362d486b30fdaf3c7a360737331767154b4ee8
> Author: Sergei Golubchik <serg@xxxxxxxxxxx>
> Date:   Mon Jul 18 22:53:27 2022 +0200
>
>     bugfix: DEFAULT NULL was allowed for NOT NULL columns
>     
>     this was detected during parsing (so NOT NULL DEFAULT NULL was an error),
>     but if a column was made NOT NULL later (e.g. as a part of primary key,
>     in mysql_prepare_create_table()), DEFAULT NULL was accepted and ignored.
>     
>     to fix this the NOT NULL DEFAULT NULL was moved into
>     mysql_prepare_create_table().


I'm not sure that in case of auto_increment we
should really disallow DEFAULT NULL under terms of this TIMESTAMP task.


Look at this script:

DROP TABLE IF EXISTS t1;
CREATE TABLE t1 (a INT NOT NULL AUTO_INCREMENT PRIMARY KEY);
INSERT INTO t1 VALUES (NULL);
Query OK, 1 row affected (0.012 sec)

NULL can be inserted without errors.


Yes, it's inserted with a side effect -
it's replaced to the next auto increment value.
But why this side effect makes NULL an invalid default?


Note, I'm not against this change. Stricter behavior is good :)

But I'm worried that:

- It can break compatibility on one hand.
- It goes without it's own MDEV on the other hand.

This change deserves it's own MDEV
(with motivation and consequences explained)
and should be properly documented and mentioned in release notes.


<cut>

> index 712b8629c09..d8471894db9 100644
> --- a/mysql-test/main/view.result
> +++ b/mysql-test/main/view.result
> @@ -4082,12 +4082,12 @@ DROP TABLE t1,t2;
>  # MDEV-6251: SIGSEGV in query optimizer (in set_check_materialized 
>  # with MERGE view)
>  #
> -CREATE TABLE t1 (a1 INT(11) NOT NULL DEFAULT NULL AUTO_INCREMENT PRIMARY KEY);
> -CREATE TABLE t2 (b1 INT(11) NOT NULL DEFAULT NULL AUTO_INCREMENT PRIMARY KEY);
> -CREATE TABLE t3 (c1 INT(11) NOT NULL DEFAULT NULL AUTO_INCREMENT PRIMARY KEY);
> -CREATE TABLE t4 (d1 INT(11) NOT NULL DEFAULT NULL AUTO_INCREMENT PRIMARY KEY);
> -CREATE TABLE t5 (e1 INT(11) NOT NULL DEFAULT NULL AUTO_INCREMENT PRIMARY KEY);
> -CREATE TABLE t6 (f1 INT(11) NOT NULL DEFAULT NULL AUTO_INCREMENT PRIMARY KEY);
> +CREATE TABLE t1 (a1 INT(11) NOT NULL AUTO_INCREMENT PRIMARY KEY);
> +CREATE TABLE t2 (b1 INT(11) NOT NULL AUTO_INCREMENT PRIMARY KEY);
> +CREATE TABLE t3 (c1 INT(11) NOT NULL AUTO_INCREMENT PRIMARY KEY);
> +CREATE TABLE t4 (d1 INT(11) NOT NULL AUTO_INCREMENT PRIMARY KEY);
> +CREATE TABLE t5 (e1 INT(11) NOT NULL AUTO_INCREMENT PRIMARY KEY);
> +CREATE TABLE t6 (f1 INT(11) NOT NULL AUTO_INCREMENT PRIMARY KEY);

It seems you removed all cases with the old syntax.
So now it's not recorded in MTR what happens on this statement:

CREATE TABLE t1 (a INT NOT NULL DEFAULT NULL AUTO_INCREMENT PRIMARY KEY);

Please cover this scenario in some relevant *.test file.

<cut>


> diff --git a/sql/sql_table.cc b/sql/sql_table.cc
> index cf0bbfb1130..3eeabe20c71 100644
> --- a/sql/sql_table.cc
> +++ b/sql/sql_table.cc

<cut>


> @@ -3647,6 +3643,15 @@ mysql_prepare_create_table(THD *thd, HA_CREATE_INFO *create_info,
>    {
>      Field::utype type= (Field::utype) MTYP_TYPENR(sql_field->unireg_check);
>  
> +    if (sql_field->default_value &&
> +        sql_field->default_value->expr->type() == Item::NULL_ITEM &&
> +        sql_field->flags & NOT_NULL_FLAG &&
> +        !(sql_field->flags & AUTO_INCREMENT_FLAG))
> +    {
> +      my_error(ER_INVALID_DEFAULT, MYF(0), sql_field->field_name.str);
> +      DBUG_RETURN(TRUE);
> +    }



^^^ now obvious errors are caught only during EXECUTE,
although it's already clear at the PREPARE stage that the DEFAULT is wrong:


MariaDB [test]> PREPARE stmt FROM 'CREATE TABLE T1 (a INT NOT NULL 
DEFAULT NULL)';
Query OK, 0 rows affected (0.000 sec)
Statement prepared

MariaDB [test]> EXECUTE stmt;
ERROR 1067 (42000): Invalid default value for 'a'


I suggest we still catch non-unambiguous cases
(when nothing depends on system variables) during PREPARE.
Inside some existing or a new Type_handler method.



Also, please add PS tests:

- for unambiguous scenarios like here

- for ambiguous scenarios, e.g. with TIMESTAMP columns changing 
OPTION_EXPLICIT_DEF_TIMESTAMP between PREPARE and EXECUTE.



It seems this code needs to be changed:
>     /*
>       Set NO_DEFAULT_VALUE_FLAG if this field doesn't have a default value and
>       it is NOT NULL, not an AUTO_INCREMENT field, not a TIMESTAMP and not
>       updated trough a NOW() function.
>     */
>     if (!sql_field->default_value &&
>         !sql_field->has_default_function() &&
>         (sql_field->flags & NOT_NULL_FLAG) &&
>         (!sql_field->is_timestamp_type() ||
>          (thd->variables.option_bits & OPTION_EXPLICIT_DEF_TIMESTAMP))&&
>         !sql_field->vers_sys_field())
>     {
>       sql_field->flags|= NO_DEFAULT_VALUE_FLAG;
>       sql_field->pack_flag|= FIELDFLAG_NO_DEFAULT;
>     }

It can only add NO DEFAULT related flags.
Shouldn't it also remove NO DEFAULT flags
if the current PS EXECUTE time option OPTION_EXPLICIT_DEF_TIMESTAMP
is unset?



<cut>


> diff --git a/storage/spider/mysql-test/spider/bg/t/spider_fixes.test b/storage/spider/mysql-test/spider/bg/t/spider_fixes.test
> index b222f494ba1..41f87e61416 100644
> --- a/storage/spider/mysql-test/spider/bg/t/spider_fixes.test
> +++ b/storage/spider/mysql-test/spider/bg/t/spider_fixes.test
> @@ -294,14 +294,14 @@ if ($USE_CHILD_GROUP2)
>  --disable_query_log
>  echo CREATE TABLE ta_l (
>    a int(11) NOT NULL DEFAULT '0',
> -  b char(1) DEFAULT NULL,
> -  c datetime DEFAULT NULL,
> +  b char(1),
> +  c datetime,
>    PRIMARY KEY (a, b, c)
>  ) MASTER_1_ENGINE MASTER_1_CHARSET MASTER_1_COMMENT5_2_1;
>  eval CREATE TABLE ta_l (
>    a int(11) NOT NULL DEFAULT '0',
> -  b char(1) DEFAULT NULL,
> -  c datetime DEFAULT NULL,
> +  b char(1),
> +  c datetime,
>    PRIMARY KEY (a, b, c)
>  ) $MASTER_1_ENGINE $MASTER_1_CHARSET $MASTER_1_COMMENT5_2_1;
>  --enable_query_log

Why this change ^^^ ?


> diff --git a/storage/spider/mysql-test/spider/t/spider_fixes.test b/storage/spider/mysql-test/spider/t/spider_fixes.test
> index 47bc225d614..75f108f1164 100644
> --- a/storage/spider/mysql-test/spider/t/spider_fixes.test
> +++ b/storage/spider/mysql-test/spider/t/spider_fixes.test
> @@ -294,14 +294,14 @@ if ($USE_CHILD_GROUP2)
>  --disable_query_log
>  echo CREATE TABLE ta_l (
>    a int(11) NOT NULL DEFAULT '0',
> -  b char(1) DEFAULT NULL,
> -  c datetime DEFAULT NULL,
> +  b char(1),
> +  c datetime,
>    PRIMARY KEY (a, b, c)
>  ) MASTER_1_ENGINE MASTER_1_CHARSET MASTER_1_COMMENT5_2_1;
>  eval CREATE TABLE ta_l (
>    a int(11) NOT NULL DEFAULT '0',
> -  b char(1) DEFAULT NULL,
> -  c datetime DEFAULT NULL,
> +  b char(1),
> +  c datetime,
>    PRIMARY KEY (a, b, c)
>  ) $MASTER_1_ENGINE $MASTER_1_CHARSET $MASTER_1_COMMENT5_2_1;
>  --enable_query_log

Same question.