- developers - lists.mariadb.org

Re: [Maria-developers] c4835af7e8c: MDEV-16026: Forbid global system_versioning_asof in non-default time zone
by Sergei Golubchik 26 Jul '21

26 Jul '21

Hi, Nikita! I suggest to squash commits from c4835af7e8c to 80e3007a5a1 into one. It'll be simpler, no intermediate juggling with MYSQL_TIME anymore. This squashed commit is ok to push (make sure to mention both MDEVs in the commit comment, but not necessarily in the first line) On Jul 26, Nikita Malyavin wrote: > revision-id: c4835af7e8c (mariadb-10.3.30-30-gc4835af7e8c) > parent(s): ec3289b6de4 > author: Nikita Malyavin > committer: Nikita Malyavin > timestamp: 2021-07-21 00:21:31 +0300 > message: > > MDEV-16026: Forbid global system_versioning_asof in 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 > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] a4a5c2d5963: MDEV-25908: -e does not work for my_print_defaults
by Sergei Golubchik 26 Jul '21

26 Jul '21

Hi, Rucha! On Jul 26, Rucha Deodhar wrote: > revision-id: a4a5c2d5963 (mariadb-10.5.11-52-ga4a5c2d5963) > parent(s): 675b72d0da1 > author: Rucha Deodhar > committer: Rucha Deodhar > timestamp: 2021-07-23 19:51:57 +0530 > message: > > MDEV-25908: -e does not work for my_print_defaults > > Analysis: get_defaults_options() rewrites the value for my_defaults_file, > my_defaults_extra_file and my_defaults_group_suffix to 0. So the config file > can't be read. > Fix: Let handle_options() handle --defaults* option and create a new command > line to pass it to load_defaults(). > > diff --git a/libmariadb b/libmariadb > index 802ce584a26..180c543704d 160000 > --- a/libmariadb > +++ b/libmariadb > @@ -1 +1 @@ > -Subproject commit 802ce584a26fdc0ba67fcf35e277bf3c7440956a > +Subproject commit 180c543704d627a50a52aaf60e24ca14e0ec4686 ^^^^^ This is wrong, please, remove it from your commit Otherwise ok to push Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

[Maria-developers] Accessing read records during join for condition pushdown
by Berrocal, Eduardo 23 Jul '21

23 Jul '21

