[Commits] review of: febc124: MDEV-8605 MariaDB not use DEFAULT value even when inserted NULL for NOT NULLABLE column

Michael Widenius monty at mariadb.org
Fri Nov 13 18:31:48 EET 2015


Hi!

> MDEV-8605 MariaDB not use DEFAULT value even when inserted NULL for NOT NULLABLE column

> NOT NULL constraint must be checked *after* the BEFORE triggers.
> That is for INSERT and UPDATE statements even NOT NULL fields
> must be able to store a NULL temporarily at least while
> BEFORE INSERT/UPDATE triggers are running.

>diff --git a/mysql-test/r/trigger.result b/mysql-test/r/trigger.result
>index 9dfa589..8c4d38d 100644
>--- a/mysql-test/r/trigger.result
>+++ b/mysql-test/r/trigger.result
>@@ -214,12 +214,14 @@ end if;
> end|
> insert into t3 values (1);
> insert into t1 values (4, "four", 1), (5, "five", 2);
>-ERROR 23000: Column 'id' cannot be null
>+Warnings:
>+Warning	1048	Column 'id' cannot be null
> select * from t1;
> id	data	fk
> 1	one	NULL
> 2	two	NULL
> 4	four	1
>+0	five	2

Don't understand why the above is now a warning and not an error.
Why is it that setting id=NULL would insert 0. I would understand if
id would be an auto-increment.

serg: That was a bug, apparently. doing new.id=NULL should produce the same effect as INSERT .... (NULL)

