Re: [Maria-developers] b97e45651d1: MDEV-16937 Strict SQL with system versioned tables causes issues
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@gmail.com> committer: Aleksey Midenkov <midenok@gmail.com> 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@mariadb.org
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@mariadb.org> 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@gmail.com> committer: Aleksey Midenkov <midenok@gmail.com> 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
Hi, Aleksey! On May 19, Aleksey Midenkov wrote:
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
what value of --parallel did you use? with --mem or without?
IMO not much duration difference. OTOH testing everything with sql-mode=traditional looks useful.
Okay, as you like. It was just a suggestion, our test suite is quite slow as it is. But a couple of seconds is fine if they add enough value.
[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
Sorry, missed that. I did look :( Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
Sergei, On Tue, May 19, 2020 at 1:52 PM Sergei Golubchik <serg@mariadb.org> wrote:
Hi, Aleksey!
On May 19, Aleksey Midenkov wrote:
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
what value of --parallel did you use? with --mem or without?
With --parallel=4; without --mem. Here are the numbers without both options: real 2m19.709s user 0m42.374s sys 0m17.671s real 2m21.355s user 0m45.825s sys 0m18.726s
IMO not much duration difference. OTOH testing everything with sql-mode=traditional looks useful.
Okay, as you like. It was just a suggestion, our test suite is quite slow as it is. But a couple of seconds is fine if they add enough value.
[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
Sorry, missed that. I did look :(
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
-- All the best, Aleksey Midenkov @midenok
participants (2)
-
Aleksey Midenkov -
Sergei Golubchik