Bug #96393 binlog_fomat=row and binlog_row_image=FULL leads to DATE validation failure
Submitted: 1 Aug 3:35 Modified: 5 Aug 12:49
Reporter: Dehao Wang (OCA) Email Updates:
Status: Verified Impact on me:
None 
Category:MySQL Server: DML Severity:S3 (Non-critical)
Version:8.0.16, 8.0.17, 5.7.27 OS:Any
Assigned to: CPU Architecture:Any
Tags: Contribution

[1 Aug 3:35] Dehao Wang
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
[1 Aug 5:45] Umesh Shastry
Hello Dehao Wang,

Thank you for the report.

regards,
Umesh
[1 Aug 5:47] Umesh Shastry
Please note that in order to submit contributions you must first sign the Oracle Contribution Agreement (OCA). More details are described in "Contributions" tab of this page, please ensure to re-send the patch via that tab. Otherwise we would not be able to accept it.
[1 Aug 7:41] Dehao Wang
Thank you for reminding me of that. I have signed OCA and sent it to the Email address left on the website just now.
[5 Aug 12:47] Dehao Wang
a alternative patch to fix this problem

(*) I confirm the code being submitted is offered under the terms of the OCA, and that I am authorized to contribute it.

Contribution: sql_insert.diff (application/octet-stream, text), 4.70 KiB.

[5 Aug 12:49] Dehao Wang
Hi, My OCA has been approved and I just submitted my patch in the "Contributions" tab.
[5 Aug 12:51] Umesh Shastry
Thank you, Dehao Wang.