Hi! Here is the review of https://mariadb.atlassian.net/browse/MDEV-452 (Add full support for auto-initialized/updated timestamp and datetime) === modified file 'mysql-test/r/log_tables.result' --- mysql-test/r/log_tables.result 2011-10-19 19:45:18 +0000 +++ mysql-test/r/log_tables.result 2012-10-17 12:43:56 +0000 @@ -53,7 +53,7 @@ ERROR HY000: You can't use locks with lo show create table mysql.general_log; Table Create Table general_log CREATE TABLE `general_log` ( - `event_time` timestamp(6) NOT NULL DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP, + `event_time` timestamp(6) NOT NULL DEFAULT CURRENT_TIMESTAMP(6) ON UPDATE CURRENT_TIMESTAMP(6), Why does all log tables now have CURRENT_TIMESTAMP(6) instead of CURRENT_TIMESTAMP ? This would cause a problem for someone that updates to MySQL 10.0 and then want to go back... Many of the other result file changes comes from the same change for general_log and slow_log. Is this also the case for MySQL 5.6 or only for MariaDB? Answer from Timour on IRC: Timour> This comes from MySQL 5.6 and we need to stay compatbile. === modified file 'sql/event_db_repository.cc' --- sql/event_db_repository.cc 2012-09-04 16:26:30 +0000 +++ sql/event_db_repository.cc 2012-10-17 12:43:56 +0000 @@ -829,9 +829,6 @@ Event_db_repository::update_event(THD *t (int) table->field[ET_FIELD_ON_COMPLETION]->val_int())) goto end; - /* Don't update create on row update. */ - table->timestamp_field_type= TIMESTAMP_NO_AUTO_SET; - Why don't we anymore need to tell the rest of the code that the automatic fields shouldn't be updated from this function? Timour> As I remember, the main change compared to the old design is Timour> that we no longer update timestamps inside the handler Timour> So the place where timestamp updates are triggered is such Timour> that such updates will no longer be triggered in these cases. Yes, mysql_event_fill_row() doesn't call fill_record_n_invoke_before_triggers(). It instead has own code that probably not calling setting of any default fields. This does however mean that one can never get automatic timestamps with events. === modified file 'sql/field.cc' --- sql/field.cc 2012-09-08 22:22:06 +0000 +++ sql/field.cc 2012-10-17 12:43:56 +0000 @@ -4419,10 +4419,12 @@ Field_timestamp::Field_timestamp(uchar * { /* For 4.0 MYD and 4.0 InnoDB compatibility */ flags|= UNSIGNED_FLAG | BINARY_FLAG; - if (unireg_check != NONE && !share->timestamp_field) + if (unireg_check != NONE) { - /* This timestamp has auto-update */ - share->timestamp_field= this; + /* + This TIMESTAMP column is hereby quietly assumed to have an insert or + update default function. + */ We need to here add a comment what the TIMESTAMP_FLAG now means, as it has lost it's orginal meaning. Add to the above comment: /* We mark the flag with TIMESTAMP_FLAG to indicate to the client that this field will be automaticly updated on insert */ flags|= TIMESTAMP_FLAG; if (unireg_check != TIMESTAMP_DN_FIELD) flags|= ON_UPDATE_NOW_FLAG; Please also fix the function comment that refers to TABLE::timestamp_field <cut> +void Field_timestamp::set_explicit_default(Item *value) +{ + if (value && + ((value->type() == Item::DEFAULT_VALUE_ITEM && + !((Item_default_value*)value)->arg) || + (!maybe_null() && value->is_null()))) + return; + flags|= HAS_EXPLICIT_DEFAULT; +} The above function confused me greatly (mostly because of names) You should not have to check for value != 0. An item should never be 0. I noticed that some code called set_explicit_default() with NULL, but it would be better to for this add a general item function: field->set_explicit_default(NULL) -> field->mark_that_field_has_been_given_a_value() Where: Field::mark_that_field_has_been_given_a_value() { flags|= HAS_EXPLICIT_VALUE; } (Note name change for HAS_EXPLICIT_DEFAULT) This can be inline and we will win some function calls. The set_explicit_default() function also needs a comment as it's very confusing. To clarify the code, lets first add a function: /* Returns 1 if the default returns the default value for a column */ Item_default_value::default_column() { return arg != 0; } And then original function would look like: /** Mark columns that has been given a value with HAS_EXPLICIT_VALUE. For timestamp columns, the only case where a column is not marked with been given a value are: - It's explicitely assigned with DEFAULT - We assign NULL to a timestamp field that is defined as NOT NULL. This is how MySQL has worked since it's start. */ void Field_timestamp::set_explicit_default(Item *value) { if (((value->type() == Item::DEFAULT_VALUE_ITEM && !(Item_default_value*) value)->default_column()) || (!maybe_null() && value->is_null())) return; mark_that_field_has_been_given_a_value(); } <cut> + +int Field_datetime::set_time() +{ + THD *thd= current_thd; current_thd -> table->in_use; + MYSQL_TIME now_time; + thd->variables.time_zone->gmt_sec_to_TIME(&now_time, thd->query_start()); + now_time.second_part= thd->query_start_sec_part(); + set_notnull(); + store_TIME(&now_time); + thd->time_zone_used= 1; + return 0; +} @@ -8857,16 +8866,37 @@ bool Create_field::init(THD *thd, char * { uint sign_len, allowed_type_modifier= 0; ulong max_field_charlength= MAX_FIELD_CHARLENGTH; + const bool on_update_is_function= + (fld_on_update_value != NULL && + fld_on_update_value->type() == Item::FUNC_ITEM); DBUG_ENTER("Create_field::init()"); field= 0; field_name= fld_name; - def= fld_default_value; flags= fld_type_modifier; option_list= create_opt; - unireg_check= (fld_type_modifier & AUTO_INCREMENT_FLAG ? - Field::NEXT_NUMBER : Field::NONE); + + if (fld_default_value != NULL && fld_default_value->type() == Item::FUNC_ITEM) + { + /* There is a function default for insertions. */ + def= NULL; + unireg_check= on_update_is_function ? + Field::TIMESTAMP_DNUN_FIELD : // for insertions and for updates. + Field::TIMESTAMP_DN_FIELD; // only for insertions. Add braces around the on_update_is... to get better indentation. + } + else + { + /* No function default for insertions. Either NULL or a constant. */ + def= fld_default_value; + if (on_update_is_function) + unireg_check= Field::TIMESTAMP_UN_FIELD; // function default for updates + else + unireg_check= (fld_type_modifier & AUTO_INCREMENT_FLAG) != 0 ? + Field::NEXT_NUMBER : // Automatic increment. + Field::NONE; Add () around expression. + } + <cut> + + +/** + Mark the field as having an explicit default value. + + @param value if available, the value that the field is being set to + + @note + Fields that have an explicit default value should not be updated + automatically via the DEFAULT or ON UPDATE functions. The functions + that deal with data change functionality (INSERT/UPDATE/LOAD), + determine if there is an explicit value for each field before performing + the data change, and call this method to mark the field. + + If the 'value' parameter is NULL, then the field is marked unconditionally + as having an explicit value. If 'value' is not NULL, then it can be further + analyzed to check if it really should count as a value. +*/ + +void Field::set_explicit_default(Item *value) +{ /* Return if value is DEFAULT */ + if (value && value->type() == Item::DEFAULT_VALUE_ITEM && + !((Item_default_value*)value)->arg) + return; + flags|= HAS_EXPLICIT_DEFAULT; +} Remove checking if value is null. flags|= HAS_EXPLICIT_DEFAULT -> flags|= HAS_EXPLICIT_VALUE === modified file 'sql/field.h' @@ -1239,12 +1270,26 @@ class Field_timestamp :public Field_str virtual int set_time(); <cut> + virtual void set_explicit_default(Item *value); + virtual int evaluate_insert_default_function() + { + int res= 0; + if (has_insert_default_function()) + res= set_time(); + return res; + } + virtual int evaluate_update_default_function() + { + int res= 0; + if (has_update_default_function()) + res= set_time(); + return res; + } /* Get TIMESTAMP field value as seconds since begging of Unix Epoch */ virtual my_time_t get_timestamp(ulong *sec_part) const; virtual void store_TIME(my_time_t timestamp, ulong sec_part) @@ -1252,7 +1297,6 @@ class Field_timestamp :public Field_str int4store(ptr,timestamp); } bool get_date(MYSQL_TIME *ltime, ulonglong fuzzydate); - timestamp_auto_set_type get_auto_set_type() const; uchar *pack(uchar *to, const uchar *from, uint max_length __attribute__((unused))) { @@ -1503,6 +1547,28 @@ class Field_datetime :public Field_tempo void sql_type(String &str) const; bool zero_pack() const { return 1; } bool get_date(MYSQL_TIME *ltime, ulonglong fuzzydate); + virtual int set_time(); + virtual void set_default() + { + if (has_insert_default_function()) + set_time(); + else + Field::set_default(); + } + virtual int evaluate_insert_default_function() + { + int res= 0; + if (has_insert_default_function()) + res= set_time(); + return res; + } + virtual int evaluate_update_default_function() + { + int res= 0; + if (has_update_default_function()) + res= set_time(); + return res; + } It's a bit of a shame that we need to have the exact same functions twice? One way to solve it would be to add these functions to Field_str, as both Field_timestamp() and Field_datetime() depends on this class. === modified file 'sql/sql_base.cc' <cut> static bool -fill_record(THD * thd, List<Item> &fields, List<Item> &values, +fill_record(THD * thd, TABLE *table_arg, List<Item> &fields, List<Item> &values, bool ignore_errors) { Why this change (was not needed for this functionality). timour> I think I had a different implementation before, then I kept this timrou> change, because it looked simpler. bool -fill_record_n_invoke_before_triggers(THD *thd, List<Item> &fields, +fill_record_n_invoke_before_triggers(THD *thd, TABLE *table, List<Item> &fields, List<Item> &values, bool ignore_errors, - Table_triggers_list *triggers, enum trg_event_type event) { bool result; - result= (fill_record(thd, fields, values, ignore_errors) || + Table_triggers_list *triggers= table->triggers; + result= (fill_record(thd, table, fields, values, ignore_errors) || (triggers && triggers->process_triggers(thd, event, TRG_ACTION_BEFORE, TRUE))); /* @@ -8925,7 +8924,6 @@ fill_record_n_invoke_before_triggers(THD */ if (!result && triggers) { - TABLE *table= 0; List_iterator_fast<Item> f(fields); Item *fld; Item_field *item_field; @@ -8933,9 +8931,7 @@ fill_record_n_invoke_before_triggers(THD { fld= (Item_field*)f++; item_field= fld->filed_for_view_update(); - if (item_field && item_field->field && - (table= item_field->field->table) && - table->vfield) + if (item_field && item_field->field && table && table->vfield) result= update_virtual_fields(thd, table, TRUE); Add: DBUG_ASSERT(table == item_field->field->table) Note that if we can only have one table here, than we could also replace: if (!result && triggers) with: if (!result && triggers && table) As the loop will not do anything if table == 0. <cut> bool -fill_record_n_invoke_before_triggers(THD *thd, Field **ptr, +fill_record_n_invoke_before_triggers(THD *thd, TABLE *table, Field **ptr, List<Item> &values, bool ignore_errors, - Table_triggers_list *triggers, enum trg_event_type event) { bool result; - result= (fill_record(thd, ptr, values, ignore_errors, FALSE) || + Table_triggers_list *triggers= table->triggers; + result= (fill_record(thd, table, ptr, values, ignore_errors, FALSE) || (triggers && triggers->process_triggers(thd, event, TRG_ACTION_BEFORE, TRUE))); /* @@ -9071,7 +9064,6 @@ fill_record_n_invoke_before_triggers(THD */ if (!result && triggers && *ptr) { - TABLE *table= (*ptr)->table; Add DBUG_ASSERT(table == (*ptr)->table); if (table->vfield) result= update_virtual_fields(thd, table, TRUE); } <cut> === modified file 'sql/sql_insert.cc' @@ -1696,13 +1660,32 @@ int write_record(THD *thd, TABLE *table, restore_record(table,record[1]); DBUG_ASSERT(info->update_fields->elements == info->update_values->elements); - if (fill_record_n_invoke_before_triggers(thd, *info->update_fields, + if (fill_record_n_invoke_before_triggers(thd, table, *info->update_fields, *info->update_values, info->ignore, - table->triggers, TRG_EVENT_UPDATE)) goto before_trg_err; + bool different_records= (!records_are_comparable(table) || + compare_record(table)); + /* + Default fields must be updated before checking view updateability. + This branch of INSERT is executed only when a UNIQUE key was violated + with the ON DUPLICATE KEY UPDATE option. In this case the INSERT + operation is transformed to an UPDATE, and the default fields must + be updated as if this is an UPDATE. + */ + if (different_records && table->default_field) + { + bool res; + enum_sql_command cmd= thd->lex->sql_command; + thd->lex->sql_command= SQLCOM_UPDATE; + res= table->update_default_fields(); + thd->lex->sql_command= cmd; + if (res) + goto err; + } Another way to do the above nicer would be to give thd->lex->sql_command as an argument to update_default_fields(). Another option would be to have: update_defaults_fields(table) { return update_defaults_fields(table, table->in_use->lex->cmd); } And use the later in this case. However this is not that critical to fix... <cut> + Field **field,**org_field, *found_next_number_field, **dfield_ptr; You need to do dfield_ptr= 0 to avoid a compiler warning. @@ -2390,6 +2370,12 @@ TABLE *Delayed_insert::get_local_table(T bitmap= (uchar*) (field + share->fields + 1); copy->record[0]= (bitmap + share->column_bitmap_size*3); memcpy((char*) copy->record[0], (char*) table->record[0], share->reclength); + if (share->default_fields) + { + copy->default_field= (Field**) client_thd->alloc((share->default_fields+1)* + sizeof(Field**)); You should check for error condition here. + dfield_ptr= copy->default_field; + } /* Make a copy of all fields. The copied fields need to point into the copied record. This is done @@ -2407,18 +2393,19 @@ TABLE *Delayed_insert::get_local_table(T (*field)->move_field_offset(adjust_ptrs); // Point at copy->record[0] if (*org_field == found_next_number_field) (*field)->table->found_next_number_field= *field; + if (share->default_fields && + ((*org_field)->has_insert_default_function() || + (*org_field)->has_update_default_function())) + { + /* Put the newly copied field into the set of default fields. */ + *dfield_ptr= *field; + (*dfield_ptr)->unireg_check= (*org_field)->unireg_check; It's strange that new_field() resets unireg_check. Don't know why this is done and we should at some point fix that it's not reset and see what happens. + dfield_ptr++; + } } *field=0; <cut> @@ -850,7 +837,7 @@ read_fixed_length(THD *thd, COPY_INFO &i ER(ER_WARN_TOO_FEW_RECORDS), thd->warning_info->current_row_for_warning()); if (!field->maybe_null() && field->type() == FIELD_TYPE_TIMESTAMP) - ((Field_timestamp*) field)->set_time(); + field->set_time(); Shouldn't the above be done now by testing for if the field has an insert_default_function? @@ -994,10 +982,11 @@ read_sep_field(THD *thd, COPY_INFO &info DBUG_RETURN(1); } field->set_null(); + field->set_explicit_default(NULL); if (!field->maybe_null()) { if (field->type() == MYSQL_TYPE_TIMESTAMP) - ((Field_timestamp*) field)->set_time(); + field->set_time(); shouldn't we use an insert_default_function here too ? else if (field != table->next_number_field) field->set_warning(MYSQL_ERROR::WARN_LEVEL_WARN, ER_WARN_NULL_TO_NOTNULL, 1); @@ -1066,7 +1056,7 @@ read_sep_field(THD *thd, COPY_INFO &info DBUG_RETURN(1); } if (!field->maybe_null() && field->type() == FIELD_TYPE_TIMESTAMP) - ((Field_timestamp*) field)->set_time(); + field->set_time(); shouldn't we use an insert_default_function here too ? /* TODO: We probably should not throw warning for each field. But how about intention to always have the same number @@ -1206,7 +1196,7 @@ read_xml_field(THD *thd, COPY_INFO &info if (!field->maybe_null()) { if (field->type() == FIELD_TYPE_TIMESTAMP) - ((Field_timestamp *) field)->set_time(); + field->set_time(); shouldn't we use an insert_default_function here too ? === modified file 'sql/sql_parse.cc' --- sql/sql_parse.cc 2012-10-06 07:30:52 +0000 +++ sql/sql_parse.cc 2012-10-18 12:57:12 +0000 @@ -269,12 +269,14 @@ void init_update_queries(void) CF_CAN_GENERATE_ROW_EVENTS; sql_command_flags[SQLCOM_CREATE_INDEX]= CF_CHANGES_DATA | CF_AUTO_COMMIT_TRANS; sql_command_flags[SQLCOM_ALTER_TABLE]= CF_CHANGES_DATA | CF_WRITE_LOGS_COMMAND | - CF_AUTO_COMMIT_TRANS | CF_REPORT_PROGRESS ; + CF_AUTO_COMMIT_TRANS | CF_REPORT_PROGRESS | + CF_INSERTS_DATA; Should we really have CF_INSERTS_DATA here? We should not update any timestamps for ALTER TABLE? === modified file 'sql/table.cc' @@ -2472,7 +2471,7 @@ int open_table_from_share(THD *thd, TABL } /* - Process virtual columns, if any. + Process virtual and default columns, if any. */ if (!share->vfields) outparam->vfield= NULL; @@ -2484,10 +2483,26 @@ int open_table_from_share(THD *thd, TABL goto err; outparam->vfield= vfield_ptr; + } + + if (!share->default_fields) + outparam->default_field= NULL; You don't need to set default_fields to NULL as outparam is zeroed in beginning of the function. (Same is true for outparam->vfield) <cut> Regards, Monty