>diff --git a/mysql-test/r/trigger_null-8605.result b/mysql-test/r/trigger_null-8605.result
>new file mode 100644
>index 0000000..119e8b9
>--- /dev/null
>+++ b/mysql-test/r/trigger_null-8605.result
>@@ -0,0 +1,275 @@
>+set sql_mode=strict_all_tables;
>+set time_zone="+02:00";
>+create table t1 (a int not null, b int, c int);
>+create trigger trgi before insert on t1 for each row set new.a=if(new.a is null,new.b,new.c);
>+insert t1 values (10, NULL, 1);
>+insert t1 values (NULL, 2, NULL);
>+insert t1 values (NULL, NULL, 20);
>+ERROR 23000: Column 'a' cannot be null
>+insert t1 values (1, 2, NULL);
>+ERROR 23000: Column 'a' cannot be null
>+select * from t1;
>+a	b	c
>+1	NULL	1
>+2	2	NULL
>+insert ignore t1 values (NULL, NULL, 30);
>+Warnings:
>+Warning	1048	Column 'a' cannot be null
>+insert ignore t1 values (1, 3, NULL);
>+Warnings:
>+Warning	1048	Column 'a' cannot be null
>+select * from t1;
>+a	b	c
>+1	NULL	1
>+2	2	NULL
>+0	NULL	30
>+0	3	NULL
>+delete from t1;
>+insert t1 (a,c) values (10, 1);
>+insert t1 (a,b) values (NULL, 2);
>+insert t1 (a,c) values (NULL, 20);
>+ERROR 23000: Column 'a' cannot be null
>+insert t1 (a,b) values (1, 2);
>+ERROR 23000: Column 'a' cannot be null
>+select * from t1;
>+a	b	c
>+1	NULL	1
>+2	2	NULL
>+delete from t1;
>+insert t1 select 10, NULL, 1;
>+insert t1 select NULL, 2, NULL;
>+insert t1 select NULL, NULL, 20;
>+ERROR 23000: Column 'a' cannot be null
>+insert t1 select 1, 2, NULL;
>+ERROR 23000: Column 'a' cannot be null
>+insert ignore t1 select NULL, NULL, 30;
>+Warnings:
>+Warning	1048	Column 'a' cannot be null
>+insert ignore t1 select 1, 3, NULL;
>+Warnings:
>+Warning	1048	Column 'a' cannot be null
>+select * from t1;
>+a	b	c
>+1	NULL	1
>+2	2	NULL
>+0	NULL	30
>+0	3	NULL
>+delete from t1;
>+insert delayed t1 values (10, NULL, 1);
>+insert delayed t1 values (NULL, 2, NULL);
>+insert delayed t1 values (NULL, NULL, 20);
>+ERROR 23000: Column 'a' cannot be null
>+insert delayed t1 values (1, 2, NULL);
>+ERROR 23000: Column 'a' cannot be null
>+select * from t1;
>+a	b	c
>+1	NULL	1
>+2	2	NULL
>+insert delayed ignore t1 values (NULL, NULL, 30);
>+Warnings:
>+Warning	1048	Column 'a' cannot be null
>+insert delayed ignore t1 values (1, 3, NULL);
>+Warnings:
>+Warning	1048	Column 'a' cannot be null
>+flush table t1;
>+select * from t1;
>+a	b	c
>+1	NULL	1
>+2	2	NULL
>+0	NULL	30
>+0	3	NULL
>+delete from t1;
>+alter table t1 add primary key (a);
>+create trigger trgu before update on t1 for each row set new.a=if(new.a is null,new.b,new.c);
>+insert t1 values (100,100,100), (200,200,200), (300,300,300);
>+insert t1 values (100,100,100) on duplicate key update a=10, b=NULL, c=1;
>+insert t1 values (200,200,200) on duplicate key update a=NULL, b=2, c=NULL;
>+insert t1 values (300,300,300) on duplicate key update a=NULL, b=NULL, c=20;
>+ERROR 23000: Column 'a' cannot be null
>+insert t1 values (300,300,300) on duplicate key update a=1, b=2, c=NULL;
>+ERROR 23000: Column 'a' cannot be null
>+select * from t1;
>+a	b	c
>+1	NULL	1
>+2	2	NULL
>+300	300	300
>+delete from t1;
>+insert t1 values (1,100,1), (2,200,2);
>+replace t1 values (10, NULL, 1);
>+replace t1 values (NULL, 2, NULL);
>+replace t1 values (NULL, NULL, 30);
>+ERROR 23000: Column 'a' cannot be null
>+replace t1 values (1, 3, NULL);
>+ERROR 23000: Column 'a' cannot be null
>+select * from t1;
>+a	b	c
>+1	NULL	1
>+2	2	NULL
>+delete from t1;
>+insert t1 values (100,100,100), (200,200,200), (300,300,300);
>+update t1 set a=10, b=NULL, c=1 where a=100;
>+update t1 set a=NULL, b=2, c=NULL where a=200;
>+update t1 set a=NULL, b=NULL, c=20 where a=300;
>+ERROR 23000: Column 'a' cannot be null
>+update t1 set a=1, b=2, c=NULL where a=300;
>+ERROR 23000: Column 'a' cannot be null
>+select * from t1;
>+a	b	c
>+1	NULL	1
>+2	2	NULL
>+300	300	300
>+set statement sql_mode='' for update t1 set a=1, b=2, c=NULL where a > 1;
>+ERROR 23000: Duplicate entry '0' for key 'PRIMARY'
>+select * from t1;
>+a	b	c
>+1	NULL	1
>+0	2	NULL
>+300	300	300
>+delete from t1;
>+create table t2 (d int, e int);
>+insert t1 values (100,100,100), (200,200,200), (300,300,300);
>+insert t2 select a,b from t1;
>+update t1,t2 set a=10, b=NULL, c=1 where b=d and e=100;
>+update t1,t2 set a=NULL, b=2, c=NULL where b=d and e=200;
>+update t1,t2 set a=NULL, b=NULL, c=20 where b=d and e=300;
>+ERROR 23000: Column 'a' cannot be null
>+update t1,t2 set a=1, b=2, c=NULL where b=d and e=300;
>+ERROR 23000: Column 'a' cannot be null
>+select * from t1;
>+a	b	c
>+1	NULL	1
>+2	2	NULL
>+300	300	300
>+delete from t1;
>+insert t2 values (2,2);
>+create view v1 as select * from t1, t2 where d=2;
>+insert v1 (a,c) values (10, 1);
>+insert v1 (a,b) values (NULL, 2);
>+insert v1 (a,c) values (NULL, 20);
>+ERROR 23000: Column 'a' cannot be null
>+insert v1 (a,b) values (1, 2);
>+ERROR 23000: Column 'a' cannot be null
>+select * from v1;
>+a	b	c	d	e
>+1	NULL	1	2	2
>+2	2	NULL	2	2
>+delete from t1;
>+drop view v1;
>+drop table t2;
>+load data infile 'mdev8605.txt' into table t1 fields terminated by ',';
>+ERROR 23000: Column 'a' cannot be null
>+select * from t1;
>+a	b	c
>+1	NULL	1
>+2	2	NULL
>+drop table t1;
>+create table t1 (a timestamp, b int auto_increment primary key);
>+create trigger trgi before insert on t1 for each row set new.a=if(new.a is null, '2000-10-20 10:20:30', NULL);
>+set statement timestamp=777777777 for insert t1 (a) values (NULL);
>+set statement timestamp=888888888 for insert t1 (a) values ('1999-12-11 10:9:8');
>+select b, a, unix_timestamp(a) from t1;
>+b	a	unix_timestamp(a)
>+1	2000-10-20 10:20:30	972030030
>+2	1998-03-03 03:34:48	888888888
>+set statement timestamp=999999999 for update t1 set b=3 where b=2;
>+select b, a, unix_timestamp(a) from t1;
>+b	a	unix_timestamp(a)
>+1	2000-10-20 10:20:30	972030030
>+3	2001-09-09 03:46:39	999999999
>+create trigger trgu before update on t1 for each row set new.a='2011-11-11 11:11:11';
>+update t1 set b=4 where b=3;
>+select b, a, unix_timestamp(a) from t1;
>+b	a	unix_timestamp(a)
>+1	2000-10-20 10:20:30	972030030
>+4	2011-11-11 11:11:11	1321002671
>+drop table t1;
>+create table t1 (a int auto_increment primary key);
>+create trigger trgi before insert on t1 for each row set new.a=if(new.a is null, 5, NULL);
>+insert t1 values (NULL);
>+insert t1 values (10);
>+select a from t1;
>+a
>+5
>+6
>+drop table t1;
>+create table t1 (a int, b int, c int);
>+create trigger trgi before insert on t1 for each row set new.a=if(new.a is null,new.b,new.c);
>+insert t1 values (10, NULL, 1);
>+insert t1 values (NULL, 2, NULL);
>+insert t1 values (NULL, NULL, 20);
>+insert t1 values (1, 2, NULL);
>+select * from t1;
>+a	b	c
>+1	NULL	1
>+2	2	NULL
>+NULL	NULL	20
>+NULL	2	NULL
>+drop table t1;
>+create table t1 (a1 tinyint not null, a2 timestamp not null,
>+a3 tinyint not null auto_increment primary key,
>+b tinyint, c int not null);
>+create trigger trgi before insert on t1 for each row
>+begin
>+if new.b=1 then set new.a1=if(new.c,new.c,null); end if;
>+if new.b=2 then set new.a2=if(new.c,new.c,null); end if;
>+if new.b=3 then set new.a3=if(new.c,new.c,null); end if;
>+end|
>+set statement timestamp=777777777 for
>+load data infile 'sep8605.txt' into table t1 fields terminated by ',';
>+ERROR 23000: Column 'a1' cannot be null
>+select * from t1;
>+a1	a2	a3	b	c
>+1	2010-11-12 01:02:03	10	0	0
>+2	2010-11-12 01:02:03	11	1	2
>+3	1994-08-25 03:22:57	12	0	0
>+4	2000-09-08 07:06:05	13	2	908070605
>+5	1994-08-25 03:22:57	14	2	0
>+6	2010-11-12 01:02:03	15	0	0
>+7	2010-11-12 01:02:03	20	3	20
>+8	2010-11-12 01:02:03	21	3	0
>+delete from t1;
>+set statement timestamp=777777777 for
>+load data infile 'sep8605.txt' into table t1 fields terminated by ','
>+   (@a,a2,a3,b,c) set a1=100- at a;
>+ERROR 23000: Column 'a1' cannot be null
>+select 100-a1,a2,a3,b,c from t1;
>+100-a1	a2	a3	b	c
>+1	2010-11-12 01:02:03	10	0	0
>+98	2010-11-12 01:02:03	11	1	2
>+3	1994-08-25 03:22:57	12	0	0
>+4	2000-09-08 07:06:05	13	2	908070605
>+5	1994-08-25 03:22:57	14	2	0
>+6	2010-11-12 01:02:03	22	0	0
>+7	2010-11-12 01:02:03	20	3	20
>+8	2010-11-12 01:02:03	23	3	0
>+delete from t1;
>+set statement timestamp=777777777 for
>+load data infile 'fix8605.txt' into table t1 fields terminated by '';
>+ERROR 23000: Column 'a1' cannot be null
>+select * from t1;
>+a1	a2	a3	b	c
>+1	2010-11-12 01:02:03	10	0	0
>+5	1994-08-25 03:22:57	14	2	0
>+8	2010-11-12 01:02:03	24	3	0
>+delete from t1;
>+set statement timestamp=777777777 for
>+load xml infile 'xml8605.txt' into table t1 rows identified by '<row>';
>+ERROR 23000: Column 'a1' cannot be null
>+select * from t1;
>+a1	a2	a3	b	c
>+1	2010-11-12 01:02:03	10	0	0
>+2	2010-11-12 01:02:03	11	1	2
>+3	1994-08-25 03:22:57	12	0	0
>+4	2000-09-08 07:06:05	13	2	908070605
>+5	1994-08-25 03:22:57	14	2	0
>+6	2010-11-12 01:02:03	25	0	0
>+7	2010-11-12 01:02:03	20	3	20
>+8	2010-11-12 01:02:03	26	3	0
>+drop table t1;
>+create table t1 (a int not null default 5, b int, c int);
>+create trigger trgi before insert on t1 for each row set new.b=new.c;
>+insert t1 values (DEFAULT,2,1);
>+select * from t1;
>+a	b	c
>+5	1	1
>+drop table t1;
>diff --git a/mysql-test/suite/funcs_1/r/innodb_trig_09.result b/mysql-test/suite/funcs_1/r/innodb_trig_09.result
>index 986506b..e6a1059 100644
>--- a/mysql-test/suite/funcs_1/r/innodb_trig_09.result
>+++ b/mysql-test/suite/funcs_1/r/innodb_trig_09.result
>@@ -189,8 +189,6 @@ Update tb3 Set f122='Test 3.5.9.4-trig', f136=NULL, f151=DEFAULT, f163=NULL
> where f122='Test 3.5.9.4';
> Warnings:
> Warning	1048	Column 'f136' cannot be null
>-Update tb3 Set f122='Test 3.5.9.4-trig', f136=0, f151=DEFAULT, f163=NULL
>-where f122='Test 3.5.9.4';

