Description:
When binlog_format is set to 'row ' and binlog_row_image is set to 'FULL', all bits in table->write_set will be set to 1 to make true every field is going to be written to the binlog. However, the time when they are set is too early in the function mark_columns_per_binlog_row_image() which is invoked by prepare_triggers_for_insert_stmt() in execute_inner() during inserting a record. If the insert-list does not contain all columns of the table, the default values of not-contained columns' will be checked in validate_default_values_of_unset_fields() in execute_inner(). Unfortunately this function is executed after prepare_triggers_for_insert_stmt() in which all bits in write_set have been modified to 1.
The validate_default_values_of_unset_fields function looks like this:
bool validate_default_values_of_unset_fields(THD *thd, TABLE *table) {
MY_BITMAP *write_set = table->write_set;
DBUG_ENTER("validate_default_values_of_unset_fields");
for (Field **field = table->field; *field; field++) {
if (!bitmap_is_set(write_set, (*field)->field_index) &&
!((*field)->flags & NO_DEFAULT_VALUE_FLAG)) {
if ((*field)->validate_stored_val(thd) && thd->is_error())
DBUG_RETURN(true);
}
}
DBUG_RETURN(false);
}
‘!bitmap_is_set(write_set, (*field)->field_index)’ means each field that is not in the write_set should be validated whether its default value is legal. But, the write_set was modified earlier! As a result, this validation will be skipped!
How to repeat:
set session binlog_format='ROW';
set session binlog_row_image='FULL';
set sql_mode='';
create table test (mydate DATE NOT NULL DEFAULT '0000-00-00');
set sql_mode=default;
insert into test values();
Suggested fix:
I think of 3 ways to fix this problem.
1.We can put the default value validation before the write_set is modified
2.Do not use table->write_set for binlog writing, create and use another one.
3.Before modifying the write_set, copy it to another bitmap. And use this new bit map to validate default values of not-contained columns.
Here is the third way's patch：
diff --git a/sql/sql_insert.cc b/sql/sql_insert.cc
index 8053104bedf..cbdce10c0d6 100644
--- a/sql/sql_insert.cc
+++ b/sql/sql_insert.cc
@@ -292,8 +292,11 @@ static bool check_valid_table_refs(const TABLE_LIST *view, List<Item> &values,
@returns false if success, true if error
*/
-bool validate_default_values_of_unset_fields(THD *thd, TABLE *table) {
+bool validate_default_values_of_unset_fields(THD *thd, TABLE *table, bool use_default_value_check_set) {
MY_BITMAP *write_set = table->write_set;
+ if (use_default_value_check_set) {
+ write_set = &table->default_value_check_set;
+ }
DBUG_ENTER("validate_default_values_of_unset_fields");
for (Field **field = table->field; *field; field++) {
@@ -560,7 +563,7 @@ bool Sql_cmd_insert_values::execute_inner(THD *thd) {
Check whether default values of the insert_field_list not specified in
column list are correct or not.
*/
- if (validate_default_values_of_unset_fields(thd, insert_table)) {
+ if (validate_default_values_of_unset_fields(thd, insert_table, insert_table->is_write_set_changed)) {
has_error = true;
break;
}
@@ -616,6 +619,9 @@ bool Sql_cmd_insert_values::execute_inner(THD *thd) {
}
thd->get_stmt_da()->inc_current_row_for_condition();
}
+ /* clear default_value_check_set for next use and reset is_write_set_changed*/
+ bitmap_clear_all(&insert_table->default_value_check_set);
+ insert_table->is_write_set_changed = false;
} // Statement plan is available within these braces
DBUG_ASSERT(has_error == thd->get_stmt_da()->is_error());
diff --git a/sql/sql_insert.h b/sql/sql_insert.h
index 01f3ba8ffb3..4a4682a20e4 100644
--- a/sql/sql_insert.h
+++ b/sql/sql_insert.h
@@ -50,7 +50,7 @@ bool check_that_all_fields_are_given_values(THD *thd, TABLE *entry,
TABLE_LIST *table_list);
void prepare_triggers_for_insert_stmt(THD *thd, TABLE *table);
bool write_record(THD *thd, TABLE *table, COPY_INFO *info, COPY_INFO *update);
-bool validate_default_values_of_unset_fields(THD *thd, TABLE *table);
+bool validate_default_values_of_unset_fields(THD *thd, TABLE *table, bool use_default_value_check_set = false);
class Query_result_insert : public Query_result_interceptor {
public:
diff --git a/sql/table.cc b/sql/table.cc
index d89e5ba1328..a22b61a37c2 100644
--- a/sql/table.cc
+++ b/sql/table.cc
@@ -3001,7 +3001,7 @@ int open_table_from_share(THD *thd, TABLE_SHARE *share, const char *alias,
*/
bitmap_size = share->column_bitmap_size;
- if (!(bitmaps = (uchar *)alloc_root(root, bitmap_size * 7))) goto err;
+ if (!(bitmaps = (uchar *)alloc_root(root, bitmap_size * 8))) goto err;
bitmap_init(&outparam->def_read_set, (my_bitmap_map *)bitmaps, share->fields,
false);
bitmap_init(&outparam->def_write_set,
@@ -3019,6 +3019,9 @@ int open_table_from_share(THD *thd, TABLE_SHARE *share, const char *alias,
bitmap_init(&outparam->pack_row_tmp_set,
(my_bitmap_map *)(bitmaps + bitmap_size * 6), share->fields,
false);
+ bitmap_init(&outparam->default_value_check_set,
+ (my_bitmap_map *)(bitmaps + bitmap_size * 7), share->fields,
+ false);
outparam->default_column_bitmaps();
/*
@@ -5756,6 +5759,9 @@ void TABLE::mark_columns_per_binlog_row_image(THD *thd) {
switch (thd->variables.binlog_row_image) {
case BINLOG_ROW_IMAGE_FULL:
if (s->primary_key < MAX_KEY) bitmap_set_all(read_set);
+ /* copy write_set to default_value_check_set for later default value validation */
+ bitmap_copy(&default_value_check_set, write_set);
+ is_write_set_changed = true;
bitmap_set_all(write_set);
break;
case BINLOG_ROW_IMAGE_NOBLOB:
diff --git a/sql/table.h b/sql/table.h
index c03b3b33a2a..1cc4fc25582 100644
--- a/sql/table.h
+++ b/sql/table.h
@@ -1391,7 +1391,7 @@ struct TABLE {
nullptr}; ///< Saved null_flags while null_row is true
/* containers */
- MY_BITMAP def_read_set, def_write_set, tmp_set, pack_row_tmp_set;
+ MY_BITMAP def_read_set, def_write_set, tmp_set, pack_row_tmp_set, default_value_check_set;
/*
Bitmap of fields that one or more query condition refers to. Only
used if optimizer_condition_fanout_filter is turned 'on'.
@@ -1495,6 +1495,8 @@ struct TABLE {
bool copy_blobs{false}; /* copy_blobs when storing */
+ bool is_write_set_changed{false}; /* write_set is changed when binlog_row_image=FULL */
+
/*
TODO: Each of the following flags take up 8 bits. They can just as easily
be put into one single unsigned long and instead of taking up 18
Description: When binlog_format is set to 'row ' and binlog_row_image is set to 'FULL', all bits in table->write_set will be set to 1 to make true every field is going to be written to the binlog. However, the time when they are set is too early in the function mark_columns_per_binlog_row_image() which is invoked by prepare_triggers_for_insert_stmt() in execute_inner() during inserting a record. If the insert-list does not contain all columns of the table, the default values of not-contained columns' will be checked in validate_default_values_of_unset_fields() in execute_inner(). Unfortunately this function is executed after prepare_triggers_for_insert_stmt() in which all bits in write_set have been modified to 1. The validate_default_values_of_unset_fields function looks like this: bool validate_default_values_of_unset_fields(THD *thd, TABLE *table) { MY_BITMAP *write_set = table->write_set; DBUG_ENTER("validate_default_values_of_unset_fields"); for (Field **field = table->field; *field; field++) { if (!bitmap_is_set(write_set, (*field)->field_index) && !((*field)->flags & NO_DEFAULT_VALUE_FLAG)) { if ((*field)->validate_stored_val(thd) && thd->is_error()) DBUG_RETURN(true); } } DBUG_RETURN(false); } ‘!bitmap_is_set(write_set, (*field)->field_index)’ means each field that is not in the write_set should be validated whether its default value is legal. But, the write_set was modified earlier! As a result, this validation will be skipped! How to repeat: set session binlog_format='ROW'; set session binlog_row_image='FULL'; set sql_mode=''; create table test (mydate DATE NOT NULL DEFAULT '0000-00-00'); set sql_mode=default; insert into test values(); Suggested fix: I think of 3 ways to fix this problem. 1.We can put the default value validation before the write_set is modified 2.Do not use table->write_set for binlog writing, create and use another one. 3.Before modifying the write_set, copy it to another bitmap. And use this new bit map to validate default values of not-contained columns. Here is the third way's patch： diff --git a/sql/sql_insert.cc b/sql/sql_insert.cc index 8053104bedf..cbdce10c0d6 100644 --- a/sql/sql_insert.cc +++ b/sql/sql_insert.cc @@ -292,8 +292,11 @@ static bool check_valid_table_refs(const TABLE_LIST *view, List<Item> &values, @returns false if success, true if error */ -bool validate_default_values_of_unset_fields(THD *thd, TABLE *table) { +bool validate_default_values_of_unset_fields(THD *thd, TABLE *table, bool use_default_value_check_set) { MY_BITMAP *write_set = table->write_set; + if (use_default_value_check_set) { + write_set = &table->default_value_check_set; + } DBUG_ENTER("validate_default_values_of_unset_fields"); for (Field **field = table->field; *field; field++) { @@ -560,7 +563,7 @@ bool Sql_cmd_insert_values::execute_inner(THD *thd) { Check whether default values of the insert_field_list not specified in column list are correct or not. */ - if (validate_default_values_of_unset_fields(thd, insert_table)) { + if (validate_default_values_of_unset_fields(thd, insert_table, insert_table->is_write_set_changed)) { has_error = true; break; } @@ -616,6 +619,9 @@ bool Sql_cmd_insert_values::execute_inner(THD *thd) { } thd->get_stmt_da()->inc_current_row_for_condition(); } + /* clear default_value_check_set for next use and reset is_write_set_changed*/ + bitmap_clear_all(&insert_table->default_value_check_set); + insert_table->is_write_set_changed = false; } // Statement plan is available within these braces DBUG_ASSERT(has_error == thd->get_stmt_da()->is_error()); diff --git a/sql/sql_insert.h b/sql/sql_insert.h index 01f3ba8ffb3..4a4682a20e4 100644 --- a/sql/sql_insert.h +++ b/sql/sql_insert.h @@ -50,7 +50,7 @@ bool check_that_all_fields_are_given_values(THD *thd, TABLE *entry, TABLE_LIST *table_list); void prepare_triggers_for_insert_stmt(THD *thd, TABLE *table); bool write_record(THD *thd, TABLE *table, COPY_INFO *info, COPY_INFO *update); -bool validate_default_values_of_unset_fields(THD *thd, TABLE *table); +bool validate_default_values_of_unset_fields(THD *thd, TABLE *table, bool use_default_value_check_set = false); class Query_result_insert : public Query_result_interceptor { public: diff --git a/sql/table.cc b/sql/table.cc index d89e5ba1328..a22b61a37c2 100644 --- a/sql/table.cc +++ b/sql/table.cc @@ -3001,7 +3001,7 @@ int open_table_from_share(THD *thd, TABLE_SHARE *share, const char *alias, */ bitmap_size = share->column_bitmap_size; - if (!(bitmaps = (uchar *)alloc_root(root, bitmap_size * 7))) goto err; + if (!(bitmaps = (uchar *)alloc_root(root, bitmap_size * 8))) goto err; bitmap_init(&outparam->def_read_set, (my_bitmap_map *)bitmaps, share->fields, false); bitmap_init(&outparam->def_write_set, @@ -3019,6 +3019,9 @@ int open_table_from_share(THD *thd, TABLE_SHARE *share, const char *alias, bitmap_init(&outparam->pack_row_tmp_set, (my_bitmap_map *)(bitmaps + bitmap_size * 6), share->fields, false); + bitmap_init(&outparam->default_value_check_set, + (my_bitmap_map *)(bitmaps + bitmap_size * 7), share->fields, + false); outparam->default_column_bitmaps(); /* @@ -5756,6 +5759,9 @@ void TABLE::mark_columns_per_binlog_row_image(THD *thd) { switch (thd->variables.binlog_row_image) { case BINLOG_ROW_IMAGE_FULL: if (s->primary_key < MAX_KEY) bitmap_set_all(read_set); + /* copy write_set to default_value_check_set for later default value validation */ + bitmap_copy(&default_value_check_set, write_set); + is_write_set_changed = true; bitmap_set_all(write_set); break; case BINLOG_ROW_IMAGE_NOBLOB: diff --git a/sql/table.h b/sql/table.h index c03b3b33a2a..1cc4fc25582 100644 --- a/sql/table.h +++ b/sql/table.h @@ -1391,7 +1391,7 @@ struct TABLE { nullptr}; ///< Saved null_flags while null_row is true /* containers */ - MY_BITMAP def_read_set, def_write_set, tmp_set, pack_row_tmp_set; + MY_BITMAP def_read_set, def_write_set, tmp_set, pack_row_tmp_set, default_value_check_set; /* Bitmap of fields that one or more query condition refers to. Only used if optimizer_condition_fanout_filter is turned 'on'. @@ -1495,6 +1495,8 @@ struct TABLE { bool copy_blobs{false}; /* copy_blobs when storing */ + bool is_write_set_changed{false}; /* write_set is changed when binlog_row_image=FULL */ + /* TODO: Each of the following flags take up 8 bits. They can just as easily be put into one single unsigned long and instead of taking up 18