maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Alexander,

I added tests and created an MDEV.
Specific replies below:

On Jul 29, Alexander Barkov wrote:
>    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.

Well, you don't even need AUTO_INCREMENT for that:

  create table t1 (a int not null);
  create trigger trg1 before insert on t1 for each row set new.a=1;
  insert into t1 values (null);

NULL can be inserted without errors :)

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

it's quite tricky. At most I can restore the old check and _some_ cases
will be reported at prepare (explicit NOT NULL DEFAULT NULL, that is).

But DEFAULT NULL PRIMARY KEY can only be detected at execute, as
PRIMARY KEY -> NOT NULL adjustment happens only then.

> 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;
> >     }

nope, works fine. I've added tests.

> > 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 ^^^ ?

PRIMARY KEY makes columns NOT NULL.

Regards,
Sergei
VP of MariaDB Server Engineering
and security@xxxxxxxxxxx