[#MDEV-19761] Before Trigger not processed for Not Null Columns

[MDEV-19761] Before Trigger not processed for Not Null Columns Created: 2019-06-14 Updated: 2023-11-28
Status:	Open
Project:	MariaDB Server
Component/s:	Data Manipulation - Update
Affects Version/s:	10.1, 10.3.15, 10.2, 10.3, 10.4, 10.5, 10.6, 10.7, 10.8, 10.9, 10.10
Fix Version/s:	10.4, 10.5, 10.6

Type:

Bug

Priority:

Major

Reporter:

langfingaz

Assignee:

Anel Husakovic

Resolution:

Unresolved

Votes:

Labels:

upstream-fixed

Environment:

Official MariaDB Docker Image (MARIADB_VERSION=1:10.3.15+maria~bionic)

Issue Links:

Relates
relates to	~~MDEV-8605~~	MariaDB not use DEFAULT value even wh...	Closed
relates to	~~MDEV-11698~~	Old Bug possibly not fixed; BEFORE IN...	Closed
relates to	~~MDEV-11842~~	Fail to insert on a table where a fie...	Closed
relates to	~~MDEV-10002~~	Before Insert trigger does not work w...	Closed

Description

Bug description:

When inserting a new row into a table without specifying a column that is set to not null will give the error "Field 'xxx' doesn't have a default value" - even if a trigger exists, that should replace the NULL value with a not-null value before inserting it.

Expected behavior:

Triggers declared as "before insert on 'table_xxx' " should be fired if they might change any of the new values to insert BEFORE column constrains are verified.

See similar SQL bug that has been fixed:

Actual behavior:

If a new row contains a null value for a column that is defined as not null but is changed in a trigger, that trigger won't be fired at all as immediately a error is thrown.

Example SQL code to reproduce the error:

drop database if exists test_database;

create database test_database;

use test_database;

create table test_table(

    id int,

    rate DECIMAL(23 , 10 ) NOT NULL,

    primary key (id)

);

create table currency(

    name varchar(4),

    rate DECIMAL(23 , 10 ) NOT NULL,

    primary key (name)

);

delimiter ///

create trigger test_trigger

	before insert on test_table

    for each row

begin

    if new.rate is null then

		set new.rate = (

			select c.rate

			from currency c

			where c.name = 'EUR'

);

    end if;

end;

///

delimiter ;

insert into currency (name, rate) values ('EUR', 1.234);

insert into test_table (id) values (1);

-- Error in query (1364): Field 'rate' doesn't have a default value --

Workaround:

define column "NULLABLE" without "NOT NULL" constraint.
Insert keyword instead of NULL value. For example -1 or 'NULL'
Inside Trigger: "if new.column = -1" or " if new.column = 'NULL' " then do something

Example SQL code to demonstrate the workaround:

(Note: Only the insert statement as well as the comparison in the trigger (if new.rate is null) changed.)

drop database if exists test_database;

create database test_database;

use test_database;

create table test_table(

    id int,

    rate DECIMAL(23 , 10 ) NOT NULL,

    primary key (id)

);

create table currency(

    name varchar(4),

    rate DECIMAL(23 , 10 ) NOT NULL,

    primary key (name)

);

delimiter ///

create trigger test_trigger

	before insert on test_table

    for each row

begin

    if new.rate = -1 then

		set new.rate = (

			select c.rate

			from currency c

			where c.name = 'EUR'

);

    end if;

end;

///

delimiter ;

insert into currency (name, rate) values ('EUR', 1.234);

insert into test_table (id, rate) values (1, -1);

select rate from test_table;

Comments

Comment by Alice Sherepa [ 2019-06-17 ]

Thanks a lot! I reproduced on 10.1-10.4:

create table t1( id int, rate int not null);

delimiter $$;

create trigger test_trigger before insert on t1 for each row

	set new.rate=if(new.rate is null,5,new.rate);

$$

delimiter ;$$

insert into t1 (id) values (1);

select * from t1;

delimiter $$;

create or replace trigger test_trigger before insert on t1 for each row

	begin if new.rate is null then set new.rate = 5; end if; end;

$$

delimiter ;$$

insert into t1 (id) values (2);

select * from t1;

MariaDB [test]> select * from t1;

+------+------+

| id   | rate |

+------+------+

|    1 |    0 |

+------+------+

1 row in set (0.000 sec)

MariaDB [test]> delimiter $$

MariaDB [test]> create or replace trigger test_trigger before insert on t1 for each row

    -> begin if new.rate is null then set new.rate = 5; end if; end;

    -> $$

Query OK, 0 rows affected (0.011 sec)

MariaDB [test]> delimiter ;

MariaDB [test]> insert into t1 (id) values (2);

Query OK, 1 row affected, 1 warning (0.006 sec)

Warning (Code 1364): Field 'rate' doesn't have a default value

##ERROR 1364 (HY000): Field 'rate' doesn't have a default value  -- in strict sql_mode

MariaDB [test]> select * from t1;

+------+------+

| id   | rate |

+------+------+

|    1 |    0 |

|    2 |    0 |

+------+------+

2 rows in set (0.000 sec)

Mysql 8.0.15

mysql> insert into t1 (id) values (1);

Query OK, 1 row affected (0.01 sec)

mysql> select * from t1;

+------+------+

| id   | rate |

+------+------+

|    1 |    5 |

+------+------+

1 row in set (0.00 sec)

Comment by Anel Husakovic [ 2019-06-26 ]

Hi serg,

Debugging codebase

Debugging code base in `10.1`:

sql_insert.cc: mysql_insert(),
sql_base.cc: switch_to_nullable_trigger_fields(),
sql_base.cc: fill_record_n_invoke_before_triggers(),
sql_base.cc: not_null_fields_have_null_values(),
sql_base.cc: fill_record(),
item.cc: switch_to_nullable_fields_processor(),
sql_trigger.cc:prepare_record_accessors(),
table.cc: validate_default_values_of_unset_fields().
sql_trigger.h: field_to_fill()
sql_insert.cc: has_no_default_value(),
mysql_com.cc #defines,
sql_insert.cc:mysql_prepare_insert() (didn't analyse to much),
field.h:maybe_null_in_table().

Observations

According to my opinion problem is that Item_field in fill_record (5) doesn't get proper field attributes null_ptr, null_bit and flags from List<Item> &fields,
when called from fill_record_n_invoke_before_triggers()(3). Here is assumption that when field has proper value trigger will do what is has to do.
I will use up to bottom analysis so far as I understand what is going on:
a) sql_base.cc: fill_record(): -> function called to store values (*values) in fields (fields) -> sql_insert.cc:939: fill_record_n_invoke_before_triggers() -> sql_base.cc:9022
a.1) List<Item> &fields is list of items which should be modifed through mysql_insert()(1),
a.2) List<Item> &values obtained from mysql_insert(): List<List_item> &values_list(1), function do only iterations, not changing the values (confirmed)

  # In case when third column is NULL (j:= null_bit, flags, null_ptr, ptr); these values are same as table->field(i)->j

        # **null_bit**