Why did you remove the above test ?
<serg> ah. just above this statement there was
<serg> Update tb3 Set f122='Test 3.5.9.4-trig', f136=NULL, f151=DEFAULT, f163=NULL
<serg>  where f122='Test 3.5.9.4';
<serg> so this second update didn't do anything (where condition never matched)
<serg> in the even older version of that file
<serg> the first update had an --error line
<serg> then the second update made sense
<serg> then somebody removed --error
<serg> and the second update became no-op

> select f118, f121, f122, f136, f151, f163 from tb3
> where f122 like 'Test 3.5.9.4-trig' order by f163;
> f118	f121	f122	f136	f151	f163
>@@ -198,7 +196,7 @@ a	NULL	Test 3.5.9.4-trig	00000	999	NULL
> select  @tr_var_b4_118, @tr_var_b4_121, @tr_var_b4_122,
> @tr_var_b4_136, @tr_var_b4_151, @tr_var_b4_163;
> @tr_var_b4_118	@tr_var_b4_121	@tr_var_b4_122	@tr_var_b4_136	@tr_var_b4_151	@tr_var_b4_163
>-a	NULL	Test 3.5.9.4-trig	0	999	NULL
>+a	NULL	Test 3.5.9.4-trig	NULL	999	NULL

Why test result change?

