maria-developers team mailing list archive

[Alexander Barkov <bar@xxxxxxxxxxx>]

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