(gdb) p rfield(0)->null_bit

$4 = 2 '\002'

(gdb) p rfield(1)->null_bit

$5 = 4 '\004'

(gdb) p rfield(2)->null_bit # good

$6 = 8 '\b'

        # **flags** are 0 for all fields

        (gdb) p rfield(2)->flags

        $7 = 0

        # **null_ptr** are same for all fields

(gdb) p rfield(0)->null_ptr

$10 = (uchar *) 0x7fffe0025818 "\377"

# **ptr** are different and not null for all fields

(gdb) p rfield(0)->ptr

$11 = (uchar *) 0x7fffe0025819 ""

(gdb) p rfield(1)->ptr

$12 = (uchar *) 0x7fffe002581d ""

(gdb) p rfield(2)->ptr

$13 = (uchar *) 0x7fffe0025821 ""

  # In case when third column is NOT NULL

        # **null_bit**

  	(gdb) p rfield(0)->null_bit

$1 = 2 '\002'

(gdb) p rfield(1)->null_bit

$2 = 4 '\004'

(gdb) p rfield(2)->null_bit

$3 = 0 '\000'

# **flags**are 0 for all NULL fields, but 4097 (see **Note 1**) for NOT null field

(gdb) p rfield(0)->flags

$29 = 0

(gdb) p rfield(1)->flags

$30 = 0

(gdb) p rfield(2)->flags

$31 = 4097

# **null_ptr** are same for all NULL fields, but 0 for NOT null field