<serg> the result shows what value of f136 was seen inside the trigger
<serg> before my patch, f136 was NOT NULL and the trigger can never see it being NULL
<serg> after my patch it can temporarily be NULL

>diff --git a/sql/field.h b/sql/field.h
>index b24561d..8524135 100644
>--- a/sql/field.h
>+++ b/sql/field.h
>@@ -844,7 +844,7 @@ class Field: public Value_source
>     my_ptrdiff_t l_offset= (my_ptrdiff_t) (table->s->default_values -
> 					  table->record[0]);
>     memcpy(ptr, ptr + l_offset, pack_length());
>-    if (null_ptr)
>+    if (null_ptr >= table->record[0] && null_ptr <= ptr)
>       *null_ptr= ((*null_ptr & (uchar) ~null_bit) |
> 		  (null_ptr[l_offset] & null_bit));
>   }

Plese add a comment like:

/*
  Copy the null bit from the default values if this is a normal null
  field, in which case null_ptr points either to the null_bit_map in
  the table->record[0]. null_ptr may also be set for not-null fields
  that are used in triggers, in which case it points to specific
  temporary-null bit map used by the trigger code.
*/

>diff --git a/sql/item.cc b/sql/item.cc
>index d802009..4242e53 100644
>--- a/sql/item.cc
>+++ b/sql/item.cc
>@@ -8176,9 +8176,8 @@ int Item_default_value::save_in_field(Field *field_arg, bool no_conversions)
>     }
>     field_arg->set_default();
>     return
>-      !field_arg->is_null_in_record(field_arg->table->s->default_values) &&
>-       field_arg->validate_value_in_record_with_warn(thd,
>-                                       field_arg->table->s->default_values) &&
>+      !field_arg->is_null() &&
>+       field_arg->validate_value_in_record_with_warn(thd, table->record[0]) &&
>        thd->is_error() ? -1 : 0;

