- developers - lists.mariadb.org

[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