- developers - lists.mariadb.org

[Maria-developers] Question about JSONPath and '**' wildcard
by Sergey Petrunia 20 Nov '20

20 Nov '20

Hi Alexey, I've found this discrepancy in JSONPath evaluation: set @json_doc3 = ' { "root": { "child1" : { "child2" : { "child1" : { "x":124 } } } } } '; select json_extract(@json_doc3, '$**.child1**.x'); MariaDB [test]> select json_extract(@json_doc3, '$**.child1**.x'); +--------------------------------------------+ | json_extract(@json_doc3, '$**.child1**.x') | +--------------------------------------------+ | NULL | +--------------------------------------------+ 1 row in set (0.001 sec) MySQL-8> select json_extract(@json_doc3, '$**.child1**.x'); +--------------------------------------------+ | json_extract(@json_doc3, '$**.child1**.x') | +--------------------------------------------+ | [124] | +--------------------------------------------+ 1 row in set (0.00 sec) Which one is right? My opinion is that MariaDB's answer is definitely incorrect. As for MySQL's answer, it depends on how the result of JSONPath expression is defined. If it is "a set of nodes in the JSON document which match the pattern", MySQL's result is correct. Coming from XPath world, I was expecting a semantics in form find the set of nodes matching the search step#1 ( $**, then apply step #2 ( .child1) then apply step #3 ( any children of those) ... in which case "124" would be in the result twice as it is reachable via two possible paths. Any thoughts about this? BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog

2 2

Re: [Maria-developers] 255e5f269d80: MDEV-23610: Slave user can't run "SHOW SLAVE STATUS" anymore after upgrade to 10.5, mysql_upgrade should take of that
by Sergei Golubchik 13 Nov '20

13 Nov '20

Hi, Sujatha! Looks good, thanks! One detail to fix - I think you've mistakenly committed an old test from the previous version of the patch. And two minor suggestions, you can change or not, as you like. otherwise ok to push On Nov 13, Sujatha wrote: > revision-id: 255e5f269d80 (mariadb-10.5.4-319-g255e5f269d80) > parent(s): 97569d3c37a6 > author: Sujatha <sujatha.sivakumar(a)mariadb.com> > committer: Sujatha <sujatha.sivakumar(a)mariadb.com> > timestamp: 2020-11-13 19:08:02 +0530 > message: > > MDEV-23610: Slave user can't run "SHOW SLAVE STATUS" anymore after upgrade to 10.5, mysql_upgrade should take of that > > Add a new privilege "SLAVE MONITOR" which will grant user the permission > to execute "SHOW SLAVE STATUS" and "SHOW RELAYLOG EVENTS" commands. > > SHOW SLAVE STATUS requires either SLAVE MONITOR/SUPER > SHOW RELAYLOG EVENTS requires SLAVE MONITOR privilege. > > diff --git a/mysql-test/main/grant_slave_monitor.test b/mysql-test/main/grant_slave_monitor.test > new file mode 100644 > index 000000000000..a4f21085e537 > --- /dev/null > +++ b/mysql-test/main/grant_slave_monitor.test > @@ -0,0 +1,101 @@ > +# ==== Purpose ==== > +# > +# SLAVE MONITOR privilege is required to execute following commands. > +# SHOW SLAVE STATUS > +# SHOW RELAYLOG EVENTS > +# > +# ==== Implementation ==== > +# > +# Step1: GRANT ALL privileges for a new user 'user1' and then REVOKE > +# SLAVE MONITOR and SUPER privileges. > +# Step2: Execute SHOW SLAVE STAUTS/SHOW RELAYLOG EVENTS commands and expect > +# ER_SPECIFIC_ACCESS_DENIED_ERROR. This also verifies that REPLICATION > +# SLAVE ADMIN privilege is not required for these two commands. > +# Step3: GRANT SLAVE MONITOR privilege and observe that both commands are > +# allowd to execute. > +# Step4: GRANT SUPER privilege and observe that only SHOW SLAVE STATUS command > +# is allowed. > +# > +# ==== References ==== > +# > +# MDEV-23610: Slave user can't run "SHOW SLAVE STATUS" anymore after upgrade > +# to 10.5, mysql_upgrade should take of that > +# MDEV-23918: admin privlege required to view contents of relay logs in 10.5 > +# > + > +--source include/not_embedded.inc > + > +CREATE USER user1@localhost IDENTIFIED BY ''; > +GRANT ALL PRIVILEGES ON *.* TO user1@localhost; > +REVOKE SLAVE MONITOR, SUPER ON *.* FROM user1@localhost; > +FLUSH PRIVILEGES; > + > +--connect(con1,localhost,user1,,) > +--connection con1 > +SHOW GRANTS; > + > +--echo # > +--echo # Verify that having REPLICATION SLAVE ADMIN doesn't allow SHOW SLAVE STATUS > +--echo # Expected error: Access denied; you need (at least one of) the SUPER, SLAVE > +--echo # MONITOR privilege(s) for this operation > +--echo # > +--error ER_SPECIFIC_ACCESS_DENIED_ERROR > +SHOW SLAVE STATUS; > + > +--echo # > +--echo # Verify that having REPLICATION SLAVE ADMIN doesn't allow SHOW RELAYLOG EVENTS > +--echo # Expected error: Access denied; you need (at least one of) the REPLICA MONITOR > +--echo # privilege(s) for this operation > +--echo # > +--disable_ps_protocol > +--error ER_SPECIFIC_ACCESS_DENIED_ERROR > +SHOW RELAYLOG EVENTS; > +--enable_ps_protocol > +--disconnect con1 > + > +--echo # > +--echo # SHOW SLAVE STATUS and SHOW RELAYLOG EVENTS are allowed with SLAVE MONITOR privilege > +--echo # > + > +--connection default > +GRANT SLAVE MONITOR ON *.* TO user1@localhost; > +FLUSH PRIVILEGES; > + > +--connect(con1,localhost,user1,,) > +--connection con1 > +SHOW GRANTS; > + > +--disable_result_log > +SHOW SLAVE STATUS; > +#--disable_ps_protocol > +SHOW RELAYLOG EVENTS; > +#--enable_ps_protocol > +--enable_result_log > +--disconnect con1 > + > +--connection default > +DROP USER user1@localhost; > + > +--echo # > +--echo # SHOW SLAVE STATUS command is allowed with SUPER privilege > +--echo # > +CREATE USER user1@localhost IDENTIFIED BY ''; > +GRANT SUPER ON *.* TO user1@localhost; > + > +--connect(con1,localhost,user1,,) > +--disable_result_log > +SHOW SLAVE STATUS; > +--enable_result_log > + > +--echo # > +--echo # SHOW RELAYLOG EVENTS is not allowed with SUPER privilege, it requires SLAVE MONITOR > +--echo # > + > +--disable_ps_protocol > +--error ER_SPECIFIC_ACCESS_DENIED_ERROR > +SHOW RELAYLOG EVENTS; > +--enable_ps_protocol > +--disconnect con1 > + > +--connection default > +DROP USER user1@localhost; > diff --git a/mysql-test/main/rpl_replica_monitor_priv.test b/mysql-test/main/rpl_replica_monitor_priv.test > --- /dev/null > +++ b/mysql-test/main/rpl_replica_monitor_priv.test > @@ -0,0 +1,90 @@ Did you check in this file by mistake? It's a replication test (source include/master-slave.inc) but not in the rpl suite. It tests the feature that does not need master-slave.inc. And it's duplicated by grant_slave_monitor.test > diff --git a/sql/privilege.h b/sql/privilege.h > index 37cdf4da01ad..d0e6e532b4b5 100644 > --- a/sql/privilege.h > +++ b/sql/privilege.h > @@ -93,13 +94,18 @@ enum privilege_t: unsigned long long > */ > }; > > +constexpr static inline privilege_t ALL_KNOWN_BITS(privilege_t x) > +{ > + return (privilege_t)((x << 1) - 1); Generally it's safer to write x | (x - 1) it won't overflow if x is already the highest possible bit. In privileges we're quite far from it at the moment, so it's not a big deal, just a style issue and setting a good example to others. > +} > > // Version markers > constexpr privilege_t LAST_100304_ACL= DELETE_HISTORY_ACL; > constexpr privilege_t LAST_100502_ACL= BINLOG_REPLAY_ACL; > +constexpr privilege_t LAST_100509_ACL= SLAVE_MONITOR_ACL; > > diff --git a/sql/sql_acl.cc b/sql/sql_acl.cc > index ef96e8f24843..b5b1472f004f 100644 > --- a/sql/sql_acl.cc > +++ b/sql/sql_acl.cc > @@ -1015,7 +1015,7 @@ class User_table_tabular: public User_table > access|= LOCK_TABLES_ACL | CREATE_TMP_ACL | SHOW_DB_ACL; > if (access & FILE_ACL) > access|= BINLOG_MONITOR_ACL | REPL_SLAVE_ACL | BINLOG_ADMIN_ACL | > - BINLOG_REPLAY_ACL; > + BINLOG_REPLAY_ACL | SLAVE_MONITOR_ACL; this does not hurt, but it's rather redundant, you set SLAVE_MONITOR_ACL below anyway. > if (access & PROCESS_ACL) > access|= SUPER_ACL | EXECUTE_ACL; > } > @@ -1057,6 +1057,9 @@ class User_table_tabular: public User_table > if (access & REPL_SLAVE_ACL) > access|= REPL_MASTER_ADMIN_ACL; > > + if (access & REPL_SLAVE_ACL) > + access|= SLAVE_MONITOR_ACL; > + > return access & GLOBAL_ACLS; > } > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] d66d5457589d: MDEV-23610: Slave user can't run "SHOW SLAVE STATUS" anymore after upgrade to 10.5, mysql_upgrade should take of that
by Sergei Golubchik 13 Nov '20

