- developers - lists.mariadb.org

Re: bd616a3733c: Auth: set thd->scramble in all cases during writing Initial Handshake Packet
by Sergei Golubchik 10 Sep '24

10 Sep '24

Hi, Nikita, On Sep 09, Nikita Malyavin wrote: > revision-id: bd616a3733c (mariadb-11.6.1-13-gbd616a3733c) > parent(s): bbbb429a1eb > author: Nikita Malyavin > committer: Nikita Malyavin > timestamp: 2024-09-07 20:46:19 +0200 > message: > > Auth: set thd->scramble in all cases during writing Initial Handshake Packet > > --- > sql/sql_acl.cc | 19 +++++++++++++------ > 1 file changed, 13 insertions(+), 6 deletions(-) > > diff --git a/sql/sql_acl.cc b/sql/sql_acl.cc > index 93efc5806cc..7845af55413 100644 > --- a/sql/sql_acl.cc > +++ b/sql/sql_acl.cc > @@ -13504,17 +13504,21 @@ static bool send_server_handshake_packet(MPVIO_EXT *mpvio, > mpvio->cached_server_packet.pkt_len= data_len; > } > > - if (data_len < SCRAMBLE_LENGTH) > + if (thd->scramble[SCRAMBLE_LENGTH] != 0) old code was obviously wrong but the new one makes no sense either. How can thd->scramble[SCRAMBLE_LENGTH] be not zero at this point? I'd think you need to execute the code below unconditionally. > { > + DBUG_ASSERT(thd->scramble[SCRAMBLE_LENGTH] == 1); // Sanity > + > if (data_len) > { > /* > the first packet *must* have at least 20 bytes of a scramble. > - if a plugin provided less, we pad it to 20 with zeros > + if a plugin provided less, we pad it to 20 with zeros, > + plus extra zero termination sign is put in thd->scramble. > + If more is provided, we'll use only 20 bytes as a handshake scramble. Not sure it's a good idea. Why not to send the complete scramble? True, it won't fit on the client side into mysql->scramble[]. Two ways to fix it: * Write the first 20 bytes. The server will only store the first 20 bytes too, and for COM_CHANGE_USER they'll use the first 20 bytes only (for the login auth they can use the complete scramble). That's a rather hackish fix, a proper fix would be * store the complete scramble in MYSQL. The first 20 bytes in MYSQL::scramble_buff, the rest in MySQL::extension.scramble_suffix > */ > - memcpy(scramble_buf, data, data_len); > - bzero(scramble_buf + data_len, SCRAMBLE_LENGTH - data_len); > - data= scramble_buf; > + size_t fill_size= MY_MIN(SCRAMBLE_LENGTH, data_len); > + memcpy(thd->scramble, data, fill_size); > + bzero(thd->scramble + fill_size, SCRAMBLE_LENGTH - fill_size + 1); > } Regards, Sergei Chief Architect, MariaDB Server and security(a)mariadb.org

2 3

Re: 982bf06d560: MDEV-12320 configurable default authentication plugin for the server
by Sergei Golubchik 09 Sep '24

09 Sep '24

Hi, Nikita, This looks good. Minor comments below. On Sep 09, Nikita Malyavin wrote: > revision-id: 982bf06d560 (mariadb-11.6.1-14-g982bf06d560) > parent(s): bd616a3733c > author: Nikita Malyavin > committer: Nikita Malyavin > timestamp: 2024-09-07 21:44:13 +0200 > message: > > MDEV-12320 configurable default authentication plugin for the server > > * Add a new cmdline-only variable "default_auth_plugin". > * A default plugin is locked at the server init and unlocked at the deinit > stages. This means that mysql_native_password and old_password_plugin, when > default, are locked/unlocked twice. doesn't matter, compiled-in plugins are only locked in debug builds, otherwise it's a no-op. > > --- a/sql/sys_vars.cc > +++ b/sql/sys_vars.cc > @@ -4538,6 +4538,14 @@ static Sys_var_plugin Sys_enforce_storage_engine( > DEFAULT(&enforced_storage_engine), NO_MUTEX_GUARD, NOT_IN_BINLOG, > ON_CHECK(check_has_super)); > > +extern const char *default_auth_plugin_name; > +extern LEX_CSTRING native_password_plugin_name; Is it ok? The correct type is Lex_ident_plugin. > +static Sys_var_charptr Sys_default_auth_plugin( there's also Sys_var_lexstring, if you prefer that. > + "default_auth_plugin", "Default plugin, that will be tried first when authenticating new connections", reformat the long line, please > + READ_ONLY GLOBAL_VAR(default_auth_plugin_name), CMD_LINE(OPT_ARG), > + DEFAULT(native_password_plugin_name.str), > + NO_MUTEX_GUARD, NOT_IN_BINLOG); > --- a/sql/sql_acl.cc > +++ b/sql/sql_acl.cc > @@ -2543,9 +2542,20 @@ bool acl_init(bool dont_read_acl_tables) > old_password_plugin= my_plugin_lock_by_name(0, > &old_password_plugin_name, MYSQL_AUTHENTICATION_PLUGIN); > > + Lex_cstring_strlen def_plugin_name(default_auth_plugin_name); > + default_auth_plugin= my_plugin_lock_by_name(NULL, &def_plugin_name, > + MYSQL_AUTHENTICATION_PLUGIN); > + > if (!native_password_plugin || !old_password_plugin) > DBUG_RETURN(1); > > + if (!default_auth_plugin) > + { > + sql_print_error("Default plugin %s could not be loaded", > + default_auth_plugin_name); see init_default_storage_engine() in mysqld.cc - it's for --default-storage-engine option. let's use similar wording for consistency: sql_print_error("Unknown/unsupported authentication plugin: %s", default_auth_plugin_name); > + DBUG_RETURN(1); > + } > + > if (dont_read_acl_tables) > { > DBUG_RETURN(0); /* purecov: tested */ Regards, Sergei Chief Architect, MariaDB Server and security(a)mariadb.org

1 0

Re: bbbb429a1eb: Auth: release the plugins from responsibility to fill scramble_buff.
by Sergei Golubchik 09 Sep '24

09 Sep '24

Hi, Nikita, Conceptually ok, see one omission below. On Sep 09, Nikita Malyavin wrote: > revision-id: bbbb429a1eb (mariadb-11.6.1-12-gbbbb429a1eb) > parent(s): db5d1cde450 > author: Nikita Malyavin > committer: Nikita Malyavin > timestamp: 2024-09-07 20:44:02 +0200 > message: > > Auth: release the plugins from responsibility to fill scramble_buff. > > Fill it once during the Initial Handshake Packet parsing. This uses the fact > that the server guarantees first 20 bytes to be the scramble. > > Submodule libmariadb de6305915f8..12d78187061: > diff --git a/libmariadb/libmariadb/mariadb_lib.c b/libmariadb/libmariadb/mariadb_lib.c > index 78195d44..07953ea4 100644 > --- a/libmariadb/libmariadb/mariadb_lib.c > +++ b/libmariadb/libmariadb/mariadb_lib.c > @@ -1954,6 +1954,8 @@ restart: > goto error; > } > } > + memmove(mysql->scramble_buff, scramble_data, SCRAMBLE_LENGTH); > + mysql->scramble_buff[SCRAMBLE_LENGTH]= 0; > /* Set character set */ > if (mysql->options.charset_name) > diff --git a/libmariadb/plugins/auth/my_auth.c b/libmariadb/plugins/auth/my_auth.c > index a2fd519d..47d26150 100644 > --- a/libmariadb/plugins/auth/my_auth.c > +++ b/libmariadb/plugins/auth/my_auth.c > @@ -126,10 +126,6 @@ static int native_password_auth_client(MYSQL_PLUGIN_VIO *vio, MYSQL *mysql) > > if (pkt_len != SCRAMBLE_LENGTH + 1) > return CR_SERVER_HANDSHAKE_ERR; > - > - /* save it in MYSQL */ > - memmove(mysql->scramble_buff, pkt, SCRAMBLE_LENGTH); > - mysql->scramble_buff[SCRAMBLE_LENGTH] = 0; This looks ok > } > > if (mysql && mysql->passwd[0]) > diff --git a/sql-common/client.c b/sql-common/client.c > index 6d030ce0a17..28b477c1e9a 100644 > --- a/sql-common/client.c > +++ b/sql-common/client.c > @@ -4178,10 +4178,6 @@ static int native_password_auth_client(MYSQL_PLUGIN_VIO *vio, MYSQL *mysql) > > if (pkt_len != SCRAMBLE_LENGTH + 1) > DBUG_RETURN(CR_SERVER_HANDSHAKE_ERR); > - > - /* save it in MYSQL */ > - memcpy(mysql->scramble, pkt, SCRAMBLE_LENGTH); > - mysql->scramble[SCRAMBLE_LENGTH] = 0; this isn't, you removed memcpy from the plugin, but didn't add it to mysql_real_connect in sql-common/client.c. > } > > if (mysql->passwd[0]) Regards, Sergei Chief Architect, MariaDB Server and security(a)mariadb.org

2 1

Re: b96afca238a: MDEV-16440 merge 5.7 P_S sysvars instrumentation and tables
by Sergei Golubchik 08 Sep '24

08 Sep '24