Hello list (first email here, hopefully sending it to the right address), I have been trying to solve this problem for a while now, and I am stuck. I wonder if what I am trying to do is even possible. I am implementing a new storage engine which handles engine condition pushdown. Now, when MariaDB is working on a JOIN using more than one table (for simplicity, let's assume 2), how can I access the read records for table 1 when working on a condition pushdown for table 2? When joining like this: table1.field1 = table2.field1, I can see 2 Item objects of type FIELD in the condition. However, I can't find where the read records for table1 are so I can effectively filter records in table2 by those values. Thanks a lot for your help, [A close up of a sign Description automatically generated] Eduardo Berrocal García de Carellán Senior Software Engineer Intel Corporation | intel.com<http://intel.com/>

3 9

Re: [Maria-developers] 412e696fd2b: MDEV-25395 server recovery hits replication event checksum error
by Sergei Golubchik 23 Jul '21

23 Jul '21

Hi, Andrei! fdle changes - ok, I've seen them in 21117. cur_log/etc - clear. What are the changes around prev_event_pos for? On Jul 23, Andrei Elkin wrote: > revision-id: 412e696fd2b (mariadb-10.3.26-136-g412e696fd2b) > parent(s): 2fa526e26e8 > author: Andrei Elkin > committer: Andrei Elkin > timestamp: 2021-04-15 17:11:07 +0300 > message: > > MDEV-25395 server recovery hits replication event checksum error > > When @@global.binlog_checksum is changing from none to > crc32 and only the first of two Binlog_checkpoint_log_event gets written to > the crc32 rotated binlog file before the server crashes, the recovery faces a checksum > verification error. > > The reason was that recovery algorithm did not realize > potentially varying binlog checksum at scanning of > a sequence of binlog files. > > That is corrected now. > The master-checksum-verify server recovery aborts now and stops > the server due to the checksum error. > A simulation test confirms the fixes. > > Induced changes: > > rpl_cant_read_event_incident got affected because the test > adopts an external not properly closed binlog file that contains > events which can't pass checksum verification. > A work-around is done to accept that binlog and evade recovery. > > binlog_recover_checksum_error needed refinement as > the master-checksum-verify configured server is made to stop > due to the checksum error. Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 1c7d4f9c06f: MDEV-25282 Auto-shutdown on idle when socket-activated
by Sergei Golubchik 23 Jul '21

23 Jul '21

Hi, Daniel! The approach is fine, see some comments below On Jul 23, Daniel Black wrote: > revision-id: 1c7d4f9c06f (mariadb-10.6.1-78-g1c7d4f9c06f) > parent(s): 71964c76fe0 > author: Daniel Black > committer: Daniel Black > timestamp: 2021-06-16 17:15:59 +1000 > message: > > MDEV-25282 Auto-shutdown on idle when socket-activated > > Adds max_idle_execution system variable with a UINT_MAX > default value that corresponds to the time in seconds > under which the mariadbd executable will run in an idle state > with no connections. > > Under systemd socket activiation this variable will get a 10 minute > default value. This will enable a service to be activated on > connection and fall back to a shutdown state after 10 minutes of > no queries. The systemd socket activation can restart the service > on the next connection transparently. > > A global variable of server_last_activity is updated on > accepted connections (where max_idle_execution < UINT_MAX) > and when the connection count (standard or extra) is down > to <= 1 to keep the number of updates on a single variable > low (an not create another performance MDEV-21212 problem). > > When the main accept loop times out on the max_idle_execution > seconds, and then the server_last_activity is checked along > with if current connection count (standard + extra) is 0 > (in case a recently started connection hasn't finished > a query). > > To make this neater, in main accept loop moved code to > handle_new_socket_connection that encompases accepting a connection > and the timeout mechanism has been separated too. > > Changed when looping though possible connections, loop until > the end of the connection list and hereby assume two connection can > occur on the same poll/select call and both will be accepted. > > diff --git a/mysql-test/main/max_idle_execution.test b/mysql-test/main/max_idle_execution.test > new file mode 100644 > --- /dev/null > +++ b/mysql-test/main/max_idle_execution.test > @@ -0,0 +1,38 @@ > +--source include/not_embedded.inc > + > +--let $_server_id= `SELECT @@server_id` > +--let $_expect_file_name= $MYSQLTEST_VARDIR/tmp/mysqld.$_server_id.expect > +--exec echo "wait" > $_expect_file_name there's no reason to exec echo, as you can write directly --write_file $_expect_file_name wait EOF > +disable_reconnect; > +disconnect default; > + > +--echo 'allow server to time out in 10 seconds' > + > +--sleep 10 better wait_until_disconnected.inc here and everywhere below > + > +--echo 'should have timed out' > +# mtr weirdness > +--replace_regex /[\/0-9]+.*// > +--error 0,ER_SERVER_SHUTDOWN,ER_CONNECTION_KILLED,2002,2006,201 > +--connect fail_me,localhost,root you won't need all that ^^^ > + > +--exec echo "restart" > $_expect_file_name > +--sleep 5 > +--connect con0,localhost,root,,test > + > +--source include/wait_until_connected_again.inc > +SELECT VARIABLE_VALUE AS THREAD_CONNECTED FROM INFORMATION_SCHEMA.GLOBAL_STATUS WHERE VARIABLE_NAME='THREADS_CONNECTED'; > +SELECT 'we are back'; > +SELECT VARIABLE_VALUE < 10 AS UPTIME_LESS_THAN_10_SECONDS FROM INFORMATION_SCHEMA.GLOBAL_STATUS WHERE VARIABLE_NAME='UPTIME'; > + > +--sleep 13 > +SELECT 'still here because the connection is open, but disconnecting now'; > +disconnect con0; > +--sleep 5 > + > +--connect con2,127.0.0.1,root,,test,$MASTER_EXTRA_PORT, > +SELECT "extra connection just created: still here, only 5 seconds since last query"; > + > +--sleep 13 > +connect default,localhost,root,,test,,; > +SELECT 'active extra connection kept the server up'; This will be a fairly slow test. 46 seconds of waits. May be you can cut away some? E.g. second test, whether a connection keeps the server up, can reasonably be considered a subset of the third test, whether an extra connection keeps the server up. So you can remove the second test. > diff --git a/mysql-test/main/mysqld--help.result b/mysql-test/main/mysqld--help.result > --- a/mysql-test/main/mysqld--help.result > +++ b/mysql-test/main/mysqld--help.result > @@ -576,6 +576,10 @@ The following specify which files/extra groups are read (specified before remain > --max-error-count=# Max number of errors/warnings to store for a statement > --max-heap-table-size=# > Don't allow creation of heap tables bigger than this > + --max-idle-execution=# > + If no new connections or running queries within this time > + (in seconds) shutdown the server (Automatically > + configured unless set explicitly) > --max-join-size=# Joins that are probably going to read more than > max_join_size records return an error > --max-length-for-sort-data=# > diff --git a/sql/handle_connections_win.cc b/sql/handle_connections_win.cc > --- a/sql/handle_connections_win.cc > +++ b/sql/handle_connections_win.cc > @@ -593,7 +593,7 @@ void network_init_win() > } > } > > -void handle_connections_win() > +void handle_connections_win(Atomic_counter<uint> *connection_count) why do you pass it as an argument? first, you always invoke handle_connections_win with the same value of the argument, second other global variables like server_last_activity are used directly. > { > int n_waits; > > @@ -631,12 +631,26 @@ void handle_connections_win() > { > DBUG_ASSERT(wait_events.size() <= MAXIMUM_WAIT_OBJECTS); > DWORD idx = WaitForMultipleObjects((DWORD)wait_events.size(), > - wait_events.data(), FALSE, INFINITE); > + wait_events.data(), FALSE, > + max_idle_execution < UINT_MAX ? > + max_idle_execution * 1000 : INFINITE); > - DBUG_ASSERT((int)idx >= 0 && (int)idx < (int)wait_events.size()); > + DBUG_ASSERT(idx == WAIT_TIMEOUT || > + ((int)idx >= 0 && (int)idx < (int)wait_events.size())); did you test it on Windows? > > if (idx == SHUTDOWN_IDX) > break; > > + if (idx == WAIT_TIMEOUT) > + { > + if (*connection_count == 0 && > + microsecond_interval_timer() > (server_last_activity + max_idle_execution * 1000000)) > + { > + sql_print_information("max_idle_execution time reached starting shutdown"); > + break; > + } > + continue; > + } > + > all_listeners[idx - LISTENER_START_IDX]->completion_callback(); > } > > diff --git a/sql/mysqld.cc b/sql/mysqld.cc > --- a/sql/mysqld.cc > +++ b/sql/mysqld.cc > @@ -474,7 +474,8 @@ ulong malloc_calls; > ulong specialflag=0; > ulong binlog_cache_use= 0, binlog_cache_disk_use= 0; > ulong binlog_stmt_cache_use= 0, binlog_stmt_cache_disk_use= 0; > -ulong max_connections, max_connect_errors; > +ulong max_connections, max_connect_errors, max_idle_execution; > +ulonglong server_last_activity; > uint max_password_errors; > ulong extra_max_connections; > uint max_digest_length= 0; > @@ -4093,6 +4094,13 @@ static int init_common_variables() > SYSVAR_AUTOSIZE(back_log, MY_MIN(900, (50 + max_connections / 5))); > } > > + /* > + max_idle_execution, defaults to 10mins under systemd socket activation, > + otherwise 136 years or so. > + */ > + if (IS_SYSVAR_AUTOSIZE(&max_idle_execution)) > + SYSVAR_AUTOSIZE(max_idle_execution, sd_listen_fds(0) ? 6000 : UINT_MAX); is that really 10 minutes? > + > unireg_init(opt_specialflag); /* Set up extern variabels */ > if (!(my_default_lc_messages= > my_locale_by_name(lc_messages))) > @@ -6064,17 +6072,60 @@ static void set_non_blocking_if_supported(MYSQL_SOCKET sock) > } > > > +static void handle_socket_timeout() > +{ please add a comment, explaining why a race condition is impossible here, that is, why no other thread can increment connection_count or extra_connection_count just after you saw them being zeros. > + if (connection_count == 0 && extra_connection_count == 0 && > + microsecond_interval_timer() > (server_last_activity + max_idle_execution * 1000000)) * 1000000ULL ? > + { > + sql_print_information("max_idle_execution time reached starting shutdown"); > + abort_loop= 1; > + } > +} > + > + > +static void handle_new_socket_connection(MYSQL_SOCKET sock) > +{ > + struct sockaddr_storage cAddr; > + uint error_count= 0; > + > + for (uint retry= 0; retry < MAX_ACCEPT_RETRY; retry++) > + { > + size_socket length= sizeof(struct sockaddr_storage); > + MYSQL_SOCKET new_sock; > + > + new_sock= mysql_socket_accept(key_socket_client_connection, sock, > + (struct sockaddr *)(&cAddr), > + &length); > + if (mysql_socket_getfd(new_sock) != INVALID_SOCKET) > + handle_accepted_socket(new_sock, sock); > + else if (socket_errno != SOCKET_EINTR && socket_errno != SOCKET_EAGAIN) > + { > + /* > + accept(2) failed on the listening port. > + There is not much details to report about the client, > + increment the server global status variable. > + */ > + statistic_increment(connection_errors_accept, &LOCK_status); > + if ((error_count++ & 255) == 0) // This can happen often > + sql_perror("Error in accept"); > + if (socket_errno == SOCKET_ENFILE || socket_errno == SOCKET_EMFILE) > + sleep(1); // Give other threads some time > + break; > + } > + } > +} > + > + > void handle_connections_sockets() > { > - MYSQL_SOCKET sock= mysql_socket_invalid(); > + MYSQL_SOCKET sock; > - uint error_count=0; > - struct sockaddr_storage cAddr; > int retval; > #ifdef HAVE_POLL > // for ip_sock, unix_sock and extra_ip_sock > Dynamic_array<struct pollfd> fds(PSI_INSTRUMENT_MEM); > #else > fd_set readFDs,clientFDs; > + struct timespec timeout; > #endif > > DBUG_ENTER("handle_connections_sockets"); > @@ -6099,6 +6150,7 @@ void handle_connections_sockets() > } > #endif > > + server_last_activity= microsecond_interval_timer(); > sd_notify(0, "READY=1\n" > "STATUS=Taking your SQL requests now...\n"); > > @@ -6106,10 +6158,12 @@ void handle_connections_sockets() > while (!abort_loop) > { > #ifdef HAVE_POLL > - retval= poll(fds.get_pos(0), fds.size(), -1); > + /* poll timeout in milliseconds */ > + retval= poll(fds.get_pos(0), fds.size(), max_idle_execution * 1000); max value of max_idle_execution is UINT_MAX. The third argument of poll() is int. Although it seems to "work", that is, you get a negative number which poll() interprets as infinite, I seriously suspect it's a UB. > #else > + timeout= { max_idle_execution, 0}; > readFDs=clientFDs; > - retval= select(FD_SETSIZE, &readFDs, NULL, NULL, NULL); > + retval= select(FD_SETSIZE, &readFDs, NULL, NULL, &timeout); > #endif > > if (retval < 0) > @@ -6132,50 +6186,27 @@ void handle_connections_sockets() > break; > > /* Is this a new connection request ? */ > -#ifdef HAVE_POLL > - for (size_t i= 0; i < fds.size(); ++i) > + sock= mysql_socket_invalid(); > + for (size_t i= 0; i < listen_sockets.size(); i++) > { > +#ifdef HAVE_POLL > if (fds.at(i).revents & POLLIN) > - { > - sock= listen_sockets.at(i); > - break; > - } > - } > #else // HAVE_POLL > - for (size_t i=0; i < listen_sockets.size(); i++) > - { > if (FD_ISSET(mysql_socket_getfd(listen_sockets.at(i)), &readFDs)) > +#endif // HAVE_POLL > { > sock= listen_sockets.at(i); > - break; > + handle_new_socket_connection(sock); > } > } > -#endif // HAVE_POLL > - > - for (uint retry=0; retry < MAX_ACCEPT_RETRY; retry++) > + /* timeout */ > + if (mysql_socket_getfd(sock) == INVALID_SOCKET) > { > - size_socket length= sizeof(struct sockaddr_storage); > - MYSQL_SOCKET new_sock; > - > - new_sock= mysql_socket_accept(key_socket_client_connection, sock, > - (struct sockaddr *)(&cAddr), > - &length); > - if (mysql_socket_getfd(new_sock) != INVALID_SOCKET) > - handle_accepted_socket(new_sock, sock); > - else if (socket_errno != SOCKET_EINTR && socket_errno != SOCKET_EAGAIN) > - { > - /* > - accept(2) failed on the listening port. > - There is not much details to report about the client, > - increment the server global status variable. > - */ > - statistic_increment(connection_errors_accept, &LOCK_status); > - if ((error_count++ & 255) == 0) // This can happen often > - sql_perror("Error in accept"); > - if (socket_errno == SOCKET_ENFILE || socket_errno == SOCKET_EMFILE) > - sleep(1); // Give other threads some time > - break; > - } > + handle_socket_timeout(); > + } > + else if (max_idle_execution < UINT_MAX) > + { > + server_last_activity= microsecond_interval_timer(); > } > } > sd_notify(0, "STOPPING=1\n" > diff --git a/sql/sql_class.h b/sql/sql_class.h > --- a/sql/sql_class.h > +++ b/sql/sql_class.h > @@ -3949,6 +3950,9 @@ class THD: public THD_count, /* this must be first */ > set_time_for_next_stage(); > if (utime_after_query >= utime_after_lock + variables.long_query_time) > server_status|= SERVER_QUERY_WAS_SLOW; > + /* If we're tracking idle execution, and we're down to the last connection */ > + if (max_idle_execution < UINT_MAX && *scheduler->connection_count <= 1) > + server_last_activity= utime_after_query; 1. you update it concurrenty. Must be atomic, right? 2. do you really want to shutdown the server if there's an idle connection? I'd think it'd be less surprising to shutdown only when there are no connections at all. If there is a runaway idle connection, it'll die after interactive_timeout or wait_timeout. And then you shutdown the server after max_idle_execution. So instead of shutting down the server when there are connections, I'd suggest to autoset interactive_timeout and wait_timeout to be in line with max_idle_execution. Like, 10 mins, or, may be if (IS_SYSVAR_AUTOSIZE(net_wait_timeout) && net_wait_timeout > max_idle_execution) SYSVAR_AUTOSIZE(net_wait_timeout, max_idle_execution); > } > inline ulonglong found_rows(void) > { > diff --git a/sql/sys_vars.cc b/sql/sys_vars.cc > --- a/sql/sys_vars.cc > +++ b/sql/sys_vars.cc > @@ -1724,6 +1724,14 @@ Sys_max_connect_errors( > VALID_RANGE(1, UINT_MAX), DEFAULT(MAX_CONNECT_ERRORS), > BLOCK_SIZE(1)); > > +static Sys_var_ulong Sys_max_idle_execution( ulong is non portable. why not make the variable uint, if max value is UINT_MAX anyway? > + "max_idle_execution", > + "If no new connections or running queries within this time (in seconds) " > + "shutdown the server", may be something like "Default to 10 minutes in systemd socket activation setup" ? I know, it means asking for troubles :( > + AUTO_SET GLOBAL_VAR(max_idle_execution), CMD_LINE(REQUIRED_ARG), > + VALID_RANGE(10, UINT_MAX), DEFAULT(UINT_MAX), > + BLOCK_SIZE(1)); > + > static Sys_var_on_access_global<Sys_var_uint, > PRIV_SET_SYSTEM_GLOBAL_VAR_MAX_PASSWORD_ERRORS> > Sys_max_password_errors( > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] ae1f4e4d727: MDEV-23836: Assertion `! is_set() || m_can_overwrite_status' in Diagnostics_area::set_error_status (interrupted ALTER TABLE under LOCK)
by Sergei Golubchik 23 Jul '21

23 Jul '21

Hi, Rucha! On Jul 23, Rucha Deodhar wrote: > On Fri, Jul 23, 2021 at 1:55 PM Sergei Golubchik <serg(a)mariadb.org> wrote: > > > > Consider this - thd->killed can be changed literally any time. We cannot > > test for it after every second line, can we? Clearly some places are > > better for this test than others. > > > There was no specific reason except it seemed a better choice for this > particular test case. > But yes, it can also arrive before warning or later. Now I think it would > be better check it before exiting handle_if_exists_option() > and return appropriate value. Yes, you're right, it would be better. but KILL can come after handle_if_exists_options() too :) Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

[Maria-developers] MariaDB Connector/J 3.x Status and Plans
by Ondrej Dubaj 23 Jul '21

23 Jul '21

Hello, I would like to ask about the current status of MariaDB Connector/J 3.x. I maintain the package in Fedora and RHEL. Currently, we are facing a decision about which major version to pack to RHEL 9, which is currently being developed. We would like to ship RHEL 9 with the latest stable version of the Java connector, and avoid rebasing to any next major release during the RHEL 9 life, if possible. However the 3.x version is only available in ALPHA quality and I can't see any plan for it to reach further stages in the Jira roadmap for the next half a year. https://jira.mariadb.org/secure/Dashboard.jspa I believe the best solution for us would be you releasing the GA, or at least BETA version ASAP, or at least in the near future. We still have time to update the package from the ALPHA version to GA version before the RHEL 9 development freeze, but the decision whether to take this road has to be made ASAP. After RHEL 9 GA, it will be very complicated for us to deal with rebasing to a new, incompatible version of mariadb-connector-j, where this rebase can break existing user applications and brings a lot of problems with it. Currently mariadb-java-client-2.7.2 is present in the repositories, but as already said, we want to aim for the 3.x version. You, of course, might have very different plans for the connector and different idea about how ready it is to become BETA or GA. We would like to hear your plans with the Java connector 3.x version and to discuss our possible paths to take. Thank you for your time in advance. Regards, Ondrej Dubaj

2 2

Re: [Maria-developers] ae1f4e4d727: MDEV-23836: Assertion `! is_set() || m_can_overwrite_status' in Diagnostics_area::set_error_status (interrupted ALTER TABLE under LOCK)
by Sergei Golubchik 23 Jul '21

23 Jul '21

Hi, Rucha! On Jul 23, Rucha Deodhar wrote: > > > diff --git a/sql/sql_table.cc b/sql/sql_table.cc > > > index f58ce8f997d..0b722984fca 100644 > > > --- a/sql/sql_table.cc > > > +++ b/sql/sql_table.cc > > > @@ -6489,6 +6489,12 @@ handle_if_exists_options(THD *thd, TABLE *table, Alter_info *alter_info, > > > ER_BAD_FIELD_ERROR, > > > ER_THD(thd, ER_BAD_FIELD_ERROR), > > > sql_field->change.str, table->s->table_name.str); > > > + /* > > > + if thread is killed (for example because local memory used execeeds > > > + maximum session memory used), return TRUE and do rollback (done later). > > > + */ > > > + if (thd->killed) > > > + DBUG_RETURN(true); > > > it.remove(); > > > > Looks like a completely arbitrary place to put a check in. > > What if the kill signal arrives later? > > We want to stop execution of the query as soon as we get kill. In this case > it happens when we are giving out a warning. > So I put the check here to return error right after giving warning. You can get KILL anytime. Was there any reason to put this check here and not few lines earlier or later? What if KILL would arrive before the warning? or after it.remove() ? Consider this - thd->killed can be changed literally any time. We cannot test for it after every second line, can we? Clearly some places are better for this test than others. Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] ae1f4e4d727: MDEV-23836: Assertion `! is_set() || m_can_overwrite_status' in Diagnostics_area::set_error_status (interrupted ALTER TABLE under LOCK)
by Sergei Golubchik 22 Jul '21

22 Jul '21

Hi, Rucha! On Jul 22, Rucha Deodhar wrote: > revision-id: ae1f4e4d727 (mariadb-10.5.11-1-gae1f4e4d727) > parent(s): dc82effa5df > author: Rucha Deodhar > committer: Rucha Deodhar > timestamp: 2021-06-21 20:24:43 +0530 > message: > > MDEV-23836: Assertion `! is_set() || m_can_overwrite_status' in > Diagnostics_area::set_error_status (interrupted ALTER TABLE under LOCK) > > Analysis: KILL_QUERY is not ignored when local memory used exceeds maximum > session memory. Hence the query proceeds, OK is sent and we end up > reopening tables that are marked for reopen. During this, kill status is > eventually checked and assertion failure happens during trying to send error > message because OK has already been sent. > Fix: If local memory used exceeds maximum session memory and query is killed > before it has completed execution, return TRUE. > diff --git a/sql/sql_table.cc b/sql/sql_table.cc > index f58ce8f997d..0b722984fca 100644 > --- a/sql/sql_table.cc > +++ b/sql/sql_table.cc > @@ -6489,6 +6489,12 @@ handle_if_exists_options(THD *thd, TABLE *table, Alter_info *alter_info, > ER_BAD_FIELD_ERROR, > ER_THD(thd, ER_BAD_FIELD_ERROR), > sql_field->change.str, table->s->table_name.str); > + /* > + if thread is killed (for example because local memory used execeeds > + maximum session memory used), return TRUE and do rollback (done later). > + */ > + if (thd->killed) > + DBUG_RETURN(true); > it.remove(); Looks like a completely arbitrary place to put a check in. What if the kill signal arrives later? Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] f5196787294: MDEV-23178: Qualified asterisk not supported in INSERT .. RETURNING
by Sergei Golubchik 22 Jul '21

22 Jul '21

Hi, Rucha! ok to push! thanks On Jul 22, Rucha Deodhar wrote: > revision-id: f5196787294 (mariadb-10.5.11-28-gf5196787294) > parent(s): b1e57179242 > author: Rucha Deodhar > committer: Rucha Deodhar > timestamp: 2021-07-22 18:54:36 +0530 > message: > > MDEV-23178: Qualified asterisk not supported in INSERT .. RETURNING > > Analysis: When we have INSERT/REPLACE returning with qualified asterisk in the > RETURNING clause, '*' is not resolved properly because of wrong context. > context->table_list is NULL or has incorrect table because context->table_list > has tables from the FROM clause. For INSERT/REPLACE...SELECT...RETURNING, > context->table_list has table we are inserting from. While in other > INSERT/REPLACE syntax, context->table_list is NULL because there is no FROM > clause. > Fix: If filling fields instead of '*' for qualified asterisk in RETURNING, > use first_name_resolution_table for correct resolution of item. > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] d91c4eea668: MDEV-23178: Qualified asterisk not supported in INSERT .. RETURNING
by Sergei Golubchik 22 Jul '21

22 Jul '21

Hi, Rucha! On Jul 22, Rucha Deodhar wrote: > revision-id: d91c4eea668 (mariadb-10.5.11-28-gd91c4eea668) > parent(s): b1e57179242 > author: Rucha Deodhar > committer: Rucha Deodhar > timestamp: 2021-07-17 18:47:15 +0530 > message: > > MDEV-23178: Qualified asterisk not supported in INSERT .. RETURNING > > Analysis: When we have INSERT/REPLACE returning with qualified asterisk in the > RETURNING clause, '*' is not resolved properly because of wrong context. > context->table_list is NULL or has incorrect table because context->table_list > has tables from the FROM clause. For INSERT/REPLACE...SELECT...RETURNING, > context->table_list has table we are inserting from. While in other > INSERT/REPLACE syntax, context->table_list is NULL because there is no FROM > clause. > Fix: If filling fields instead of '*' for qualified asterisk in RETURNING, > use first_name_resolution_table for correct resolution of item. > @@ -271,6 +289,8 @@ INSERT INTO t2(id2, val2) SELECT * FROM t1 WHERE id1=2 RETURNING(SELECT > --error ER_SUBQUERY_NO_1_ROW > INSERT INTO t2(id2,val2) SELECT * FROM t1 WHERE id1=2 RETURNING (SELECT > id2 FROM t2); > +#--error ER_BAD_TABLE_ERROR > +#INSERT INTO t2(id2,val2) SELECT t1.* FROM t1 WHERE id1=2 RETURNING t1.*; Why is this test commented out? (same for REPLACE) > @@ -8031,10 +8034,20 @@ insert_fields(THD *thd, Name_resolution_context *context, const char *db_name, > If table names are qualified, then loop over all tables used in the query, > else treat natural joins as leaves and do not iterate over their underlying > tables. > + If filling fields instead of '*' for qualified asterisk in RETURNING clause, > + use first_name_resolution_table for correct resolution of item because > + context->table_list is either NULL or has incorrect table because > + context->table_list has tables from the FROM clause. > + In case of INSERT/REPLACE...SELECT..RETURNING context->table_list will have > + table we are inserting and NULL for other cases because there is no > + FROM clause. > */ > - TABLE_LIST *first= context->first_name_resolution_table; > - TABLE_LIST *TABLE_LIST::* next= &TABLE_LIST::next_name_resolution_table; > - if (table_name) > + if (returning_field || !table_name) > + { > + first= context->first_name_resolution_table; > + next= &TABLE_LIST::next_name_resolution_table; These are the values already set before, so assignments are redundant. Either remove initializers, like TABLE_LIST *first; TABLE_LIST *TABLE_LIST::* next; if (returning_field || !table_name) or reverse your condition: TABLE_LIST *first= context->first_name_resolution_table; TABLE_LIST *TABLE_LIST::* next= &TABLE_LIST::next_name_resolution_table; if (table_name && !returning_field) > + } > + else > { > first= context->table_list; > next= &TABLE_LIST::next_local; Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 5e16a49f5d9: MDEV-26013 distinct not work properly in some cases for spider tables
by Sergei Golubchik 22 Jul '21

22 Jul '21

Hi, Nayuta! On Jul 21, Nayuta Yanagisawa wrote: > revision-id: 5e16a49f5d9 (mariadb-10.4.20-32-g5e16a49f5d9) > parent(s): 78735dcaf75 > author: Nayuta Yanagisawa > committer: Nayuta Yanagisawa > timestamp: 2021-07-21 09:58:34 +0000 > message: > > MDEV-26013 distinct not work properly in some cases for spider tables > > diff --git a/sql/sql_select.cc b/sql/sql_select.cc > index d8116b20cbe..cfd04d93662 100644 > --- a/sql/sql_select.cc > +++ b/sql/sql_select.cc > @@ -3274,9 +3274,17 @@ bool JOIN::make_aggr_tables_info() > > if (ht && ht->create_group_by) > { > - /* Check if the storage engine can intercept the query */ > - Query query= {&all_fields, select_distinct, tables_list, conds, > - group_list, order ? order : group_list, having}; > + /* > + Check if the storage engine can intercept the query. > + > + JOIN::optimize_stage2() might convert DISTINCT into GROUP BY and then > + optimize away GROUP BY. In such a case, we need to notify a storage engine > + supporting a group by handler of the existence of the original DISTINCT. > + Thus, we set select_distinct || (no_order && group_optimized_away) to > + Query::distinct. > + */ > + Query query= {&all_fields, select_distinct || (no_order && group_optimized_away), > + tables_list, conds, group_list, order ? order : group_list, having}; If DISTINCT was coverted to a GROUP BY, why would the engine need to know whether there was DISTINCT or not originally? There is no DISTINCT on the execution plan now, that should be sufficient, shouldn't it? Why does the query fail with GROUP BY? > group_by_handler *gbh= ht->create_group_by(thd, &query); > > if (gbh) Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 4

Re: [Maria-developers] 2f9bbf392072: MDEV-18734 optimization by taking ownership via String::swap()
by Sergei Golubchik 21 Jul '21

21 Jul '21

Hi, Aleksey! Don't be confused, the commit header and the comment come from your last commit, but the diff below includes all three commits that mention MDEV-18734. On Nov 02, Aleksey Midenkov wrote: > revision-id: 2f9bbf392072 (mariadb-10.2.31-543-g2f9bbf392072) > parent(s): 1f4960e3f2ac > author: Aleksey Midenkov <midenok(a)gmail.com> > committer: Aleksey Midenkov <midenok(a)gmail.com> > timestamp: 2020-11-02 14:26:04 +0300 > message: > > MDEV-18734 optimization by taking ownership via String::swap() > > ha_partition stores records in array of m_ordered_rec_buffer and uses > it for prio queue in ordered index scan. When the records are restored > from the array the blob buffers may be already freed or > rewritten. This can happen when blob buffers are cached in > Field_blob::value or Field_blob::read_value. > > Previous solution worked via copying blob buffer into > mem_root-allocated buffer. That was not optimal as it requires more > memory and processing time. > > This patch avoids memory copy by taking temporary ownership of cached > blob buffers via String::swap(). It also fixes problem when > value.str_length is wrong after Field_blob::store(). > diff --git a/mysql-test/suite/federated/federated_partition.test b/mysql-test/suite/federated/federated_partition.test > index ef1e27ec5054..33ee025442f4 100644 > --- a/mysql-test/suite/federated/federated_partition.test > +++ b/mysql-test/suite/federated/federated_partition.test > @@ -50,4 +50,30 @@ drop table federated.t1_2; > > --echo End of 5.1 tests > > +--echo # > +--echo # MDEV-18734 ASAN heap-use-after-free upon sorting by blob column from partitioned table > +--echo # > +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; > +--replace_result $SLAVE_MYPORT SLAVE_PORT > +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 values (1, 1), (2, 2), (3, 3), (4, 4), (5, 5), (6, 6), (7, 7), (8, 8); > +insert t1 select x + 8, x + 8 from t1; > +insert t1 select x + 16, x + 16 from t1; > +insert t1 select x + 49, repeat(x + 49, 100) from t1; > +--echo # This produces wrong result before MDEV-17573 > +select * from t1; What do you mean, simple `select * from t1` produces incorrect result? > +flush tables; > +select x, b from t1 where x > 30 and x < 60 order by b; Is this ASAN-only test? > +drop table t1; > +connection slave; > +drop table t1_1, t1_2; > + > source include/federated_cleanup.inc; > diff --git a/mysql-test/suite/vcol/t/partition.test b/mysql-test/suite/vcol/t/partition.test > index 889724fb1c5c..f10ee0491ccc 100644 > --- a/mysql-test/suite/vcol/t/partition.test > +++ b/mysql-test/suite/vcol/t/partition.test > @@ -30,3 +30,28 @@ subpartition by hash(v) subpartitions 3 ( > insert t1 set i= 0; > set statement sql_mode= '' for update t1 set i= 1, v= 2; > drop table t1; > + > +--echo # > +--echo # MDEV-18734 ASAN heap-use-after-free in my_strnxfrm_simple_internal upon update on versioned partitioned table > +--echo # > +--echo # 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 didn't fail for me in an ASAN build on the vanilla 10.5 without your fix. That is, I'm not sure it actually tests the fix. > + > +--echo # 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; This fails without a fix all right. Still, I wonder whether it's possible to create a test that'll fail in a normal optimized build w/o ASAN > +drop table t1; > diff --git a/sql/field.cc b/sql/field.cc > index bdaaecc20269..0fd40c979d2c 100644 > --- a/sql/field.cc > +++ b/sql/field.cc > @@ -8310,6 +8310,7 @@ int Field_blob::store(const char *from,uint length,CHARSET_INFO *cs) > copy_length= copier.well_formed_copy(field_charset, > (char*) value.ptr(), new_length, > cs, from, length); > + value.length(copy_length); good! Could this be a distinct bug with its own test case? > Field_blob::store_length(copy_length); > bmove(ptr+packlength,(uchar*) &tmp,sizeof(char*)); > > diff --git a/sql/ha_partition.cc b/sql/ha_partition.cc > index 9d82df0235c7..7c658082d397 100644 > --- a/sql/ha_partition.cc > +++ b/sql/ha_partition.cc > @@ -5125,14 +5128,14 @@ bool ha_partition::init_record_priority_queue() > uint alloc_len; > uint used_parts= bitmap_bits_set(&m_part_info->read_partitions); > /* Allocate record buffer for each used partition. */ > - m_priority_queue_rec_len= m_rec_length + PARTITION_BYTES_IN_POS; > + m_priority_queue_rec_len= m_rec_length + ORDERED_REC_OFFSET; > if (!m_using_extended_keys) > m_priority_queue_rec_len += m_file[0]->ref_length; > alloc_len= used_parts * m_priority_queue_rec_len; > /* Allocate a key for temporary use when setting up the scan. */ > alloc_len+= table_share->max_key_length; > > - if (!(m_ordered_rec_buffer= (uchar*)my_malloc(alloc_len, MYF(MY_WME)))) > + if (!(m_ordered_rec_buffer= (uchar*) alloc_root(&m_ordered_root, alloc_len))) I don't see why you need a memroot here. memroot is needed when you allocate later at difeerent points in time some initially unpredictable amount of memory that has the same lifetime and needs to be freed all at once. But here it seems that you only allocate once. You could just make m_priority_queue_rec_len = m_rec_length + PARTITION_BYTES_IN_POS + table->s->blob_fields * sizeof(Ordered_blob_storage) (plus necessary alignment). > DBUG_RETURN(true); > > /* > @@ -6178,7 +6203,11 @@ int ha_partition::handle_ordered_index_scan(uchar *buf, bool reverse_order) > */ > error= file->read_range_first(m_start_key.key? &m_start_key: NULL, > end_range, eq_range, TRUE); > - memcpy(rec_buf_ptr, table->record[0], m_rec_length); > + if (!error) > + { > + memcpy(rec_buf_ptr, table->record[0], m_rec_length); > + } did you have a bug because of that? something didn't work? > + > reverse_order= FALSE; > break; > } > @@ -6310,6 +6349,43 @@ int ha_partition::handle_ordered_index_scan_key_not_found() > } > > > +void ha_partition::swap_blobs(uchar * rec_buf, Ordered_blob_storage ** storage, bool restore) > +{ > + uint *ptr, *end; > + uint blob_n= 0; > + table->move_fields(table->field, rec_buf, table->record[0]); > + for (ptr= table->s->blob_field, end= ptr + table->s->blob_fields; > + ptr != end; ++ptr, ++blob_n) > + { > + DBUG_ASSERT(*ptr < table->s->fields); > + Field_blob *blob= (Field_blob*) table->field[*ptr]; > + DBUG_ASSERT(blob->flags & BLOB_FLAG); > + DBUG_ASSERT(blob->field_index == *ptr); > + if (!bitmap_is_set(table->read_set, *ptr) || blob->is_null()) > + continue; > + > + Ordered_blob_storage &s= *storage[blob_n]; > + > + if (restore) > + { > + if (!s.blob.is_empty()) > + blob->swap(s.blob, s.set_read_value); looks somewhat risky, why blob.length == 0 means that you don't need to restore the value? > + } > + else > + { > + bool set_read_value; > + String *cached= blob->cached(set_read_value); > + if (cached) > + { > + cached->swap(s.blob); > + s.set_read_value= set_read_value; is it indeed possible here for a value to be either in `value` or in `read_value` ? When happens what? > + } > + } > + } > + table->move_fields(table->field, table->record[0], rec_buf); > +} > + > + > /* > Common routine to handle index_next with ordered results > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 11

Re: [Maria-developers] 5d19829df5f: MDEV-25908: -e does not work for my_print_defaults
by Sergei Golubchik 21 Jul '21

21 Jul '21

Hi, Rucha! Here the test is ok, buildbot is green. But you've only removed -e. What about -c, -n, -g ? On Jul 21, Rucha Deodhar wrote: > revision-id: 5d19829df5f (mariadb-10.6.1-39-g5d19829df5f) > parent(s): 2c4d1fb5440 > author: Rucha Deodhar > committer: Rucha Deodhar > timestamp: 2021-07-15 13:29:25 +0530 > message: > > MDEV-25908: -e does not work for my_print_defaults > > Analysis: -e is very inconsistent with how --default* options work in any other > executable. > Fix: Removed the support for -e starting from 10.7 to avoid inconsistency in the > long term. > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 61dcf8d940c: MDEV-25908: -e does not work for my_print_defaults
by Sergei Golubchik 21 Jul '21

21 Jul '21

Hi, Rucha! On Jul 21, Rucha Deodhar wrote: > revision-id: 61dcf8d940c (mariadb-10.5.11-35-g61dcf8d940c) > parent(s): f88d130e710 > author: Rucha Deodhar > committer: Rucha Deodhar > timestamp: 2021-07-15 16:07:18 +0530 > message: > > MDEV-25908: -e does not work for my_print_defaults > > Analysis: get_defaults_options() rewrites the value for my_defaults_file, > my_defaults_extra_file and my_defaults_group_suffix to 0. So the config file > can't be read. > Fix: Let handle_options() handle --defaults* option and create a new command > line to pass it to load_defaults(). The code change looks ok. But I thought we've seen from your previous push that this test doesn't work. Haven't we? See how it failed all over in buildbot: https://buildbot.askmonty.org/buildbot/grid?category=main&branch=bb-10.5-MD… Please, generally, first check that your commit passes buildbot, then ask for a review. Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] cf6d83e7d68: MDEV-24760 SELECT..CASE statement syntax error at Spider Engine table
by Sergei Golubchik 21 Jul '21

21 Jul '21

Hi, Nayuta! Great, ok to push! On Jul 21, Nayuta Yanagisawa wrote: > revision-id: cf6d83e7d68 (mariadb-10.3.30-28-gcf6d83e7d68) > parent(s): efa311ab8ea > author: Nayuta Yanagisawa > committer: Nayuta Yanagisawa > timestamp: 2021-07-19 15:16:25 +0000 > message: > > MDEV-24760 SELECT..CASE statement syntax error at Spider Engine table > > The root cause of the bug is in `spider_db_mbase_util::open_item_func()`. > The function handles an instance of the `Item_func` class based on its > `Item_func::Functype`. > The `Functype` of `CASE WHEN ... THEN` is `CASE_SEARCHED_FUNC`. > However, the Spider SE doesn't recognize this `Functype` because > `CASE_SEARCHED_FUNC` is newly added by 4de0d92. This results in the wrong > handling of `CASE WHEN ... THEN`. > > The above also applies to `CASE_SIMPLE_FUNC`. > > --- /dev/null > +++ b/storage/spider/mysql-test/spider/bugfix/r/mdev_24760.result ... > +) ENGINE=Spider DEFAULT CHARSET=utf8 COMMENT='table "tbl_a"' > +PARTITION BY HASH(id) ( > +PARTITION pt1 COMMENT='srv "s_2_1"' > +); > +SELECT id, CASE WHEN name='Alice' THEN "A" WHEN name='Bob' THEN "B" END FROM tbl_a; > +id CASE WHEN name='Alice' THEN "A" WHEN name='Bob' THEN "B" END > +1 A > +2 B > +SELECT id, CASE name WHEN 'Alice' THEN "A" WHEN 'Bob' THEN "B" END FROM tbl_a; > +id CASE name WHEN 'Alice' THEN "A" WHEN 'Bob' THEN "B" END > +1 A > +2 B a _very_ slight preference - could you use different names for local (spider) and remote tables? That is, by all means, please, follow the conventions that are used in existing spider tests. But if it's possible to use different tables names for remote and local without violating spider test conventions, I think it'd make tests easier to read. Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 38a888da0f1: MDEV-17554 Auto-create new partition for system versioned tables with history partitioned by INTERVAL/LIMIT
by Sergei Golubchik 20 Jul '21

20 Jul '21

Hi, Aleksey! Like last time, it's a review of the diff bc04ded2353 22158b3c05a, so not only commit 38a888da0f1. The main thing below - I didn't understand a couple of changes and asked for clarifications. On May 30, Aleksey Midenkov wrote: > revision-id: 38a888da0f1 (mariadb-10.5.2-653-g38a888da0f1) > parent(s): bc04ded2353 > author: Aleksey Midenkov <midenok(a)gmail.com> > committer: Aleksey Midenkov <midenok(a)gmail.com> > timestamp: 2021-04-22 23:35:06 +0300 > message: > > MDEV-17554 Auto-create new partition for system versioned tables with history partitioned by INTERVAL/LIMIT > diff --git a/mysql-test/suite/versioning/r/delete_history.result b/mysql-test/suite/versioning/r/delete_history.result > --- a/mysql-test/suite/versioning/r/delete_history.result > +++ b/mysql-test/suite/versioning/r/delete_history.result > @@ -154,3 +152,21 @@ select * from t1; > a > 1 > drop table t1; > +# > +# MDEV-17554 Auto-create new partition for system versioned tables with history partitioned by INTERVAL/LIMIT > +# > +# Don't auto-create new partition on DELETE HISTORY: > +set timestamp= unix_timestamp('2000-01-01 00:00:00'); > +create or replace table t (a int) with system versioning > +partition by system_time interval 1 hour auto; > +set timestamp= unix_timestamp('2000-01-01 10:00:00'); perhaps, make it only 02:00:00 to avoid any chance that a partition won't be created because of max-auto-create limit. it's possible we put the limit at 10, but 2 is very unlikely. or does max-auto-create limit always result in a warning? > +delete history from t; > +set timestamp= default; > +show create table t; > +Table Create Table > +t CREATE TABLE `t` ( > + `a` int(11) DEFAULT NULL > +) ENGINE=DEFAULT_ENGINE DEFAULT CHARSET=latin1 WITH SYSTEM VERSIONING > + PARTITION BY SYSTEM_TIME INTERVAL 1 HOUR STARTS TIMESTAMP'2000-01-01 00:00:00' AUTO > +PARTITIONS 2 > +drop table t; > diff --git a/mysql-test/suite/versioning/r/partition.result b/mysql-test/suite/versioning/r/partition.result > --- a/mysql-test/suite/versioning/r/partition.result > +++ b/mysql-test/suite/versioning/r/partition.result > @@ -1236,27 +1270,826 @@ t1 CREATE TABLE `t1` ( > PARTITION `p5` HISTORY ENGINE = DEFAULT_ENGINE, > PARTITION `pn` CURRENT ENGINE = DEFAULT_ENGINE) > alter table t1 add partition partitions 8; > +drop table t1; > +# > +# End of 10.5 tests > +# > +# > +# MDEV-17554 Auto-create new partition for system versioned tables with history partitioned by INTERVAL/LIMIT > +# > +create or replace table t1 (x int) with system versioning > +partition by system_time limit 1 auto; > show create table t1; > Table Create Table > t1 CREATE TABLE `t1` ( > `x` int(11) DEFAULT NULL > ) ENGINE=DEFAULT_ENGINE DEFAULT CHARSET=latin1 WITH SYSTEM VERSIONING > - PARTITION BY SYSTEM_TIME LIMIT 1 > -(PARTITION `p2` HISTORY ENGINE = DEFAULT_ENGINE, > - PARTITION `p8` HISTORY ENGINE = DEFAULT_ENGINE, > + PARTITION BY SYSTEM_TIME LIMIT 1 AUTO > +PARTITIONS 2 > +insert into t1 values (1); > +create or replace table t2 (y int); > +insert into t2 values (2); > +insert into t1 select * from t2; > +insert into t2 select * from t1; > +show create table t1; > +Table Create Table > +t1 CREATE TABLE `t1` ( > + `x` int(11) DEFAULT NULL > +) ENGINE=DEFAULT_ENGINE DEFAULT CHARSET=latin1 WITH SYSTEM VERSIONING > + PARTITION BY SYSTEM_TIME LIMIT 1 AUTO > +PARTITIONS 2 > +# Too many partitions error > +set timestamp= unix_timestamp('2000-01-01 00:00:00'); > +create or replace table t1 (x int) with system versioning > +partition by system_time interval 1 hour auto; > +set timestamp= unix_timestamp('2001-01-01 00:01:00'); > +update t1 set x= x + 1; > +ERROR HY000: Too many partitions (including subpartitions) were defined > +# Partition overflow error and manual fix seems to be a wrong comment? no manual fix here. > +set timestamp= unix_timestamp('2000-01-01 00:00:00'); > +create or replace table t1 (x int) with system versioning > +partition by system_time interval 1 hour; > +insert into t1 values (333); > +set timestamp= unix_timestamp('2000-01-01 01:00:00'); > +update t1 set x= x + 1; > +Warnings: > +Warning 4114 Versioned table `test`.`t1`: last HISTORY partition (`p0`) is out of INTERVAL, need more HISTORY partitions > +drop table t1; > +# Partition overflow error and manual fix > +set timestamp= unix_timestamp('2000-01-01 00:00:00'); > +create or replace table t1 (x int) with system versioning > +partition by system_time interval 1 hour; > +insert into t1 values (440); > +set timestamp= unix_timestamp('2000-01-01 00:10:00'); > +update t1 set x= x + 1; > +# Check how pruning boundaries work > +explain partitions select * from t1 for system_time as of '2000-01-01 00:59:58'; > +id select_type table partitions type possible_keys key key_len ref rows Extra > +1 SIMPLE t1 p0,pn # NULL NULL NULL NULL # # > +explain partitions select * from t1 for system_time as of '2000-01-01 00:59:59'; > +id select_type table partitions type possible_keys key key_len ref rows Extra > +1 SIMPLE t1 pn # NULL NULL NULL NULL # # Why the first select looks in both partitions and the second one - only in pn? > +explain partitions select * from t1 for system_time as of '2000-01-01 01:00:00'; > +id select_type table partitions type possible_keys key key_len ref rows Extra > +1 SIMPLE t1 pn # NULL NULL NULL NULL # # > +select * from t1 for system_time as of '2000-01-01 00:09:59'; > +x > +440 > +set timestamp= unix_timestamp('2000-01-01 02:00:00'); > +update t1 set x= x + 1; > +Warnings: > +Warning 4114 Versioned table `test`.`t1`: last HISTORY partition (`p0`) is out of INTERVAL, need more HISTORY partitions > +select * from t1 for system_time as of '2000-01-01 01:00:00'; > +x > +select * from t1 partition (p0) order by x; > +x > +440 > +441 > +alter table t1 add partition partitions 3; > +select * from t1 for system_time as of '2000-01-01 01:00:00'; > +x > +441 > +select * from t1 partition (p0) order by x; > +x > +440 would be good to have EXPLAINs after the overflow and after the manual fix. to show how ALTER changes what partition will be pruned > +drop table t1; > +create or replace table t1 (x int) with system versioning > +partition by system_time interval 3600 second > +starts '2000-01-01 00:00:00' auto partitions 3; > +affected rows: 0 > +insert into t1 values (1); > +affected rows: 1 > +show create table t1; > +Table Create Table > +t1 CREATE TABLE `t1` ( > + `x` int(11) DEFAULT NULL > +) ENGINE=DEFAULT_ENGINE DEFAULT CHARSET=latin1 WITH SYSTEM VERSIONING > + PARTITION BY SYSTEM_TIME INTERVAL 3600 SECOND STARTS TIMESTAMP'2000-01-01 00:00:00' AUTO > +PARTITIONS 3 > ... > diff --git a/mysql-test/suite/versioning/r/rpl_mix.result b/mysql-test/suite/versioning/r/rpl_mix.result > --- a/mysql-test/suite/versioning/r/rpl_mix.result > +++ b/mysql-test/suite/versioning/r/rpl_mix.result > @@ -8,4 +8,59 @@ DELETE HISTORY FROM t1; > connection slave; > connection master; > drop table t1; > +# > +# MDEV-25347 DML events for auto-partitioned tables are written into binary log twice > +# > +flush binary logs; > +create table t1 (a int) with system versioning > +partition by system_time limit 1 auto; > +insert into t1 values (1); > +update t1 set a= a + 1; > +update t1 set a= a + 1; better do a+2 the second time. otherwise it's a bit confusing, the bug says "written twice" and you indeed have update a=a+1 written twice :) with clearly different statements the test will prove that every event is written once, and not one is skipped and the other is duplicated > +show create table t1; > +Table Create Table > +t1 CREATE TABLE `t1` ( > + `a` int(11) DEFAULT NULL > +) ENGINE=ENGINE DEFAULT CHARSET=latin1 WITH SYSTEM VERSIONING > + PARTITION BY SYSTEM_TIME LIMIT 1 AUTO > +PARTITIONS 3 > +select * from t1; > +a > +3 > +include/show_binlog_events.inc > +Log_name Pos Event_type Server_id End_log_pos Info > +master-bin.000002 # Binlog_checkpoint # # master-bin.000002 > +master-bin.000002 # Gtid # # GTID #-#-# > +master-bin.000002 # Query # # use `test`; create table t1 (a int) with system versioning > +partition by system_time limit 1 auto > +master-bin.000002 # Gtid # # BEGIN GTID #-#-# > +master-bin.000002 # Annotate_rows # # insert into t1 values (1) > +master-bin.000002 # Table_map # # table_id: # (test.t1) > +master-bin.000002 # Write_rows_v1 # # table_id: # flags: STMT_END_F > +master-bin.000002 # Query # # COMMIT > +master-bin.000002 # Gtid # # BEGIN GTID #-#-# > +master-bin.000002 # Annotate_rows # # update t1 set a= a + 1 > +master-bin.000002 # Table_map # # table_id: # (test.t1) > +master-bin.000002 # Update_rows_v1 # # table_id: # > +master-bin.000002 # Write_rows_v1 # # table_id: # flags: STMT_END_F > +master-bin.000002 # Query # # COMMIT > +master-bin.000002 # Gtid # # BEGIN GTID #-#-# > +master-bin.000002 # Annotate_rows # # update t1 set a= a + 1 > +master-bin.000002 # Table_map # # table_id: # (test.t1) > +master-bin.000002 # Update_rows_v1 # # table_id: # > +master-bin.000002 # Write_rows_v1 # # table_id: # flags: STMT_END_F > +master-bin.000002 # Query # # COMMIT > +connection slave; > +show create table t1; > +Table Create Table > +t1 CREATE TABLE `t1` ( > + `a` int(11) DEFAULT NULL > +) ENGINE=ENGINE DEFAULT CHARSET=latin1 WITH SYSTEM VERSIONING > + PARTITION BY SYSTEM_TIME LIMIT 1 AUTO > +PARTITIONS 3 > +select * from t1; > +a > +3 > +connection master; > +drop table t1; > include/rpl_end.inc > diff --git a/mysql-test/suite/versioning/r/rpl_row.result b/mysql-test/suite/versioning/r/rpl_row.result > --- a/mysql-test/suite/versioning/r/rpl_row.result > +++ b/mysql-test/suite/versioning/r/rpl_row.result > @@ -11,4 +11,59 @@ connection slave; > connection master; > drop table t1; > set binlog_row_image= @old_row_image; > +# > +# MDEV-25347 DML events for auto-partitioned tables are written into binary log twice > +# > +flush binary logs; > +create table t1 (a int) with system versioning > +partition by system_time limit 1 auto; > +insert into t1 values (1); > +update t1 set a= a + 1; > +update t1 set a= a + 1; same. as a general rule it's helpful to use slightly different statement, to be able to map them to binlog events uniquely. a+2, or 1+a, or whatever. > +show create table t1; > +Table Create Table > +t1 CREATE TABLE `t1` ( > + `a` int(11) DEFAULT NULL > +) ENGINE=ENGINE DEFAULT CHARSET=latin1 WITH SYSTEM VERSIONING > + PARTITION BY SYSTEM_TIME LIMIT 1 AUTO > +PARTITIONS 3 > +select * from t1; > +a > +3 > +include/show_binlog_events.inc > +Log_name Pos Event_type Server_id End_log_pos Info > +master-bin.000002 # Binlog_checkpoint # # master-bin.000002 > +master-bin.000002 # Gtid # # GTID #-#-# > +master-bin.000002 # Query # # use `test`; create table t1 (a int) with system versioning > +partition by system_time limit 1 auto > +master-bin.000002 # Gtid # # BEGIN GTID #-#-# > +master-bin.000002 # Annotate_rows # # insert into t1 values (1) > +master-bin.000002 # Table_map # # table_id: # (test.t1) > +master-bin.000002 # Write_rows_v1 # # table_id: # flags: STMT_END_F > +master-bin.000002 # Query # # COMMIT > +master-bin.000002 # Gtid # # BEGIN GTID #-#-# > +master-bin.000002 # Annotate_rows # # update t1 set a= a + 1 > +master-bin.000002 # Table_map # # table_id: # (test.t1) > +master-bin.000002 # Update_rows_v1 # # table_id: # > +master-bin.000002 # Write_rows_v1 # # table_id: # flags: STMT_END_F > +master-bin.000002 # Query # # COMMIT > +master-bin.000002 # Gtid # # BEGIN GTID #-#-# > +master-bin.000002 # Annotate_rows # # update t1 set a= a + 1 > +master-bin.000002 # Table_map # # table_id: # (test.t1) > +master-bin.000002 # Update_rows_v1 # # table_id: # > +master-bin.000002 # Write_rows_v1 # # table_id: # flags: STMT_END_F > +master-bin.000002 # Query # # COMMIT > +connection slave; > +show create table t1; > +Table Create Table > +t1 CREATE TABLE `t1` ( > + `a` int(11) DEFAULT NULL > +) ENGINE=ENGINE DEFAULT CHARSET=latin1 WITH SYSTEM VERSIONING > + PARTITION BY SYSTEM_TIME LIMIT 1 AUTO > +PARTITIONS 3 > +select * from t1; > +a > +3 > +connection master; > +drop table t1; > include/rpl_end.inc > diff --git a/mysql-test/suite/versioning/t/partition.test b/mysql-test/suite/versioning/t/partition.test > --- a/mysql-test/suite/versioning/t/partition.test > +++ b/mysql-test/suite/versioning/t/partition.test > @@ -1068,13 +1078,456 @@ alter table t1 add partition partitions 2; > --replace_result $default_engine DEFAULT_ENGINE > show create table t1; > alter table t1 add partition partitions 8; > +drop table t1; > + > +--echo # > +--echo # End of 10.5 tests > +--echo # > + > +--echo # > +--echo # MDEV-17554 Auto-create new partition for system versioned tables with history partitioned by INTERVAL/LIMIT > +--echo # > +create or replace table t1 (x int) with system versioning > +partition by system_time limit 1 auto; > +--replace_result $default_engine DEFAULT_ENGINE > +show create table t1; > + in the previous version of this patch you had clarifying comments here, like --echo # INSERT, INSERT .. SELECT don't auto-create partitions or --echo # Number of partitions goes from 3 to 5 I kind of miss them now, they were helpful > +insert into t1 values (1); > + > +create or replace table t2 (y int); > +insert into t2 values (2); > + > +insert into t1 select * from t2; > +insert into t2 select * from t1; > +--replace_result $default_engine DEFAULT_ENGINE > +show create table t1; > + > ... > diff --git a/sql/ha_partition.h b/sql/ha_partition.h > --- a/sql/ha_partition.h > +++ b/sql/ha_partition.h > @@ -1607,7 +1607,7 @@ class ha_partition :public handler > for (; part_id < part_id_end; ++part_id) > { > handler *file= m_file[part_id]; > - DBUG_ASSERT(bitmap_is_set(&(m_part_info->read_partitions), part_id)); > + bitmap_set_bit(&(m_part_info->read_partitions), part_id); would be good to have a comment before the method that it's only used in vers_set_hist_part(), if (vers_info->limit). because if it's a generic method to return the number of rows in a partition (incl. subpartitions) then it's totally not clear why it modifies read_partitions bitmap. or may be you'd want to rename the method to make it look more specific for LIMIT partitions in vers_set_hist_part() ? > file->info(HA_STATUS_VARIABLE | HA_STATUS_NO_LOCK | HA_STATUS_OPEN); > part_recs+= file->stats.records; > } > diff --git a/sql/handler.cc b/sql/handler.cc > --- a/sql/handler.cc > +++ b/sql/handler.cc > @@ -1574,6 +1574,8 @@ int ha_commit_trans(THD *thd, bool all) > DBUG_ASSERT(thd->transaction->stmt.ha_list == NULL || > trans == &thd->transaction->stmt); > > + DBUG_ASSERT(!thd->in_sub_stmt); > if (thd->in_sub_stmt) > { > DBUG_ASSERT(0); This is a bit redundant, don't you think? One DBUG_ASSERT would be enough. (personally, I'd prefer the one you added, not DBUG_ASSERT(0)) > diff --git a/sql/lock.cc b/sql/lock.cc > --- a/sql/lock.cc > +++ b/sql/lock.cc > @@ -662,16 +662,28 @@ MYSQL_LOCK *mysql_lock_merge(MYSQL_LOCK *a,MYSQL_LOCK *b) > DBUG_PRINT("enter", ("a->lock_count: %u b->lock_count: %u", > a->lock_count, b->lock_count)); > > - if (!(sql_lock= (MYSQL_LOCK*) > - my_malloc(key_memory_MYSQL_LOCK, sizeof(*sql_lock) + > - sizeof(THR_LOCK_DATA*)*((a->lock_count+b->lock_count)*2) + > - sizeof(TABLE*)*(a->table_count+b->table_count),MYF(MY_WME)))) > - DBUG_RETURN(0); // Fatal error > + const size_t lock_size= sizeof(*sql_lock) + > + sizeof(THR_LOCK_DATA *) * ((a->lock_count + b->lock_count) * 2) + > + sizeof(TABLE *) * (a->table_count + b->table_count); > + if (thd) > + { > + sql_lock= (MYSQL_LOCK *) thd->alloc(lock_size); > + if (!sql_lock) > + DBUG_RETURN(0); > + sql_lock->flags= GET_LOCK_ON_THD; why? the commit comment for MDEV-23639 doesn't mention it. generally, I think it's a good idea and, likely, should be used more than just in recover_from_failed_open(). But why right now and in this commit? Does this commit (22158b3c) rely on locks being on THD memroot? How so? > + } > + else > + { > + sql_lock= (MYSQL_LOCK *) > + my_malloc(key_memory_MYSQL_LOCK, lock_size, MYF(MY_WME)); > + if (!sql_lock) > + DBUG_RETURN(0); > + sql_lock->flags= 0; > + } > sql_lock->lock_count=a->lock_count+b->lock_count; > sql_lock->table_count=a->table_count+b->table_count; > sql_lock->locks=(THR_LOCK_DATA**) (sql_lock+1); > sql_lock->table=(TABLE**) (sql_lock->locks+sql_lock->lock_count*2); > - sql_lock->flags= 0; > memcpy(sql_lock->locks,a->locks,a->lock_count*sizeof(*a->locks)); > memcpy(sql_lock->locks+a->lock_count,b->locks, > b->lock_count*sizeof(*b->locks)); > @@ -705,8 +717,10 @@ MYSQL_LOCK *mysql_lock_merge(MYSQL_LOCK *a,MYSQL_LOCK *b) > a->lock_count, b->lock_count); > > /* Delete old, not needed locks */ > - my_free(a); > - my_free(b); > + if (!(a->flags & GET_LOCK_ON_THD)) > + my_free(a); > + if (!(b->flags & GET_LOCK_ON_THD)) > + my_free(b); this even looks like a bug, no check that old locks weren't on THD memroot. (probably impossible in this particular case, but still rather fragile) thanks! > DBUG_RETURN(sql_lock); > } > > diff --git a/sql/share/errmsg-utf8.txt b/sql/share/errmsg-utf8.txt > --- a/sql/share/errmsg-utf8.txt > +++ b/sql/share/errmsg-utf8.txt > @@ -7985,3 +7985,5 @@ ER_JSON_TABLE_MULTIPLE_MATCHES > eng "Can't store multiple matches of the path in the column '%s' of JSON_TABLE '%s'." > ER_WITH_TIES_NEEDS_ORDER > eng "FETCH ... WITH TIES requires ORDER BY clause to be present" > +ER_VERS_HIST_PART_FAILED > + eng "Versioned table %`s.%`s: adding HISTORY partition failed" Can you come up with a test for this error? > diff --git a/sql/sql_base.cc b/sql/sql_base.cc > --- a/sql/sql_base.cc > +++ b/sql/sql_base.cc > @@ -931,7 +934,7 @@ void close_thread_table(THD *thd, TABLE **table_ptr) > DBUG_PRINT("tcache", ("table: '%s'.'%s' %p", table->s->db.str, > table->s->table_name.str, table)); > DBUG_ASSERT(!table->file->keyread_enabled()); > - DBUG_ASSERT(!table->file || table->file->inited == handler::NONE); > + DBUG_ASSERT(table->file->inited == handler::NONE); indeed > > /* > The metadata lock must be released after giving back > @@ -1625,6 +1625,125 @@ bool is_locked_view(THD *thd, TABLE_LIST *t) > } > > > +#ifdef WITH_PARTITION_STORAGE_ENGINE > +/** > + Switch part_info->hist_part and request partition creation if needed. > + > + @retval true Error or partition creation was requested. > + @retval false No error > +*/ > +bool TABLE::vers_switch_partition(THD *thd, TABLE_LIST *table_list, > + Open_table_context *ot_ctx) > +{ > + if (!part_info || part_info->part_type != VERSIONING_PARTITION || > + table_list->vers_conditions.delete_history || > + thd->stmt_arena->is_stmt_prepare() || > + table_list->lock_type < TL_WRITE_ALLOW_WRITE || > + table_list->mdl_request.type < MDL_SHARED_WRITE || > + table_list->mdl_request.type == MDL_EXCLUSIVE) > + { > + return false; > + } > + > + switch (thd->lex->sql_command) > + { > + case SQLCOM_INSERT: > + if (thd->lex->duplicates != DUP_UPDATE) > + return false; > + break; > + case SQLCOM_LOAD: > + if (thd->lex->duplicates != DUP_REPLACE) > + return false; > + break; > + case SQLCOM_LOCK_TABLES: > + case SQLCOM_DELETE: > + case SQLCOM_UPDATE: > + case SQLCOM_REPLACE: > + case SQLCOM_REPLACE_SELECT: > + case SQLCOM_DELETE_MULTI: > + case SQLCOM_UPDATE_MULTI: > + break; > + default: > + /* > + TODO: make row events set thd->lex->sql_command appropriately. > + > + Sergei Golubchik: f.ex. currently row events increment > + thd->status_var.com_stat[] each event for its own SQLCOM_xxx, it won't be > + needed if they'll just set thd->lex->sql_command. > + */ > + if (thd->rgi_slave && thd->rgi_slave->current_event && > + thd->lex->sql_command == SQLCOM_END) > + { > + switch (thd->rgi_slave->current_event->get_type_code()) > + { > + case UPDATE_ROWS_EVENT: > + case UPDATE_ROWS_EVENT_V1: > + case DELETE_ROWS_EVENT: > + case DELETE_ROWS_EVENT_V1: > + break; > + default:; > + return false; > + } > + } > + break; > + } > + > + TABLE *table= this; > + > + /* > + NOTE: The semantics of vers_set_hist_part() is double: even when we twofold > + don't need auto-create, we need to update part_info->hist_part. > + */ > + uint *create_count= table_list->vers_skip_create ? > + NULL : &ot_ctx->vers_create_count; > + table_list->vers_skip_create= true; > + if (table->part_info->vers_set_hist_part(thd, create_count)) > + { > + MYSQL_UNBIND_TABLE(table->file); > + tc_release_table(table); > + return true; > + } > + if (ot_ctx->vers_create_count) > + { > + Open_table_context::enum_open_table_action action; > + TABLE_LIST *table_arg; > + mysql_mutex_lock(&table->s->LOCK_share); > + if (!table->s->vers_skip_auto_create) > + { > + table->s->vers_skip_auto_create= true; > + action= Open_table_context::OT_ADD_HISTORY_PARTITION; > + table_arg= table_list; > + } > + else > + { > + /* > + NOTE: this may repeat multiple times until creating thread acquires > + MDL_EXCLUSIVE. Since auto-creation is rare operation this is acceptable. > + We could suspend this thread on cond-var but we must first exit > + MDL_SHARED_WRITE first and we cannot store cond-var into TABLE_SHARE > + because it is already released and there is no guarantee that it will > + be same instance if we acquire it again. > + */ > + table_list->vers_skip_create= false; > + ot_ctx->vers_create_count= 0; > + action= Open_table_context::OT_REOPEN_TABLES; > + table_arg= NULL; > + } I'm afraid I don't understand. All this business with vers_skip_create and vers_skip_auto_create, it wasn't in the previous version of the patch. So, I believe it was a fix for one of the MDEV bugs reported and fixed meanwhile. What MDEV was it? Could you explain what was the issue and what was the fix, please? > + mysql_mutex_unlock(&table->s->LOCK_share); > + if (!thd->locked_tables_mode) > + { > + MYSQL_UNBIND_TABLE(table->file); > + tc_release_table(table); > + } > + ot_ctx->request_backoff_action(action, table_arg); > + return true; > + } > + > + return false; > +} > +#endif /* WITH_PARTITION_STORAGE_ENGINE */ > + > + > /** > Open a base table. > > @@ -2572,11 +2699,13 @@ void Locked_tables_list::mark_table_for_reopen(THD *thd, TABLE *table) > table_list; table_list= table_list->next_global) > { > if (table_list->table->s == share) > + { > table_list->table->internal_set_needs_reopen(true); > + some_table_marked_for_reopen= 1; > + } > } > /* This is needed in the case where lock tables where not used */ > table->internal_set_needs_reopen(true); why ^^^ this doesn't count as "some_table_marked_for_reopen" ? > - some_table_marked_for_reopen= 1; > } > > > @@ -3172,13 +3304,49 @@ Open_table_context::recover_from_failed_open() > break; > case OT_DISCOVER: > case OT_REPAIR: > - if ((result= lock_table_names(m_thd, m_thd->lex->create_info, > - m_failed_table, NULL, > - get_timeout(), 0))) > + case OT_ADD_HISTORY_PARTITION: > + if (!m_thd->locked_tables_mode) > + result= lock_table_names(m_thd, m_thd->lex->create_info, m_failed_table, > + NULL, get_timeout(), 0); > + else > + { > + DBUG_ASSERT(!result); > + DBUG_ASSERT(m_action == OT_ADD_HISTORY_PARTITION); It seems you've solved recover_from_failed_open under LOCK TABLES, so it could be possible to make OT_DISCOVER and OT_REPAIR to work there too. Is that right? It's just a question, I don't imply it should be part of this MDEV. > + } > + /* > + We are now under MDL_EXCLUSIVE mode. Other threads have no table share > + acquired: they are blocked either at open_table_get_mdl_lock() in > + open_table() or at lock_table_names() here. > + */ > + if (result) > + { > + if (m_action == OT_ADD_HISTORY_PARTITION) > + { > + TABLE_SHARE *share= tdc_acquire_share(m_thd, m_failed_table, > + GTS_TABLE, NULL); > + if (share) > + { > + share->vers_skip_auto_create= false; > + tdc_release_share(share); > + } > + if (m_thd->get_stmt_da()->sql_errno() == ER_LOCK_WAIT_TIMEOUT) > + { > + // MDEV-23642 Locking timeout caused by auto-creation affects original DML > + m_thd->clear_error(); > + vers_create_count= 0; > + result= false; > + } > + } > break; > + } > > - tdc_remove_table(m_thd, m_failed_table->db.str, > - m_failed_table->table_name.str); > + /* > + We don't need to remove share under OT_ADD_HISTORY_PARTITION. > + Moreover fast_alter_partition_table() works with TABLE instance. > + */ > + if (m_action != OT_ADD_HISTORY_PARTITION) > + tdc_remove_table(m_thd, m_failed_table->db.str, > + m_failed_table->table_name.str); > > switch (m_action) > { > @@ -3206,6 +3374,70 @@ Open_table_context::recover_from_failed_open() > case OT_REPAIR: > result= auto_repair_table(m_thd, m_failed_table); > break; > + case OT_ADD_HISTORY_PARTITION: > + { > + result= false; > + TABLE *table= open_ltable(m_thd, m_failed_table, TL_WRITE, > + MYSQL_OPEN_HAS_MDL_LOCK | MYSQL_OPEN_IGNORE_LOGGING_FORMAT); > + if (table == NULL) > + { > + m_thd->clear_error(); > + break; > + } > + > + DBUG_ASSERT(vers_create_count); > + result= vers_create_partitions(m_thd, m_failed_table, vers_create_count); > + vers_create_count= 0; > + if (!m_thd->transaction->stmt.is_empty()) > + trans_commit_stmt(m_thd); > + DBUG_ASSERT(!result || > + !m_thd->locked_tables_mode || > + m_thd->lock->lock_count); > + if (result) > + break; > + if (!m_thd->locked_tables_mode) > + { > + /* > + alter_partition_lock_handling() does mysql_lock_remove() but > + does not clear thd->lock completely. > + */ > + DBUG_ASSERT(m_thd->lock->lock_count == 0); > + if (!(m_thd->lock->flags & GET_LOCK_ON_THD)) > + my_free(m_thd->lock); > + m_thd->lock= NULL; > + } > + else if (m_thd->locked_tables_mode == LTM_PRELOCKED) > + { > + MYSQL_LOCK *lock; > + MYSQL_LOCK *merged_lock; > + > + /* > + In LTM_LOCK_TABLES table was reopened via locked_tables_list, > + but not in prelocked environment where we have to reopen > + the table manually. > + */ > + Open_table_context ot_ctx(m_thd, MYSQL_OPEN_REOPEN); > + if (open_table(m_thd, m_failed_table, &ot_ctx)) > + { > + result= true; > + break; > + } > + TABLE *table= m_failed_table->table; > + table->reginfo.lock_type= m_thd->update_lock_default; > + m_thd->in_lock_tables= 1; > + lock= mysql_lock_tables(m_thd, &table, 1, > + MYSQL_OPEN_REOPEN | MYSQL_LOCK_USE_MALLOC); > + m_thd->in_lock_tables= 0; > + if (lock == NULL || > + !(merged_lock= mysql_lock_merge(m_thd->lock, lock, m_thd))) > + { > + result= true; > + break; > + } > + m_thd->lock= merged_lock; What MDEV does it fix? I wonder why other cases in recover_from_failed_open doesn't do that > + } > + break; > + } > case OT_BACKOFF_AND_RETRY: > case OT_REOPEN_TABLES: > case OT_NO_ACTION: > @@ -4370,6 +4611,8 @@ bool open_tables(THD *thd, const DDL_options_st &options, > { > if (ot_ctx.can_recover_from_failed_open()) > { > + // FIXME: is this really used? > + DBUG_ASSERT(0); I take it, you weren't able to get this assert to fire? > close_tables_for_reopen(thd, start, > ot_ctx.start_of_statement_svp()); > if (ot_ctx.recover_from_failed_open()) Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 13

[Maria-developers] Progress of improve IO_CACHE (MDEV-24676)
by Vladisav Kakurin 20 Jul '21

20 Jul '21

To this moment I have implemented medium-grained granularity, and also move IO_CACHE to C++ entitled RingBuffer. In future, it will be fine-grained with slots-pattern. https://github.com/MagHErmit/server/commit/7c066a97403115f6d7702049b741d466…

1 0

Re: [Maria-developers] [Commits] 6f1628b: MDEV-25565 Crash on 2-nd execution of SP/PS for query calculating window functions
by Vicențiu Ciorbaru 20 Jul '21

20 Jul '21

Hi Igor! The patch looks good, however I suggest the following changes to make it smaller and easier to read & maintain. 1. You do not need to have special if statements for save_partition_list and save_order_list when you do the cleanup. 2. You do not need to have special code that saves the partition list and the order list *only* when it is overwritten. You can instead always set save_partition_list to be equal to partition_list in the Window_spec's constructor. Same for save_order_list and order_list. Then, during cleanup, you can always execute order_list= save_order_list; partition_list= save_partition_list; The advantage of this change is that it removes all the references to save_xxxx_list in sql_window.cc. You then only have a simple cleanup function sql_union.cc. 3. I would prefer to name the variables "initial_order_list" and "initial_partition_list". The patch then becomes: diff --git a/sql/sql_union.cc b/sql/sql_union.cc index e5648e6989b..54a33bb7863 100644 --- a/sql/sql_union.cc +++ b/sql/sql_union.cc @@ -30,6 +30,7 @@ #include "filesort.h" // filesort_free_buffers #include "sql_view.h" #include "sql_cte.h" +#include "item_windowfunc.h" bool mysql_union(THD *thd, LEX *lex, select_result *result, SELECT_LEX_UNIT *unit, ulong setup_tables_done_option) @@ -1551,6 +1552,21 @@ static void cleanup_order(ORDER *order) } +static void cleanup_window_funcs(List<Item_window_func> &win_funcs) +{ + List_iterator_fast<Item_window_func> it(win_funcs); + Item_window_func *win_func; + while ((win_func= it++)) + { + Window_spec *win_spec= win_func->window_spec; + if (!win_spec) + continue; + win_spec->partition_list= win_spec->initial_partition_list; + win_spec->order_list= win_spec->initial_order_list; + } +} + + bool st_select_lex::cleanup() { bool error= FALSE; @@ -1559,6 +1575,8 @@ bool st_select_lex::cleanup() cleanup_order(order_list.first); cleanup_order(group_list.first); + cleanup_window_funcs(window_funcs); + if (join) { List_iterator<TABLE_LIST> ti(leaf_tables); diff --git a/sql/sql_window.h b/sql/sql_window.h index e0c1563e5bb..cf84e26c822 100644 --- a/sql/sql_window.h +++ b/sql/sql_window.h @@ -99,8 +99,10 @@ class Window_spec : public Sql_alloc LEX_STRING *window_ref; SQL_I_List<ORDER> *partition_list; + SQL_I_List<ORDER> *initial_partition_list; SQL_I_List<ORDER> *order_list; + SQL_I_List<ORDER> *initial_order_list; Window_frame *window_frame; @@ -111,7 +113,8 @@ class Window_spec : public Sql_alloc SQL_I_List<ORDER> *ord_list, Window_frame *win_frame) : window_names_are_checked(false), window_ref(win_ref), - partition_list(part_list), order_list(ord_list), + partition_list(part_list), initial_partition_list(part_list), + order_list(ord_list), initial_order_list(ord_list), window_frame(win_frame), referenced_win_spec(NULL) {} virtual char *name() { return NULL; } On Sat, 10 Jul 2021 at 04:57, IgorBabaev <igor(a)mariadb.com> wrote: > revision-id: 6f1628b917d365ecfc9c4c9951011613f4212592 > (mariadb-10.2.31-1048-g6f1628b) > parent(s): fb0b28932ce82903f2fcfb690a71bff52355507f > author: Igor Babaev > committer: Igor Babaev > timestamp: 2021-07-09 18:56:34 -0700 > message: > > MDEV-25565 Crash on 2-nd execution of SP/PS for query calculating window > functions > from view > > A crash of the server happened when executing a stored procedure whose the > only query calculated window functions over a mergeable view specified > as a select from non-mergeable view. The crash could be reproduced if > the window specifications of the window functions were identical and both > contained PARTITION lists and ORDER BY lists. A crash also happened on > the second execution of the prepared statement created for such query. > If to use derived tables or CTE instead of views the problem still > manifests itself crashing the server. > > When optimizing the window specifications of a window function the > server can substitute the partition lists and the order lists for > the corresponding lists from another window specification in the case > when the lists are identical. This substitution is not permanent and should > be rolled back before the second execution. It was not done and this > ultimately led to a crash when resolving the column names at the second > execution of SP/PS. > > --- > mysql-test/r/win.result | 287 > ++++++++++++++++++++++++++++++++++++++++++++++++ > mysql-test/t/win.test | 147 +++++++++++++++++++++++++ > sql/sql_union.cc | 26 +++++ > sql/sql_window.cc | 12 ++ > sql/sql_window.h | 5 +- > 5 files changed, 476 insertions(+), 1 deletion(-) > > diff --git a/mysql-test/r/win.result b/mysql-test/r/win.result > index 8a31dcc..bc017ea 100644 > --- a/mysql-test/r/win.result > +++ b/mysql-test/r/win.result > @@ -3911,5 +3911,292 @@ sum(i) over () IN ( SELECT 1 FROM t1 a) > 0 > DROP TABLE t1; > # > +# MDEV-25565: 2-nd call of SP with SELECT from view / derived table / CTE > +# returning the result of calculation of 2 window > +# functions that use the same window specification > +# > +create table t1 (a int); > +insert into t1 values (3), (7), (1), (7), (1), (1), (3), (1), (5); > +create view v2 as select a from t1 group by a; > +create view v1 as select * from v2; > +create procedure sp1() select v1.a, > +sum(v1.a) over (partition by v1.a order by v1.a) as k, > +avg(v1.a) over (partition by v1.a order by v1.a) as m > +from v1; > +call sp1(); > +a k m > +1 1 1.0000 > +3 3 3.0000 > +5 5 5.0000 > +7 7 7.0000 > +call sp1(); > +a k m > +1 1 1.0000 > +3 3 3.0000 > +5 5 5.0000 > +7 7 7.0000 > +prepare stmt from "select v1.a, > +sum(v1.a) over (partition by v1.a order by v1.a) as k, > +avg(v1.a) over (partition by v1.a order by v1.a) as m > +from v1"; > +execute stmt; > +a k m > +1 1 1.0000 > +3 3 3.0000 > +5 5 5.0000 > +7 7 7.0000 > +execute stmt; > +a k m > +1 1 1.0000 > +3 3 3.0000 > +5 5 5.0000 > +7 7 7.0000 > +deallocate prepare stmt; > +create procedure sp2() select * from > +( select dt1.a, > +sum(dt1.a) over (partition by dt1.a order by dt1.a) as k, > +avg(dt1.a) over (partition by dt1.a order by dt1.a) as m > +from (select * from v2) as dt1 > +) as dt; > +call sp2(); > +a k m > +1 1 1.0000 > +3 3 3.0000 > +5 5 5.0000 > +7 7 7.0000 > +call sp2(); > +a k m > +1 1 1.0000 > +3 3 3.0000 > +5 5 5.0000 > +7 7 7.0000 > +prepare stmt from "select * from > +( select dt1.a, > +sum(dt1.a) over (partition by dt1.a order by dt1.a) as k, > +avg(dt1.a) over (partition by dt1.a order by dt1.a) as m > +from (select * from v2) as dt1 > +) as dt"; > +execute stmt; > +a k m > +1 1 1.0000 > +3 3 3.0000 > +5 5 5.0000 > +7 7 7.0000 > +execute stmt; > +a k m > +1 1 1.0000 > +3 3 3.0000 > +5 5 5.0000 > +7 7 7.0000 > +deallocate prepare stmt; > +create procedure sp3() select * from > +( select dt1.a, > +sum(dt1.a) over (partition by dt1.a order by dt1.a) as k, > +avg(dt1.a) over (partition by dt1.a order by dt1.a) as m > +from ( select * from (select * from t1 group by a) as dt2 ) as dt1 > +) as dt; > +call sp3(); > +a k m > +1 1 1.0000 > +3 3 3.0000 > +5 5 5.0000 > +7 7 7.0000 > +call sp3(); > +a k m > +1 1 1.0000 > +3 3 3.0000 > +5 5 5.0000 > +7 7 7.0000 > +prepare stmt from "select * from > +( select dt1.a, > +sum(dt1.a) over (partition by dt1.a order by dt1.a) as k, > +avg(dt1.a) over (partition by dt1.a order by dt1.a) as m > +from ( select * from (select * from t1 group by a) as dt2 ) as dt1 > +) as dt"; > +execute stmt; > +a k m > +1 1 1.0000 > +3 3 3.0000 > +5 5 5.0000 > +7 7 7.0000 > +execute stmt; > +a k m > +1 1 1.0000 > +3 3 3.0000 > +5 5 5.0000 > +7 7 7.0000 > +deallocate prepare stmt; > +create procedure sp4() with cte1 as (select * from (select * from t1 > group by a) as dt2), > +cte as > +( select cte1.a, > +sum(cte1.a) over (partition by cte1.a order by cte1.a) as k, > +avg(cte1.a) over (partition by cte1.a order by cte1.a) as m > +from cte1 ) > +select * from cte; > +call sp4(); > +a k m > +1 1 1.0000 > +3 3 3.0000 > +5 5 5.0000 > +7 7 7.0000 > +call sp4(); > +a k m > +1 1 1.0000 > +3 3 3.0000 > +5 5 5.0000 > +7 7 7.0000 > +prepare stmt from "with cte1 as (select * from (select * from t1 group by > a) as dt2), > +cte as > +( select cte1.a, > +sum(cte1.a) over (partition by cte1.a order by cte1.a) as k, > +avg(cte1.a) over (partition by cte1.a order by cte1.a) as m > +from cte1 ) > +select * from cte"; > +execute stmt; > +a k m > +1 1 1.0000 > +3 3 3.0000 > +5 5 5.0000 > +7 7 7.0000 > +execute stmt; > +a k m > +1 1 1.0000 > +3 3 3.0000 > +5 5 5.0000 > +7 7 7.0000 > +deallocate prepare stmt; > +create procedure sp5() with cte1 as (select * from v2), > +cte as > +( select cte1.a, > +sum(cte1.a) over (partition by cte1.a order by cte1.a) as k, > +avg(cte1.a) over (partition by cte1.a order by cte1.a) as m > +from cte1 ) > +select * from cte; > +call sp5(); > +a k m > +1 1 1.0000 > +3 3 3.0000 > +5 5 5.0000 > +7 7 7.0000 > +call sp5(); > +a k m > +1 1 1.0000 > +3 3 3.0000 > +5 5 5.0000 > +7 7 7.0000 > +prepare stmt from "with cte1 as (select * from v2), > +cte as > +( select cte1.a, > +sum(cte1.a) over (partition by cte1.a order by cte1.a) as k, > +avg(cte1.a) over (partition by cte1.a order by cte1.a) as m > +from cte1 ) > +select * from cte"; > +execute stmt; > +a k m > +1 1 1.0000 > +3 3 3.0000 > +5 5 5.0000 > +7 7 7.0000 > +execute stmt; > +a k m > +1 1 1.0000 > +3 3 3.0000 > +5 5 5.0000 > +7 7 7.0000 > +deallocate prepare stmt; > +create procedure sp6() with > +cte1 as (with cte2 as (select * from t1 group by a) select * from cte2), > +cte as > +( select cte1.a, > +sum(cte1.a) over (partition by cte1.a order by cte1.a) as k, > +avg(cte1.a) over (partition by cte1.a order by cte1.a) as m > +from cte1 ) > +select * from cte; > +call sp6(); > +a k m > +1 1 1.0000 > +3 3 3.0000 > +5 5 5.0000 > +7 7 7.0000 > +call sp6(); > +a k m > +1 1 1.0000 > +3 3 3.0000 > +5 5 5.0000 > +7 7 7.0000 > +prepare stmt from "with > +cte1 as (with cte2 as (select * from t1 group by a) select * from cte2), > +cte as > +( select cte1.a, > +sum(cte1.a) over (partition by cte1.a order by cte1.a) as k, > +avg(cte1.a) over (partition by cte1.a order by cte1.a) as m > +from cte1 ) > +select * from cte"; > +execute stmt; > +a k m > +1 1 1.0000 > +3 3 3.0000 > +5 5 5.0000 > +7 7 7.0000 > +execute stmt; > +a k m > +1 1 1.0000 > +3 3 3.0000 > +5 5 5.0000 > +7 7 7.0000 > +deallocate prepare stmt; > +create procedure sp7() with > +cte2 as (select * from v1), > +cte1 as (select * from cte2), > +cte as > +( select cte1.a, > +sum(cte1.a) over (partition by cte1.a order by cte1.a) as k, > +avg(cte1.a) over (partition by cte1.a order by cte1.a) as m > +from cte1 ) > +select * from cte; > +call sp7(); > +a k m > +1 1 1.0000 > +3 3 3.0000 > +5 5 5.0000 > +7 7 7.0000 > +call sp7(); > +a k m > +1 1 1.0000 > +3 3 3.0000 > +5 5 5.0000 > +7 7 7.0000 > +prepare stmt from "with > +cte2 as (select * from v1), > +cte1 as (select * from cte2), > +cte as > +( select cte1.a, > +sum(cte1.a) over (partition by cte1.a order by cte1.a) as k, > +avg(cte1.a) over (partition by cte1.a order by cte1.a) as m > +from cte1 ) > +select * from cte"; > +execute stmt; > +a k m > +1 1 1.0000 > +3 3 3.0000 > +5 5 5.0000 > +7 7 7.0000 > +execute stmt; > +a k m > +1 1 1.0000 > +3 3 3.0000 > +5 5 5.0000 > +7 7 7.0000 > +deallocate prepare stmt; > +drop procedure sp1; > +drop procedure sp2; > +drop procedure sp3; > +drop procedure sp4; > +drop procedure sp5; > +drop procedure sp6; > +drop procedure sp7; > +drop view v1,v2; > +drop table t1; > +# > # End of 10.2 tests > # > diff --git a/mysql-test/t/win.test b/mysql-test/t/win.test > index c07a81f..72e789d 100644 > --- a/mysql-test/t/win.test > +++ b/mysql-test/t/win.test > @@ -2557,5 +2557,152 @@ SELECT sum(i) over () IN ( SELECT 1 FROM t1 a) > FROM t1; > DROP TABLE t1; > > --echo # > +--echo # MDEV-25565: 2-nd call of SP with SELECT from view / derived > table / CTE > +--echo # returning the result of calculation of 2 window > +--echo # functions that use the same window specification > +--echo # > + > +create table t1 (a int); > +insert into t1 values (3), (7), (1), (7), (1), (1), (3), (1), (5); > + > +create view v2 as select a from t1 group by a; > +create view v1 as select * from v2; > + > +let $q1= > +select v1.a, > + sum(v1.a) over (partition by v1.a order by v1.a) as k, > + avg(v1.a) over (partition by v1.a order by v1.a) as m > +from v1; > + > +eval create procedure sp1() $q1; > +call sp1(); > +call sp1(); > + > +eval prepare stmt from "$q1"; > +execute stmt; > +execute stmt; > +deallocate prepare stmt; > + > +let $q2= > +select * from > + ( select dt1.a, > + sum(dt1.a) over (partition by dt1.a order by dt1.a) as k, > + avg(dt1.a) over (partition by dt1.a order by dt1.a) as m > + from (select * from v2) as dt1 > + ) as dt; > + > +eval create procedure sp2() $q2; > +call sp2(); > +call sp2(); > + > +eval prepare stmt from "$q2"; > +execute stmt; > +execute stmt; > +deallocate prepare stmt; > + > +let $q3= > +select * from > + ( select dt1.a, > + sum(dt1.a) over (partition by dt1.a order by dt1.a) as k, > + avg(dt1.a) over (partition by dt1.a order by dt1.a) as m > + from ( select * from (select * from t1 group by a) as dt2 ) as dt1 > + ) as dt; > + > +eval create procedure sp3() $q3; > +call sp3(); > +call sp3(); > + > +eval prepare stmt from "$q3"; > +execute stmt; > +execute stmt; > +deallocate prepare stmt; > + > +let $q4= > +with cte1 as (select * from (select * from t1 group by a) as dt2), > + cte as > + ( select cte1.a, > + sum(cte1.a) over (partition by cte1.a order by cte1.a) as > k, > + avg(cte1.a) over (partition by cte1.a order by cte1.a) as m > + from cte1 ) > +select * from cte; > + > +eval create procedure sp4() $q4; > +call sp4(); > +call sp4(); > + > +eval prepare stmt from "$q4"; > +execute stmt; > +execute stmt; > +deallocate prepare stmt; > + > +let $q5= > +with cte1 as (select * from v2), > + cte as > + ( select cte1.a, > + sum(cte1.a) over (partition by cte1.a order by cte1.a) as > k, > + avg(cte1.a) over (partition by cte1.a order by cte1.a) as m > + from cte1 ) > +select * from cte; > + > +eval create procedure sp5() $q5; > +call sp5(); > +call sp5(); > + > +eval prepare stmt from "$q5"; > +execute stmt; > +execute stmt; > +deallocate prepare stmt; > + > +let $q6= > +with > +cte1 as (with cte2 as (select * from t1 group by a) select * from cte2), > + cte as > + ( select cte1.a, > + sum(cte1.a) over (partition by cte1.a order by cte1.a) as > k, > + avg(cte1.a) over (partition by cte1.a order by cte1.a) as m > + from cte1 ) > +select * from cte; > + > +eval create procedure sp6() $q6; > +call sp6(); > +call sp6(); > + > +eval prepare stmt from "$q6"; > +execute stmt; > +execute stmt; > +deallocate prepare stmt; > + > +let $q7= > +with > + cte2 as (select * from v1), > + cte1 as (select * from cte2), > + cte as > + ( select cte1.a, > + sum(cte1.a) over (partition by cte1.a order by cte1.a) as > k, > + avg(cte1.a) over (partition by cte1.a order by cte1.a) as m > + from cte1 ) > +select * from cte; > + > +eval create procedure sp7() $q7; > +call sp7(); > +call sp7(); > + > +eval prepare stmt from "$q7"; > +execute stmt; > +execute stmt; > +deallocate prepare stmt; > + > + > +drop procedure sp1; > +drop procedure sp2; > +drop procedure sp3; > +drop procedure sp4; > +drop procedure sp5; > +drop procedure sp6; > +drop procedure sp7; > +drop view v1,v2; > +drop table t1; > + > +--echo # > --echo # End of 10.2 tests > --echo # > diff --git a/sql/sql_union.cc b/sql/sql_union.cc > index 7baedfb..f3c90b8 100644 > --- a/sql/sql_union.cc > +++ b/sql/sql_union.cc > @@ -30,6 +30,7 @@ > #include "filesort.h" // filesort_free_buffers > #include "sql_view.h" > #include "sql_cte.h" > +#include "item_windowfunc.h" > > bool mysql_union(THD *thd, LEX *lex, select_result *result, > SELECT_LEX_UNIT *unit, ulong setup_tables_done_option) > @@ -1550,6 +1551,29 @@ static void cleanup_order(ORDER *order) > } > > > +static void cleanup_window_funcs(List<Item_window_func> &win_funcs) > +{ > + List_iterator_fast<Item_window_func> it(win_funcs); > + Item_window_func *win_func; > + while ((win_func= it++)) > + { > + Window_spec *win_spec= win_func->window_spec; > + if (!win_spec) > + continue; > + if (win_spec->save_partition_list) > + { > + win_spec->partition_list= win_spec->save_partition_list; > + win_spec->save_partition_list= NULL; > + } > + if (win_spec->save_order_list) > + { > + win_spec->order_list= win_spec->save_order_list; > + win_spec->save_order_list= NULL; > + } > + } > +} > + > + > bool st_select_lex::cleanup() > { > bool error= FALSE; > @@ -1558,6 +1582,8 @@ bool st_select_lex::cleanup() > cleanup_order(order_list.first); > cleanup_order(group_list.first); > > + cleanup_window_funcs(window_funcs); > + > if (join) > { > List_iterator<TABLE_LIST> ti(leaf_tables); > diff --git a/sql/sql_window.cc b/sql/sql_window.cc > index 612c6e6..3ef751b 100644 > --- a/sql/sql_window.cc > +++ b/sql/sql_window.cc > @@ -479,9 +479,15 @@ int > compare_window_funcs_by_window_specs(Item_window_func *win_func1, > Let's use only one of the lists. > */ > if (!win_spec1->name() && win_spec2->name()) > + { > + win_spec1->save_partition_list= win_spec1->partition_list; > win_spec1->partition_list= win_spec2->partition_list; > + } > else > + { > + win_spec2->save_partition_list= win_spec2->partition_list; > win_spec2->partition_list= win_spec1->partition_list; > + } > > cmp= compare_order_lists(win_spec1->order_list, > win_spec2->order_list); > @@ -494,9 +500,15 @@ int > compare_window_funcs_by_window_specs(Item_window_func *win_func1, > Let's use only one of the lists. > */ > if (!win_spec1->name() && win_spec2->name()) > + { > + win_spec1->save_order_list= win_spec2->order_list; > win_spec1->order_list= win_spec2->order_list; > + } > else > + { > + win_spec1->save_order_list= win_spec2->order_list; > win_spec2->order_list= win_spec1->order_list; > + } > > cmp= compare_window_frames(win_spec1->window_frame, > win_spec2->window_frame); > diff --git a/sql/sql_window.h b/sql/sql_window.h > index e0c1563..417d0bc 100644 > --- a/sql/sql_window.h > +++ b/sql/sql_window.h > @@ -99,8 +99,10 @@ class Window_spec : public Sql_alloc > LEX_STRING *window_ref; > > SQL_I_List<ORDER> *partition_list; > + SQL_I_List<ORDER> *save_partition_list; > > SQL_I_List<ORDER> *order_list; > + SQL_I_List<ORDER> *save_order_list; > > Window_frame *window_frame; > > @@ -111,7 +113,8 @@ class Window_spec : public Sql_alloc > SQL_I_List<ORDER> *ord_list, > Window_frame *win_frame) > : window_names_are_checked(false), window_ref(win_ref), > - partition_list(part_list), order_list(ord_list), > + partition_list(part_list), save_partition_list(NULL), > + order_list(ord_list), save_order_list(NULL), > window_frame(win_frame), referenced_win_spec(NULL) {} > > virtual char *name() { return NULL; } > _______________________________________________ > commits mailing list > commits(a)mariadb.org > https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits

1 0

Re: [Maria-developers] 19aebbd1b89: MDEV-16481: set global system_versioning_asof=sf() crashes in specific case
by Sergei Golubchik 19 Jul '21

19 Jul '21

Hi, Nikita! On Jul 12, Nikita Malyavin wrote: > revision-id: 19aebbd1b89 (mariadb-10.3.26-129-g19aebbd1b89) > parent(s): 10c163e4a18 > author: Nikita Malyavin <nikita.malyavin(a)mariadb.com> > committer: Nikita Malyavin <nikita.malyavin(a)mariadb.com> > timestamp: 2021-04-06 21:32:43 +0300 > message: > > MDEV-16481: set global system_versioning_asof=sf() crashes in specific case > > * sys_vars.h: add `MYSQL_TIME` field to `set_var::save_result` > * sys_vars.ic: get rid of calling `var->value->get_date()` from `Sys_var_vers_asof::update()` > * versioning.sysvars: add test; remove double warning > > diff --git a/sql/set_var.h b/sql/set_var.h > index 12e025e4696..6351fb5b666 100644 > --- a/sql/set_var.h > +++ b/sql/set_var.h > @@ -296,6 +296,7 @@ class set_var :public set_var_base > plugin_ref *plugins; ///< for Sys_var_pluginlist > Time_zone *time_zone; ///< for Sys_var_tz > LEX_STRING string_value; ///< for Sys_var_charptr and others > + MYSQL_TIME time; ///< for Sys_var_vers_asof old sizeof(save_result) is 16 bytes, sizeof(MYSQL_TIME) is 40. It's just an observation, set_var isn't a structure that's constantly in memory in large quantities. So, not a problem at all. > const void *ptr; ///< for Sys_var_struct > } save_result; > LEX_CSTRING base; /**< for structured variables, like keycache_name.variable_name */ > diff --git a/sql/sys_vars.ic b/sql/sys_vars.ic > index 8d094d3a1d1..eff6a59e65b 100644 > --- a/sql/sys_vars.ic > +++ b/sql/sys_vars.ic > @@ -2650,50 +2650,47 @@ class Sys_var_vers_asof: public Sys_var_enum > virtual bool do_check(THD *thd, set_var *var) > { > if (!Sys_var_enum::do_check(thd, var)) > + { > + var->save_result.time.time_type= MYSQL_TIMESTAMP_NONE; > return false; > - MYSQL_TIME ltime; > - bool res= var->value->get_date(&ltime, TIME_NO_ZERO_IN_DATE|TIME_NO_ZERO_DATE); > - if (!res) > + } > + bool res= var->value->get_date(&var->save_result.time, > + TIME_NO_ZERO_IN_DATE | TIME_NO_ZERO_DATE); > + if (res) > { > - var->save_result.ulonglong_value= SYSTEM_TIME_AS_OF; > + var->save_result.time.time_type= MYSQL_TIMESTAMP_ERROR; > } > return res; > } > > private: > - bool update(set_var *var, vers_asof_timestamp_t &out, system_variables& pool) > + bool update(THD *thd, set_var *var, vers_asof_timestamp_t *out) > { > - 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) > + MYSQL_TIME &ltime= var->save_result.time; > + uint error= 0; > + > + if (var->value && ltime.time_type >= 0) // any valid value is ok > { > - if (var->value) > - { > - res= var->value->get_date(&ltime, TIME_NO_ZERO_IN_DATE > - | TIME_NO_ZERO_DATE); > - 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.type= SYSTEM_TIME_UNSPECIFIED; > - res= false; > - } > + out->type = SYSTEM_TIME_AS_OF; > + out->unix_time = thd->variables.time_zone->TIME_to_gmt_sec(&ltime, > + &error); why are you doing it here? You could've converted in ::check(), and as a bonus you wouldn't need MYSQL_TIME in the save_result. And what would you do if TIME_to_gmt_sec would fail? ::update() isn't expected to fail, ::check() should guarantee that as much as possible. > + out->second_part= ltime.second_part; > } > - return res; > + else // DEFAULT is set > + { > + out->type= SYSTEM_TIME_UNSPECIFIED; > + } > + return (error != 0); > } > > public: > virtual bool global_update(THD *thd, set_var *var) > { > - return update(var, global_var(vers_asof_timestamp_t), thd->variables); > + return update(thd, var, &global_var(vers_asof_timestamp_t)); > } > virtual bool session_update(THD *thd, set_var *var) > { > - return update(var, session_var(thd, vers_asof_timestamp_t), thd->variables); > + return update(thd, var, &session_var(thd, vers_asof_timestamp_t)); > } > > private: > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 3

Re: [Maria-developers] a11756e4fba: MDEV-23178: Qualified asterisk not supported in INSERT .. RETURNING
by Sergei Golubchik 16 Jul '21

16 Jul '21

Hi, Rucha! On Jul 15, Rucha Deodhar wrote: > revision-id: a11756e4fba (mariadb-10.5.11-27-ga11756e4fba) > parent(s): f0f47cbca18 > author: Rucha Deodhar <rucha.deodhar(a)mariadb.com> > committer: Rucha Deodhar <rucha.deodhar(a)mariadb.com> > timestamp: 2021-07-05 20:14:19 +0530 > message: > > MDEV-23178: Qualified asterisk not supported in INSERT .. RETURNING > > Analysis: When we have INSERT/REPLACE returning with qualified asterisk in the > RETURNING clause, '*' is not resolved properly because of wrong context. > context->table_list is NULL or has incorrect table because context->table_list > has tables from the FROM clause. > Fix: If filling fields instead of '*' for qualified asterisk in RETURNING, > use first_name_resolution_table for correct resolution of item. please, add a couple of tests with a qualified asterisk, where it's qualified _not_ by the table you're inserting into. Like INSERT INTO t1(id1,val1) VALUES(14,'m') RETURNING t2.* not very interesting case, an obvious error ^^^ INSERT INTO t2 SELECT t1.* FROM t1 WHERE id1=2 RETURNING t1.*; but what will happen here? ^^^ > diff --git a/sql/sql_base.cc b/sql/sql_base.cc > index 1476d708d75..60d14c6dbea 100644 > --- a/sql/sql_base.cc > +++ b/sql/sql_base.cc > @@ -8032,12 +8032,13 @@ insert_fields(THD *thd, Name_resolution_context *context, const char *db_name, > else treat natural joins as leaves and do not iterate over their underlying > tables. > */ > - for (TABLE_LIST *tables= (table_name ? context->table_list : > - context->first_name_resolution_table); > + TABLE_LIST *tables= returning_field || !table_name ? context->first_name_resolution_table : > + context->table_list; > + for (; > tables; > - tables= (table_name ? tables->next_local : > - tables->next_name_resolution_table) > + tables= returning_field || !table_name ? tables->next_name_resolution_table : > + tables->next_local > ) > { > Field *field; > TABLE *table= tables->table; 1. please also update the comment to mention RETURNING. 2. Could you apply the following patch as a separate commit before your fix: ============================ --- a/sql/sql_base.cc +++ b/sql/sql_base.cc @@ -8098,12 +8098,14 @@ insert_fields(THD *thd, Name_resolution_context *context, const char *db_name, else treat natural joins as leaves and do not iterate over their underlying tables. */ - for (TABLE_LIST *tables= (table_name ? context->table_list : - context->first_name_resolution_table); - tables; - tables= (table_name ? tables->next_local : - tables->next_name_resolution_table) - ) + TABLE_LIST *first= context->first_name_resolution_table; + TABLE_LIST *TABLE_LIST::* next= &TABLE_LIST::next_name_resolution_table; + if (table_name) + { + first= context->table_list; + next= &TABLE_LIST::next_local; + } + for (TABLE_LIST *tables= first; tables; tables= tables->*next) { Field *field; TABLE *table= tables->table; ============================ it'll remove the condition from inside the loop, will put it in one place only. And will make your change simpler and clearer. Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 1

Re: [Maria-developers] 70252733d41: MDEV-25908: -e does not work for my_print_defaults
by Sergei Golubchik 15 Jul '21

15 Jul '21

Hi, Rucha! On Jul 15, Rucha Deodhar wrote: > revision-id: 70252733d41 (mariadb-10.5.11-35-g70252733d41) > parent(s): f88d130e710 > author: Rucha Deodhar <rucha.deodhar(a)mariadb.com> > committer: Rucha Deodhar <rucha.deodhar(a)mariadb.com> > timestamp: 2021-07-14 22:55:21 +0530 > message: > > MDEV-25908: -e does not work for my_print_defaults > > Analysis: get_defaults_options() rewrites the value for my_defaults_file, > my_defaults_extra_file and my_defaults_group_suffix to 0. So the config file > can't be read. > Fix: Let handle_options() handle --defaults* option and create a new command > line to pass it to load_defaults(). > > --- > extra/my_print_defaults.c | 37 +++++++++++++++++----- > mysql-test/main/my_print_defaults.result | 35 ++++++++++++++++++++- > mysql-test/main/my_print_defaults.test | 54 +++++++++++++++++++++++++++++++- > mysql-test/main/mysqldump.result | 1 + > 4 files changed, 117 insertions(+), 10 deletions(-) > > diff --git a/extra/my_print_defaults.c b/extra/my_print_defaults.c > index 1dec667949f..409ef273b44 100644 > --- a/extra/my_print_defaults.c > +++ b/extra/my_print_defaults.c > @@ -141,28 +141,49 @@ static int get_options(int *argc,char ***argv) > return 0; > } > > +static char *make_args(const char *s1, const char *s2) > +{ > + char *s= malloc(strlen(s1) + strlen(s2) + 1); > + strmov(strmov(s, s1), s2); > + return s; > +} > > int main(int argc, char **argv) > { > - int count, error, args_used; > - char **load_default_groups= 0, *tmp_arguments[6]; > + int count= 0, error, no_defaults= 0; > + char **load_default_groups= 0; > char **argument, **arguments, **org_argv; > int nargs, i= 0; > MY_INIT(argv[0]); > > org_argv= argv; > - args_used= get_defaults_options(argv); > - > - /* Copy defaults-xxx arguments & program name */ > - count=args_used; > - arguments= tmp_arguments; > - memcpy((char*) arguments, (char*) org_argv, count * sizeof(*org_argv)); > + if (*argv && !strcmp(*argv, "--no-defaults")) > + { > + argv++; > + ++count; > + no_defaults= 1; > + } > + /* Copy program name and --no-defaults if present*/ > + memcpy((char*) arguments, (char*) org_argv, (++count)*sizeof(*org_argv)); isn't this wrong? `arguments` is uninitialized at this point. > arguments[count]= 0; > > /* Check out the args */ > if (get_options(&argc,&argv)) > cleanup_and_exit(1); > > +--echo # Testing -e > +--exec $MYSQL_MY_PRINT_DEFAULTS -e $MYSQLTEST_VARDIR/tmp/tmp1.cnf --mysqld mysql.server > +--echo #Testing --defaults-extra-file > +--exec $MYSQL_MY_PRINT_DEFAULTS --defaults-extra-file=$MYSQLTEST_VARDIR/tmp/tmp1.cnf --mysqld mysql.server > + > +--echo # Testing -c option > +--exec $MYSQL_MY_PRINT_DEFAULTS -c $MYSQLTEST_VARDIR/tmp/tmp2.cnf --mysqld mysql.server > +--echo # Testing --defaults-file > +--exec $MYSQL_MY_PRINT_DEFAULTS --defaults-file=$MYSQLTEST_VARDIR/tmp/tmp2.cnf --mysqld mysql.server > + > +--echo # Testing -g option > +--exec $MYSQL_MY_PRINT_DEFAULTS -c $MYSQLTEST_VARDIR/tmp/tmp3.cnf --mysqld mysql.server -g .1 > +--echo # Testing --defaults-group-suffix > +--exec $MYSQL_MY_PRINT_DEFAULTS -c $MYSQLTEST_VARDIR/tmp/tmp3.cnf --mysqld mysql.server --defaults-group-suffix=.1 you don't seem have a --no-defaults test Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] da70f190279: MDEV-25985 Spider handle ">=" as ">" in some cases
by Sergei Golubchik 13 Jul '21

13 Jul '21

Hi, Nayuta! The code change is good, no comments about it at all. But why did you create a new suite spider/bugfix and all those files in it? I know that some later spider version has it. Still, I'd expect it'd be much simpler and a smaller change to drop the test file into the exising spider suite. On Jul 12, Nayuta Yanagisawa wrote: > revision-id: da70f190279 (mariadb-10.2.39-44-gda70f190279) > parent(s): 99f700a820e > author: Nayuta Yanagisawa <nayuta.yanagisawa(a)hey.com> > committer: Nayuta Yanagisawa <nayuta.yanagisawa(a)hey.com> > timestamp: 2021-07-08 14:12:45 +0000 > message: > > MDEV-25985 Spider handle ">=" as ">" in some cases > > The function spider_db_append_key_where_internal() converts > HA_READ_AFTER_KEY to '>'. The conversion seems to be correct for > single-column indexes because HA_READ_AFTER_KEY means "read the > key after the provided value." > > However, how about multi-column indexes? Assume that there is a > multi-column index on c1 and c2 and we search with the condition > 'c1 >= 100 AND c2 > 200'. The key_range.flag corresponds to the > search condition could be HA_READ_AFTER_KEY. In such a case, > we could not simply convert HA_READ_AFTER_KEY to '>'. > > The correct conversion is to convert HA_READ_AFTER_KEY to '>' > only for the last column in key_part_map and to convert > HA_READ_AFTER_KEY to '>=' for the other column. > > The similar discussion also applies to the conversion from > key_range.flag to a sign of inequality. > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 1

Re: [Maria-developers] a6e96ce3c96: MDEV-21916: COM_STMT_BULK_EXECUTE with RETURNING insert wrong values
by Sergei Golubchik 12 Jul '21

12 Jul '21

Hi, Oleksandr! ok to push! On Jul 12, Oleksandr Byelkin wrote: > revision-id: a6e96ce3c96 (mariadb-10.5.4-666-ga6e96ce3c96) > parent(s): f4425d3a3db > author: Oleksandr Byelkin <sanja(a)mariadb.com> > committer: Oleksandr Byelkin <sanja(a)mariadb.com> > timestamp: 2021-06-09 10:43:24 +0200 > message: > > MDEV-21916: COM_STMT_BULK_EXECUTE with RETURNING insert wrong values > > The problem is that array binding uses net buffer to read parameters for each > execution while each execiting with RETURNING write in the same buffer. > > Solution is to allocate new net buffer to avoid changing buffer we are reading > from. Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] b1881e07797: MDEV-20131 Assertion `!pk->has_virtual ()' failed in instant_alter_column_possible
by Sergei Golubchik 12 Jul '21

12 Jul '21

Hi, Sachin! On Jul 12, Sachin Kumar wrote: > revision-id: b1881e07797 (mariadb-10.4.20-9-gb1881e07797) > parent(s): 8a2b4d531dc > author: Sachin Kumar <sachin.setiya(a)mariadb.com> > committer: Sachin Kumar <sachin.setiya(a)mariadb.com> > timestamp: 2021-06-11 14:26:19 +0100 > message: > > MDEV-20131 Assertion `!pk->has_virtual ()' failed in instant_alter_column_possible > > Problem:- Primary key on long unique columns is not allowed but when > `CREATE TABLE t2 (a TEXT, PRIMARY KEY(a(1871))) ENGINE=InnoDB;` > is executed with innodb_page_size=8k or 4k, primary key on DB_ROW_HASH_1 column > is created. Reason of this is in mysql_prepare_create_table we check against > max_key_part_length() to see whether key_part length is more then engine supported > key_part length , and if it is true error is thrown for primary key and long > unique index is created for unique key. But in this case max_key_part_length() > returns 3072 which is more the max_key_length() , which later in code triggers > creating of long unique index. > > Solution:- max_supported_key_part_length() should return according to innodb > page size. > > diff --git a/storage/innobase/handler/ha_innodb.cc b/storage/innobase/handler/ha_innodb.cc > index ce2e4b6990f..e5e864bffc5 100644 > --- a/storage/innobase/handler/ha_innodb.cc > +++ b/storage/innobase/handler/ha_innodb.cc > @@ -6537,14 +6537,21 @@ ha_innobase::clone( > DBUG_RETURN(new_handler); > } > > - > -uint > -ha_innobase::max_supported_key_part_length() const > -/*==============================================*/ > -{ > - /* A table format specific index column length check will be performed > - at ha_innobase::add_index() and row_create_index_for_mysql() */ > - return(REC_VERSION_56_MAX_INDEX_COL_LEN); > +uint ha_innobase::max_supported_key_part_length() const > +{ > + /* A table format specific index column length check will be performed > + at ha_innobase::add_index() and row_create_index_for_mysql() */ > + /* FIXME: rewrite this as well as ha_innobase::max_supported_key_length() > + using an API that considers the PRIMARY KEY as well as secondary index > + metadata and the ROW_FORMAT and KEY_BLOCK_SIZE */ > + switch (srv_page_size) { > + case 4096: > + return 1173; > + case 8192: > + return 1536; > + default: > + return REC_VERSION_56_MAX_INDEX_COL_LEN; > + } dunno, why would you duplicate the whole switch logic? can you just - return(REC_VERSION_56_MAX_INDEX_COL_LEN); - return MY_MIN(max_supported_key_length(), REC_VERSION_56_MAX_INDEX_COL_LEN); > } > > /******************************************************************//** > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0