← Back to team overview

maria-developers team mailing list archive

Re: Please review: MDEV-7824 [Bug #68041] Zero date can be inserted in strict no-zero mode through a default value


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

and is a blocker for:

MDEV-3929 Add full support for auto-initialized/updated timestamp and

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?



Follow ups