maria-developers team mailing list archive

[Aleksey Midenkov <midenok@xxxxxxxxx>]

 Hi, Sergei!

The patches with your comments applied are:

e8ad2360d82b5652b23360dc7ed78c4a276ea514 (bb-10.3-midenok)
9aea660d7078143a6d3e50106136bd380c188ff0 (bb-10.4-midenok2)

On Sun, May 17, 2020 at 11:59 AM Sergei Golubchik <serg@xxxxxxxxxxx> wrote:
>
> Hi, Aleksey!
>
> First - is commit b97e45651d1 the one I was supposed to review?

Yes.

>
> On May 17, Aleksey Midenkov wrote:
> > revision-id: b97e45651d1 (mariadb-10.4.7-33-gb97e45651d1)
> > parent(s): 7587975bf06
> > author: Aleksey Midenkov <midenok@xxxxxxxxx>
> > committer: Aleksey Midenkov <midenok@xxxxxxxxx>
> > timestamp: 2019-08-19 11:58:07 +0300
> > message:
> >
> > MDEV-16937 Strict SQL with system versioned tables causes issues
> >
> > Vers SQL: respect system fields in NO_ZERO_DATE mode.
> >
> > This is the subject for refactoring in tempesta-tech/mariadb#379
> >
> > diff --git a/mysql-test/suite/versioning/engines.combinations b/mysql-test/suite/versioning/engines.combinations
> > index 26b5bab23f1..57e2af6cd06 100644
> > --- a/mysql-test/suite/versioning/engines.combinations
> > +++ b/mysql-test/suite/versioning/engines.combinations
> > @@ -7,5 +7,10 @@ default-storage-engine=innodb
> >  [myisam]
> >  default-storage-engine=myisam
> >
> > +[traditional]
> > +default-storage-engine=myisam
> > +sql-mode=traditional
>
> I'm not sure about it. It's, of course, safe to run all tests with every
> possible settings, but it really blows up testing times.
>
> May be it'd make sense to have a more targeted test here?

Versioning suite without "traditional":

real    0m32.805s
user    0m44.173s
sys     0m17.346s

With "traditional":

real    0m34.107s
user    0m50.038s
sys     0m20.001s

IMO not much duration difference. OTOH testing everything with
sql-mode=traditional looks useful.

>
> > +
> >  [heap]
> >  default-storage-engine=memory
> > +
> > diff --git a/mysql-test/suite/versioning/r/select.result b/mysql-test/suite/versioning/r/select.result
> > index 3569268ce1d..68df246af6b 100644
> > --- a/mysql-test/suite/versioning/r/select.result
> > +++ b/mysql-test/suite/versioning/r/select.result
> > @@ -45,7 +45,7 @@ ASOF_x      y
> >  7    107
> >  8    108
> >  9    109
> > -select x as FROMTO_x, y from t1 for system_time from timestamp '0-0-0 0:0:0' to timestamp @t1;
> > +select x as FROMTO_x, y from t1 for system_time from timestamp '1970-01-01 00:00:00' to timestamp @t1;
>
> Does this test use a fixed time zone?

Yes, this test uses fixed time zone +00:00 set in common.inc

> If not, is 1970-01-01 00:00:00 a valid timestamp in all time zones?
> If unsure, better use 1971, that will definitely be fine everywhere.

Ok, got that hint.

>
> >  FROMTO_x     y
> >  0    100
> >  1    101
> > diff --git a/sql/field.cc b/sql/field.cc
> > index 969c32a5180..f5580239a9f 100644
> > --- a/sql/field.cc
> > +++ b/sql/field.cc
> > @@ -11139,6 +11139,13 @@ bool Field::save_in_field_default_value(bool view_error_processing)
> >  {
> >    THD *thd= table->in_use;
> >
> > +  /*
> > +     TODO: refactor setting the system fields via default_value mechanism.
> > +     This condition will go away as well as other conditions with VERS_SYSTEM_FIELD.
> > +  */
>
> MDEV?
> There's a pull request, but it is marked as obsolete

Updated comment to:
MDEV-19597 Refactor TABLE::vers_update_fields() via stored virtual columns

>
> > +  if (flags & VERS_SYSTEM_FIELD)
> > +    return false;
> > +
> >    if (unlikely(flags & NO_DEFAULT_VALUE_FLAG &&
> >                 real_type() != MYSQL_TYPE_ENUM))
> >    {
> > diff --git a/sql/unireg.cc b/sql/unireg.cc
> > index d019b5f8a75..484ad0334e4 100644
> > --- a/sql/unireg.cc
> > +++ b/sql/unireg.cc
> > @@ -1060,7 +1060,8 @@ static bool make_empty_rec(THD *thd, uchar *buff, uint table_options,
> >          !f_bit_as_char(field->pack_flag))
> >        null_count+= field->length & 7;
> >
> > -    error= make_empty_rec_store_default(thd, regfield, field->default_value);
> > +    if (!(field->flags & VERS_SYSTEM_FIELD))
> > +      error= make_empty_rec_store_default(thd, regfield, field->default_value);
>
> will the field be initialized at all? It should have something, even
> bzero will do, but we shouldn't dump uninited memory to frm.

Fixed.

>
> >      delete regfield; // Avoid memory leaks
> >      if (error)
> >        goto err;
> >
> Regards,
> Sergei
> VP of MariaDB Server Engineering
> and security@xxxxxxxxxxx



-- 
All the best,

Aleksey Midenkov
@midenok