(gdb) p rfield(0)->null_ptr

$25 = (uchar *) 0x7fffe0025818 "\377"

(gdb) p rfield(1)->null_ptr

$26 = (uchar *) 0x7fffe0025818 "\377"

(gdb) p rfield(2)->null_ptr

$27 = (uchar *) 0x0

Observations

b) switch_to_nullable_trigger_fields(fields, table)(2) called from mysql_insert():911
b.1) List<Item> &field is obtained from mysql_insert()(1) and is used only in mysql_prepare_insert()(12) before (2) is called.
b.2) TABLE *table -> original table
I) This function is first creating an array Field** field= table->field_to_fill(); and checking is it the same as as original one table->field.
II) If it is not the same, new field array (field)is created and we will go through list of fields (items in general) and call appropriate switch_to_nullable_fields_processor() in this case Item_field
and pass the field.

# In case when third column is NULL this function {{Field** field= table->field_to_fill();}}will be the same as {{table->field}}.

# and no field processor will be called.

# In case when third column is NOT NULL:

# field_to_fill() will return `record0_field` which is created in `prepare_record_accessors()`(7)

# Here are doubts, which are tested on proposed patch:

# Question_1) `sql_trigger.cc: 1121` why `f->null_bit= null_bit (// which is always set to `1`)` ?

# Question_2) `uchar *extra_null_bitmap` is allocated and in (7) is `null_ptr` set to it, but the value is never set, it is always empty, why?

     	If it is not set what is the meaning of `reset_extra_null_bitmap()`?

**Note:** According to `maybe_null_in_table()`(13) null_ptr should point to `extra_null_bitmap` and it does.

# Question_3) Added get_extra_null_bitmap() and setting nptr=table->triggers->get_extra_null_bitmap(); not working,do we need extra_null_bitmap?

# When `record0_field` is returned, function `item.cc(2442): bool Item_field::switch_to_nullable_fields_processor(uchar *arg)` will be called.

# This function is calling `set_field_to_new_field()`.

        # Here are problems that in field_list during iterations there will be no `NOT NULL` column, so no new field will be created for `NOT NULL` column.

        # Proposed patch is suggesting to use it **after** new field is created, but before that to cheat server to use `null_ptr` from some of NULL fields.

Notes

*Note 1*: meaning of flags can be seen from (11)
So for example 4097 is same as (NO_DEFAULT_VALUE_FLAG | NOT_NULL_FLAG) since in (11) it is defined as:

#define NOT_NULL_FLAG	1	/* Field can't be NULL */

#define NO_DEFAULT_VALUE_FLAG 4096	/* Field doesn't have default value */

*Note 2*: warnings are comming from `has_no_default_value()` (10)

insert into t1 (id1,id2) values (20,30);

Warnings:

Warning	1364	Field 'rate' doesn't have a default value

*Note 3*: In case of nullable field `null_bit` is set to i-th bit of the i-th position of field in field_list.

*Note 4*: Only test I have found with embedded server is `funcs_1.is_triggers_embedded ` and is skipped

funcs_1.is_triggers_embedded ( skipped ) Bug#37456 funcs_1: Several tests crash when used with embedded server

Patch

Patch: bb-anel-MDEV-19761
Commit:2a8a4a532d76c840e
Buildbot: bb-10.1-anel-MDEV-19761

Comment by Sergei Golubchik [ 2023-07-21 ]

I don't understand. What is the reason for the bug? What is your commit trying to do? In particular, this loop looks suspicious:

    uint16 cnt=0;

    uchar *nptr;

    nptr= (uchar*)alloc_root(&table->mem_root, (table->s->fields - table->s->null_fields + 7)/8);

    // First find null_ptr for NULL field in case of mixed NULL and NOT NULL fields

    for (Field **f=field; *f; f++)

      if (table->field[cnt]->null_ptr)

        nptr= table->field[cnt]->null_ptr;

        break;

because you don't use f inside the loop and you don't change cnt inside the loop.

Generated at Thu Feb 08 08:54:10 UTC 2024 using Jira 8.20.16#820016-sha1:9d11dbea5f4be3d4cc21f03a88dd11d8c8687422.

[MDEV-19761] Before Trigger not processed for Not Null Columns Created: 2019-06-14 Updated: 2023-11-28