13 Nov '20

Hi, Sujatha! On Nov 12, Sujatha wrote: > revision-id: d66d5457589d (mariadb-10.5.4-315-gd66d5457589d) > parent(s): 10b2d5726fa2 > author: Sujatha <sujatha.sivakumar(a)mariadb.com> > committer: Sujatha <sujatha.sivakumar(a)mariadb.com> > timestamp: 2020-11-11 18:41:44 +0530 > message: > > MDEV-23610: Slave user can't run "SHOW SLAVE STATUS" anymore after upgrade to 10.5, mysql_upgrade should take of that > > Add a new privilege "REPLICA MONITOR" which will grant user the permission > to execute "SHOW SLAVE STATUS" and "SHOW RELAYLOG EVENTS" commands. > > SHOW SLAVE STATUS requires either REPLICA MONITOR/SUPER > SHOW RELAYLOG EVENTS requires REPLICA MONITOR privilege. > > diff --git a/mysql-test/main/grant.result b/mysql-test/main/grant.result > index 01daf0271867..a41b38aaa0ba 100644 > --- a/mysql-test/main/grant.result > +++ b/mysql-test/main/grant.result > @@ -1966,10 +1967,11 @@ GRANT REPLICATION SLAVE ON *.* TO mysqltest_u1@localhost; > GRANT SHOW DATABASES ON *.* TO mysqltest_u1@localhost; > GRANT SHUTDOWN ON *.* TO mysqltest_u1@localhost; > GRANT USAGE ON *.* TO mysqltest_u1@localhost; > +GRANT REPLICA MONITOR ON *.* TO mysqltest_u1@localhost; I don't think we should have a mix of REPLICA and SLAVE terms. Call it SLAVE MONITOR and when we flip the switch on grants, we'll do it for all grants at the same time. > SHOW GRANTS FOR mysqltest_u1@localhost; > Grants for mysqltest_u1@localhost > -GRANT RELOAD, SHUTDOWN, PROCESS, FILE, SHOW DATABASES, REPLICATION SLAVE, BINLOG MONITOR, CREATE USER ON *.* TO `mysqltest_u1`@`localhost` > +GRANT RELOAD, SHUTDOWN, PROCESS, FILE, SHOW DATABASES, REPLICATION SLAVE, BINLOG MONITOR, CREATE USER, REPLICA MONITOR ON *.* TO `mysqltest_u1`@`localhost` > GRANT CREATE TEMPORARY TABLES, LOCK TABLES, EXECUTE, CREATE ROUTINE, ALTER ROUTINE, EVENT ON `mysqltest_db1`.* TO `mysqltest_u1`@`localhost` > connect con1,localhost,mysqltest_u1,,mysqltest_db1; > connection con1; > diff --git a/mysql-test/main/grant.test b/mysql-test/main/grant.test > index 38baf6738255..63be18ecb292 100644 > --- a/mysql-test/main/grant.test > +++ b/mysql-test/main/grant.test > @@ -1833,6 +1833,7 @@ GRANT REPLICATION SLAVE ON *.* TO mysqltest_u1@localhost; > GRANT SHOW DATABASES ON *.* TO mysqltest_u1@localhost; > GRANT SHUTDOWN ON *.* TO mysqltest_u1@localhost; > GRANT USAGE ON *.* TO mysqltest_u1@localhost; > +GRANT REPLICA MONITOR ON *.* TO mysqltest_u1@localhost; But the parser should, of course, accept both, as everywhere. > diff --git a/mysql-test/suite/rpl/t/rpl_replica_monitor_priv.test b/mysql-test/suite/rpl/t/rpl_replica_monitor_priv.test > new file mode 100644 > index 000000000000..f7b2af2e0c51 > --- /dev/null > +++ b/mysql-test/suite/rpl/t/rpl_replica_monitor_priv.test > @@ -0,0 +1,90 @@ > +# ==== Purpose ==== > +# > +# REPLICA MONITOR privilege is required to execute following commands. > +# SHOW SLAVE STATUS > +# SHOW RELAYLOG EVENTS > +# > +# ==== Implementation ==== > +# > +# Step1: GRANT ALL privileges for a new user 'user1' and then REVOKE REPLICA > +# MONITOR and SUPER privileges. > +# Step2: Execute SHOW SLAVE STAUTS/SHOW RELAYLOG commands and expect > +# ER_SPECIFIC_ACCESS_DENIED_ERROR. This also verifies that REPLICATION > +# SLAVE ADMIN privilege is not required for these two commands. > +# Step3: GRANT REPLICA MONITOR privilege and observe that both commands are > +# allowd to execute. > +# Step4: GRANT SUPER privilege and observe that only SHOW SLAVE STATUS command > +# is allowed. why do you need a full master-slave replication setup for that? > +# ==== References ==== > +# > +# MDEV-23610: Slave user can't run "SHOW SLAVE STATUS" anymore after upgrade > +# to 10.5, mysql_upgrade should take of that > +# MDEV-23918: admin privlege required to view contents of relay logs in 10.5 > +# > + > +--source include/not_embedded.inc > +--source include/have_binlog_format_mixed.inc > +--source include/master-slave.inc > + > +--connection master > +CREATE USER user1@localhost IDENTIFIED BY ''; > +GRANT ALL PRIVILEGES ON *.* TO user1@localhost; > +REVOKE REPLICA MONITOR, SUPER ON *.* FROM user1@localhost; > +FLUSH PRIVILEGES; > +--sync_slave_with_master > +--connect(con1,127.0.0.1,user1,,test,$SLAVE_MYPORT) > +--connection con1 > +SELECT CURRENT_USER(); > +SHOW GRANTS; > +--echo "Verify that having REPLICATION SLAVE ADMIN doesn't allow SHOW SLAVE STATUS" > +--echo "Expected error:Access denied; you need (at least one of) the SUPER, REPLICA MONITOR privilege(s) for this operation" better use not quotes, but #, like elsewhere. > +--error ER_SPECIFIC_ACCESS_DENIED_ERROR > +SHOW SLAVE STATUS; > +--echo "Verify that having REPLICATION SLAVE ADMIN doesn't allow SHOW RELAYLOG EVENTS" > +--echo "Expected error: Access denied; you need (at least one of) the REPLICA MONITOR privilege(s) for this operation" > +--disable_ps_protocol why? > +--error ER_SPECIFIC_ACCESS_DENIED_ERROR > +SHOW RELAYLOG EVENTS; > +--enable_ps_protocol > +--disconnect con1 > + > diff --git a/sql/privilege.h b/sql/privilege.h > index 37cdf4da01ad..e7686a206b84 100644 > --- a/sql/privilege.h > +++ b/sql/privilege.h > @@ -115,8 +117,12 @@ constexpr privilege_t ALL_KNOWN_ACL_100304 = > constexpr privilege_t ALL_KNOWN_ACL_100502= > (privilege_t) ((LAST_100502_ACL << 1) - 1); > > +// A combination of all bits defined in 10.5.8 > +constexpr privilege_t ALL_KNOWN_ACL_100508= > + (privilege_t) ((LAST_100508_ACL << 1) - 1); > + > // A combination of all bits defined as of the current version > -constexpr privilege_t ALL_KNOWN_ACL= ALL_KNOWN_ACL_100502; > +constexpr privilege_t ALL_KNOWN_ACL= ALL_KNOWN_ACL_100508; may be (privilege_t) ((LAST_CURRENT_ACL << 1) - 1) and then we won't need to change it anymore? Also, I think this deserves a function, like static inline privilege_t ALL_ACLS_TO(privilege_t x) { return (x << 1) - 1; } or, rather, a bit safer return x | (x - 1) > // Unary operators > @@ -505,9 +511,11 @@ constexpr privilege_t PRIV_STMT_STOP_SLAVE= REPL_SLAVE_ADMIN_ACL | SUPER_ACL; > // Was SUPER_ACL prior to 10.5.2 > constexpr privilege_t PRIV_STMT_CHANGE_MASTER= REPL_SLAVE_ADMIN_ACL | SUPER_ACL; > // Was (SUPER_ACL | REPL_CLIENT_ACL) prior to 10.5.2 > -constexpr privilege_t PRIV_STMT_SHOW_SLAVE_STATUS= REPL_SLAVE_ADMIN_ACL | SUPER_ACL; > +// Was REPL_SLAVE_ADMIN_ACL from 10.5.2 to 10.5.7 // Was SUPER_ACL | REPL_SLAVE_ADMIN_ACL from 10.5.2 to 10.5.7 otherwise it's unclear why you have SUPER_ACL below and not just REPLICA_MONITOR_ACL > +constexpr privilege_t PRIV_STMT_SHOW_SLAVE_STATUS= REPLICA_MONITOR_ACL | SUPER_ACL; > // Was REPL_SLAVE_ACL prior to 10.5.2 > -constexpr privilege_t PRIV_STMT_SHOW_RELAYLOG_EVENTS= REPL_SLAVE_ADMIN_ACL; > +// Was REPL_SLAVE_ADMIN_ACL from 10.5.2 to 10.5.7 > +constexpr privilege_t PRIV_STMT_SHOW_RELAYLOG_EVENTS= REPLICA_MONITOR_ACL; > > /* > Privileges related to binlog replying. > diff --git a/sql/sql_acl.cc b/sql/sql_acl.cc > index ef96e8f24843..8a3551c14f53 100644 > --- a/sql/sql_acl.cc > +++ b/sql/sql_acl.cc > @@ -1057,6 +1057,9 @@ class User_table_tabular: public User_table > if (access & REPL_SLAVE_ACL) > access|= REPL_MASTER_ADMIN_ACL; > > + if (access & FILE_ACL) > + access|= REPLICA_MONITOR_ACL; Why is that? > + > return access & GLOBAL_ACLS; > } > > @@ -1541,19 +1548,22 @@ class User_table_json: public User_table > */ > if (access & SUPER_ACL) > { > - if (access & REPL_SLAVE_ACL) > - { > - /* > - The user could do both before the upgrade: > - - set global variables (because of SUPER_ACL) > - - execute "SHOW SLAVE HOSTS" (because of REPL_SLAVE_ACL) > - Grant all new privileges that were splitted from SUPER (in 10.5.2), > - and REPLICATION MASTER ADMIN, so it still can do "SHOW SLAVE HOSTS". > - */ > - access|= REPL_MASTER_ADMIN_ACL; > - } > access|= GLOBAL_SUPER_ADDED_SINCE_USER_TABLE_ACLS; > } > + if (access & REPL_SLAVE_ACL) > + { > + /* > + The user could do both before the upgrade: > + - set global variables (because of SUPER_ACL) this looked fine where it was before, but not anymore after you moved it out of `if (access & SUPER_ACL)` > + - execute "SHOW SLAVE HOSTS" (because of REPL_SLAVE_ACL) > + Grant all new privileges that were splitted from SUPER (in 10.5.2), > + and REPLICATION MASTER ADMIN, so it still can do "SHOW SLAVE HOSTS". > + - execute "SHOW RELAYLOG EVENTS" (because of REPL_SLAVE_ACL) > + */ > + access|= (REPL_MASTER_ADMIN_ACL | REPLICA_MONITOR_ACL); and REPL_MASTER_ADMIN_ACL allows to modify global variables was was earlier possible only with SUPER_ACL. So, indeed, old code was correct, that if() should be inside SUPER_ACL, one should only get REPL_MASTER_ADMIN_ACL if one had both SUPER_ACL and REPL_SLAVE_ACL. > + } > + if (access & BINLOG_MONITOR_ACL) > + access|= REPLICA_MONITOR_ACL; > } > > if (orig_access & ~mask) > @@ -8974,7 +8984,7 @@ static const char *command_array[]= > "CREATE USER", "EVENT", "TRIGGER", "CREATE TABLESPACE", "DELETE HISTORY", > "SET USER", "FEDERATED ADMIN", "CONNECTION ADMIN", "READ_ONLY ADMIN", > "REPLICATION SLAVE ADMIN", "REPLICATION MASTER ADMIN", "BINLOG ADMIN", > - "BINLOG REPLAY" > + "BINLOG REPLAY", "REPLICA MONITOR" SLAVE MONITOR > }; > > static uint command_lengths[]= Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 2