Hi, Nikita, Despite the email subject, it's a review of everything in bb-10.10-MDEV-16440, *except* 9c57bbfef92. That is 37e1ccbf678..c16b2429f02 without notifiable_work_zone.h I'll send a separate email about notifiable_work_zone.h On Sep 08, Nikita Malyavin wrote: > revision-id: b96afca238a (mariadb-10.6.1-472-gb96afca238a) > parent(s): 37e1ccbf678 > author: Nikita Malyavin > committer: Nikita Malyavin > timestamp: 2022-06-19 14:52:30 +0300 > message: > > MDEV-16440 merge 5.7 P_S sysvars instrumentation and tables > diff --git a/mysql-test/suite/perfschema/t/variables_stress.test b/mysql-test/suite/perfschema/t/variables_stress.test > new file mode 100644 > index 00000000000..be3012bf8af > --- /dev/null > +++ b/mysql-test/suite/perfschema/t/variables_stress.test > @@ -0,0 +1,71 @@ stress tests generally don't belong in mtr and aren't reliable enough for the mtr. Also, you forgot to check in the result file for it. on the other hand, looking at the patch I got the impression that this stress test helped to find a lot of bugs. > +let $i = 64; > +enable_connect_log; > +while ($i) { > + echo i = $i; > + connect(con_$i, localhost, root); > +# set debug_sync = "tp_hanger wait_for banger"; > + send set debug_dbug = "+d,tp_wanger"; > + dec $i; > +} > +connection default; > + > +SELECT THREAD_ID, PROCESSLIST_ID FROM performance_schema.threads; > +set debug_sync = "now signal banger"; > +connect(banger_1, localhost, root); > +let $banger1= `SELECT THREAD_ID FROM performance_schema.threads WHERE PROCESSLIST_ID = CONNECTION_ID()`; > +send select count(*) from performance_schema.variables_by_thread where variable_name="time_zone"; > +connect(banger_2, localhost, root); > +let $banger2= `SELECT THREAD_ID FROM performance_schema.threads WHERE PROCESSLIST_ID = CONNECTION_ID()`; > +send select count(*) from performance_schema.variables_by_thread where variable_name="time_zone"; > +connect(banger_3, localhost, root); > +let $banger3= `SELECT THREAD_ID FROM performance_schema.threads WHERE PROCESSLIST_ID = CONNECTION_ID()`; > +send select count(*) from performance_schema.variables_by_thread where variable_name="time_zone"; > +connect(banger_4, localhost, root); > +let $banger4= `SELECT THREAD_ID FROM performance_schema.threads WHERE PROCESSLIST_ID = CONNECTION_ID()`; > +send select count(*) from performance_schema.variables_by_thread where variable_name="time_zone"; > +connect(banger_5, localhost, root); > +let $banger5= `SELECT THREAD_ID FROM performance_schema.threads WHERE PROCESSLIST_ID = CONNECTION_ID()`; > + > +send select count(*) from performance_schema.variables_by_thread where variable_name="time_zone"; > + > +let $i = 2000; > +let $j=65; > +while ($i) { > + echo i = $i; > + connection banger_1; > + reap; > + send select count(*) from performance_schema.variables_by_thread where variable_name="time_zone"; > + connection banger_2; > + reap; > + send select count(*) from performance_schema.variables_by_thread where variable_name="time_zone"; > + connection banger_3; > + reap; > + send select count(*) from performance_schema.variables_by_thread where variable_name="time_zone"; > + connection banger_4; > + reap; > + send select count(*) from performance_schema.variables_by_thread where variable_name="time_zone"; > + connection banger_5; > + reap; > + send select count(*) from performance_schema.variables_by_thread where variable_name="time_zone"; > + dec $i; > + > + let $threads= `show status where variable_name = 'threads_connected'`; > + if ($threads < 63) { > + connect(con_$i, localhost, root); > + send set debug_dbug = "+d,tp_wanger"; > + inc $j; > + } > +} > +reap; > +select * from performance_schema.variables_by_thread where variable_name="time_zone"; > +echo $banger1; > +echo $banger2; > +echo $banger3; > +echo $banger4; > +echo $banger5; > +# eval select * from performance_schema.variables_by_thread where variable_name like "%id" and thread_id=197; > +connection default; > +# set global debug_dbug = ""; > +set debug_sync = reset; > + > +show status where variable_name = 'threads_connected'; > diff --git a/sql/sql_plugin.h b/sql/sql_plugin.h > index d4df8c6468f..3d02d1c7509 100644 > --- a/sql/sql_plugin.h > +++ b/sql/sql_plugin.h > @@ -186,6 +186,10 @@ extern SHOW_COMP_OPTION plugin_status(const char *name, size_t len, int type); > extern bool check_valid_path(const char *path, size_t length); > extern void plugin_mutex_init(); > > +template <class T> class Dynamic_array; you can simply include sql_array.h, can't you? > + > +void plugin_lock_by_sys_var_array(THD *thd, Dynamic_array<SHOW_VAR> *vars); > + > typedef my_bool (plugin_foreach_func)(THD *thd, > plugin_ref plugin, > void *arg); > diff --git a/sql/sql_parse.cc b/sql/sql_parse.cc > index 0597b086b7b..ec86389d08f 100644 > --- a/sql/sql_parse.cc > +++ b/sql/sql_parse.cc > @@ -1296,6 +1296,9 @@ dispatch_command_return do_command(THD *thd, bool blocking) > goto out; > } > > + if (unlikely(thd->apc_target.have_apc_requests())) > + thd->apc_target.process_apc_requests(); 1. why here? 2. there generally shouldn't be direct checks of apc requests, the code should do check_killed() often enough. So if you really need a check here (see question 1), add if (thd->check_killed())... > + > packet= (char*) net->read_pos; > /* > 'packet_length' contains length of data, as it was stored in packet > diff --git a/vio/viopipe.c b/vio/viopipe.c > index aeaad311b7e..5106c6d6513 100644 > --- a/vio/viopipe.c > +++ b/vio/viopipe.c > @@ -53,8 +53,9 @@ static size_t wait_overlapped_result(Vio *vio, int timeout) > } > else > { > + DWORD last_error= GetLastError(); > /* Error or timeout, cancel the pending I/O operation. */ > - CancelIo(vio->hPipe); > + CancelIoEx(vio->hPipe, &vio->overlapped); why, CancelIoEx resets last error, but CancelIo doesn't? > /* > If the wait timed out, set error code to indicate a > diff --git a/sql/threadpool_win.cc b/sql/threadpool_win.cc > index ed68e31c755..25844d1f3ef 100644 > --- a/sql/threadpool_win.cc > +++ b/sql/threadpool_win.cc > @@ -131,6 +131,11 @@ void TP_pool_win::resume(TP_connection* c) > SubmitThreadpoolWork(((TP_connection_win*)c)->work); > } > > +int TP_pool_win::wake(TP_connection *c) > +{ > + return 0; > +} How is that supposed to work? How do you wake up threadpool threads on Windows? > + > #define CHECK_ALLOC_ERROR(op) \ > do \ > { \ > diff --git a/mysys/my_thr_init.c b/mysys/my_thr_init.c > index cde34e5bdb9..89f190ee989 100644 > --- a/mysys/my_thr_init.c > +++ b/mysys/my_thr_init.c > @@ -432,9 +432,21 @@ const char *my_thread_name(void) > extern void **my_thread_var_dbug() > { > struct st_my_thread_var *tmp; > + int last_error; > if (!my_thread_global_init_done) > return NULL; > +#ifdef WIN32 > + last_error= GetLastError(); > +#else > + last_error= errno; > +#endif > + > tmp= my_thread_var; > +#ifdef WIN32 > + SetLastError(last_error); > +#else > + errno= last_error; > +#endif huh? my_thread_var can change errno? > return tmp && tmp->init ? &tmp->dbug : 0; > } > #endif /* DBUG_OFF */ > diff --git a/storage/perfschema/pfs_variable.h b/storage/perfschema/pfs_variable.h > index e59b02f2af8..f509e97048c 100644 > --- a/storage/perfschema/pfs_variable.h > +++ b/storage/perfschema/pfs_variable.h > @@ -212,7 +212,7 @@ class Find_THD_variable : public Find_THD_Impl > return false; > > /* Hold this lock to keep THD during materialization. */ > - mysql_mutex_lock(&thd->LOCK_thd_data); > + mysql_mutex_lock(&thd->LOCK_thd_kill); see commits 736a54f49c72 and c6c2a2b8d463 > return true; > } > void set_unsafe_thd(THD *unsafe_thd) { m_unsafe_thd= unsafe_thd; } > diff --git a/sql/threadpool_generic.h b/sql/threadpool_generic.h > index b7a35b7cbf0..e16ec4a4966 100644 > --- a/sql/threadpool_generic.h > +++ b/sql/threadpool_generic.h > @@ -88,9 +89,17 @@ struct TP_connection_generic :public TP_connection > ulonglong abs_wait_timeout; > ulonglong enqueue_time; > TP_file_handle fd; > + /** > + Designates whether fd is currently connected to the poll denoted by > + thread_group->pollfd. See also change_group. > + */ > bool bound_to_poll_descriptor; > int waiting; > bool fix_group; > +#ifndef WIN32 > + Notifiable_work_zone work_zone; > + bool leave_work_zone() final; > +#endif Do you need this #ifndef? WIN32 is *never* defined, it's _WIN32. and _WIN32 is never defined *here*, on Windows we use TP_connection_win. > #ifdef _WIN32 > win_aiosocket win_sock{}; > void init_vio(st_vio *vio) override > diff --git a/sql/sql_class.cc b/sql/sql_class.cc > index a0ba1d7e5a3..bfe49cddbf4 100644 > --- a/sql/sql_class.cc > +++ b/sql/sql_class.cc > @@ -1909,13 +1910,13 @@ void THD::awake_no_mutex(killed_state state_to_set) > } > > /* Broadcast a condition to kick the target if it is waiting on it. */ > -void THD::abort_current_cond_wait(bool force) > +void THD::abort_current_cond_wait(bool force, bool mark_abort) a bit illogical, these values aren't orthogonal. you want a 3-state enum, like ABORT_NONE, ABORT_CONNECTIONS_BUT_NOT_SYSTEM_THREADS, ABORT_ALL > { > mysql_mutex_assert_owner(&LOCK_thd_kill); > if (mysys_var) > { > mysql_mutex_lock(&mysys_var->mutex); > - if (!system_thread || force) // Don't abort locks > + if (mark_abort && (!system_thread || force)) // Don't abort locks > mysys_var->abort=1; > > /* > @@ -4373,6 +4374,14 @@ my_bool thd_net_is_killed(THD *thd) > return thd && thd->killed ? 1 : 0; > } > > +/* returns true if any APC was processed */ > +void thd_net_process_apc_requests(THD *thd) even though it's called from net_serv.cc, there's nothing about NET here, so calling it thd_net_process_apc_requests is misleading > +{ > + if (unlikely(thd->apc_target.have_apc_requests())) > + thd->apc_target.process_apc_requests(); you also call it from the threadpool_common and a couple of times you have these two lines verbatim in other places (e.g. sql_parse.cc) why couldn't you call thd->check_killed() instead? > + DEBUG_SYNC(thd, "net_after_apc"); > +} > + > > void thd_increment_bytes_received(void *thd, size_t length) > { > diff --git a/sql/my_apc.h b/sql/my_apc.h > index cc98e36bbe4..ae60bdc2763 100644 > --- a/sql/my_apc.h > +++ b/sql/my_apc.h > @@ -81,7 +83,7 @@ class Apc_target > */ > inline bool have_apc_requests() > { > - return MY_TEST(apc_calls); > + return MY_TEST(apc_calls.load(std::memory_order_acquire)); why? > } > > inline bool is_enabled() { return enabled; } > @@ -95,14 +97,19 @@ class Apc_target > virtual ~Apc_call() {} > }; > > + class Call_request; > /* Make a call in the target thread (see function definition for details) */ > - bool make_apc_call(THD *caller_thd, Apc_call *call, int timeout_sec, bool *timed_out); > + bool make_apc_call(THD *caller_thd, Apc_call *call, > + int timeout_sec, bool *timed_out); > + > + void enqueue_request(Call_request *request_buff, Apc_call *call); > + int wait_for_completion(THD *caller_thd, Call_request *request, > + int timeout_sec); You wrote > In pfs_variable.cc we will have to make an additional step between > enqueue_request and wait_for_completion. These two functions will be called > directly and therefore both should have a public interface and I don't think I quite agree with that. Your "additional step" seems to be abort_current_cond_wait() and notify_apc(). It's not an exotic additional step that only pfs_variable.cc needs, anyone, who makes an apc call needs the target thread to wake up and process it. It's something that should be part of apc code, not done only in pfs_variable.cc And this case you likely won't need enqueue_request and wait_for_completion as a public api > > #ifndef DBUG_OFF > int n_calls_processed; /* Number of calls served by this target */ > #endif > private: > - class Call_request; > > /* > Non-zero value means we're enabled. It's an int, not bool, because one can > diff --git a/sql/threadpool.h b/sql/threadpool.h > index 7737d056b4a..08e85d362e7 100644 > --- a/sql/threadpool.h > +++ b/sql/threadpool.h > @@ -112,6 +119,8 @@ struct TP_connection > > /* Read for the next client command (async) with specified timeout */ > virtual int start_io() = 0; > + IF_WIN(,virtual) bool leave_work_zone(){ return true; } > + IF_WIN(Notifiable_work_zone work_zone;,) // Dummy object. Ugh, this is unreadable. IF_WIN was supposed to be used in expressions. And why do you need virtual methods here, again? > > virtual void wait_begin(int type)= 0; > virtual void wait_end() = 0; > diff --git a/sql/threadpool_generic.cc b/sql/threadpool_generic.cc > index eb08441a4d5..f61dd90606b 100644 > --- a/sql/threadpool_generic.cc > +++ b/sql/threadpool_generic.cc > @@ -114,7 +114,8 @@ struct pool_timer_t > > static pool_timer_t pool_timer; > > -static void queue_put(thread_group_t *thread_group, TP_connection_generic *connection); > +IF_DBUG(,static) you can remove this IF_DBUG, I don't see you use queue_put outside of this file. > +void queue_put(thread_group_t *thread_group, TP_connection_generic *connection); > static void queue_put(thread_group_t *thread_group, native_event *ev, int cnt); > static int wake_thread(thread_group_t *thread_group,bool due_to_stall); > static int wake_or_create_thread(thread_group_t *thread_group, bool due_to_stall=false); > @@ -1485,8 +1520,41 @@ static int change_group(TP_connection_generic *c, > } > > > +#ifndef WIN32 > +bool TP_connection_generic::leave_work_zone() > +{ > + auto leave_state= work_zone.try_leave(); > + while (unlikely(leave_state == Notifiable_work_zone::SIGNAL)) > + { > + if (thd->apc_target.have_apc_requests()) > + thd->apc_target.process_apc_requests(); > + leave_state= work_zone.try_leave(); > + } > + return leave_state == Notifiable_work_zone::OWNER; > +} > +#endif > + > +#ifndef DBUG_OFF > +#include <random> > +static thread_local std::mt19937 mt_random{std::random_device()()}; > +static thread_local std::uniform_int_distribution<int> kill_dist{1, 100000}; > +void queue_put(thread_group_t *thread_group, TP_connection_generic *connection); > +void tp_send(TP_connection_type* c); > +#endif > + > int TP_connection_generic::start_io() > { > + if(DBUG_IF("tp_wanger")) > + { > + bool survive= kill_dist(mt_random) != 555; does that compile? you define kill_dist under #ifndef DBUG_OFF. You can fix this with, like #ifndef DBUG_OFF if (DBUG_IF("tp_wanger")) { static thread_local std::mt19937 ... etc } #endif > + if (survive) > + { > + state= TP_STATE_INTERRUPTED; > + tp_send(this); > + } > + return survive ? 0 : 1; > + } > + > /* > Usually, connection will stay in the same group for the entire > connection's life. However, we do allow group_count to > diff --git a/vio/viosocket.c b/vio/viosocket.c > index 002ff274b74..45c3d80a33d 100644 > --- a/vio/viosocket.c > +++ b/vio/viosocket.c > @@ -909,20 +916,53 @@ static my_bool socket_peek_read(Vio *vio, uint *bytes) > */ > > #ifndef _WIN32 > +#ifdef _GNU_SOURCE > +#define PFD_SIZE 1 > +static inline int vio_poll(struct pollfd pfd[], nfds_t nr, int timeout) > +{ > + struct timespec tm, *tm_arg= NULL; > + sigset_t signals; > + /* Convert the timeout, in milliseconds, to seconds and microseconds. */ > + if (timeout >= 0) > + { > + tm.tv_sec= timeout / 1000; > + tm.tv_nsec= (timeout % 1000) * 1000 * 1000; > + tm_arg= &tm; > + } > + sigemptyset(&signals); > + > + return ppoll(pfd, 1, tm_arg, &signals); > +} > +#else > +#define PFD_SIZE 2 > +#define vio_poll poll > +#endif why do you need that? How is your vio_poll() different from poll() ? > + > int vio_io_wait(Vio *vio, enum enum_vio_io_event event, int timeout) > { > int ret; > short revents __attribute__((unused)) = 0; > - struct pollfd pfd; > + const short revents_read = MY_POLL_SET_IN | MY_POLL_SET_ERR | POLLRDHUP; > + struct pollfd pfd[PFD_SIZE]; > my_socket sd= mysql_socket_getfd(vio->mysql_socket); > + int pfd_size; > MYSQL_SOCKET_WAIT_VARIABLES(locker, state) /* no ';' */ > DBUG_ENTER("vio_io_wait"); > DBUG_PRINT("enter", ("timeout: %d", timeout)); > > - memset(&pfd, 0, sizeof(pfd)); > + memset(pfd, 0, sizeof(pfd)); > > - pfd.fd= sd; > + pfd[0].fd= sd; > > +#ifndef _GNU_SOURCE > + my_socket self_pipe= threadlocal_get_self_pipe(); > + if (self_pipe) > + { > + pfd[1].fd= self_pipe; > + pfd[1].events= MY_POLL_SET_IN; > + } > +#endif > + pfd_size= PFD_SIZE; really? I'd expect it to be essentially pfd_size= self_pipe ? 2 : 1; > /* > Set the poll bitmask describing the type of events. > The error flags are only valid in the revents bitmask. > @@ -970,16 +1023,16 @@ int vio_io_wait(Vio *vio, enum enum_vio_io_event event, int timeout) > } > > #else > - > int vio_io_wait(Vio *vio, enum enum_vio_io_event event, int timeout) > { > int ret; > - struct timeval tm; > - my_socket fd= mysql_socket_getfd(vio->mysql_socket); > - fd_set readfds, writefds, exceptfds; > MYSQL_SOCKET_WAIT_VARIABLES(locker, state) /* no ';' */ > DBUG_ENTER("vio_io_wait"); > > + /* Not read , e.g connect or write - use select() based wait */ > + struct timeval tm; > + my_socket fd= mysql_socket_getfd(vio->mysql_socket); > + fd_set readfds, writefds, exceptfds; it's C, you cannot mix declarations and code > /* Convert the timeout, in milliseconds, to seconds and microseconds. */ > if (timeout >= 0) > { > @@ -1013,14 +1066,17 @@ int vio_io_wait(Vio *vio, enum enum_vio_io_event event, int timeout) > ret= select(0, &readfds, &writefds, &exceptfds, (timeout >= 0) ? &tm : NULL); > > END_SOCKET_WAIT(locker, timeout); > /* Set error code to indicate a timeout error. */ > if (ret == 0) > WSASetLastError(SOCKET_ETIMEDOUT); > > /* Error or timeout? */ > - if (ret <= 0) > + if (ret <= 0){ indentation > + DBUG_PRINT("vio", ("vio_io_wait: error return: %d. Last error: %d", > + ret, socket_errno)); > DBUG_RETURN(ret); > + } > > /* The requested I/O event is ready? */ > switch (event) > diff --git a/sql/sql_connect.cc b/sql/sql_connect.cc > index 9beb3cca8f8..1c8975cfb23 100644 > --- a/sql/sql_connect.cc > +++ b/sql/sql_connect.cc > @@ -23,7 +23,53 @@ > #include "mariadb.h" > #include "mysqld.h" > #include "sql_priv.h" > -#ifndef _WIN32 > +#ifdef _WIN32 > +#ifdef _WIN32_WINNT > +#undef _WIN32_WINNT > +#endif > + > +#define _WIN32_WINNT 0x0500 > +#include "windows.h" // QueueUserAPC2 > +#define WINDOWS_KERNEL32_DLLNAME_W "kernel32" > + > +// https://github.com/dotnet/runtime/pull/55649/files > + > +// These declarations are for a new special user-mode APC feature introduced in Windows. These are not yet available in Windows > +// SDK headers, so some names below are prefixed with "CLONE_" to avoid conflicts in the future. Once the prefixed declarations > +// become available in the Windows SDK headers, the prefixed declarations below can be removed in favor of the SDK ones. > + > +//enum CLONE_QUEUE_USER_APC_FLAGS > +//{ > +// CLONE_QUEUE_USER_APC_FLAGS_NONE = 0x0, > +// CLONE_QUEUE_USER_APC_FLAGS_SPECIAL_USER_APC = 0x1, > +// > +// CLONE_QUEUE_USER_APC_CALLBACK_DATA_CONTEXT = 0x10000 > +//}; > +//typedef BOOL (WINAPI *QueueUserAPC2Proc)(PAPCFUNC ApcRoutine, HANDLE Thread, ULONG_PTR Data, CLONE_QUEUE_USER_APC_FLAGS Flags); > +//void c() > +//{ > +// HMODULE hKernel32 = LoadLibraryExA(WINDOWS_KERNEL32_DLLNAME_W, NULL, LOAD_LIBRARY_SEARCH_SYSTEM32); > +// > +// // See if QueueUserAPC2 exists > +// QueueUserAPC2Proc pfnQueueUserAPC2Proc = (QueueUserAPC2Proc)GetProcAddress(hKernel32, "QueueUserAPC2"); > +// if (pfnQueueUserAPC2Proc == nullptr) > +// { > +// abort(); > +// } > +// > +// // See if QueueUserAPC2 supports the special user-mode APC with a callback that includes the interrupted CONTEXT. A special > +// // user-mode APC can interrupt a thread that is in user mode and not in a non-alertable wait. > +// if (!(*pfnQueueUserAPC2Proc)(EmptyApcCallback, GetCurrentThread(), 0, SpecialUserModeApcWithContextFlags)) > +// { > +// return; > +// } > +// > +// return pfnQueueUserAPC2Proc; > +//} > +// > +//static QueueUserAPC2Proc pfnQueueUserAPC2 = InitializeSpecialUserModeApc(); what's that big commented block for? and win-something defines/includes - still needed? > + > +#else > #include <netdb.h> // getservbyname, servent > #endif > #include "sql_audit.h" > @@ -1353,6 +1399,107 @@ bool thd_is_connection_alive(THD *thd) > return FALSE; > } > > +#if !defined(_WIN32) > +static void self_pipe_write(); > +#if !defined(_GNU_SOURCE) > +class Thread_apc_context; > +static thread_local Thread_apc_context *_THR_APC_CTX= NULL; may be it should be part of THD (rather, Apc_target) or mysys_var? that is, accessible as current_thd->...->self_pipe or my_thread_var->self_pipe the first one (in Apc_target) seems quite logical > +#endif > + > +class Thread_apc_context > +{ > +public: > +#ifndef _GNU_SOURCE > + my_socket self_pipe[2]{}; > +#endif > + > + bool setup_thread_apc() > + { > +#if defined(_GNU_SOURCE) > + struct sigaction act {}; > + act.sa_handler= [](int) -> void {self_pipe_write();}; why? it doesn't do anything, as far as I can see > + act.sa_flags= 0; > + int ret= sigaction(SIG_APC_NOTIFY, &act, NULL); > + DBUG_ASSERT(ret == 0); > + > + sigset_t signals; > + ret|= sigemptyset(&signals); > + DBUG_ASSERT(ret == 0); > + ret|= sigaddset(&signals, SIG_APC_NOTIFY); > + DBUG_ASSERT(ret == 0); > + > + ret|= pthread_sigmask(SIG_BLOCK, &signals, NULL); > + DBUG_ASSERT(ret == 0); > +#else > + // Self-pipe trick. Create a new pipe and store it thread-locally > + // It can be accessed from Vio later. See also vio_io_wait() > + // From the other end, it is accessed through a threadlocal > + // _THR_APC_CTX, from a SIG_APC_NOTIFY signal handler. > + int ret= pipe(self_pipe); > + > + struct sigaction act {}; > + act.sa_handler= [](int) -> void { self_pipe_write(); }; > + act.sa_flags= 0; > + ret|= sigaction(SIG_APC_NOTIFY, &act, NULL); why? you send a signal and convert it to a self-pipe write. why not to write to the pipe in the first place? like Apc_target::wake() which will send a signal or write to a pipe, as appropriate. And TP_pool::wake() will call thd->apc_target->wake(); > + return ret == 0; > +#endif > + return true; > + } > + bool inited; > + Thread_apc_context() > + { > + inited = setup_thread_apc(); > +#ifndef _GNU_SOURCE > + if (inited) > + _THR_APC_CTX= this; > +#endif > + } > +#ifndef _GNU_SOURCE > + ~Thread_apc_context() > + { > + _THR_APC_CTX= NULL; > + closesocket(self_pipe[0]); > + closesocket(self_pipe[1]); > + } > +#endif > +}; > + > +#ifdef _GNU_SOURCE > +static void self_pipe_write() > +{ > + // No self-pipe is actually used. Instead, ppoll is used to wake up on signal. > +} > +#else > + > +my_socket threadlocal_get_self_pipe() > +{ > + return _THR_APC_CTX ? _THR_APC_CTX->self_pipe[0] : 0; > +} > + > +static void self_pipe_write() > +{ > + DBUG_ASSERT(IF_WIN(0, pthread_self() == select_thread) || _THR_APC_CTX); > + if (!_THR_APC_CTX) > + return; // SIGUSR1 is used in thr_alarm, which can wake up main thread. > + char buf[4]{}; > + send(_THR_APC_CTX->self_pipe[1], buf, sizeof buf, 0); > +} > +#endif > +#endif /* !_WIN32 */ > + > +bool thread_per_connection_notify_apc(THD *thd) > +{ > +#ifdef WIN32 > + auto vio= thd->net.vio; > + HANDLE h= > + (vio->type == VIO_TYPE_NAMEDPIPE) ? vio->hPipe : (HANDLE)vio->mysql_socket.fd; > + return CancelIoEx(h, NULL); > +#else > + bool ret = pthread_kill(thd->mysys_var->pthread_self, SIG_APC_NOTIFY) == 0; > + DBUG_ASSERT(ret || errno == EAGAIN); > + return ret; > +#endif > +} 1. I'd prefer APC framework implementation to stay in my_apc.cc. not being spread over three (?) different files. 2. how is it diferent from ::wake() ? > > void do_handle_one_connection(CONNECT *connect, bool put_in_cache) > { > diff --git a/sql/threadpool_common.cc b/sql/threadpool_common.cc > index f72f46b3a6b..8f24bc9e4f7 100644 > --- a/sql/threadpool_common.cc > +++ b/sql/threadpool_common.cc > @@ -320,6 +346,10 @@ static void threadpool_remove_connection(THD *thd) > end_connection(thd); > close_connection(thd, 0); > unlink_thd(thd); > + // The rest of APC requests should be processed after unlinking. > + // This guarantees that no new APC requests can be added. > + // TODO: better notify the requestor with some KILLED state here. > + thd->apc_target.process_apc_requests(); not sure it guarantees much. One can find the thd before the unlink and add a new request after the unlink. or is it impossible? > PSI_CALL_delete_current_thread(); // before THD is destroyed > delete thd; > > diff --git a/sql/my_apc.cc b/sql/my_apc.cc > index 9777deb399a..a89d9c8bc19 100644 > --- a/sql/my_apc.cc > +++ b/sql/my_apc.cc > @@ -47,11 +47,12 @@ void Apc_target::init(mysql_mutex_t *target_mutex) > void Apc_target::enqueue_request(Call_request *qe) > { > mysql_mutex_assert_owner(LOCK_thd_kill_ptr); > - if (apc_calls) > + Call_request *old_apc_calls= apc_calls; eh, what? you modify apc_calls without a lock somewhere? > + if (old_apc_calls) > { > - Call_request *after= apc_calls->prev; > - qe->next= apc_calls; > - apc_calls->prev= qe; > + Call_request *after= old_apc_calls->prev; > + qe->next= old_apc_calls; > + old_apc_calls->prev= qe; > > qe->prev= after; > after->next= qe; > @@ -100,89 +109,118 @@ void init_show_explain_psi_keys(void) > if (PSI_server == NULL) > return; > > - PSI_server->register_cond("sql", show_explain_psi_conds, > - array_elements(show_explain_psi_conds)); > + PSI_server->register_cond("sql", apc_request_psi_conds, > + array_elements(apc_request_psi_conds)); > + PSI_server->register_mutex("sql", apc_request_psi_locks, > + array_elements(apc_request_psi_locks)); > } > #endif > > - > -/* > - Make an APC (Async Procedure Call) to another thread. > - > - @detail > - Make an APC call: schedule it for execution and wait until the target > - thread has executed it. > - > - - The caller is responsible for making sure he's not posting request > - to the thread he's calling this function from. > - > - - The caller must have locked target_mutex. The function will release it. > - > - @retval FALSE - Ok, the call has been made > - @retval TRUE - Call wasnt made (either the target is in disabled state or > - timeout occurred) > -*/ > - > -bool Apc_target::make_apc_call(THD *caller_thd, Apc_call *call, > - int timeout_sec, bool *timed_out) > +/** > + Wait gracefully until the request is completed. > + @retval 0 -- Success > + @retval 1 -- Timeout > + */ > +int Apc_target::wait_for_completion(THD *caller_thd, Call_request *apc_request, > + int timeout_sec) > { > - bool res= TRUE; > - *timed_out= FALSE; > - > - if (enabled) > - { > - /* Create and post the request */ > - Call_request apc_request; > - apc_request.call= call; > - apc_request.processed= FALSE; > - mysql_cond_init(key_show_explain_request_COND, &apc_request.COND_request, > - NULL); > - enqueue_request(&apc_request); > - apc_request.what="enqueued by make_apc_call"; > - > struct timespec abstime; > const int timeout= timeout_sec; > set_timespec(abstime, timeout); > > + DBUG_EXECUTE_IF("apc_timeout", set_timespec_nsec(abstime, 1000000);); > + int res = 1; > int wait_res= 0; > PSI_stage_info old_stage; > - caller_thd->ENTER_COND(&apc_request.COND_request, LOCK_thd_kill_ptr, > + > + mysql_mutex_unlock(LOCK_thd_kill_ptr); I don't understand how this works. After you release LOCK_thd_kill_ptr, the THD might be destroyed. How do you detect that? By the wait timing out? Who will destroy the Call_request? > + > + mysql_mutex_lock(&apc_request->LOCK_request); > + caller_thd->ENTER_COND(&apc_request->COND_request, &apc_request->LOCK_request, > &stage_show_explain, &old_stage); > /* todo: how about processing other errors here? */ > - while (!apc_request.processed && (wait_res != ETIMEDOUT)) > + while (!apc_request->processed && (wait_res != ETIMEDOUT)) > { > - /* We own LOCK_thd_kill_ptr */ > - wait_res= mysql_cond_timedwait(&apc_request.COND_request, > - LOCK_thd_kill_ptr, &abstime); > - // &apc_request.LOCK_request, &abstime); > + wait_res= mysql_cond_timedwait(&apc_request->COND_request, > + &apc_request->LOCK_request, &abstime); > if (caller_thd->killed) > break; > + > + if (caller_thd->apc_target.have_apc_requests()) > + { > + mysql_mutex_unlock(&apc_request->LOCK_request); > + caller_thd->apc_target.process_apc_requests(); > + mysql_mutex_lock(&apc_request->LOCK_request); > + } > } > > - if (!apc_request.processed) > + if (!apc_request->processed) > { > /* > The wait has timed out, or this thread was KILLed. > - Remove the request from the queue (ok to do because we own > - LOCK_thd_kill_ptr) > + We can't delete it from the queue, because LOCK_thd_kill_ptr is already > + released. It can't be reacquired because of the ordering with > + apc_request->LOCK_request. > + However, apc_request->processed is guarded by this lock. > + Set processed= TRUE and transfer the ownership to the processor thread. > + It should free the resources itself. > + apc_request cannot be referred after unlock anymore in this case. > */ > - apc_request.processed= TRUE; > - dequeue_request(&apc_request); > - *timed_out= TRUE; > - res= TRUE; > + apc_request->processed= TRUE; > + res= 1; > } > else > { > /* Request was successfully executed and dequeued by the target thread */ > - res= FALSE; > + res= 0; > } > - /* > - exit_cond() will call mysql_mutex_unlock(LOCK_thd_kill_ptr) for us: > - */ > + > + /* EXIT_COND() will call mysql_mutex_unlock(LOCK_request) for us */ > caller_thd->EXIT_COND(&old_stage); > > - /* Destroy all APC request data */ > - mysql_cond_destroy(&apc_request.COND_request); > + return res; > +} > + > +/** Create and post the request */ > +void Apc_target::enqueue_request(Call_request *request_buff, Apc_call *call) > +{ > + request_buff->call= call; > + request_buff->processed= FALSE; > + enqueue_request(request_buff); > + request_buff->what="enqueued by make_apc_call"; > +} > + > +/** > + Make an APC (Async Procedure Call) to another thread. > + > + @detail > + Make an APC call: schedule it for execution and wait until the target > + thread has executed it. > + > + - The caller is responsible for making sure he's not posting request > + to the thread he's calling this function from. > + > + - The caller must have locked target_mutex. The function will release it. > + > + @retval FALSE - Ok, the call has been made > + @retval TRUE - Call wasnt made (either the target is in disabled state or > + timeout occurred) > +*/ > + > +bool Apc_target::make_apc_call(THD *caller_thd, Apc_call *call, > + int timeout_sec, bool *timed_out) > +{ > + bool res= TRUE; > + *timed_out= FALSE; > + > + if (enabled) > + { > + Call_request *apc_request= new Call_request; > + enqueue_request(apc_request, call); > + res= wait_for_completion(caller_thd, apc_request, timeout_sec); > + *timed_out= res; > + if (!*timed_out) > + delete apc_request; > } > else > { > diff --git a/storage/perfschema/pfs_variable.cc b/storage/perfschema/pfs_variable.cc > index 17b7dfc200c..698ff1eca6c 100644 > --- a/storage/perfschema/pfs_variable.cc > +++ b/storage/perfschema/pfs_variable.cc > @@ -164,7 +167,11 @@ int PFS_system_variable_cache::do_materialize_global(void) > during materialization. > */ > if (!m_external_init) > + { > + mysql_mutex_lock(&LOCK_plugin); > init_show_var_array(OPT_GLOBAL, true); > + mysql_mutex_unlock(&LOCK_plugin); 1. why not move the locking inside init_show_var_array()? 2. init_show_var_array() locks LOCK_system_variables_hash - is that not enough? > + } > > /* Resolve the value for each SHOW_VAR in the array, add to cache. */ > for (SHOW_VAR *show_var= m_show_var_array.front(); > @@ -230,37 +236,26 @@ int PFS_system_variable_cache::do_materialize_all(THD *unsafe_thd) > m_materialized= false; > m_cache.clear(); > > - /* Block plugins from unloading. */ > - mysql_mutex_lock(&LOCK_plugin_delete); also please remove all references of LOCK_plugin_delete > - > /* > Build array of SHOW_VARs from system variable hash. Do this within > LOCK_plugin_delete to ensure that the hash table remains unchanged > while this thread is materialized. > */ > if (!m_external_init) > + { > + mysql_mutex_lock(&LOCK_plugin); > init_show_var_array(OPT_SESSION, false); > + mysql_mutex_unlock(&LOCK_plugin); > + } > > /* Get and lock a validated THD from the thread manager. */ > if ((m_safe_thd= get_THD(unsafe_thd)) != NULL) > { > DEBUG_SYNC(m_current_thd, "materialize_session_variable_array_THD_locked"); > - for (SHOW_VAR *show_var= m_show_var_array.front(); > - show_var->value && (show_var != m_show_var_array.end()); show_var++) > - { > - /* Resolve value, convert to text, add to cache. */ > - System_variable system_var(m_safe_thd, show_var, m_query_scope, false); > - m_cache.push(system_var); > - } > - > - /* Release lock taken in get_THD(). */ > - mysql_mutex_unlock(&m_safe_thd->LOCK_thd_data); > + ret= make_call(&PFS_system_variable_cache::refresh_vars, 1); > > m_materialized= true; > - ret= 0; > } > - > - mysql_mutex_unlock(&LOCK_plugin_delete); > return ret; > } > > @@ -312,6 +307,105 @@ void PFS_system_variable_cache::free_mem_root(void) > } > } > } > +class PFS_system_variable_cache_apc: public Apc_target::Apc_call > +{ > +public: > + typedef PFS_system_variable_cache::Request_func Request; > + PFS_system_variable_cache_apc(PFS_system_variable_cache *pfs, Request func, > + uint param) > + : m_pfs(pfs), m_func(func), m_param(param) {} > +private: > + PFS_system_variable_cache *m_pfs; > + Request m_func; > + uint m_param; > + > + void call_in_target_thread() override > + { > + call(m_pfs, m_func, m_param); > + } > +public: > + static void call(PFS_system_variable_cache *pfs, Request func, uint param) > + { > + THD *safe_thd= pfs->safe_thd(); > + > + DBUG_ASSERT(pfs->query_scope() == OPT_SESSION); > + > + mysql_mutex_lock(&LOCK_global_system_variables); > + if (!safe_thd->variables.dynamic_variables_ptr || > + global_system_variables.dynamic_variables_head > > + safe_thd->variables.dynamic_variables_head) > + { > + mysql_prlock_rdlock(&LOCK_system_variables_hash); > + sync_dynamic_session_variables(safe_thd, false); > + mysql_prlock_unlock(&LOCK_system_variables_hash); > + } > + mysql_mutex_unlock(&LOCK_global_system_variables); > + > + (pfs->*func)(param); > + } > +}; > + > +void PFS_system_variable_cache::refresh_vars(uint all) > +{ > + for (SHOW_VAR *show_var= m_show_var_array.front(); > + show_var->value && (show_var != m_show_var_array.end()); show_var++) > + { > + sys_var *value= (sys_var *)show_var->value; > + > + /* Match the system variable scope to the target scope. */ > + if (all || match_scope(value->scope())) > + { > + /* Resolve value, convert to text, add to cache. */ > + System_variable system_var(m_safe_thd, show_var, m_query_scope, false); > + m_cache.push(system_var); > + } > + } > +} > +void PFS_system_variable_cache::refresh_one_var(uint index) > +{ > + SHOW_VAR *show_var= &m_show_var_array.at(index); > + > + if (show_var && show_var->value && > + (show_var != m_show_var_array.end())) > + { > + sys_var *value= (sys_var *)show_var->value; > + > + /* Match the system variable scope to the target scope. */ > + if (match_scope(value->scope())) > + { > + /* Resolve value, convert to text, add to cache. */ > + System_variable system_var(m_safe_thd, show_var, m_query_scope, false); > + m_cache.push(system_var); > + } > + } > +} > + > + > +#define MAKE_CALL_MAX_RETRIES 3 unused > + > +int PFS_system_variable_cache::make_call(Request_func func, uint param) > +{ > + int ret= 0; > + THD *requestor_thd= m_current_thd; > + if (requestor_thd == m_safe_thd) > + { > + mysql_mutex_unlock(&m_safe_thd->LOCK_thd_kill); > + PFS_system_variable_cache_apc::call(this, func, param); > + } > + else > + { > + PFS_system_variable_cache_apc apc_call(this, func, param); > + auto *request= new Apc_target::Call_request; > + m_safe_thd->apc_target.enqueue_request(request, &apc_call); > + m_safe_thd->abort_current_cond_wait(false, false); > + m_safe_thd->scheduler->notify_apc(m_safe_thd); > + DEBUG_SYNC(requestor_thd, "apc_after_notify"); > + ret= m_safe_thd->apc_target.wait_for_completion(requestor_thd, request, 10); > + if (ret == 0) > + delete request; > + } > + return ret; > +} > > /** > Build a SESSION system variable cache for a pfs_thread. Regards, Sergei Chief Architect, MariaDB Server and security(a)mariadb.org

1 0

Re: c46e7c5cb64: MDEV-12252 ROW data type for stored function return values
by Sergei Golubchik 05 Sep '24

05 Sep '24

Hi, Alexander, Looks good, thanks. Great commit comment too. Just a few questions and test suggestions - see below. Regards, Sergei Chief Architect, MariaDB Server and security(a)mariadb.org On Aug 23, Alexander Barkov wrote: > revision-id: c46e7c5cb64 (mariadb-11.4.1-5-gc46e7c5cb64) > parent(s): ae6684d79f4 > author: Alexander Barkov > committer: Alexander Barkov > timestamp: 2024-02-20 13:49:54 +0400 > message: > > MDEV-12252 ROW data type for stored function return values > diff --git a/mysql-test/main/sp-anchor-row-type-table.test b/mysql-test/main/sp-anchor-row-type-table.test > index 7123f9160db..2fbcfbffc1b 100644 > --- a/mysql-test/main/sp-anchor-row-type-table.test > +++ b/mysql-test/main/sp-anchor-row-type-table.test > @@ -939,3 +939,70 @@ DROP TABLE t1; > --echo # > --echo # End of 10.4 tests > --echo # > + > +--echo # > +--echo # Start of 11.3 tests > +--echo # > + > +--echo # > +--echo # MDEV-12252 ROW data type for stored function return values > +--echo # > + > +--error ER_PARSE_ERROR > +CREATE FUNCTION f1() RETURNS ROW TYPE OF step1.step2.step3 RETURN ROW(1,2); > + > +CREATE FUNCTION f1() RETURNS ROW TYPE OF t1 RETURN ROW(1,2); you don't resolve t1 at CREATE FUNCTION time. why? is it how TYPE OF works in DECLARE too? > +SHOW CREATE FUNCTION f1; > +--error ER_NO_SUCH_TABLE > +SELECT f1(); > +DROP FUNCTION f1; > + > +CREATE FUNCTION f1() RETURNS ROW TYPE OF test.t1 RETURN ROW(1,2); > +SHOW CREATE FUNCTION f1; > +--error ER_NO_SUCH_TABLE > +SELECT f1(); > +DROP FUNCTION f1; > + > + > +CREATE TABLE t1 (a INT, b VARCHAR(32)); > +CREATE FUNCTION f1() RETURNS ROW TYPE OF test.t1 RETURN (SELECT * FROM t1); > +SHOW CREATE FUNCTION f1; > +DELIMITER $$; > +CREATE PROCEDURE p1() > +BEGIN > + DECLARE r ROW TYPE OF t1 DEFAULT f1(); > + SELECT r.a, r.b; > +END; > +$$ > +DELIMITER ;$$ > + > +# Testing with no rows > +CALL p1(); > +--error ER_OPERAND_COLUMNS > +SELECT f1(); > +SELECT f1()=ROW(1,'b1') AS c; > +SELECT f1()=ROW(NULL,NULL) AS c; > + > +# Testing with one row > +INSERT INTO t1 VALUES (1,'b1'); > +CALL p1(); > +--error ER_OPERAND_COLUMNS > +SELECT f1(); > +SELECT f1()=ROW(1,'b1') AS c; > +SELECT f1()=ROW(1,'') AS c; > +SELECT f1()=ROW(2,'b1') AS c; > +SELECT f1()=ROW(1,NULL) AS c; > +SELECT f1()=ROW(NULL,'b1') AS c; do one more SHOW CREATE FUNCTION here > + > +# Testing with two rows > +INSERT INTO t1 VALUES (2,'b2'); > +--error ER_SUBQUERY_NO_1_ROW > +CALL p1(); > +--error ER_OPERAND_COLUMNS > +SELECT f1(); > +--error ER_SUBQUERY_NO_1_ROW > +SELECT f1()=ROW(1,'b1') AS c; > + > +DROP PROCEDURE p1; > +DROP FUNCTION f1; > +DROP TABLE t1; > diff --git a/mysql-test/main/sp-anchor-type.test b/mysql-test/main/sp-anchor-type.test > index 0e24ef900d8..fed53fd011a 100644 > --- a/mysql-test/main/sp-anchor-type.test > +++ b/mysql-test/main/sp-anchor-type.test > @@ -767,3 +767,78 @@ BEGIN NOT ATOMIC > END; > $$ > DELIMITER ;$$ > + > + > +--echo # > +--echo # Start of 11.3 tests > +--echo # > + > +--echo # > +--echo # MDEV-12252 ROW data type for stored function return values > +--echo # > + > +--error ER_PARSE_ERROR > +CREATE FUNCTION f1() RETURNS TYPE OF a RETURN 1; > + > +CREATE FUNCTION f1() RETURNS TYPE OF t1.a RETURN (SELECT min(a) FROM t1); > +SHOW CREATE FUNCTION f1; > +--error ER_NO_SUCH_TABLE > +SELECT f1(); > +CREATE TABLE t1 (b INT); > +--error ER_BAD_FIELD_ERROR > +SELECT f1(); > +DROP TABLE t1; > +DROP FUNCTION f1; > + > +CREATE DATABASE db1; > +DELIMITER $$; > +CREATE FUNCTION db1.f1() RETURNS TYPE OF db1.t1.a > +BEGIN > + RETURN (SELECT min(a) FROM t1); > +END; > +$$ > +DELIMITER ;$$ > +SHOW CREATE FUNCTION db1.f1; > +--error ER_NO_SUCH_TABLE > +SELECT db1.f1(); > +CREATE TABLE db1.t1 (b TIME); > +--error ER_BAD_FIELD_ERROR > +SELECT db1.f1(); > +DROP TABLE db1.t1; > +CREATE TABLE db1.t1 (a TIME); > +SELECT db1.f1(); > +INSERT INTO db1.t1 VALUES ('10:20:30'); > +SELECT db1.f1(); > +DROP TABLE db1.t1; > +DROP FUNCTION db1.f1; > +DROP DATABASE db1; > + > +CREATE TABLE t1 (a TIME); > +CREATE FUNCTION f1() RETURNS TYPE OF test.t1.a RETURN (SELECT min(a) FROM t1); > +SHOW CREATE FUNCTION f1; > +SELECT f1(); > +DROP FUNCTION f1; > +DROP TABLE t1; > + > +CREATE TABLE t1 (a TIME); > +DELIMITER $$; > +CREATE FUNCTION f1() RETURNS TYPE OF t1.a > +BEGIN > + RETURN (SELECT min(a) FROM t1); > +END; > +$$ > +DELIMITER ;$$ > +SHOW CREATE FUNCTION f1; > +SELECT f1(); > +CREATE TABLE t2 AS SELECT f1(); > +SHOW CREATE TABLE t2; > +SELECT * FROM t2; > +DROP TABLE t2; > +INSERT INTO t1 VALUES ('10:20:30'); > +SELECT f1(); > +CREATE TABLE t2 AS SELECT f1(); > +SHOW CREATE TABLE t2; > +SELECT * FROM t2; > +DROP TABLE t2; please, also try here DROP TABLE t1; CREATE TABLE t1 (a INT); INSERT INTO t1 VALUES (10); CREATE TABLE t2 AS SELECT f1(); the point is to change TYPE OF t1.a after the function has been successfully called, and thus is already in the sp cache. > +DROP FUNCTION f1; > +DROP TABLE t1; > diff --git a/mysql-test/suite/compat/oracle/t/sp-package.test b/mysql-test/suite/compat/oracle/t/sp-package.test > index aefede41c8b..7c231d736c5 100644 > --- a/mysql-test/suite/compat/oracle/t/sp-package.test > +++ b/mysql-test/suite/compat/oracle/t/sp-package.test > @@ -3109,3 +3109,79 @@ DROP TABLE t1; > DROP FUNCTION f1_deterministic; > DROP FUNCTION f2_not_deterministic; > DROP PACKAGE pkg1; > + > +--echo # > +--echo # MDEV-12252 ROW data type for stored function return values > +--echo # > + > +--echo # > +--echo # Testing fixed ROW type with package routines > +--echo # > + > +DELIMITER $$; > +CREATE PACKAGE pkg > +AS > + FUNCTION f1() RETURN ROW(a INT, b VARCHAR(32)); > + PROCEDURE p1(r ROW(a INT, b VARCHAR(32))); > + PROCEDURE p2(); > +END; > +$$ > +CREATE PACKAGE BODY pkg > +AS > + FUNCTION f1() RETURN ROW(a INT, b VARCHAR(32)) AS > + BEGIN > + RETURN ROW(1,'b1'); > + END; > + PROCEDURE p1(r ROW(a INT, b VARCHAR(32))) AS > + BEGIN > + SELECT r.a, r.b; > + END; > + PROCEDURE p2() AS > + BEGIN > + CALL p1(f1()); > + END; > +END; > +$$ > +DELIMITER ;$$ > +CALL pkg.p1(pkg.f1()); > +CALL pkg.p2; > +DROP PACKAGE pkg; > + > + > +--echo # > +--echo # Testing table%ROWTYPE with package routines > +--echo # now you can also add package tests outside of oracle mode > + > +CREATE TABLE t1 (a INT, b VARCHAR(32)); > +INSERT INTO t1 VALUES (1,'b1'); > +DELIMITER /; > +CREATE PACKAGE pkg > +AS > + FUNCTION f1 RETURN t1%ROWTYPE; > + PROCEDURE p1(r t1%ROWTYPE); > + PROCEDURE p2; > +END; > +/ > +CREATE PACKAGE BODY pkg > +AS > + FUNCTION f1 RETURN t1%ROWTYPE AS > + r t1%ROWTYPE; > + BEGIN > + SELECT * INTO r FROM t1; > + RETURN r; > + END; > + PROCEDURE p1(r t1%ROWTYPE) AS > + BEGIN > + SELECT r.a || ' ' || r.b; > + END; > + PROCEDURE p2 AS > + BEGIN > + p1(f1()); > + END; > +END; > +/ > +DELIMITER ;/ > +CALL pkg.p1(pkg.f1()); > +CALL pkg.p2; > +DROP PACKAGE pkg; > +DROP TABLE t1; > diff --git a/sql/field.h b/sql/field.h > index 7f1c243a180..7af069a9735 100644 > --- a/sql/field.h > +++ b/sql/field.h > @@ -1373,6 +1374,20 @@ class Field: public Value_source > in str and restore it with set() if needed > */ > virtual void sql_type(String &str) const =0; > + void sql_type_for_sp_returns(String &str) const This is oddly specific. Strange that we don't need to print the field type with charset/collation anywhere else, but for sp returns we do. > + { > + sql_type(str); > + if (has_charset()) > + { > + str.append(STRING_WITH_LEN(" CHARSET ")); > + str.append(charset()->cs_name); > + if (Charset(charset()).can_have_collate_clause()) > + { > + str.append(STRING_WITH_LEN(" COLLATE ")); > + str.append(charset()->coll_name); > + } > + } > + } > virtual void sql_rpl_type(String *str) const { sql_type(*str); } > virtual uint size_of() const =0; // For new field > inline bool is_null(my_ptrdiff_t row_offset= 0) const > diff --git a/sql/field.cc b/sql/field.cc > index 35be045620d..23b050de221 100644 > --- a/sql/field.cc > +++ b/sql/field.cc > @@ -2768,6 +2768,32 @@ bool Field_row::sp_prepare_and_store_item(THD *thd, Item **value) > } > > > +uint Field_row::cols() const > +{ > + // The table with ROW members must be already instantiated > + DBUG_ASSERT(m_table); > + return m_table->s->fields; > +} > + > + > +void Field_row::sql_type(String &res) const would be more logical to call this Field_row::sql_type_for_sp_returns() > +{ > + res.set_ascii(STRING_WITH_LEN("row(")); > + for (uint i= 0; i < m_table->s->fields; i++) > + { > + if (i > 0) > + res.append(','); > + Field *field= m_table->field[i]; > + res.append(field->field_name); > + res.append(' '); > + StringBuffer<64> col; > + field->sql_type_for_sp_returns(col); > + res.append(col.to_lex_cstring()); > + } > + res.append(')'); > +} > + > + > /**************************************************************************** > Functions for the Field_decimal class > This is an number stored as a pre-space (or pre-zero) string > diff --git a/sql/item.cc b/sql/item.cc > index 54ad511cb6b..0a125752979 100644 > --- a/sql/item.cc > +++ b/sql/item.cc > @@ -3001,10 +3003,31 @@ Item_sp::init_result_field(THD *thd, uint max_length, uint maybe_null, > dummy_table->s->table_name= empty_clex_str; > dummy_table->maybe_null= maybe_null; > > + if (m_sp->m_return_field_def.is_column_type_ref() && > + m_sp->m_return_field_def.column_type_ref()-> > + resolve_type_ref(thd, &m_sp->m_return_field_def)) > + DBUG_RETURN(TRUE); Here you overwrite old m_return_field_def value which was a column_type_ref with its actual type, don't you? What will happen if you change the column's type after the sp was already executed once? > + > if (!(sp_result_field= m_sp->create_result_field(max_length, name, > dummy_table))) > DBUG_RETURN(TRUE); > > + /* > + In case of a ROW return type we need to create Item_field_row > + on top of Field_row, and remember it in sp_result_item_field_row. > + ROW members are later accessed using sp_result_item_field_row->addr(i), > + e.g. when copying the function return value to a local variable. > + For scalar return types no Item_field is needed around sp_result_field, > + as the value is fetched directly from sp_result_field, > + inside Item_func_sp::val_xxx() methods. Sorry, I don't understand that. Why cannot you use Field_row here? > + */ > + if (Field_row *field_row= dynamic_cast<Field_row*>(sp_result_field)) > + { > + if (!(sp_result_item_field_row= > + m_sp->m_return_field_def.make_item_field_row(thd, field_row))) > + DBUG_RETURN(true); > + } > + > if (sp_result_field->pack_length() > sizeof(result_buf)) > { > void *tmp; > diff --git a/sql/sp.cc b/sql/sp.cc > index ddaeeb8cb7a..7eab8304c31 100644 > --- a/sql/sp.cc > +++ b/sql/sp.cc > @@ -1091,18 +1136,19 @@ sp_returns_type(THD *thd, String &result, const sp_head *sp) > bzero((char*) &share, sizeof(share)); > table.in_use= thd; > table.s = &share; > - field= sp->create_result_field(0, 0, &table); > - field->sql_type(result); > + field= create_result_field(0, 0, &table); > > - if (field->has_charset()) > + if (m_return_field_def.is_row()) > { > - result.append(STRING_WITH_LEN(" CHARSET ")); > - result.append(field->charset()->cs_name); > - if (Charset(field->charset()).can_have_collate_clause()) > - { > - result.append(STRING_WITH_LEN(" COLLATE ")); > - result.append(field->charset()->coll_name); > - } > + Field_row *field_row= dynamic_cast<Field_row*>(field); > + if (!field_row->row_create_fields( > + thd, m_return_field_def.row_field_definitions())) > + field->sql_type(result); when is this used? you're creating a TABLE and Field's - only to print types and charsets. This might be tolerable, but only if it's not needed often > + } > + else > + { > + DBUG_ASSERT(m_return_field_def.type_handler()->is_scalar_type()); > + field->sql_type_for_sp_returns(result); > } > > delete field; > diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy > index 5977e309ae4..f11e4f89466 100644 > --- a/sql/sql_yacc.yy > +++ b/sql/sql_yacc.yy > @@ -18347,7 +18338,50 @@ sp_parameters: > ; > > sf_returned_type_clause: > - RETURNS_SYM sf_return_type > + RETURNS_SYM > + { > + LEX *lex= Lex; you still do this lex=Lex? :) it became obsolete, like, 15 years ago. If not more. > + lex->init_last_field(&lex->sphead->m_return_field_def, > + &empty_clex_str); > + } > + sf_return_type > + ; > + > +sf_return_type: > + field_type > + { > + if (unlikely(Lex->sf_return_fill_definition($1))) > + MYSQL_YYABORT; > + } > + | ROW_SYM row_type_body > + { > + if (Lex->sf_return_fill_definition_row($2)) > + MYSQL_YYABORT; > + } > + | ROW_SYM TYPE_SYM OF_SYM ident > + { > + if (Lex->sf_return_fill_definition_rowtype_of( > + Qualified_column_ident(&$4))) > + MYSQL_YYABORT; > + } > + | ROW_SYM TYPE_SYM OF_SYM ident '.' ident > + { > + if (Lex->sf_return_fill_definition_rowtype_of( > + Qualified_column_ident(&$4, &$6))) > + MYSQL_YYABORT; > + } > + | TYPE_SYM OF_SYM ident '.' ident > + { > + if (Lex->sf_return_fill_definition_type_of( > + Qualified_column_ident(&$3, &$5))) > + MYSQL_YYABORT; > + } > + | TYPE_SYM OF_SYM ident '.' ident '.' ident > + { > + if (Lex->sf_return_fill_definition_type_of( > + Qualified_column_ident(thd, &$3, &$5, &$7))) > + MYSQL_YYABORT; > + } > ; > > package_implementation_item_declaration:

2 1

Clean up executable test result files:
by Otto Kekäläinen 04 Sep '24

04 Sep '24

Hi! In https://github.com/MariaDB/server/commit/9cb75f333fa2ae816909f95a4e19e6ddac… Alexander made some test result files executable (look at 100644 → 100755 in git diff). I sent twice a PR to fix this: - Oct 2023: https://github.com/MariaDB/server/pull/2779 - Feb 2024: https://github.com/MariaDB/server/pull/3042 Unfortunately, I can see that on the main branch this is executable again: mariadb-server main+ ± find mysql-test -executable -name *.test -or -executable -name *.result mysql-test/main/lowercase_table2.result Could the next person doing merges fix this tiny annoyance once and for all? Thanks :)

1 0

Re: [PATCH] MDEV-32014 Rename binlog cache temporary file to binlog file for large transaction
by Kristian Nielsen 03 Sep '24

03 Sep '24

Andrei Elkin <andrei.elkin(a)mariadb.com> writes: >> So please do a small refactoring of trx_group_commit_leader() and keep >> only ... > > and no need for refactoring really arises. Well, the refactoring is good, even without the MDEV-32014 patch. The function trx_group_commit_leader() is already too long and complicated, and does too much in a single function. And the first part (obtaining the queue) is cleanly separated from the second part (committing each entry in the queue in-order in binlog and participating engines). It's just nice that this improvement of the existing code also makes the MDEV-32014 patch integrate cleaner. > I noted earlier on this part on the PR page and LiBing seems to have > practically proven (by the most recent commit that you must've missed) that > conducting the rename inside the flush stage of > trx_group_commit_leader() is viable: > > https://github.com/MariaDB/server/pull/3402/commits/287511a703f7dff47939633… > > By that measure the flushing is expanded onto possibly multiple "implicitly" > rotating log files Hm, I don't like the change 287511a703f7dff47939633475edd436a542cf6f. In fact, that is one of the things I like the most about Libing Song's patch, that it so cleanly separates the commit-by-rotate logic from the complex group commit logic in queue_for_group_commit() and trx_group_commit_leader(), which is already too complex. The original patch without 287511a703f7dff47939633475edd436a542cf6f seems much cleaner. It is better to do as in the original patch, and treat commit-by-rotate like a normal binlog rotate, not try to participate that transaction in binlog group commit. We cannot share the fsync anyway, as the files are different. And it introduces a lot of complexities to suddenly have binlog rotations in the middle of group commit - what about the complex accounting around xid_count and binlog checkpoints? There will be a lot of corner cases eg. if _two_ commit-by-rotate happens in the same group commit, with lots of potential for subtle bugs that will occur only very rarely in production and be almost impossible to track down. I strongly prefer the original patch I reviewed (with my suggested changes to write_transaction_to_binlog_events() and trx_group_commit_leader()), where the commit-by-rotate logic is cleanly separated from the group commit logic and implemented mostly using the existing binlog rotate logic. - Kristian.