Wonder why we didn't use field_arg->is_null() here before...


>   }
>   return Item_field::save_in_field(field_arg, no_conversions);
>@@ -8372,6 +8371,7 @@ bool Item_trigger_field::set_value(THD *thd, sp_rcontext * /*ctx*/, Item **it)
>   int err_code= item->save_in_field(field, 0);
> 
>   field->table->copy_blobs= copy_blobs_saved;
>+  field->set_explicit_default(item);

Why do we need this now, when we didn't need it before?
Was this missing in the old code or is there something new that require it?

<serg> ah, it was a bug

>+++ b/sql/sql_base.cc
>@@ -8843,7 +8843,74 @@ fill_record(THD *thd, TABLE *table_arg, List<Item> &fields, List<Item> &values,
> }
> 
> 
>-/*
>+/**
>+  Prepare the List<Item> for fill_record_n_invoke_before_triggers()
>+
>+  This means redirecting all Item_field-s from table->field to
>+  table->field_to_fill(), if needed.
>+
>+  Most of the complexity comes from LOAD DATA ... SET.
>+  For UPDATE and INSERT 'fields' is always a non-empty list of Item_field-s
>+*/
>+void switch_to_nullable_trigger_fields(List<Item> &fields)
>+{
>+  List_iterator_fast<Item> it(fields);
>+  Item *item;
>+  while ((item= it++))
>+  {
>+    if (item->real_item()->type() == Item::FIELD_ITEM)
>+      break;
>+  }
>+  if (!item)
>+    return;
>+
>+  TABLE *tbl= static_cast<Item_field*>(item->real_item())->field->table;
>+  Field** field= tbl->field_to_fill();
>+

Shouldn't we do the above recursively and both for fields and values?

To be able to handle things like:

insert into t1 set a_not_null=NULL, b=a;
update t1 set a=null, b=a+1;

Add comment:

/* If table has triggers that updates not-null fields */

>+  if (field != tbl->field)
>+    do {
>+      if (item->real_item()->type() == Item::FIELD_ITEM)
>+      {
>+        Item_field *f= (Item_field*)(item->real_item());
>+        f->field= field[f->field->field_index];
>+      }
>+    } while ((item= it++));
>+}
>+
>+
>+/**
>+  Test NOT NULL constraint after BEFORE triggers
>+*/
>+static bool not_null_fields_have_null_values(TABLE *table)
>+{
>+  Field **orig_field= table->field;
>+  Field **filled_field= table->field_to_fill();
>+
>+  if (filled_field != orig_field)
>+  {
>+    THD *thd=table->in_use;
>+    for (uint i=0; i < table->s->fields; i++)
>+    {
>+      Field *of= orig_field[i];
>+      Field *ff= filled_field[i];
>+      if (ff != of)
>+      {
>+        // copy after-update flags to of, copy before-update flags to ff
>+        swap_variables(uint32, of->flags, ff->flags);
>+        if (ff->is_real_null())
>+        {
>+          ff->set_notnull(); // for next row WHERE condition in UPDATE
>+          if (convert_null_to_field_value_or_error(of) || thd->is_error())
>+            return true;

What happens with the other fields that where marked as null in case
of error?
Where do we reset the trigger not-null-field bitmap between statements?

>+++ b/sql/sql_trigger.cc
> 
>+  if (has[TRG_EVENT_UPDATE] || has[TRG_EVENT_DELETE])
>+  {
>+    if (!(record1_field= (Field **)alloc_root(&table->mem_root,
>+                                              (table->s->fields + 1) *
>+                                              sizeof(Field*))))
>+      return 1;
>+
>+    for (fld= table->field, trg_fld= record1_field; *fld; fld++, trg_fld++)
>+    {
>+      /*
>+        QQ: it is supposed that it is ok to use this function for field
>+        cloning...
>+      */

Please remove the above old comment

>+      if (!(*trg_fld= (*fld)->make_new_field(&table->mem_root, table,
>+                                             table == (*fld)->table)))
>+        return 1;
>+      (*trg_fld)->move_field_offset((my_ptrdiff_t)(table->record[1] -
>+                                                   table->record[0]));
>+    }
>+    *trg_fld= 0;
>+  }
>   return 0;
> }

Regards,
Monty


More information about the commits mailing list