Re: [Maria-developers] e3f45b2f9ea: MDEV-10267 Add ngram fulltext parser plugin
by Sergei Golubchik 11 Nov '20

11 Nov '20

Hi, Rinat! On Nov 02, Rinat Ibragimov wrote: > > > I can't decide. From my point of view, the current approach is > > > fine. Please pick a variant, and I'll try to implement that. > > > > No, I cannot guess which approach will produce more relevant > > searches. Implement something and then we test what works better > > Variable-length n-grams approach is too innovative, and hard to reason > about. I've never heard about such an approach, and it doesn't look > good to me. So I'll stick with a simple slicer. If you mean that variant where it splits "n-grams approach" to "n-gr", "gra", "ram", "ams", "ms a", "s ap", "app", ... then it's just "n letters in every chunk" very easy to explain. But ok, let's start simple and benchmark. > > Of course, it can. Note that fts_get_word() doesn't generate n-grams > > either, it gets the whole word and the n-gram plugin later splits it > > into n-grams. Similarly param->mysql_parse() will extract words for > > you and you'll split them into n-grams. > > Changed to use param->mysql_parse(). > > Turns out that in Aria, MyISAM, and InnoDB, param->mysql_parser() does > call back param->mysql_add_word(). Is it part of the plugin API? Yes, it is. E.g. slide 12 from my old presentation: http://conferences.oreillynet.com/presentations/mysql06/golubchik_sergei.pdf shows that there are three points where a plugin can add functionality. * It can extract the text and then call param->mysql_parser(), this allows to parse, say, gzip-ed texts or EXIF comments in images. * It can replace param->mysql_parser(), to use different rules for spliting the text into words. This is what the n-gram plugin normally does * It can replace param->mysql_add_word() to post-process every word after the built-in parser did the splitting. For example, stemming or soundex plugin can do that. > Comments in include/mysql/plugin_ftparser.h do not mention that at > all. That's why I initially thought that param->mysql_parse() will > parse the string like the default parser do, without any ways to > interact with the process. I've edited the comment to mention this possibility. Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 0cd5377e9ff: MDEV-18734 ASAN heap-use-after-free in my_strnxfrm_simple_internal upon update on versioned partitioned table
by Sergei Golubchik 02 Nov '20

