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 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 committer: Aleksey Midenkov 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@mariadb.org

-- All the best, Aleksey Midenkov @midenok