Hi Sergei, Thanks for review! See my comments inline: On 05/07/2015 06:57 PM, Sergei Golubchik wrote:
Hi, Alexander!
On Mar 25, Alexander Barkov wrote:
Hi Sergei,
Please review a patch for mdev-7824.
It's based on a MySQL patch for http://bugs.mysql.com/bug.php?id=68041
and is a blocker for:
MDEV-3929 Add full support for auto-initialized/updated timestamp and datetime
That looks good.
But I don't like it that you verify defaults for every row. It's not very cheap. And neither defaults nor sql_mode can change in the duration of one statement. It should be enough to test only once.
This is a good idea. I copied the patch from MySQL. So it will be nice to have this implemented in MariaDB in a faster way.
I'm not sure, though, how to do that with a minimal overhead. A couple of bool's in TABLE, like
bool all_default_are_checked; bool write_set_defaults_are_checked;
that are set to false at the beginning on INSERT,INSERT...SELECT,LOAD. And used like that
bool TABLE::restore_record_and_validate_unset_fields(THD *thd, bool all) { restore_record(this, s->default_values); if (all ? all_default_are_checked : write_set_defaults_are_checked) return false;
if (all) all_default_are_checked= true; else write_set_defaults_are_checked= true;
return validate_default_values_of_unset_fields(thd); }
and in mysql_insert():
if (fields.elements || !value_count) { if (table->restore_record_and_validate_unset_fields(thd, !value_count))
Sorry, I did't understand your idea about having two separate flags for "all" and "write_set". Is it for the cases when one mixes empty and non-empty parenthesized value lists, like these: insert into (a,b) t1 values (),(10,20); insert into (a,b) t1 values (10,20),(); ??? Note, this currently does not work and return this error: ERROR 1136 (21S01): Column count doesn't match value count at row 2 Looks like a bug. I guess this could work. If so, would you like me to report and fix it before mdev-7824? Thanks.
Regards, Sergei