2 2

Re: [PATCH] MDEV-34857: Implement --slave-abort-blocking-timeout
by Michael Widenius 03 Sep '24

03 Sep '24

Review of MDEV-34857: Implement --slave-abort-blocking-timeout If a slave replicating an event has waited for more than @@slave_abort_blocking_timeout for a conflicting metadata lock held by a non-replication thread, the blocking query is killed to allow replication to proceed and not be blocked indefinitely by a user query. <cut> diff --git a/sql/mdl.cc b/sql/mdl.cc index faccd1c9476..9845718e165 100644 --- a/sql/mdl.cc +++ b/sql/mdl.cc <cut> @@ -2397,14 +2398,39 @@ MDL_context::acquire_lock(MDL_request *mdl_request, double lock_wait_timeout) find_deadlock(); - struct timespec abs_timeout, abs_shortwait; + struct timespec abs_timeout, abs_shortwait, abs_abort_blocking_timeout; + bool abort_blocking_enabled= false; + double abort_blocking_timeout= slave_abort_blocking_timeout; Add a comment here. Something like /* This code implements aborting of queries on the slaves that stops replication from continuing. */ + if (abort_blocking_timeout < lock_wait_timeout && + m_owner->get_thd()->rgi_slave) + { + set_timespec_nsec(abs_abort_blocking_timeout, + (ulonglong)(abort_blocking_timeout * 1000000000ULL)); + abort_blocking_enabled= true; + } set_timespec_nsec(abs_timeout, (ulonglong)(lock_wait_timeout * 1000000000ULL)); - set_timespec(abs_shortwait, 1); wait_status= MDL_wait::EMPTY; - while (cmp_timespec(abs_shortwait, abs_timeout) <= 0) + for (;;) { + bool abort_blocking= false; + set_timespec(abs_shortwait, 1); + if (abort_blocking_enabled && + cmp_timespec(abs_shortwait, abs_abort_blocking_timeout) >= 0) + { + /* + If a slave DDL has waited for --slave-abort-select-timeout, then notify + any blocking SELECT once before continuing to wait until the full + timeout. + */ + abs_shortwait= abs_abort_blocking_timeout; + abort_blocking= true; + abort_blocking_enabled= false; + } + if (cmp_timespec(abs_shortwait, abs_timeout) > 0) + break; Is the above right last compare right? Assuming that cmp_timespec(abs_shortwait, abs_abort_blocking_timeout) > 0 then the above will not break and we will not signal other threads to break even if the previous code expectrs us to abort blocking. (I think the cmp_timespec() return test should be the same in both cases) Shouldn't the code be: for (;;) { bool abort_blocking= false; set_timespec(abs_shortwait, 1); if (abort_blocking_enabled && cmp_timespec(abs_shortwait, abs_abort_blocking_timeout) >= 0) { /* If a slave DDL has waited for --slave-abort-select-timeout, then notify any blocking SELECT once before continuing to wait until the full timeout. */ abort_blocking= true; abort_blocking_enabled= false; } else if (cmp_timespec(abs_shortwait, abs_timeout) > 0) break; Or have I missed something? /* abs_timeout is far away. Wait a short while and notify locks. */ wait_status= m_wait.timed_wait(m_owner, &abs_shortwait, FALSE, mdl_request->key.get_wait_state_name()); @@ -2425,9 +2451,8 @@ MDL_context::acquire_lock(MDL_request *mdl_request, double lock_wait_timeout) mysql_prlock_wrlock(&lock->m_rwlock); if (lock->needs_notification(ticket)) - lock->notify_conflicting_locks(this); + lock->notify_conflicting_locks(this, abort_blocking); mysql_prlock_unlock(&lock->m_rwlock); - set_timespec(abs_shortwait, 1); I like the new code in that we are not setting abs_shortwait two times anymore. (Using a while loop in the is case was sub optimal. Ok to push after the above change or if you have a good argument why the current code is correct

1 0

Re: [PATCH] MDEV-32014 Rename binlog cache temporary file to binlog file for large transaction
by andrei.elkin＠pp.inet.fi 02 Sep '24

02 Sep '24

(Re-sending to the list) Howdy Kristian. > > The patch calls the trx_group_commit_leader() function. That is ackward, as > commit-by-rotate does not actually participate in group commit. But the > problem is with the existing function trx_group_commit_leader(), which does > too much in a single function. I noted earlier on this part on the PR page and LiBing seems to have practically proven (by the most recent commit that you must've missed) that conducting the rename inside the flush stage of trx_group_commit_leader() is viable: https://github.com/MariaDB/server/pull/3402/commits/287511a703f7dff47939633… By that measure the flushing is expanded onto possibly multiple "implicitly" rotating log files > > So please do a small refactoring of trx_group_commit_leader() and keep > only ... and no need for refactoring really arises. Could you please assess that approach, cheers, Andrei

1 0

Next LTS version in February 2025?
by Otto Kekäläinen 30 Aug '24

30 Aug '24

Hi! Traditionally there has been a new major MariaDB version in February of the year Debian has a new major version. What is the plan for February 2025? I was reading https://mariadb.org/about/#maintenance-policy and old mariadb.orb blog posts but none was explicit about this.

2 2