maria-developers team mailing list archive
-
maria-developers team
-
Mailing list archive
-
Message #04961
Review of mdev-452
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