02 Nov '20

Hi, Aleksey! On Oct 20, Aleksey Midenkov wrote: > revision-id: 0cd5377e9ff (mariadb-10.2.31-357-g0cd5377e9ff) > parent(s): dc716da4571 > author: Aleksey Midenkov <midenok(a)gmail.com> > committer: Aleksey Midenkov <midenok(a)gmail.com> > timestamp: 2020-08-17 00:52:35 +0300 > message: > > MDEV-18734 ASAN heap-use-after-free in my_strnxfrm_simple_internal upon update on versioned partitioned table > > update_virtual_fields() may reallocate Field_blob::value buffer due to > different reasons (f.ex. different types, see > Field_str::memcpy_field_possible()). But the buffer address may be > already in m_ordered_rec_buffer which is then used in ordered index > scan queue. > > The patch updates blob virtual fields when the ordered index scan > queue is sorted as well as the top record is restored from the queue. > > New VCOL_UPDATE_BLOBS mode is introduced to update only blobs. Swap is > not needed as it was already done if it was needed. > > Related to ea1b2504 No, I think we need to go deeper here. The bug is not only not caused by system versioning, it is also not caused by virtual columns. The problem is that partitioning copies record[0] out and then back to save and restore values. This does not work for blobs, because record[0] may store the pointer to the memory that Field_blob owns and can free or reallocate as needed. Normally it's not the problem because typically after row reads the engine owns the memory, not Field_blob. When a virtual column is set to a calculated value - then Field_blob owns the memory and we have this bug. But there are engines that use field->store() to set values - in this case Field_blob will own the memory too. Here's a test case with FederatedX: ============================================================= source include/have_partition.inc; source include/have_sequence.inc; source have_federatedx.inc; source include/federated.inc; connection slave; use federated; create table t1_1 (x int, b text, key(x)); create table t1_2 (x int, b text, key(x)); connection master; eval create table t1 (x int, b text, key(x)) engine=federated partition by range columns (x) ( partition p1 values less than (40) connection='mysql://root@127.0.0.1:$SLAVE_MYPORT/federated/t1_1', partition pn values less than (maxvalue) connection='mysql://root@127.0.0.1:$SLAVE_MYPORT/federated/t1_2' ); insert t1 select seq, seq from seq_1_to_35; insert t1 select seq, repeat(seq, 100) from seq_50_to_90; flush tables; select x, b from t1 where x > 30 and x < 60 order by x; select x, b from t1 where x > 30 and x < 60 order by b; drop table t1; connection slave; drop table t1_1, t1_2; source include/federated_cleanup.inc; ============================================================= first select doesn't trigger ASAN error, but it produces incorrect results (I'm showing it here, so that you could create a non-ASAN test that could be run on all builders). the second select gets the same ASAN error as your test below. > diff --git a/mysql-test/suite/vcol/r/partition.result b/mysql-test/suite/vcol/r/partition.result > index bd1353fa145..74c3c3394dc 100644 > --- a/mysql-test/suite/vcol/r/partition.result > +++ b/mysql-test/suite/vcol/r/partition.result > @@ -28,3 +28,26 @@ set statement sql_mode= '' for update t1 set i= 1, v= 2; > Warnings: > Warning 1906 The value specified for generated column 'v' in table 't1' has been ignored > drop table t1; > +# > +# MDEV-18734 ASAN heap-use-after-free in my_strnxfrm_simple_internal upon update on versioned partitioned table > +# > +# Cover queue_fix() in ha_partition::handle_ordered_index_scan() > +create table t1 ( > +x int auto_increment primary key, > +b text, v mediumtext as (b) virtual, > +index (v(10)) > +) partition by range columns (x) ( > +partition p1 values less than (3), > +partition p2 values less than (6), > +partition pn values less than (maxvalue)); > +insert into t1 (b) values ('q'), ('a'), ('b'); > +update t1 set b= 'bar' where v > 'a'; > +drop table t1; this test above doesn't cause ASAN failures for me without your fix. the test below fails all right. > +# Cover return_top_record() in ha_partition::handle_ordered_index_scan() > +create table t1 (x int primary key, b tinytext, v text as (b) virtual) > +partition by range columns (x) ( > +partition p1 values less than (4), > +partition pn values less than (maxvalue)); > +insert into t1 (x, b) values (1, ''), (2, ''), (3, 'a'), (4, 'b'); > +update t1 set b= 'bar' where x > 0 order by v limit 2; > +drop table t1; Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 3

