maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Aleksey!

First - is commit b97e45651d1 the one I was supposed to review?

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?

> +
>  [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?
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.

>  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

> +  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.

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