Re: [Maria-developers] 7807d9b6939: MDEV-22974: mysql_native_password make "invalid" valid
by Sergei Golubchik 31 Oct '20

31 Oct '20

Hi, Daniel! On Oct 22, Daniel Black wrote: > revision-id: 7807d9b6939 (mariadb-10.5.4-62-g7807d9b6939) > parent(s): 054f10365c4 > author: Daniel Black <daniel(a)mariadb.org> > committer: Daniel Black <daniel(a)mariadb.org> > timestamp: 2020-07-16 17:06:36 +1000 > message: > > MDEV-22974: mysql_native_password make "invalid" valid > > Per b9f3f06857ac, mysql_system_tables_data.sql creates > a mysql_native_password with a salted hash of "invalid" so that `set password` > will detect a native password can be pplied:. "applied" > > SHOW CREATE USER; dilignently uses this value in its output > generating the SQL: > > MariaDB [(none)]> show create user; > > +---------------------------------------------------------------------------------------------------+ > | CREATE USER for dan@localhost | > +---------------------------------------------------------------------------------------------------+ > | CREATE USER `dan`@`localhost` IDENTIFIED VIA mysql_native_password USING 'invalid' OR unix_socket | > +---------------------------------------------------------------------------------------------------+ > > Attempting to execute this before this patch resutls in: > > MariaDB [(none)]> CREATE USER `dan2`@`localhost` IDENTIFIED VIA mysql_native_password USING 'invalid' OR unix_socket; > ERROR 1372 (HY000): Password hash should be a 41-digit hexadecimal number > > As such deep the the implementation of mysql_native_password we make > "invalid" valid (pun intended) such that the above create user will > succeed. > > native_password_get_salt is only called in the context of > set_user_salt, so all setting of native passwords to hashed content of > 'invalid', quite literally create an invalid password. > > So other forms of "invalid" are valid SQL in creating invalid passwords: > > MariaDB [(none)]> set password = 'invalid'; > Query OK, 0 rows affected (0.001 sec) > > MariaDB [(none)]> alter user dan@localhost IDENTIFIED BY PASSWORD 'invalid'; > Query OK, 0 rows affected (0.000 sec) > > diff --git a/sql/sql_acl.cc b/sql/sql_acl.cc > index d94016b7815..3cd7a67ae1a 100644 > --- a/sql/sql_acl.cc > +++ b/sql/sql_acl.cc > @@ -14539,6 +14539,12 @@ static int native_password_get_salt(const char *hash, size_t hash_length, > > if (hash_length != SCRAMBLED_PASSWORD_CHAR_LENGTH) > { > + if (hash_length == 7 && strcmp(hash, "invalid") == 0) > + { > + memcpy(out, "invalid", 7); > + *out_length= 7; > + return 0; > + } okay. After you said ASAN, I think I can see why this could be problematic. You can, of course, pad it to SCRAMBLED_PASSWORD_CHAR_LENGTH, but then you'll create a *valid* scramble that would correspond to some actual password. One option would be to allow "invalid" literal in set_user_auth(), before even any plugin checks. But I'm unsure of the implications. > my_error(ER_PASSWD_LENGTH, MYF(0), SCRAMBLED_PASSWORD_CHAR_LENGTH); > return 1; > } > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 3

Re: [Maria-developers] f112c4a07475: MDEV-20787: Script dgcov.pl does not work
by Sergei Golubchik 31 Oct '20

31 Oct '20

Hi, Anel! On Oct 30, Anel Husakovic wrote: > revision-id: f112c4a07475 (mariadb-10.2.31-541-gf112c4a07475) > parent(s): 784473b98662 > author: Anel Husakovic <anel(a)mariadb.org> > committer: Anel Husakovic <anel(a)mariadb.org> > timestamp: 2020-10-26 14:25:41 +0100 > message: > > MDEV-20787: Script dgcov.pl does not work > > - Patch is changing CMakeList with `--coverage` flag as an alias for > `-fprofile-arcs -ftest-coverage -lgcov` in addition. > > - About the usage of `dgcov`: > When the server is compiled with `-DENABLE_GCOV=ON`, from object files are generated > `.gcno` and `.gcda` files. > `./mtr --gcov is_check_constraint` is invoking the following script calls: > - `./dgcov.pl --purge`, `./mtr is_check_constraint` and > - `./dgcov.pl --generate>/var/last_changes.dgcov`. > The `purge` flag is clearing `.gcda` files (and others extensions), > while running the test (which follows after the purge) new `.gcda` files are generated. > With dgcov `generate` flag, `gcov -i` (`intermediate format`) is called > on generated `<source-files-name>.gcda` files (`dbug.c.gcda` e.g.). > The patch is tested on `gcov 6.3` and `gcov 7.4` versions > and can be seen that resulting `.gcov` file for `6.3` creates > `<full path>.gcov` (`dbug.c.gcda.gcov` e.g.) file, > where `gcov 7.4+` is still creating `source-file-names.gcov`(`dbug.c.gcov`) files > as `gcov` in general is doing. > Results are stored in `./mysql-test/var/last_changes.dgcov`. > Note: for the newer versions of `gcov` intermediate format is > deprecated `-i` and should be used `--json-format`. does it mean that eventually we'll have to rewrite dgcov to support json format? > > - This patch doesn't take gcov versions in account since is extracting > filename of a `.gcov` file, which is generated with `gcov -i > <source-file>.gcda` during `dgcov.pl --generate`. > > - There is a check if file exists. > Reason for that is because some generated `gcov` files are outliers from above > rule (that a file generated in intermediate format(gcov) has the same source name). > Example 1: `aestables.cpp.gcda` is not generating `aestables.cpp.gcov`, > but only the files used in #include `modes.hpp.gcov runtime.hpp.gcov` > Example 2: `my_sha256.cc.gcda` is generating `my_sha.ic.gcov` > > - Gcov with out-of-source is not working, this patch will solve that > (probably?) > Make sure to test `MTR_BINDIR` which gets set during out-of-source > build and generate an error when running `./mysql-test/mtr --gcov alias`. > > diff --git a/mysql-test/dgcov.pl b/mysql-test/dgcov.pl > index fbc5540e6973..f2bbdad4d20e 100755 > --- a/mysql-test/dgcov.pl > +++ b/mysql-test/dgcov.pl > @@ -161,8 +161,14 @@ sub gcov_one_file { > system($cmd)==0 or die "system($cmd): $? $!"; > } I don't see any changes since the last review. Is it the correct commit? > - # now, read the generated file > - open FH, '<', "$_.gcov" or die "open(<$_.gcov): $!"; > + (my $filename = $_)=~ s/\.[^.]+$//; # remove extension I think this contradicts your explanation in the commit comment. You wrote that the file name can be either dbug.c.gcda.gcov or dbug.c.gcov. Here $_ is dbug.c.gcda, you unconditionally remove .gcda and then below you open $filename.gcov, that is dbug.c.gcov Which means this won't work with an older gcov. Is this your intention? I think it's fine not to support old gcov. But this should be a conscious decision, e.g. you can write in the commit comment "Now dgcov.pl only supports gcov 7.4+" - if that's indeed what you want. Also, if you do it this way, then there's no need to use s///, it's simpler to do, like sub gcov_one_file { return unless /\.gcda$/; + my $filename= $`; unless ($opt_skip_gcov) { or, may be even sub gcov_one_file { return unless /\.gcda$/; + my $gcov_file_path= "$File::Find::dir$`.gcov"; unless ($opt_skip_gcov) { > + my $gcov_file_path= $File::Find::dir."/$filename.gcov"; still an extra slash > + if (! -f $gcov_file_path) > + { > + return; > + } still not return unless -f $gcov_file_path; > + open FH, '<', "$gcov_file_path" or die "open(<$gcov_file_path): $!"; still extra quotes > + > my $fname; > while (<FH>) { > chomp; > diff --git a/mysql-test/mysql-test-run.pl b/mysql-test/mysql-test-run.pl > index 900ef736a455..2189529d82a7 100755 > --- a/mysql-test/mysql-test-run.pl > +++ b/mysql-test/mysql-test-run.pl > @@ -1791,7 +1791,7 @@ sub command_line_setup { > # -------------------------------------------------------------------------- > # Gcov flag > # -------------------------------------------------------------------------- > - if ( ($opt_gcov or $opt_gprof) and ! $source_dist ) > + if ( ($opt_gcov or $opt_gprof) and (! $source_dist or -d $ENV{MTR_BINDIR})) > { > mtr_error("Coverage test needs the source - please use source dist"); > } > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] MDEV-24015: SQL Error (1038): Out of sort memory when enough memory for the sort buffer is provided
by Sergey Petrunia 27 Oct '20

27 Oct '20

Hi Varun, I have only cosmetic comments, please find them below. Ok to push after they are addressed. > The issue here was with the re-execution of a subquey, after few executions > the subquery execution returned error that not enough space was provided inside the sort > buffer. > This is happening because with the introduction of packed sort keys, during the phase > where sort length is calculated the parameters of the sort keys are not reset and > we keep up adding the sort length to the previous calculated sort length for the > earlier executions of the subquery. This happens because the sort keys structure is > allocated only once and so if their is re-execution of a subquery involving sorting > then we should be resetting the parameters of the sort keys. The comment is hard to read. How about being more specific and using shorter sentences: The error was: filesort was done in a subquery which was executed multiple times. During each execution, sortlength() computed total sort key length in Sort_keys::sort_length, without resetting it first. Eventually Sort_keys::sort_length got larger than @@sort_buffer_size, which caused filesort() to be aborted with error. Fixed by making sortlength() to compute lengths only during the first invocation. Subsequent invocations return pre-computed values. > @@ -2518,7 +2526,7 @@ void Sort_param::try_to_pack_sortkeys() > return; > > const uint sz= Sort_keys::size_of_length_field; > - uint sort_len= sort_keys->get_sort_length(); > + uint sort_len= sort_keys->get_sort_length_with_original_values(); > > /* > Heuristic introduced, skip packing sort keys if saving less than 128 bytes > diff --git a/sql/sql_sort.h b/sql/sql_sort.h > index 40f0c5ede5f..6951d4d190a 100644 > --- a/sql/sql_sort.h > +++ b/sql/sql_sort.h > @@ -258,7 +258,9 @@ class Sort_keys :public Sql_alloc, > Sort_keys_array(arr, count), > m_using_packed_sortkeys(false), > size_of_packable_fields(0), > - sort_length(0) > + sort_length_with_original_values(0), > + sort_length_with_memcmp_values(0), > + first_execution(true) This violates the coding style. Please ident the member initialization (see e.g. item.h for examples) > { > DBUG_ASSERT(!is_null()); > }a ... > @@ -307,9 +319,12 @@ class Sort_keys :public Sql_alloc, > > void increment_original_sort_length(uint len) > { > - sort_length+= len; > + sort_length_with_original_values+= len; > } > > + bool is_first_execution() { return first_execution; } > + void set_first_execution(bool val) { first_execution= val; } A comment just about the naming: I would not drag "execution" into here. "Sort_keys" are not really executed. How about changing the functions to be bool is_parameters_computed(); bool set_parameters_computed(); and the variable to be "bool parameters_computed" ? > + > static const uint size_of_length_field= 4; > > private: BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog

1 0

Re: [Maria-developers] 5150dfd6ab3: MDEV-17891 Assertion failures in select_insert::abort_result_set and mysql_load upon attempt to replace into a full table
by Sergei Golubchik 26 Oct '20

26 Oct '20

Hi, Nikita! On Apr 27, Nikita Malyavin wrote: > revision-id: 5150dfd6ab3 (mariadb-10.3.12-230-g5150dfd6ab3) > parent(s): cf78b8c699d > author: Nikita Malyavin <nikitamalyavin(a)gmail.com> > committer: Nikita Malyavin <nikitamalyavin(a)gmail.com> > timestamp: 2019-06-12 22:11:45 +1000 > message: > > MDEV-17891 Assertion failures in select_insert::abort_result_set and mysql_load upon attempt to replace into a full table > > * set modified_non_trans_table in one missed place This commit needs way more text than that. This is just too misterious now. What asserts fails? What has the test case to do with the fix? Why modified_non_trans_table even matters without replication? Why it's specific to versioning and partitioning? Where trans_safe becomes false? > diff --git a/mysql-test/suite/versioning/r/partition.result b/mysql-test/suite/versioning/r/partition.result > index 3fcb59bdb40..99a297417d5 100644 > --- a/mysql-test/suite/versioning/r/partition.result > +++ b/mysql-test/suite/versioning/r/partition.result > @@ -541,6 +541,29 @@ t1 CREATE TABLE `t1` ( > PARTITION BY SYSTEM_TIME INTERVAL 7 SECOND > (PARTITION `ver_p1` HISTORY ENGINE = DEFAULT_ENGINE, > PARTITION `ver_pn` CURRENT ENGINE = DEFAULT_ENGINE) > +set timestamp= default; > +# MDEV-17891 Assertion failures in select_insert::abort_result_set and > +# mysql_load upon attempt to replace into a full table > +set @@max_heap_table_size= 1024*1024; > +create or replace table t1 ( > +pk integer auto_increment, > +primary key (pk), > +f varchar(45000) > +) with system versioning engine=memory > +partition by system_time interval 1 year (partition p1 history, > +partition pn current); > +# fill the table until full > +insert into t1 () values (),(),(),(),(),(),(),(),(),(),(),(),(),(),(),(); > +insert into t1 (f) select f from t1; > +ERROR HY000: The table 't1' is full > +# leave space for exactly one record in current partition > +delete from t1 where pk = 1; > +# copy all data into history partition > +replace into t1 select * from t1; > +replace into t1 select * from t1; > +ERROR HY000: The table 't1' is full > +drop table t1; > +set @@max_heap_table_size= 1048576; > # Test cleanup > drop database test; > create database test; > diff --git a/mysql-test/suite/versioning/t/partition.test b/mysql-test/suite/versioning/t/partition.test > index d5c83b4d3bb..23768836efc 100644 > --- a/mysql-test/suite/versioning/t/partition.test > +++ b/mysql-test/suite/versioning/t/partition.test > @@ -474,6 +474,7 @@ set timestamp=1523466002.799571; > insert into t1 values (11),(12); > set timestamp=1523466004.169435; > delete from t1 where pk in (11, 12); > +set timestamp= default; > > --echo # > --echo # MDEV-18136 Server crashes in Item_func_dyncol_create::prepare_arguments > @@ -489,6 +490,34 @@ partition by system_time interval column_get(column_create(7,7), 7 as int) secon > --replace_result $default_engine DEFAULT_ENGINE > show create table t1; > > +--echo # MDEV-17891 Assertion failures in select_insert::abort_result_set and > +--echo # mysql_load upon attempt to replace into a full table > + > +--let $max_heap_table_size_orig= `select @@max_heap_table_size;` > +set @@max_heap_table_size= 1024*1024; > +create or replace table t1 ( > + pk integer auto_increment, > + primary key (pk), > + f varchar(45000) > +) with system versioning engine=memory > + partition by system_time interval 1 year (partition p1 history, > + partition pn current); > + > +--echo # fill the table until full > +insert into t1 () values (),(),(),(),(),(),(),(),(),(),(),(),(),(),(),(); > +--error ER_RECORD_FILE_FULL > +insert into t1 (f) select f from t1; > +--echo # leave space for exactly one record in current partition > +delete from t1 where pk = 1; > +--echo # copy all data into history partition > +replace into t1 select * from t1; > +--error ER_RECORD_FILE_FULL > +replace into t1 select * from t1; > + > +# cleanup > +drop table t1; > +eval set @@max_heap_table_size= $max_heap_table_size_orig; > + > --echo # Test cleanup > drop database test; > create database test; > diff --git a/sql/sql_insert.cc b/sql/sql_insert.cc > index 1f3a70721fc..44502ea6704 100644 > --- a/sql/sql_insert.cc > +++ b/sql/sql_insert.cc > @@ -1953,6 +1953,8 @@ int write_record(THD *thd, TABLE *table,COPY_INFO *info) > if (likely(!error)) > { > info->deleted++; > + if (!table->file->has_transactions()) > + thd->transaction.stmt.modified_non_trans_table= TRUE; > if (table->versioned(VERS_TIMESTAMP)) > { > store_record(table, record[2]); Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 1

Re: [Maria-developers] 05968211699: MDEV-16026: Global system_versioning_asof must not be used if client sessions can have non-default time zone
by Sergei Golubchik 25 Oct '20

25 Oct '20

Hi, Nikita! Okay. This reverts my "ok to push" on MDEV-16481 :) If you want to store the value in unix timestamp, you don't need MYSQL_TIME in save_result. You should calculate the value as a unix timestamp in ::check and store that in save_result. Now you still have timezone conversion in ::update and that can fail. Everything that can fail should be done in ::check, that's the contract. May be also I reapplied your patches to 10.3 incorrectly when I was reviewing. Perhaps you could rebase them yourself - to be sure I'm looking at the correct patch? On Oct 22, Nikita Malyavin wrote: > revision-id: 05968211699 (mariadb-10.4.11-291-g05968211699) > parent(s): efb1023b6f4 > author: Nikita Malyavin <nikitamalyavin(a)gmail.com> > committer: Sergei Golubchik <serg(a)mariadb.com> > timestamp: 2020-10-22 17:07:03 +0200 > message: > > MDEV-16026: Global system_versioning_asof must not be used if client sessions can have non-default time zone > > * store `system_versioning_asof` in unix time; > * both session and global vars are processed in session timezone; > * setting `default` does not copy global varibale abymore. Instead, it sets system_time to SYSTEM_TIME_UNSPECIFIED, which means that no 'AS OF' time is applied and `now()` can be assumed > As a regression, we cannot assign values below 1970 anymore > > --- > mysql-test/suite/versioning/r/sysvars.result | 67 +++++++++++++++++++++++ > mysql-test/suite/versioning/t/sysvars.test | 80 ++++++++++++++++++++++++---- > sql/mysqld.h | 3 +- > sql/sql_select.cc | 6 ++- > sql/sys_vars.ic | 27 ++++++---- > 5 files changed, 162 insertions(+), 21 deletions(-) > > diff --git a/sql/mysqld.h b/sql/mysqld.h > index bd45ff7b798..884e5a06066 100644 > --- a/sql/mysqld.h > +++ b/sql/mysqld.h > @@ -194,7 +194,8 @@ enum vers_system_time_t > struct vers_asof_timestamp_t > { > ulong type; > - MYSQL_TIME ltime; > + my_time_t unix_time; > + ulong second_part; > }; > > enum vers_alter_history_enum > diff --git a/sql/sql_select.cc b/sql/sql_select.cc > index d0bb0c816ec..4d62a9829eb 100644 > --- a/sql/sql_select.cc > +++ b/sql/sql_select.cc > @@ -722,8 +722,12 @@ bool vers_select_conds_t::init_from_sysvar(THD *thd) > if (type != SYSTEM_TIME_UNSPECIFIED && type != SYSTEM_TIME_ALL) > { > DBUG_ASSERT(type == SYSTEM_TIME_AS_OF); > + MYSQL_TIME ltime; > + thd->variables.time_zone->gmt_sec_to_TIME(&ltime, in.unix_time); > + ltime.second_part = in.second_part; > + > start.item= new (thd->mem_root) > - Item_datetime_literal(thd, &in.ltime, TIME_SECOND_PART_DIGITS); > + Item_datetime_literal(thd, &ltime, TIME_SECOND_PART_DIGITS); > if (!start.item) > return true; > } > diff --git a/sql/sys_vars.ic b/sql/sys_vars.ic > index f33f469b160..417fa7842c2 100644 > --- a/sql/sys_vars.ic > +++ b/sql/sys_vars.ic > @@ -2646,9 +2646,11 @@ class Sys_var_vers_asof: public Sys_var_enum > } > > private: > - bool update(set_var *var, vers_asof_timestamp_t &out) > + bool update(set_var *var, vers_asof_timestamp_t &out, system_variables& pool) > { > bool res= false; > + uint error; > + MYSQL_TIME ltime; > out.type= static_cast<enum_var_type>(var->save_result.ulonglong_value); > if (out.type == SYSTEM_TIME_AS_OF) > { > @@ -2658,11 +2660,14 @@ class Sys_var_vers_asof: public Sys_var_enum > Datetime::Options opt(TIME_CONV_NONE | > TIME_NO_ZERO_IN_DATE | > TIME_NO_ZERO_DATE, thd); > - res= var->value->get_date(thd, &out.ltime, opt); > + res= var->value->get_date(thd, &ltime, opt); > + out.unix_time= pool.time_zone->TIME_to_gmt_sec(&ltime, &error); > + out.second_part= ltime.second_part; > + res|= (error != 0); > } > else // set DEFAULT from global var > { > - out= global_var(vers_asof_timestamp_t); > + out.type= SYSTEM_TIME_UNSPECIFIED; > res= false; > } > } > @@ -2672,11 +2677,11 @@ class Sys_var_vers_asof: public Sys_var_enum > public: > virtual bool global_update(THD *thd, set_var *var) > { > - return update(var, global_var(vers_asof_timestamp_t)); > + return update(var, global_var(vers_asof_timestamp_t), thd->variables); > } > virtual bool session_update(THD *thd, set_var *var) > { > - return update(var, session_var(thd, vers_asof_timestamp_t)); > + return update(var, session_var(thd, vers_asof_timestamp_t), thd->variables); > } > > private: > @@ -2690,10 +2695,14 @@ class Sys_var_vers_asof: public Sys_var_enum > case SYSTEM_TIME_AS_OF: > { > uchar *buf= (uchar*) thd->alloc(MAX_DATE_STRING_REP_LENGTH); > - if (buf &&!my_datetime_to_str(&val.ltime, (char*) buf, 6)) > + MYSQL_TIME ltime; > + > + thd->variables.time_zone->gmt_sec_to_TIME(&ltime, val.unix_time); > + ltime.second_part= val.second_part; > + > + if (buf && !my_datetime_to_str(&ltime, (char*) buf, 6)) > { > - // TODO: figure out variable name > - my_error(ER_WRONG_VALUE_FOR_VAR, MYF(0), "system_versioning_asof_timestamp", "NULL (wrong datetime)"); > + my_error(ER_WRONG_VALUE_FOR_VAR, MYF(0), name.str, "NULL (wrong datetime)"); > return (uchar*) thd->strdup("Error: wrong datetime"); > } > return buf; > @@ -2701,7 +2710,7 @@ class Sys_var_vers_asof: public Sys_var_enum > default: > break; > } > - my_error(ER_WRONG_VALUE_FOR_VAR, MYF(0), "system_versioning_asof_timestamp", "NULL (wrong range type)"); > + my_error(ER_WRONG_VALUE_FOR_VAR, MYF(0), name.str, "NULL (wrong range type)"); > return (uchar*) thd->strdup("Error: wrong range type"); > } > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 1