- developers - lists.mariadb.org

Re: [PATCH] MDEV-31949 parallel slave xa Round-Robin distribution
by Kristian Nielsen 22 Oct '23

22 Oct '23

Kristian Nielsen <knielsen(a)knielsen-hq.org> writes: Hi Andrei, > commit 96bd9e6b780f3738b5008b89aae6b0b086f15943 > Author: Andrei <andrei.elkin(a)mariadb.com> > Date: Sat Aug 19 19:49:25 2023 +0300 > XA-Prepare group of events > > XA START xid > ... > XA END xid > XA PREPARE xid > > and its XA-"complete" terminator > > XA COMMIT or > XA ROLLBACK > > are made distributed Round-Robin across slave parallel workers. > The former hash-based policy was proven to attribute to execution > latency through creating a big - many times larger than the size > of the worker pool - queue of binlog-ordered transactions > to commit. > > Acronyms and notations used below: > > XAP := XA-Prepare event or the whole prepared XA group of events > XAC := XA-"complete", which is a solitary group of events > |W| := the size of the slave worker pool > Subscripts like `_k' denote order in a corresponding sequence > (e.g binlog file). Here is my review of this patch. > The list is arranged as a sliding window with the size of 2*|W| to account > a possibility of XAP_k -> XAP_k+2|W|-1 the largest (in the group-of-events > count sense) dependency. I spent a lot of effort trying to understand why the factor 2 in the size of 2*|W| occurs. Since each transaction must wait_for_prior_commit for the prior transaction, there should never be more than |W| transactions active. In your example with |W|=4, you say that > Worker #4 can take on its T_8 when T_1 is yet at the beginning of its processing But that does not seem possible. T_8 cannot start until T_1 (and 2..4) have done wakeup_subsequent_commits(), which surely shouldn't happen "at the beginning of its processing", but only at the end. But I think I now understand. The problem is that the XA code calls wakeup_subsequent_commits _before_ updating the XID hash (in XA PREPARE) / deleting from the XID hash (in XA COMMIT/ROLLBACK). Right? This I think is the fundamental issue to address. The wait_for_prior_commit mechanism is to ensure that the prior transaction has completed its commit, which means the state of the transaction is "committed" in memory. Thus, the XID hash, which records information about the status of the (XA) transactions, must also be updated before wakeup_prior_commit may be done. I can see that the XA PREPARE/COMMIT/ROLLBACK already uses the trx_group_commit_leader() code. So what you should do is to arrange for the update of / delete from the XID hash happens inside there, just before the wakeup_subsequent_commits() gets called: if (current->cache_mngr->using_xa && likely(!current->error) && DBUG_EVALUATE_IF("skip_commit_ordered", 0, 1)) { mysql_mutex_lock(&current->thd->LOCK_thd_data); run_commit_ordered(current->thd, current->all); mysql_mutex_unlock(&current->thd->LOCK_thd_data); } current->thd->wakeup_subsequent_commits(current->error); I checked the code, the XID hash update/delete currently happens shortly after that in the code, so there should be no problems moving it in there. [Incidentally, I also noticed that the XA PREPARE/COMMIT does not use the commit_ordered mechanism. While this seems (I think) unrelated to this patch, I think it is something you need to look into. The commit_ordered mechansim is central to ensuring that InnoDB and binlog commit in the same order, and I don't see how this is guaranteed for the current XA code (and I suspect this might also mean that binlog recovery will be broken in some cases).] Once the XID hash update/delete is moved as described, we have the very nice property that after an XA PREPARE/COMMIT/ROLLBACK T_i has done wakeup_subsequent_commits(), it is safe for a following T_j that refers to the same XID to replicate. This means that you can now use the existing wait_for_prior_commit mechanism to handle the dependencies between XA event groups with the same XID. Thus, there is no need to introduce a separate (and very complicated) wait mechanism of xid_cache_insert_maybe_wait() and xid_cache_search_maybe_wait(), solely for parallel replication of user XA. Thus, in the SQL driver thread, using your sliding window (which need only be of size |W| I believe), you can mark a "T1: XA COMMIT <xid> " followed by "T2: XA PREPARE <xid>" of a duplicate XID, that T2 must do a wait_for_prior_commit(T1) before running. This is simple, it can use the existing mechanims for that, using entry->last_committed_sub_id. Just like rgi->wait_commit_sub_id and rgi->wait_commit_group_info, we can introduce eg.: rgi->pre_dependency_sub_id rgi->pre_dependency_group_info and then do a wait_for_prior_commit(&rgi->pre_dependency_group_info->commit_orderer) if the rgi->pre_dependency_sub_id > entry->last_committed_sub_id) Similarly, the sliding window can record for "T3: XA PREPARE" and "T4: XA COMMIT", that T4 should do a wait_for_prior_commit(T3) before running. Then T4 will be sure that the XA transaction is ready. This will also be a good preparation for later introducing more of this kind of pre-calculated dependencies to the parallel replication scheduling, along the lines of the "balanced applier" that you have written about previously. These ideas I believe have tremendous potential, and handling this in a generic way is a very big improvement. I'm not suggesting to implement more than what is necessary now for XA, but keep it in mind that this dependency calculation in the SQL driver thread can be used for other than user XA in the future. You can also name the new introduced dependency fields appropriately (ie. with generic names not specific to XA when applicable). This will greatly simplify the patch, I believe; and more importantly it will integrate the XA-required scheduling in a clean and generic way in the parallel replication code. Following is more detailed comments on the patch: > - DBUG_ASSERT( > - !(thd->rgi_slave && > - !thd->rgi_slave->worker_error && > - thd->rgi_slave->did_mark_start_commit) || > - (thd->transaction->xid_state.is_explicit_XA() || > - (thd->rgi_slave->gtid_ev_flags2 & Gtid_log_event::FL_PREPARED_XA))); > - > + DBUG_ASSERT(!(thd->rgi_slave && > + !thd->rgi_slave->worker_error && > + thd->rgi_slave->did_mark_start_commit) || > + (thd->transaction->xid_state.is_explicit_XA() || > + (thd->rgi_slave->gtid_ev_flags2 & > + (Gtid_log_event::FL_PREPARED_XA | > + Gtid_log_event::FL_COMPLETED_XA)))); > if (thd->rgi_slave && > !thd->rgi_slave->worker_error && > thd->rgi_slave->did_mark_start_commit) > thd->rgi_slave->unmark_start_commit(); Why is it allowed to rollback here while did_mark_start_commit is true for XA PREPARE or XA COMMIT? The comment above should be extended to explain this. And why does the following if () statement then still do an emergency unmark_start_commit(), when the condition is not caught by the assertion? It looks like something is wrong in the earlier code, the intention here is that it should never be necessary to unmark_start_commit() here, and if it is, then it is a bug flagged by the assertion. > @@ -1751,7 +1753,8 @@ binlog_flush_cache(THD *thd, binlog_cache_mngr *cache_mngr, > > if ((using_stmt && !cache_mngr->stmt_cache.empty()) || > (using_trx && !cache_mngr->trx_cache.empty()) || > - thd->transaction->xid_state.is_explicit_XA()) > + (thd->transaction->xid_state.is_explicit_XA() || > + (thd->rgi_slave && thd->rgi_slave->is_async_xac))) Why is this extra condition thd->rgi_slave->is_async_xac necessary? There are many of these conditions spread around the code in the patch. I think perhaps it is because the transaction is somehow not "connected" to the THD? Because it is in the XID cache? But the situation must be similar on the master, if the XA COMMIT happens in a different transaction from XA PREPARE. So this should be done the same way on the slave, so that the XA transaction gets connected to the THD of the worker thread processing it, and so that these extra rgi_slave->is_async_xac conditions are not needed. It is very fragile to have such conditions spread around the code, it will be impossible to avoid bugs due to forgetting such extra condition on one place or another. > + /* > + While xid_state.get_xid() is a robust method to access `xid` > + it can't be used on slave by the asynchronously running XA-"complete". > + In the latter case thd->lex->xid is safely accessible. > + */ > + buflen= serialize_with_xid(is_async_xac? thd->lex->xid : > + thd->transaction->xid_state.get_xid(), Same here, it should be possible to use the same xid_state in the slave thread as on the master, so we avoid special cases all over the code for the slave threads. > +static bool acquire_xid(THD *thd) > +{ > + bool rc= false; > + > + if (thd->rgi_slave && thd->rgi_slave->is_async_xac && > + thd->rgi_slave->gtid_ev_flags2 & Gtid_log_event::FL_COMPLETED_XA) > + { > + XID_STATE &xid_state= thd->transaction->xid_state; This function actually seems to do part of "XA transaction gets connected to the THD". But it's called from binlog_commit_by_xid() and binlog_rollback_by_xid(), which means it has to have yet another of these special-condition checks for thd->rgi_slave->is_async_xac etc. Instead, call this from the code that applies the "XA COMMIT" event (eg. in log_event_server.cc), and make sure it fully connects the XA transaction to the the THD. Then you get rid of the checks for is_async_xac in a lot of places, and also avoid polluting with replication details the code than runs the original transaction on the master. > + rc= binlog_commit(thd, TRUE, FALSE); > + thd->ha_data[binlog_hton->slot].ha_info[1].reset(); > + } > + if (!rc) > + { > + rc= acquire_xid(thd); > + } > + if (thd->is_current_stmt_binlog_disabled()) > + { > + thd->wakeup_subsequent_commits(rc); > + } So IIUC, here we first binlog the XA COMMIT and then wakeup_subsequent_commits(). But the engine commit only happens afterwards, in ha_commit_or_rollback_by_xid. Why wakeup_subsequent_commits() here? It seems too early, the transaction is not yet committed in the engine, how do you avoid that the commits in the engine will happen in the wrong order? And then a mariabackup might take a backup with T2 committed but T1 not and not have a valid replication position to provision a slave. And also, is this skipping the binlog transaction coordinator and two-phase commit with the engines? And in particular, commit_ordered()? Then again, it seems we have the problem that the engine can commit in the opposite order from the binlog. I wonder if this will also affect crash recovery if we crash with different commit order in binlog and engine? So I think wakeup_subsequent_commits() here is wrong, should be removed. > @@ -2185,7 +2293,9 @@ int binlog_commit(THD *thd, bool all, bool ro_1pc) > } > > if (cache_mngr->trx_cache.empty() && > - (thd->transaction->xid_state.get_state_code() != XA_PREPARED || > + ((thd->transaction->xid_state.get_state_code() != XA_PREPARED && > + !(thd->rgi_slave && thd->rgi_slave->is_parallel_exec && > + thd->lex->sql_command == SQLCOM_XA_COMMIT)) || As explained above, this condition (and the similar in binlog_rollback() should be avoided. > @@ -10510,13 +10624,20 @@ int TC_LOG_BINLOG::unlog_xa_prepare(THD *thd, bool all) > > binlog_cache_mngr *cache_mngr= thd->binlog_setup_trx_data(); > int cookie= 0; > + int rc= 0; > + > + if (thd->rgi_slave && thd->is_current_stmt_binlog_disabled()) > + { > + rc= thd->wait_for_prior_commit(); > + if (rc == 0) > + thd->wakeup_subsequent_commits(rc); > + return rc; > + } Why is this necessary? If it is really necessary, then put it in the code that applies the XA PREPARE event, not as a random special case in generic code. > - bool rc= false; > - > > rc= write_empty_xa_prepare(thd, cache_mngr); // normally gains need_unlog > > static bool write_empty_xa_prepare(THD *thd, binlog_cache_mngr *cache_mngr) > { > return binlog_commit_flush_xa_prepare(thd, true, cache_mngr); > } Smaller point: since you're fixing the type of `rc` to be int (which is correct), also fix the return type of write_empty_xa_prepare() to be int and not bool - as binlog_commit_flush_xa_prepare() as well as unlog_xa_prepare() return int, not bool. > @@ -3314,16 +3314,22 @@ Gtid_log_event::Gtid_log_event(THD *thd_arg, uint64 seq_no_arg, > XID_STATE &xid_state= thd->transaction->xid_state; > if (is_transactional) > { > - if (xid_state.is_explicit_XA() && > - (thd->lex->sql_command == SQLCOM_XA_PREPARE || > - xid_state.get_state_code() == XA_PREPARED)) > + bool is_async_xac= false; > + if ((xid_state.is_explicit_XA() && > + (thd->lex->sql_command == SQLCOM_XA_PREPARE || > + xid_state.get_state_code() == XA_PREPARED)) || > + (is_async_xac= (thd->rgi_slave && thd->rgi_slave->is_async_xac))) Again, this is the generic GTID event contructor, it shouldn't need this kind of logic. It's very strange that a simple class constructor returns something different depending on which thread it runs in! Hopefully this is no longer necessary after XA transaction gets properly connected to the THD. But else, it should be handled by passing in suitable flag from the caller, or by the caller setting the required modifications after construction. > +using std::max; Please don't. Using std::max() explicit is not long and makes it explicit what `max` implementation is used. > @@ -760,7 +762,8 @@ convert_kill_to_deadlock_error(rpl_group_info *rgi) > return; > err_code= thd->get_stmt_da()->sql_errno(); > if ((rgi->speculation == rpl_group_info::SPECULATE_OPTIMISTIC && > - err_code != ER_PRIOR_COMMIT_FAILED) || > + (err_code != ER_PRIOR_COMMIT_FAILED && > + err_code != ER_XAER_NOTA)) || Why? What is special about the ER_XAER_NOTA error? Is it really safe to speculatively run the XA COMMIT in parallel, and then retry it for any other error than ER_XAER_NOTA ? > @@ -2467,6 +2463,7 @@ free_rpl_parallel_entry(void *element) > } > mysql_cond_destroy(&e->COND_parallel_entry); > mysql_mutex_destroy(&e->LOCK_parallel_entry); > + e->concurrent_xaps_window.~Dynamic_array(); > my_free(e); No, let's not do this, calling a destructor explicitly in a POD managed by my_malloc() / my_free(). Why not just make concurrent_xaps_window a pointer to the Dynamic_array and manage with new / delete? Then you can also allocate it lazily, so it will not need to be allocated at all for the 99.9xx% of users who are not replicating user XA transactions. The size of the sliding window is fixed anyway (until slave_parallel_threads is changed), so why use Dynamic_array at all, why not just allocate a plain array of the right size? > +template <> > +struct std::hash<XID> > +{ > + std::size_t operator()(const XID& xid) const > + { > + return my_hash_sort(&my_charset_bin, xid.key(), xid.key_length()); I don't understand, what is the purpose of introducing this function object? Why not just call my_hash_sort() directly (seems it's only used in one place) and avoid this code-obfuscation? > + Dynamic_array<std::pair<std::size_t, uint32>> concurrent_xaps_window; This is dangerous as Dynamic_array has a destructor but rpl_parallel_entry is a POD where we don't call any constructor/destructor. Should instead use a pointer to an object constructed with new (or not use Dynamic_array at all as suggested above). > + /* > + When true indicates that the user xa transaction is going to > + complete (with COMMIT or ROLLBACK) by the worker thread, > + *while* another worker is still preparing it. Once the latter is done > + the xid will be acquired and the flag gets reset. > + */ > + bool is_async_xac; > + I don't understand this comment. Surely it is not possible to *complete*, with XA COMMIT, a transaction before the corresponding XA PREPARE has completed (possibly in another worker thread)? Do you mean that it is possible for a worker thread W2 to *start* applying the XA COMMIT speculatively, before another worker W1 has completed the XA PREPARE? But I think this is not necessary, see below the discussion that the XA COMMIT worker can just do a wait_for_prior_commit() on the event group of its XA PREPARE. Then this should not be necessary. > @@ -137,7 +137,7 @@ template <class Elem> class Dynamic_array > */ > Elem& at(size_t idx) > { > - DBUG_ASSERT(idx < array.elements); > + DBUG_ASSERT(idx < max_size()); > return *(((Elem*)array.buffer) + idx); No, this cannot possibly be right. This is a patch about XA replication, why would it suddenly allow accessing deleted elements of any Dynamic_array used in the server?!? Surely you can allocate the elements you need for your sliding window. In fact, since the window is fixed size anyway, why use Dynamic_array at all? @@ -2367,6 +2367,15 @@ struct wait_for_commit event group is fully done. */ bool wakeup_blocked; + /* + The condition variable servers as a part of facilities to handle various + commit time additional dependency between groups of replication events, e.g + XA-Prepare -> XA-Commit, or XA-Prepare -> XA-Prepare all with the same xid. + */ + mysql_cond_t COND_wait_commit_dep; No, this doesn't belong in struct wait_for_commit. wait_for_commit is a low-level mechanism for ordering commits, it should not need any knowledge even of replication, certainly not of user-XA replication. The COND_wait_commit_dep isn't even used in any function related to wait_for_commit or even sql_class.cc, it's only used in sql/xa.cc. [But as discussed at the start, we can use the existing wait_for_prior_commit instead of introducing this new mechanism just for replicating user XA.] > -static XID_cache_element *xid_cache_search(THD *thd, XID *xid) > +XID_cache_element *xid_cache_search(THD *thd, XID *xid) Why remove the `static`? The function is not used outside of sql/xa.cc > @@ -254,16 +259,221 @@ static XID_cache_element *xid_cache_search(THD *thd, XID *xid) > + if (thd->rgi_slave && thd->rgi_slave->is_parallel_exec) > + { > + DBUG_ASSERT(thd->lex->sql_command == SQLCOM_XA_COMMIT || > + thd->lex->sql_command == SQLCOM_XA_ROLLBACK); > + thd->rgi_slave->is_async_xac= true; Here again we have some logic that relates to replication being inserted somewhat randomly in low-level code, requiring this thd->rgi_slave->is_parallel_exec condition. Instead of doing this, the caller higher up should handle this depending on the return of xid_cache_search(). This way, code paths that are not related to replication don't get affected by the replication-specific logic. > +bool xid_cache_insert_maybe_wait(THD* thd) > +{ This new wait mechanism just for parallel replication of user XA seems very complex. Spin loop, and extra locking and condition variables and locking on a lock-free hash. As explained at the start, let's instead avoid introducing a new mechanism and use the existing wait_for_prior_commit, which is central to parallel replication and highly optimized for this. IIUC, the real problem here is that after wakeup_subsequent_commits() of XA COMMIT, the XID is still in the xid_hash. This is because of this code in trans_xa_commit(): ha_commit_or_rollback_by_xid(thd->lex->xid, !res); if (!res && thd->is_error()) { // hton completion error retains xs/xid in the cache, // unless there had been already one as reflected by `res`. res= true; goto _end_external_xid; } xid_cache_delete(thd, xs); xid_deleted= true; ha_commit_or_rollback_by_xid() ends up in queue_for_group_commit() which will do the wakeup_subsequent_commits. But the xid_cache_delete() happens just after that. This is too late. The wakeup_subsequent_commits() must not occur until after the transaction is fully committed in memory. So instead, move the xid_cache_delete() call so it happens inside the group commit code. Then when wakeup_subsequent_commits() is done after XA COMMIT, the XID entry will be gone. It would go here in MYSQL_BIN_LOG::trx_group_commit_leader(): if (current->cache_mngr->using_xa && likely(!current->error) && DBUG_EVALUATE_IF("skip_commit_ordered", 0, 1)) { mysql_mutex_lock(&current->thd->LOCK_thd_data); run_commit_ordered(current->thd, current->all); mysql_mutex_unlock(&current->thd->LOCK_thd_data); } current->thd->wakeup_subsequent_commits(current->error); I noticed that commit_ordered is not done for user XA transactions in replication. The direct reason for that seems to be that they do not use log_and_order(), which sets cache_mngr->using_xa. Can you explain why the XA PREPARE and XA COMMIT is not following the commit_ordered protocol, and what mechanism exists instead to ensure that everything still works correctly? For example that commit order in binlog and InnoDB will be the same, and that binlog recovery works? And finally, just for the record, I still do not agree with the way replication of user XA has been changed (aka MDEV-32020), and this patch does not fix the underlying problem. But changing it to use the wait_for_prior_commit() mechanism as explained at the start will improve the integration with the parallel replication code and be a step towards solving also some of the other underlying issues. - Kristian.

1 0

Locking thd->LOCK_thd_data during group commit
by Kristian Nielsen 21 Oct '23

21 Oct '23

Hi Serg, I came upon this commit by you: > commit 6b685ea7b0776430d45b095cb4be3ef0739a3c04 > Author: Sergei Golubchik <serg(a)mariadb.org> > Date: Wed Sep 28 18:55:15 2022 +0200 > > correctness assert > > thd_get_ha_data() can be used without a lock, but only from the > current thd thread, when calling from anoher thread it *must* > be protected by thd->LOCK_thd_data > > * fix group commit code to take thd->LOCK_thd_data > * remove innobase_close_connection() from the innodb background thread, > it's not needed after 87775402cd0c and was failing the assert with > current_thd==0 > @@ -8512,7 +8512,11 @@ MYSQL_BIN_LOG::trx_group_commit_leader(group_commit_entry *leader) > ++num_commits; > if (current->cache_mngr->using_xa && likely(!current->error) && > DBUG_EVALUATE_IF("skip_commit_ordered", 0, 1)) > + { > + mysql_mutex_lock(&current->thd->LOCK_thd_data); > run_commit_ordered(current->thd, current->all); > + mysql_mutex_unlock(&current->thd->LOCK_thd_data); > + } This seems _really_ expensive :-( This code runs for every transaction commit in replication, and it runs under a global LOCK_commit_ordered which needs to be held for as short as possible. The commit message doesn't mention anything about what goes wrong during the group commit code. And the patch doesn't have any test case that shows the problem, it just adds an assertion that will trigger during the group commit. So what is the actual reason for this change? Note that at this point, the group commit leader thread _is_ effectively acting as if it was each of the participating threads, the other threads are blocked from running during commit_ordered. If it is just so that we can have this assertion, then that needs to be rolled back. We _really_ don't want to take/release N mutexes in release builds during LOCK_commit_ordered, just to have an assertion in debug builds. If there is some reason, then the correct fix could be to set current_thd for the duration of run_commit_ordered, which will satisfy the assertion and presumably the actual bug? Well, it depends _what_ the actual reason/bug is. - Kristian.

3 5

Re: Review of MDEV-31273: Eliminate Log_event::checksum_alg
by Kristian Nielsen 17 Oct '23

17 Oct '23

Hi Monty, Lots of good comments, thanks! Replies inline: Michael Widenius <michael.widenius(a)gmail.com> writes: > enum enum_binlog_checksum_alg > > In c++ we can drop the first 'enum' > You can fix that by doing: Ack, will fix. > event.select_checksum_alg() > > This is called a lot. Would it not be easier to store the checksum_alg > in the event when it is created and access the variable directly? > On the other hand, the number of calls to event.select_checksum_alg() may > not change as we will only create one Yes, this is called only once per event, when writing the event into the binlog or the event cache. So it seems unnecessary to use extra memory to store it. And we don't necessarily even have binlog_cache_data when constructing the event to pass to select_checksum_alg(), so I think it is correct to select the checksum algorithm to use only when we write the event. > @@ -1258,11 +1258,10 @@ Log_event* Log_event::read_log_event(const > uchar *buf, uint event_len, > > if (ev) > { > - ev->checksum_alg= alg; > #ifdef MYSQL_CLIENT > if (ev->checksum_alg != BINLOG_CHECKSUM_ALG_OFF && > ev->checksum_alg != BINLOG_CHECKSUM_ALG_UNDEF) > ev->crc= uint4korr(buf + (event_len)); > + ev->read_checksum_alg= alg; > > Why move the setting of read_checksum_alg inside MYQL_CLIENT? It is because with this patch, ev->checksum_alg is no longer needed in the server (the class member is removed). It is needed in the client (mysqlbinlog), so I added a new member read_checksum_alg, which is the algorithm that was used to read the event. This is needed so that mysqlbinlog can output the checksum (if any) in textual form. But it is not needed in the server > To ensure that this is not wrongly used, please add > #ifdef MYSQL_CLIENT > > here: > > class Log_event: > > #ifdef MYSQL_CLIENT > + enum enum_binlog_checksum_alg read_checksum_alg; > #endif Yes, agree, that is the intention - the read_checksum_alg is only present in the class in the client, not in the server (and we want to avoid using memory for it in the server). This code is already in the #else branch of #ifdef MYSQL_SERVER: #ifdef MYSQL_SERVER ... #else /* The checksum algorithm used (if any) when the event was read. */ enum enum_binlog_checksum_alg read_checksum_alg; I thought this would have the same effect as your suggestion with #ifdef MYSQL_CLIENT. It probably does, but it seems somewhat complex when MYSQL_CLIENT and MYSQL_SERVER are defined. Maybe they can even both be defined, though probably not where log_event.h is included? It's a bit confusing that we have both MYSQL_CLIENT and MYSQL_SERVER used for conditional compilation in log_event.h. But probably not something that should be cleaned up in this patch. Do you think I should still add the #ifdef MYSQL_CLIENT as you suggested (even though it should be redundant in the #else branch of #ifdef MYSQL_SERVER)? It can't hurt after all, even if we always have MYSQL_CLIENT == !MYSQL_SERVER. > in Format_description_log_event(const uchar > > We have: > > else > { > checksum_alg= BINLOG_CHECKSUM_ALG_UNDEF; > used_checksum_alg= BINLOG_CHECKSUM_ALG_OFF; > } > > The used_checksum_alg is not needed as you set it at function start. Sorry, I don't understand? At the start we set it to "undef": + used_checksum_alg= BINLOG_CHECKSUM_ALG_UNDEF; This value will remain if there's an error return; else the else branch will change it to "off". Did you miss that it's set differently at the start of the function and in this else { } branch? Or did I miss something? > sql/log_event.h > > enum enum_binlog_checksum_alg set_checksum_alg(enum > enum_binlog_checksum_alg alg) > As we are just copying and restoring checksum_length, why not just do > > Format_description_log_event::write(Log_event_writer *writer) > > org_checksum_length= checksum_length... > .... > checksum_length= org_checksum_length; > > instead of calling set_checksum_alg() again ? > (The last set we can even do undonditionally which saves us an if) Agree, that code looks silly, I don't know why I did it that way. I'll fix it, remove the function set_checksum_alg() and just do: org_checksum_length= writer->checksum_length; writer->checksum_length= BINLOG_CHECKSUM_LEN; ... writer->checksum_length= org_checksum_length; > log_event_server.cc > > enum enum_binlog_checksum_alg Log_event::select_checksum_alg() > > Much simpler, but we do not check if cache_type is correct > > DBUG_ASSERT(((get_type_code() != ROTATE_EVENT && > get_type_code() != STOP_EVENT) || > get_type_code() != FORMAT_DESCRIPTION_EVENT) || > cache_type == Log_event::EVENT_NO_CACHE); > > Do you think this is not needed? > (This could be added just before: > > return BINLOG_CHECKSUM_ALG_OFF; Agree, I was happy to remove a lot of complexity and the need for a number of asserts, but this assertion could be kept. I'll add it back. > Anyway, please remove the else after: > > if (cache_type == Log_event::EVENT_NO_CACHE) > return (enum_binlog_checksum_alg)binlog_checksum_options; Ack will do. > Format_description_log_event::write(Log_event_writer *writer) > > > uint8 checksum_byte= (uint8) (used_checksum_alg != BINLOG_CHECKSUM_ALG_UNDEF ? > used_checksum_alg : BINLOG_CHECKSUM_ALG_OFF); > > Is it possible the alg can be UNDEF? > You have several asserts in the code that this should not happen. Yeah, honestly, I'm not sure. It's one of the annoying things in the code that I did not get to fix, that we have both the UNDEF and the OFF state for the checksum algorithm. Format_description code is not performance critical (only once per binlog file), so I suggest I leave the extra check for UNDEF as defensive code, but add an assertion that it is not UNDEF and see if it passes the testsuite. Does that sound ok? > slave.cc > > @@ -1870,10 +1873,8 @@ static int get_master_version_and_clock(MYSQL* > mysql, Master_info* mi) > until it has received a new FD_m. > */ > mi->rli.relay_log.description_event_for_queue->checksum_alg= > mi->rli.relay_log.relay_log_checksum_alg; > > Not sure why this is removed. > > Are these guaranteed to be the same here? > If yes, why not add an assert for this? This patch removes Log_event::checksum_alg and instead adds a field Format_description_log_event::used_checksum_alg which gets set in the Format_description_log_event constructor. So this assignment - mi->rli.relay_log.description_event_for_queue->checksum_alg= - mi->rli.relay_log.relay_log_checksum_alg; is replaced by this code just above which serves the same purpose: - Format_description_log_event(4, mysql->server_version); + Format_description_log_event(4, mysql->server_version, + mi->rli.relay_log.relay_log_checksum_alg); So I think adding an assert DBUG_ASSERT(mi->rli.relay_log.description_event_for_queue->checksum_alg == mi->rli.relay_log.relay_log_checksum_alg); is somewhat redundant, but let me know if you still think it will be useful and I'll add it. - Kristian.

2 1

Re: [MariaDB commits] [PATCH 1/4] MDEV-31273: Replace Log_event::writer with function parameter
by Kristian Nielsen 16 Oct '23

16 Oct '23

Hi Monty, [Following up on review on IRC] Kristian Nielsen via commits <commits(a)lists.mariadb.org> writes: > Replace Log_event::writer with just passing the writer object as a > function parameter to Log_event::write(). We wondered how the compiled code will look when passing the writer object as a parameter to Log_event::write() vs. temporarily storing it in the Log_event object. Interestingly, the code is actually "better" after my patch despite an extra parameter being passed along. This is because the compiler can cache the function parameter in a register across nested calls. While with a class member, the compiler cannot determine that the member will not be changed by the nested calls, so the value needs to be repeatedly re-loaded from memory. Some more details on this finding below. My reason for the change was probably more general, based on this code: > - ev->writer= this; > - int res= ev->write(); > - IF_DBUG(ev->writer= 0,); // writer must be set before every Log_event::write > + int res= ev->write(this); It's clear that the intention with this code is to pass the Log_event_writer object `this` to the ev->write() method, and it's a strange way to do it to temporarily set it as a member in the object rather than using a function parameter. Needing an IF_DBUG() to enforce/check that this is done correctly is a warning sign, and I was worried that as I did further changes I would introduce subtle bugs this way. It is interesting that this makes the code clearer not only for the developer (me), but also for the compiler in this case. Function parameter passing is quite well optimized on amd64, but it was still good to see this in detail in this case. And as you predicted we learned something from it! As we discussed, I will extend the commit message to explain these reasons better. Thanks for the suggestion; it's always good in the reviews to question the reason for changes like this. To check the compiled code in detail, I did a release build (BUILD/compile-pentium64-max) with and without this patch, and disassembled the User_var_log_event::write() function, which has a number of calls now passing the writer object as a function parameter: > - return write_header(event_length) || > - write_data(buf, sizeof(buf)) || > - write_data(name, name_len) || > - write_data(buf1, buf1_length) || > - write_data(pos, val_len) || > - write_data(&flags, unsigned_len) || > - write_footer(); > + return write_header(writer, event_length) || > + write_data(writer, buf, sizeof(buf)) || > + write_data(writer, name, name_len) || > + write_data(writer, buf1, buf1_length) || > + write_data(writer, pos, val_len) || > + write_data(writer, &flags, unsigned_len) || > + write_footer(writer); It looks like this would need to pass the `writer` argument many times to called functions. However, write_data() and write_footer() are actually inline functions: bool write_data(const uchar *buf, size_t data_length) { return writer->write_data(buf, data_length); } bool write_footer() { return writer->write_footer(); } So it's actually only write_header() where we now pass an extra parameter. For write_data() and write_footer() it was already being passed as the (implicit) `this` parameter. With the patch, the compiler saves the `writer` parameter in register r12 and uses it for each call: movq %rsi, %r12 ... movq %r12, %rsi # `writer` pointer movq %rbx, %rdi # `this` pointer call _ZN9Log_event12write_headerEP16Log_event_writerm@PLT ... movl $4, %edx # `data_length` parameter movq %r12, %rdi # `writer` pointer call _ZN16Log_event_writer10write_dataEPKhm@PLT ... leaq 137(%rbx), %rsi # `buf` parameter movq %r14, %rdx # `data_length` parameter movq %r12, %rdi # `writer` pointer call _ZN16Log_event_writer10write_dataEPKhm@PLT ... movq %r12, %rdi # `writer` pointer call _ZN16Log_event_writer12write_footerEv@PLT Before the patch, the code for User_var_log_event::write() loads this->writer for each call except write_header(): movq %rbx, %rdi # Here we don't pass `writer`, write_header() loads it from Log_event::writer call _ZN9Log_event12write_headerEm@PLT ... movq 88(%rbx), %rdi # this->writer leaq -126(%rbp), %rsi # `buf` parameter movl $4, %edx # `data_length` parameter call _ZN16Log_event_writer10write_dataEPKhm@PLT ... movq 88(%rbx), %rdi # this->writer leaq 153(%rbx), %rsi # `buf` parameter movq %r13, %rdx # `data_length` parameter call _ZN16Log_event_writer10write_dataEPKhm@PLT ... movq 88(%rbx), %rdi # this->writer call _ZN16Log_event_writer12write_footerEv@PLT I think the reason for the repeated loads of this->writer from 88(%rbx) is because the compiler cannot be sure that this->writer will not be modified by calls to write_data(). So it needs to reload the pointer each time. With the patch the pointer is a function parameter which cannot be modified by nested calls, so it can be saved in a register. Thus, we save some memory loads with my patch for each call of write_data() / write_footer(), which should be a bit faster (though perhaps not noticeably so). In general, passing parameters to functions is cheap (on amd64), as they are passed in registers, and register-to-register moves are very fast on high-performance CPUs, while memory loads have a relative high latency, even when they hit the cache. And like we see here, using local variables helps the compiler do alias analysis and cache values in registers, while this is a lot harder for the compiler to do for class members. - Kristian.

1 0

Re: d4c173ae6df: MDEV-27595 Backport SQL service, introduced by MDEV-19275.
by Sergei Golubchik 16 Oct '23

16 Oct '23

Hi, Alexey, On Oct 16, Alexey Botchkov wrote: > revision-id: d4c173ae6df (mariadb-10.4.30-117-gd4c173ae6df) > parent(s): 837b777bfd7 > author: Alexey Botchkov > committer: Alexey Botchkov > timestamp: 2023-09-05 13:18:12 +0400 > message: > > MDEV-27595 Backport SQL service, introduced by MDEV-19275. > > Fixes to make SQL SERVICE working. > > Conflicts: > storage/spider/spd_table.cc ^^^ better remove these two lines. normally git removes them automatically, and there's no spd_table.cc in this commit anyway. Regards, Sergei Chief Architect, MariaDB Server and security(a)mariadb.org

1 0

Re: 837b777bfd7: MDEV-27595 Backport SQL service, introduced by MDEV-19275.
by Sergei Golubchik 16 Oct '23

16 Oct '23

Hi, Alexey, On Oct 16, Alexey Botchkov wrote: > revision-id: 837b777bfd7 (mariadb-10.4.30-116-g837b777bfd7) > parent(s): a49b9314c16 > author: Alexey Botchkov > committer: Alexey Botchkov > timestamp: 2023-09-05 13:18:12 +0400 > message: > > MDEV-27595 Backport SQL service, introduced by MDEV-19275. > > The SQL SERVICE backported into the 10.4. > > diff --git a/plugin/test_sql_service/CMakeLists.txt b/plugin/test_sql_service/CMakeLists.txt > new file mode 100644 > index 00000000000..615508bdc4e > --- /dev/null > +++ b/plugin/test_sql_service/CMakeLists.txt > @@ -0,0 +1,18 @@ > +# Copyright (C) 2013 Alexey Botchkov and SkySQL Ab > +# > +# This program is free software; you can redistribute it and/or modify > +# it under the terms of the GNU General Public License as published by > +# the Free Software Foundation; version 2 of the License. > +# > +# This program is distributed in the hope that it will be useful, > +# but WITHOUT ANY WARRANTY; without even the implied warranty of > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > +# GNU General Public License for more details. > +# > +# You should have received a copy of the GNU General Public License > +# along with this program; if not, write to the Free Software > +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1335 USA > + > +SET(SOURCES test_sql_service.c) > + > +MYSQL_ADD_PLUGIN(test_sql_service ${SOURCES} MODULE_ONLY) COMPONENT Test > diff --git a/plugin/test_sql_service/COPYING b/plugin/test_sql_service/COPYING > new file mode 100644 > index 00000000000..6e475df5526 > --- /dev/null > +++ b/plugin/test_sql_service/COPYING > @@ -0,0 +1,339 @@ > + GNU GENERAL PUBLIC LICENSE No need to, we already have one in the CMAKE_SOURCE_DIR, why do you want to copy it just for one plugin? Regards, Sergei Chief Architect, MariaDB Server and security(a)mariadb.org

1 0

Re: About Release Model
by Kristian Nielsen 16 Oct '23

16 Oct '23

Hi Sergei, Sergei Golubchik <serg(a)mariadb.org> writes: > You've said that it's unclear how the release model works, when we > release what and where a feature can go. > > I've documented the current release model in the knowledge base, > https://mariadb.com/kb/en/mariadb-release-model/ > would you like to take a look and see if it answers your questions? This is a fine overview page, and if this info was not previously available, then it's surely good to have it documented. However, it doesn't help explain how to meaningfully participate as a core developer in the releases. Where does the discussion happen about the progress and planning of the release, and addressing problems? How to partipate in those discussions? How to push a patch into a preview release, or a release, how to make a release? Etc. My assumption is that this is all restricted to employees? But even that isn't explained on the page. - Kristian.

2 3

Question about lacking git commit message in dc9498b
by Otto Kekäläinen 12 Oct '23

12 Oct '23

Hi Sergei and others! While reviewing changes in MariaDB 10.11.5 I was wondering about this commit: https://github.com/mariadb/server/commit/dc9498beb6f79f08edd02fb7f7153e5150… It has only the default message from issuing 'git revert' and a human written explanation *why* the revert was done is missing. AFAIK the reverted commit https://github.com/MariaDB/server/commit/1d0e3d80d84c2b375a444cdbb41c28afd7… was correct and good - so why was it reverted?

2 1

Re: 6cb526bd363: MDEV-32123: require_secure_transport doesn't allow TCP connections
by Sergei Golubchik 10 Oct '23

10 Oct '23

Hi, Dmitry, Looks pretty good! Basically no comments, some minor nitpicking below, feel free to ignore. On Oct 10, Dmitry Shulga wrote: > revision-id: 6cb526bd363 (mariadb-11.2.1-9-g6cb526bd363) > parent(s): 872ed5342d8 > author: Dmitry Shulga > committer: Dmitry Shulga > timestamp: 2023-10-03 18:07:35 +0700 > message: > > MDEV-32123: require_secure_transport doesn't allow TCP connections > > In case the option require_secure_transport is on the user can't > establish a secure ssl connection over TCP protocol. Inability to set up > a ssl session over TCP was caused by the fact that a type of client's > connection was checked before ssl handshake performed (ssl handshake > happens at the function acl_authenticate()). At that moment vio type has > the value VIO_TYPE_TCPIP for client connection that uses TCP transport. > In result, checking for allowable vio type for fails despite the fact > that SSL session being established. To fix the issue move checking of > vio type for allowable values inside the function > parse_client_handshake_packet() > right after client's capabilities discovered that SSL is not requested > by the client. > > diff --git a/mysql-test/main/require_secure_transport.result b/mysql-test/main/require_secure_transport.result > index 94474d0fa0e..8fa273cbae4 100644 > --- a/mysql-test/main/require_secure_transport.result > +++ b/mysql-test/main/require_secure_transport.result > @@ -1,8 +1,13 @@ > -CREATE TABLE t1 (t int(1)); > SET GLOBAL require_secure_transport=ON; > -ERROR HY000: Connections using insecure transport are prohibited while --require_secure_transport=ON. > +ERROR 08004: Connections using insecure transport are prohibited while --require_secure_transport=ON. I wonder why sqlstate has changed now, as far as I remember it was the previous patch that changed it. Was the test skipped until now? > +SELECT (VARIABLE_VALUE <> '') AS have_ssl FROM INFORMATION_SCHEMA.SESSION_STATUS WHERE VARIABLE_NAME='Ssl_cipher'; > +have_ssl > +1 > +disconnect with_ssl; > connection default; > SET GLOBAL require_secure_transport=OFF; > +SELECT (VARIABLE_VALUE <> '') AS have_ssl FROM INFORMATION_SCHEMA.SESSION_STATUS WHERE VARIABLE_NAME='Ssl_cipher'; > +have_ssl > +0 > disconnect without_ssl; > connection default; > -DROP TABLE t1; > diff --git a/mysql-test/main/require_secure_transport.test b/mysql-test/main/require_secure_transport.test > index e238e732423..f3da73a5167 100644 > --- a/mysql-test/main/require_secure_transport.test > +++ b/mysql-test/main/require_secure_transport.test > @@ -1,15 +1,19 @@ > -- source include/have_ssl_communication.inc > -CREATE TABLE t1 (t int(1)); > SET GLOBAL require_secure_transport=ON; > --disable_query_log > --error ER_SECURE_TRANSPORT_REQUIRED > connect without_ssl,localhost,root,,,,,TCP NOSSL; > + > +connect with_ssl,localhost,root,,,,,TCP SSL; > --enable_query_log I'd suggest to remove disable/enable_query_log and let connect commmand show in the result file. It'll make them easier to read. You'll need replace_result before the failing connect to replace the port away. > +SELECT (VARIABLE_VALUE <> '') AS have_ssl FROM INFORMATION_SCHEMA.SESSION_STATUS WHERE VARIABLE_NAME='Ssl_cipher'; > +disconnect with_ssl; > + > connection default; > SET GLOBAL require_secure_transport=OFF; > --disable_query_log > connect without_ssl,localhost,root,,,,,TCP NOSSL; > --enable_query_log > +SELECT (VARIABLE_VALUE <> '') AS have_ssl FROM INFORMATION_SCHEMA.SESSION_STATUS WHERE VARIABLE_NAME='Ssl_cipher'; > disconnect without_ssl; > connection default; > -DROP TABLE t1; Regards, Sergei Chief Architect, MariaDB Server and security(a)mariadb.org

1 0

Re: 217a7a7ceff: online alter: rework savepoints
by Sergei Golubchik 09 Oct '23

09 Oct '23

Hi, Nikita, On Sep 29, Nikita Malyavin wrote: > revision-id: 217a7a7ceff (mariadb-11.2.1-10-g217a7a7ceff) > parent(s): ad6a305d84d > author: Nikita Malyavin > committer: Nikita Malyavin > timestamp: 2023-09-23 11:46:36 +0400 > message: > > online alter: rework savepoints > > Use standard handlerton functions for savepoint add/rollback. > > To identify the savepoint, the pointer passed is used. > > Every table that has online alter in progress maintains a list of > savepoints independently. > > Also this removes setting a value to a global variable savepoint_alloc_size > without any protection, which was a race condition bug. > > --- a/sql/online_alter.cc > +++ b/sql/online_alter.cc > @@ -6,7 +6,19 @@ > > static handlerton *online_alter_hton; > > -using Online_alter_cache_list= ilist<online_alter_cache_data>; > +using sv_id_t= void *; typedef, please > + > +struct Online_alter_cache_list: ilist<online_alter_cache_data> > +{ > + sv_id_t savepoint_id= 0; > +}; > @@ -281,13 +308,11 @@ static int online_alter_log_init(void *p) > { > online_alter_hton= (handlerton *)p; > online_alter_hton->db_type= DB_TYPE_ONLINE_ALTER; > - online_alter_hton->savepoint_offset= sizeof(my_off_t); > + online_alter_hton->savepoint_offset= 1; you can even use 0, I guess. Otherwise it'll still be 8, as the server will need to align allocation, I suppose. But I suggest to try to swap the implementation. Make online_alter_hton->savepoint_offset=sizeof(void*); and in savepoint_set create a list or an array of all offsets and store it in the savepoint. Something like this: struct { uint savepoints; my_off_t log_prev_pos[1]; } oasv; int online_alter_savepoint_set(handlerton *hton, THD *thd, oasv **sv) { uint count= 0, i=0; for (auto &cache: cache_list) count++; DBUG_ASSERT(count); *sv= malloc(sizeof(oasv) + sizeof(my_off_t)*(count - 1)); (*sv)->savepoints= count; for (auto &cache: cache_list) (*sv)->log_prev_pos[i]= cache.get_byte_position(); } and so on. You just need to make sure you always use push_back and not push_front for adding caches to the cache_list. instead of a structure with a counter you can use an array with MY_OFF_T_UNDEF (not 0!) at the end. > online_alter_hton->close_connection= online_alter_close_connection; > > - online_alter_hton->savepoint_set= // Done by online_alter_savepoint_set > - [](handlerton *, THD *, void *){ return 0; }; > - online_alter_hton->savepoint_rollback= // Done by online_alter_savepoint_rollback > - [](handlerton *, THD *, void *){ return 0; }; > + online_alter_hton->savepoint_set= online_alter_savepoint_set; > + online_alter_hton->savepoint_rollback= online_alter_savepoint_rollback; > online_alter_hton->savepoint_rollback_can_release_mdl= > [](handlerton *hton, THD *thd){ return true; }; Regards, Sergei Chief Architect, MariaDB Server and security(a)mariadb.org

2 2

Re: ad6a305d84d: online alter: use thd->ha_data to store cache_list
by Sergei Golubchik 09 Oct '23

09 Oct '23

Hi, Nikita, On Sep 29, Nikita Malyavin wrote: > revision-id: ad6a305d84d (mariadb-11.2.1-9-gad6a305d84d) > parent(s): 132c56f1325 > author: Nikita Malyavin > committer: Nikita Malyavin > timestamp: 2023-09-23 11:46:36 +0400 > message: > > online alter: use thd->ha_data to store cache_list nice > diff --git a/sql/online_alter.cc b/sql/online_alter.cc > index 3255fc06c3e..5ad2156cd21 100644 > --- a/sql/online_alter.cc > +++ b/sql/online_alter.cc > @@ -6,6 +6,7 @@ > > static handlerton *online_alter_hton; > > +using Online_alter_cache_list= ilist<online_alter_cache_data>; please, use typedef for that > class online_alter_cache_data: public Sql_alloc, public ilist_node<>, > public binlog_cache_data > @@ -246,7 +268,11 @@ int online_alter_savepoint_rollback(THD *thd, LEX_CSTRING name) > > static int online_alter_close_connection(handlerton *hton, THD *thd) > { > - DBUG_ASSERT(thd->online_alter_cache_list.empty()); > + Ha_data &ha= thd->ha_data[online_alter_hton->slot]; > + auto *cache_list= (Online_alter_cache_list*)ha.ha_ptr; why not thd_get_ha_data()? and don't forget thd_set_ha_data(thd, online_alter_hton, 0); THD can be reused, I suppose, so let's play safe. > + > + DBUG_ASSERT(!cache_list || cache_list->empty()); > + delete cache_list; > return 0; > } Regards, Sergei Chief Architect, MariaDB Server and security(a)mariadb.org

2 3

Re: MariaDB 10.11.5 mtr --big results from amd64/arm64/armhf/ppc64el/riscv64/s390x
by Otto Kekäläinen 09 Oct '23

09 Oct '23

Hi! > > I just wanted to share the summary of mariadb-test-run with --big from > > all available architectures on Launchpad > > (https://launchpad.net/~otto/+archive/ubuntu/mariadb/+builds?build_text=&bui…) > > I have tried to fix type_test_double and maria.maria-purge in 10.6. > With 'tried', I mean that I have not been able to repeat the test, but > I have a clue what is going on. > I will push later today. > Any chance you can try to run bb-10.6-monty on launchpad to test my fixes? I cherry-picked these two commits: https://github.com/MariaDB/server/commit/8e093d7597664290b0b5e0eb401b3a180f… https://github.com/MariaDB/server/commit/630cb99823f8e83545ffa16d60c9ac75cd… .. and pushed to https://launchpad.net/~otto/+archive/ubuntu/mariadb/+builds?build_text=&bui… and indeed both tests are now fixed: maria.maria-purge w1 [ pass ] 2134 type_test.type_test_double w1 [ pass ] 59 (from https://launchpadlibrarian.net/691041067/buildlog_ubuntu-mantic-amd64.maria…) Thanks Monty! Last failing that remains after Monty's and Daniel's fixes is: - unit.conc_connection: CONC-640/MDEV-30885

1 0

MariaDB 10.11.5 mtr --big results from amd64/arm64/armhf/ppc64el/riscv64/s390x
by Otto Kekäläinen 07 Oct '23

07 Oct '23

Hi! I just wanted to share the summary of mariadb-test-run with --big from all available architectures on Launchpad (https://launchpad.net/~otto/+archive/ubuntu/mariadb/+builds?build_text=3D&= build_state=3Dall): s390x Completed: Failed 13/5644 tests, 99.77% were successful. Failing test(s): innodb.row_size_error_log_warnings_3 disks.disks_notembedded type_test.type_test_double maria.maria-purge unit.conc_connection main.func_json_notembedded riscv64 Completed: Failed 6/6289 tests, 99.90% were successful. Failing test(s): disks.disks_notembedded type_test.type_test_double unit.conc_connection ppc64el Completed: Failed 9/6291 tests, 99.86% were successful. Failing test(s): disks.disks_notembedded type_test.type_test_double maria.maria-purge unit.conc_connection armhf Too many failed: Failed 19/1767 tests, 98.92% were successful. Failing test(s): federated.federatedx_versioning spider/bg.basic_sql spider/bg.spider_fixes_part spider.basic_sql spider.spider_fixes_part versioning.online versioning.partition period.versioning arm64 completed: Failed 9/6291 tests, 99.86% were successful. Failing test(s): disks.disks_notembedded type_test.type_test_double maria.maria-purge unit.conc_connection amd64 Completed: Failed 9/6291 tests, 99.86% were successful. Failing test(s): disks.disks_notembedded type_test.type_test_double maria.maria-purge unit.conc_connection As you can see, these seem to affect all architectures: - maria.maria-purge: MDEV-31349, easily repeatable failure on all architectures on Debian CI, Salsa CI and Launchpad - type_test.type_test_double: MDEV-22243 wasn't actually fixed yet, and failure visible on all architectures and platforms tested - unit.conc_connection: CONC-640/MDEV-30885 visible on all architectures tested - disk.disk{_notembedded}: MDEV-21587 Of these, Daniel Black is currently fixing the item 4 in the list (disk.disk) at https://github.com/MariaDB/server/pull/2777. For the items 1-3 help would be needed. The armhf situation does not map into a single MDEV. Do we have any Raspberry Pi enthusiasts here who would like to run the mariadb-test-run suite on their Pi and try to fix some of the failures? Overall it is nice to see nearly 6k tests pass on so many architectures! The Debian builds at https://buildd.debian.org/status/package.php?mariadb only run post-build the main suite (~1060 tests), and some architectures don't run any tests (e.g. m68k), just the build, but still nice to see the architecture support situation being very good at the moment for 10.11.5 across official and in-official Debian architectures as well :) Many thanks in particular to Daniel, Marko, Sergei, Tuukka, Faustin and Andrew for helping to hunt for fixes to the MariaDB code and tests to have wide architecture support! - Otto

1 0

Re: 132c56f1325: online alter: extract the source to a separate file
by Sergei Golubchik 29 Sep '23

29 Sep '23

Hi, Nikita, On Sep 29, Nikita Malyavin wrote: > revision-id: 132c56f1325 (mariadb-11.2.1-8-g132c56f1325) > parent(s): a71b13433f9 > author: Nikita Malyavin > committer: Nikita Malyavin > timestamp: 2023-09-23 11:46:36 +0400 > message: > > online alter: extract the source to a separate file > > Move all the functions dedicated to online alter to a newly created > online_alter.cc. Copyright/GPL header in both new files, please. Also one comment below. > With that, make many functions static and simplify the static functions > naming. > > Also, rename binlog_log_row_online_alter -> online_alter_log_row. > > diff --git a/sql/log_event.h b/sql/log_event.h > index 848ad0cead9..35142e034f3 100644 > --- a/sql/log_event.h > +++ b/sql/log_event.h > @@ -729,19 +729,6 @@ enum Log_event_type > }; > > > -/* > - Bit flags for what has been writing to cache. Used to > - discard logs with table map events but not row events and > - nothing else important. This is stored by cache. > -*/ > - > -enum enum_logged_status > -{ > - LOGGED_TABLE_MAP= 1, > - LOGGED_ROW_EVENT= 2, > - LOGGED_NO_DATA= 4, > - LOGGED_CRITICAL= 8 > -}; > > static inline bool LOG_EVENT_IS_QUERY(enum Log_event_type type) > { > @@ -979,6 +966,19 @@ typedef struct st_print_event_info > } PRINT_EVENT_INFO; > #endif > > +/** > + Bit flags for what has been writing to cache. Used to > + discard logs with table map events but not row events and > + nothing else important. This is stored by cache. > +*/ > +enum enum_logged_status > +{ > + LOGGED_TABLE_MAP= 1, > + LOGGED_ROW_EVENT= 2, > + LOGGED_NO_DATA= 4, > + LOGGED_CRITICAL= 8 > +}; > + why did you move it? looks unintentional. Regards, Sergei Chief Architect, MariaDB Server and security(a)mariadb.org

1 0

Re: a71b13433f9: MDEV-32126 Assertion fails upon online ALTER and binary log enabled
by Sergei Golubchik 29 Sep '23

29 Sep '23

Hi, Nikita, On Sep 29, Nikita Malyavin wrote: > revision-id: a71b13433f9 (mariadb-11.2.1-7-ga71b13433f9) > parent(s): 883e7e5f7e7 > author: Nikita Malyavin > committer: Nikita Malyavin > timestamp: 2023-09-22 17:55:14 +0400 > message: > > MDEV-32126 Assertion fails upon online ALTER and binary log enabled > > Assertion `!writer.checksum_len || writer.remains == 0' fails upon > concurrent online ALTER and transactions with failing statements and binary > log enabled. > Also another assertion, `pos != (~(my_off_t) 0)', fails in my_seek, upon > reinit_io_cache, on a simplified test. This means that IO_CACHE wasn't > properly initialized, or had an error before. > > The overall problem is a deep interference with the effect of an installed > binlog_hton. As I wrote in Jira, please, provide a better explanation here. I had to debug the issue myself and as far as I can see, the problem happens because you register binlog_hton for a transaction, but doesn't prepare the cache manager for it. This can be fixed simply with @@ -2279,6 +2279,12 @@ int binlog_log_row_online_alter(TABLE* table, const ucha> trans_register_ha(thd, false, binlog_hton, 0); if (thd->in_multi_stmt_transaction_mode()) trans_register_ha(thd, true, binlog_hton, 0); + if (mysql_bin_log.is_open()) + { + binlog_cache_mngr *cache_mngr= thd->binlog_get_cache_mngr(); + if (cache_mngr && cache_mngr->trx_cache.get_prev_position() == MY_OFF_T_UNDEF) + thd->binlog_set_stmt_begin(); + } } which solves all cases from the bug report. Don't get me wrong, I don't suggest this fix, it's kind of hackish, and I liked how other pieces of the puzzle fell into a place after a separate handlerton, like savepoints and online_alter_cache_list. I'm just saying that "magical interference with the binlog_hton" doesn't explain the problem. At all. > Solution: > Extract online alter operations into a separate handlerton. > > --- a/sql/CMakeLists.txt > +++ b/sql/CMakeLists.txt > @@ -218,6 +218,8 @@ MYSQL_ADD_PLUGIN(partition ha_partition.cc STORAGE_ENGINE DEFAULT STATIC_ONLY > RECOMPILE_FOR_EMBEDDED) > MYSQL_ADD_PLUGIN(sql_sequence ha_sequence.cc STORAGE_ENGINE MANDATORY STATIC_ONLY > RECOMPILE_FOR_EMBEDDED) > +MYSQL_ADD_PLUGIN(online_alter_log log.cc STORAGE_ENGINE MANDATORY STATIC_ONLY > +NOT_EMBEDDED) better add it next to builtin_maria_binlog_plugin, instead of via MYSQL_ADD_PLUGIN. > ADD_LIBRARY(sql STATIC ${SQL_SOURCE}) > MAYBE_DISABLE_IPO(sql) > diff --git a/sql/log.cc b/sql/log.cc > index ee78844464e..132ec254adf 100644 > --- a/sql/log.cc > +++ b/sql/log.cc > @@ -12522,3 +12512,61 @@ void wsrep_register_binlog_handler(THD *thd, bool trx) > } > > #endif /* WITH_WSREP */ > + > +static int online_alter_close_connection(handlerton *hton, THD *thd) > +{ > + DBUG_ASSERT(thd->online_alter_cache_list.empty()); > + return 0; > +} > + > +handlerton *online_alter_hton; > + > +int online_alter_log_init(void *p) > +{ > + online_alter_hton= (handlerton *)p; > + online_alter_hton->db_type= DB_TYPE_ONLINE_ALTER; > + online_alter_hton->savepoint_offset= sizeof(my_off_t); > + online_alter_hton->close_connection= online_alter_close_connection; > + > + online_alter_hton->savepoint_set= // Done by online_alter_savepoint_set > + [](handlerton *, THD *, void *){ return 0; }; > + online_alter_hton->savepoint_rollback= // Done by online_alter_savepoint_rollback > + [](handlerton *, THD *, void *){ return 0; }; > + online_alter_hton->savepoint_rollback_can_release_mdl= > + [](handlerton *hton, THD *thd){ return true; }; > + > + online_alter_hton->commit= [](handlerton *, THD *thd, bool all) > + { return binlog_online_alter_end_trans(thd, all, true); }; > + online_alter_hton->rollback= [](handlerton *, THD *thd, bool all) > + { return binlog_online_alter_end_trans(thd, all, false); }; > + online_alter_hton->commit_by_xid= [](handlerton *hton, XID *xid) > + { return binlog_online_alter_end_trans(current_thd, true, true); }; > + online_alter_hton->rollback_by_xid= [](handlerton *hton, XID *xid) > + { return binlog_online_alter_end_trans(current_thd, true, false); }; should it be so? commit_by_xid/rollback_by_xid take XID as an argument ad you ignore it. If commit_by_xid/rollback_by_xid shouldn't be called for online alter, don't set them at all. Or set to something that always fails (and add an assert too). > + > + online_alter_hton->drop_table= [](handlerton *, const char*) { return -1; }; yeah. Like this. > + online_alter_hton->flags= HTON_NOT_USER_SELECTABLE | HTON_HIDDEN > + | HTON_NO_ROLLBACK; better remove HTON_NO_ROLLBACK. As far as I can see it doesn't do anything here anyway, so the only effect it has - everyone reading the code will stop and thing "why HTON_NO_ROLLBACK is here?" Takes at least 5-10 minutes to look through all places where HTON_NO_ROLLBACK is tested to see that online_alter_hton cannot be there. > + return 0; > +} > + > +struct st_mysql_storage_engine online_alter_storage_engine= > +{ MYSQL_HANDLERTON_INTERFACE_VERSION }; > + > +maria_declare_plugin(online_alter_log) > +{ > + MYSQL_STORAGE_ENGINE_PLUGIN, > + &online_alter_storage_engine, > + "online_alter_log", > + "MariaDB PLC", "plc" lowercase > + "This is a pseudo storage engine to represent the online alter log in a transaction", > + PLUGIN_LICENSE_GPL, > + online_alter_log_init, > + NULL, > + 0x0100, // 1.0 > + NULL, // no status vars > + NULL, // no sysvars > + "1.0", > + MariaDB_PLUGIN_MATURITY_STABLE > +} > +maria_declare_plugin_end; Regards, Sergei Chief Architect, MariaDB Server and security(a)mariadb.org

1 0

Re: f9c1546bd7d: MDEV-32123: require_secure_transport doesn't allow TCP connections
by Sergei Golubchik 28 Sep '23

28 Sep '23

Hi, Dmitry, On Sep 28, Dmitry Shulga wrote: > revision-id: f9c1546bd7d (mariadb-11.2.1-6-gf9c1546bd7d) > parent(s): c4cbafe0c4c > author: Dmitry Shulga > committer: Dmitry Shulga > timestamp: 2023-09-28 16:20:36 +0700 > message: > > MDEV-32123: require_secure_transport doesn't allow TCP connections > > diff --git a/mysql-test/main/require_secure_transport.test b/mysql-test/main/require_secure_transport.test > index e238e732423..4ad18afb96d 100644 > --- a/mysql-test/main/require_secure_transport.test > +++ b/mysql-test/main/require_secure_transport.test > @@ -5,11 +5,16 @@ SET GLOBAL require_secure_transport=ON; > --error ER_SECURE_TRANSPORT_REQUIRED > connect without_ssl,localhost,root,,,,,TCP NOSSL; > --enable_query_log > + > +connect with_ssl,localhost,root,,,,,TCP SSL; where's a check that a connection was established successfully and it indeed uses SSL with require_secure_transport enabled? also, you didn't add any tests for --require-secure-transport=1 > +disconnect with_ssl; > + > connection default; > SET GLOBAL require_secure_transport=OFF; > --disable_query_log > connect without_ssl,localhost,root,,,,,TCP NOSSL; > --enable_query_log > disconnect without_ssl; > + > connection default; > DROP TABLE t1; > diff --git a/sql/sql_acl.cc b/sql/sql_acl.cc > index 1f00a8670a6..3ce1c363c86 100644 > --- a/sql/sql_acl.cc > +++ b/sql/sql_acl.cc > @@ -14519,6 +14546,22 @@ bool acl_authenticate(THD *thd, uint com_change_user_pkt_len) > > if (initialized) // if not --skip-grant-tables > { > + /* > + Check whether the option require_secure_transport is on and in case > + it is true that the secured connection type is used, that is either > + unix socket or named pipe or ssl is in use. > + */ > + if(check_require_secured_transport(thd)) > + { > + Host_errors errors; > + errors.m_ssl= 1; > + inc_host_errors(mpvio.auth_info.thd->security_ctx->ip, &errors); > + status_var_increment(thd->status_var.access_denied_errors); > + my_error(ER_SECURE_TRANSPORT_REQUIRED, MYF(0)); > + > + DBUG_RETURN(1); > + } No, no. This is wrong. The point of "require_secure_transport" is to make sure that *everything* is sent over the secure transport. In particular, passwords must never be sent if the transport is not secure. You cannot check for it after the authentication, you must abort the connection is soon as possible. (technically, the server can do nothing to prevent the client from sending passwords in plain-text over the insecure connection. but practically clients use mysql_native_password by default, so the password won't leak) Regards, Sergei Chief Architect, MariaDB Server and security(a)mariadb.org

1 0

Re: [PATCH] MDEV-31872 Deprecate ENCODE()/DECODE()
by Kristian Nielsen 26 Sep '23

26 Sep '23

Kristian Nielsen <knielsen(a)knielsen-hq.org> writes: > From: Sergei Golubchik <serg(a)mariadb.org> > @@ -2724,6 +2725,10 @@ bool Item_func_encode::seed() > hash_password(rand_nr, key->ptr(), key->length()); > sql_crypt.init(rand_nr); > > + push_warning_printf(thd, Sql_condition::WARN_LEVEL_WARN, ER_WARN_DEPRECATED_SYNTAX, > + ER_THD(thd, ER_WARN_DEPRECATED_SYNTAX_NO_REPLACEMENT), > + func_name_cstring().str); // since 11.3 > + NAK. As I already explained in the MDEV-31872, deprecating these functions is not appropriate and will hurt users, not help them. Don't do this. You need to see this from the user's point of view. She is a DBA, she has an application with columns encoded with ENCODE(). What do you expect her to do, rewrite the application because you or someone does not like these functions? That's very disrespectful. If you want to give a warning for these functions, then you need to create an SQL mode or some other handle for the DBA to disable the warning and keep their application running. And the warning should explain why it's there, and what should be done to avoid it. And you need to carefully justify how burdening the DBA with this task of tracking down the source of the new warning and the way to remove it, is worth the benefit. And deprecation suggests eventual removal, which is even worse, please don't do that. You haven't even updated the documentation https://mariadb.com/kb/en/encode/ to eg. say "encode" or "scramble" instead of encrypt. "base64_encode" also does not do encryption, nor is rot13 cryptographically safe, but that doesn't justify removing them either. Most columns in mariadb databases are not encrypted or encoded at all, how is ENCODE() any less secure that that? - Kristian.

3 10

Re: 025ca90e5df: MDEV-32101 CREATE PACKAGE [BODY] for sql_mode=DEFAULT
by Sergei Golubchik 22 Sep '23

22 Sep '23

Hi, Alexander, On Sep 15, Alexander Barkov wrote: > revision-id: 025ca90e5df (mariadb-11.0.1-231-g025ca90e5df) > parent(s): 8ad1e26b1ba > author: Alexander Barkov > committer: Alexander Barkov > timestamp: 2023-09-05 17:33:27 +0400 > message: > > MDEV-32101 CREATE PACKAGE [BODY] for sql_mode=DEFAULT > > This patch adds PACKAGE support with SQL/PSM dialect for sql_mode=DEFAULT: > diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy > index 7452f6a083a..22c60bd4277 100644 > --- a/sql/sql_yacc.yy > +++ b/sql/sql_yacc.yy > @@ -18454,6 +18518,22 @@ drop_routine: > if (Lex->stmt_drop_procedure($3, $4)) > MYSQL_YYABORT; > } > + | DROP PACKAGE_MARIADB_SYM opt_if_exists sp_name > + { > + LEX *lex= Lex; > + lex->set_command(SQLCOM_DROP_PACKAGE, $3); > + if (unlikely(lex->sphead)) > + my_yyabort_error((ER_SP_NO_DROP_SP, MYF(0), "PACKAGE")); > + lex->spname= $4; > + } > + | DROP PACKAGE_MARIADB_SYM BODY_MARIADB_SYM opt_if_exists sp_name > + { > + LEX *lex= Lex; > + lex->set_command(SQLCOM_DROP_PACKAGE_BODY, $4); > + if (unlikely(lex->sphead)) > + my_yyabort_error((ER_SP_NO_DROP_SP, MYF(0), "PACKAGE BODY")); > + lex->spname= $5; > + } > ; at least here and in SHOW you can use package: PACKAGE_MARIADB_SYM | PACKAGE_ORACLE_SYM ; and then move drop_routine out of ifdef > > > @@ -18498,6 +18578,248 @@ create_routine: > (Item_result) $8, $10)) > MYSQL_YYABORT; > } > + > + | create_or_replace definer_opt PACKAGE_MARIADB_SYM > + opt_if_not_exists sp_name opt_create_package_chistics_init > + remember_name > + { > + sp_package *pkg; > + if (unlikely(!(pkg= Lex-> > + create_package_start(thd, > + SQLCOM_CREATE_PACKAGE, > + &sp_handler_package_spec, > + $5, $1 | $4)))) > + MYSQL_YYABORT; > + pkg->set_c_chistics(Lex->sp_chistics); > + } > + opt_sqlpsm_package_specification_element_list END > + remember_end_opt > + { > + if (unlikely(Lex->create_package_finalize(thd, $5, NULL, $7, $11))) > + MYSQL_YYABORT; > + } > + | create_or_replace definer_opt PACKAGE_MARIADB_SYM BODY_MARIADB_SYM > + opt_if_not_exists sp_name opt_create_package_chistics_init > + remember_name > + { > + sp_package *pkg; > + if (unlikely(!(pkg= Lex-> > + create_package_start(thd, > + SQLCOM_CREATE_PACKAGE_BODY, > + &sp_handler_package_body, > + $6, $1 | $5)))) > + MYSQL_YYABORT; > + pkg->set_c_chistics(Lex->sp_chistics); > + Lex->sp_block_init(thd); > + } > + sqlpsm_package_implementation_declare_section > + { > + if (unlikely(Lex->sp_block_with_exceptions_finalize_declarations(thd))) > + MYSQL_YYABORT; > + } > + sqlpsm_package_implementation_executable_section > + { > + $10.hndlrs+= $12.hndlrs; > + if (unlikely(Lex->sp_block_finalize(thd, $10))) > + MYSQL_YYABORT; > + } > + remember_end_opt > + { > + if (unlikely(Lex->create_package_finalize(thd, $6, NULL, $8, $14))) > + MYSQL_YYABORT; > + } this can also use `package` rule as above and be moved outside of %ifdef. you'll only need %ifdef MARIADB sp_tail_is: _empty ; opt_sp_name: _empty { $$= NULL; } ; %else sp_tail_is: IS | AS ; opt_sp_name: _empty { $$= NULL; } | sp_name { $$= $1; } ; %endif > + ; > + > + > +opt_sqlpsm_package_specification_element_list: > + _empty > + | sqlpsm_package_specification_element_list > + ; and so on, this can be called simply opt_package_specification_element_list if placed under %ifdef MARIADB > + > +sqlpsm_package_specification_element_list: > + sqlpsm_package_specification_element > + | sqlpsm_package_specification_element_list > + sqlpsm_package_specification_element > + ; ... Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 1

Re: 14a12f98eb8: MDEV-15935 Clean up string sys_var types.
by Sergei Golubchik 14 Sep '23

14 Sep '23

Hi, Yuchen, ok to push, after making one change (const) below. On Sep 13, Yuchen Pei wrote: > revision-id: 14a12f98eb8 (mariadb-11.0.1-232-g14a12f98eb8) > parent(s): e987b9350cb > author: Yuchen Pei > committer: Yuchen Pei > timestamp: 2023-09-13 11:50:41 +1000 > message: > > MDEV-15935 Clean up string sys_var types. > > Merge sys_var_charptr with sys_var_charptr_base, as well as merge > Sys_var_session_lexstring into Sys_var_lexstring. Also refactored > update methods of sys_var_charptr accordingly. > > Because the class is more generic, session_update() calls > sys_var_charptr::session_update() which does not assume a buffer field > associated with THD, but instead call strdup/free, we get rid of > THD::default_master_connection_buff accordingly You could've mentioned here "this also makes THD smaller by 192 bytes, and there can be many thousands of concurrent THDs" I mean, it's a good result, one of the benefits of your cleanup. > diff --git a/sql/sys_vars.inl b/sql/sys_vars.inl > index 31ebbae01ca..5bac91a0db3 100644 > --- a/sql/sys_vars.inl > +++ b/sql/sys_vars.inl > @@ -492,14 +492,18 @@ class Sys_var_mybool: public Sys_var_typelib > Backing store: char* > > @note > - This class supports only GLOBAL variables, because THD on destruction > - does not destroy individual members of SV, there's no way to free > - allocated string variables for every thread. > + > + Note that the memory management for SESSION_VAR's is manual, the > + value must be strdup'ed in THD::init() and freed in > + plugin_thdvar_cleanup(). TODO: it should be done automatically when > + we'll have more session string variables to justify it. Maybe some > + kind of a loop over all variables, like sys_var_end() in set_var.cc? > */ > -class Sys_var_charptr_base: public sys_var > +class Sys_var_charptr: public sys_var > { > + size_t max_length= 2000; Should be const, right? No need to store max_length in every Sys_var_charptr instance, if you aren't going to modify it per instance. > public: > - Sys_var_charptr_base(const char *name_arg, > + Sys_var_charptr(const char *name_arg, > const char *comment, int flag_args, ptrdiff_t off, size_t size, > CMD_LINE getopt, > const char *def_val, PolyLock *lock=0, Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 1

Re: 9db04b3dc73: MDEV-15935 Adding global/session system var redirect_url
by Sergei Golubchik 13 Sep '23

13 Sep '23

Hi, Yuchen, Thanks. Ok to push both commits into preview-11.3-preview branch On Sep 13, Yuchen Pei wrote: > revision-id: 9db04b3dc73 (mariadb-11.0.1-233-g9db04b3dc73) > parent(s): 14a12f98eb8 > author: Yuchen Pei > committer: Yuchen Pei > timestamp: 2023-09-13 11:50:42 +1000 > message: > > MDEV-15935 Adding global/session system var redirect_url > > Adding a global/session var `redirect_url' of string type. The initial > value is empty. Can be supplied in mysqld with --redirect-url or set > in --init-connect. A valid redirect_url should be of the format > > {mysql,mariadb}://host[:port] > > where <host> is an arbitrary string not containing colons, and <port> > is a number between 0 and 65535 inclusive. > > The variable will be used by the server to notify clients that they > should connect to another server, specified by the value of the > variable, if not empty. > > The notification is done by the inclusion of the variable in > session_track_system_variable. Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: 8ac17273b14: MDEV-15935 Adding global/session system var redirect_url
by Sergei Golubchik 13 Sep '23

13 Sep '23

Hi, Yuchen, Right. This is pretty much the whole feature. Mostly ok, few minor comments below. On Sep 08, Yuchen Pei wrote: > revision-id: 8ac17273b14 (mariadb-11.0.1-104-g8ac17273b14) > parent(s): 365faf7229f > author: Yuchen Pei > committer: Yuchen Pei > timestamp: 2023-09-08 14:46:50 +1000 > message: > > MDEV-15935 Adding global/session system var redirect_url > > Adding a global/session var `redirect_url' of string type. The initial > value is empty. Can be supplied in mysqld with --redirect-url or set > in --init-connect. A valid redirect_url should be of the format > > {mysql,mariadb}://host[:port] > > where <host> is an arbitrary string not containing colons, and <port> > is a number between 0 and 65535 inclusive. > > The variable will be used by the server to notify clients that they > should connect to another server, specified by the value of the > variable, if not empty. > > The notification is done by the inclusion of the variable in > session_track_system_variable. > > diff --git a/mysql-test/suite/sys_vars/r/sysvars_server_embedded.result b/mysql-test/suite/sys_vars/r/sysvars_server_embedded.result > index fa3bf6b4354..85dc924fe5f 100644 > --- a/mysql-test/suite/sys_vars/r/sysvars_server_embedded.result > +++ b/mysql-test/suite/sys_vars/r/sysvars_server_embedded.result > @@ -3232,6 +3232,16 @@ NUMERIC_BLOCK_SIZE 1 > ENUM_VALUE_LIST NULL > READ_ONLY NO > COMMAND_LINE_ARGUMENT REQUIRED > +VARIABLE_NAME REDIRECT_URL > +VARIABLE_SCOPE SESSION > +VARIABLE_TYPE VARCHAR > +VARIABLE_COMMENT URL to redirect to, if not empty see if you can make it a bit more descriptive. if not, ok as is. hmm, and a second thought. it's in the sysvars_server_embedded test. it doesn't make sense there (embedded doesn't accept connections, and doesn't have session tracking, iirc), so, may be hide it under #ifndef EMBEDDED_LIBRARY ? > +NUMERIC_MIN_VALUE NULL > +NUMERIC_MAX_VALUE NULL > +NUMERIC_BLOCK_SIZE NULL > +ENUM_VALUE_LIST NULL > +READ_ONLY NO > +COMMAND_LINE_ARGUMENT REQUIRED > VARIABLE_NAME REQUIRE_SECURE_TRANSPORT > VARIABLE_SCOPE GLOBAL > VARIABLE_TYPE BOOLEAN > diff --git a/sql/sql_class.h b/sql/sql_class.h > index cf2b5b96a22..c86a76f6a08 100644 > --- a/sql/sql_class.h > +++ b/sql/sql_class.h > @@ -878,6 +878,7 @@ typedef struct system_variables > > Time_zone *time_zone; > char *session_track_system_variables; > + char *redirect_url; > > /* Some wsrep variables */ > ulonglong wsrep_trx_fragment_size; > diff --git a/sql/sql_plugin.cc b/sql/sql_plugin.cc > index 4724e2200e6..70aba9cc4d5 100644 > --- a/sql/sql_plugin.cc > +++ b/sql/sql_plugin.cc > @@ -3307,6 +3309,10 @@ void plugin_thdvar_init(THD *thd) > #ifndef EMBEDDED_LIBRARY > thd->session_tracker.sysvars.init(thd); > #endif > + thd->variables.redirect_url= > + my_strdup(PSI_INSTRUMENT_ME, global_system_variables.redirect_url, and here, key_memory_Sys_var_charptr_value > + MYF(MY_WME | MY_THREAD_SPECIFIC)); > + > DBUG_VOID_RETURN; > } > > @@ -3375,6 +3381,8 @@ void plugin_thdvar_cleanup(THD *thd) > #ifndef EMBEDDED_LIBRARY > thd->session_tracker.sysvars.deinit(thd); > #endif > + my_free(thd->variables.redirect_url); > + thd->variables.redirect_url= 0; also, perhaps in cleanup_variables() ? > my_free((char*) thd->variables.default_master_connection.str); > thd->variables.default_master_connection.str= 0; > thd->variables.default_master_connection.length= 0; > diff --git a/sql/sys_vars.cc b/sql/sys_vars.cc > index 064c60ab2b0..a93e68f40d7 100644 > --- a/sql/sys_vars.cc > +++ b/sql/sys_vars.cc > @@ -1343,6 +1343,61 @@ Sys_init_connect( > DEFAULT(""), &PLock_sys_init_connect, NOT_IN_BINLOG, > ON_CHECK(check_init_string)); > > +/** > + Validate a redirect_url string. > + > + A valid string is either empty, or of the format mysql://host[:port], > + where host is an arbitrary string without any colon ':'. > + > + @param str A string to validate > + @param len Length of the string > + @retval false The string is valid > + @retval true The string is invalid > +*/ > +static bool sysvar_validate_redirect_url(sys_var *, THD *, set_var *var) > +{ > + char *str= var->save_result.string_value.str; > + size_t len= var->save_result.string_value.length; > + /* Empty string is valid */ > + if (len == 0) > + return false; > + const char* end= str + len; > + if (!strncmp(str, "mysql://", strlen("mysql://"))) use !strncmp(str, STRING_WITH_LEN("mysql://")) avoids no strlen and no need to write the string twice. same below. > + str+= strlen("mysql://"); ah. Then, I'd suggest instead LEX_CSTRING mysql_prefix={STRING_WITH_LEN("mysql://")}; if (!strncmp(str, mysql_prefix.str, mysql_prefix.length)) str+= mysql_prefix.length; > + else if (!strncmp(str, "mariadb://", strlen("mariadb://"))) > + str+= strlen("mariadb://"); > + else > + return true; > + /* Host name cannot be empty */ > + if (str == end) > + return true; > + /* Find the colon, if any */ > + while (str < end && *str != ':') > + str++; > + /* Found colon */ > + if (str < end) > + { > + /* Should have at least one number after the colon */ > + if (str + 1 == end) > + return true; > + int p= 0; > + while (str < end && isdigit(*++str)) > + p= p * 10 + (*str - '0'); > + /* Should be all numbers after the colon */ > + if (str < end) > + return true; > + /* The port number should be between 0 and 65535 */ > + if (p > 65535) > + return true; check for overflow too. you can either put the p>65535 check inside the loop or at the end check that, like, (str - colon_ptr) < 7. > + } > + return false; > +} > + > +static Sys_var_charptr Sys_redirect_url( > + "redirect_url", "URL to redirect to, if not empty", > + SESSION_VAR(redirect_url), CMD_LINE(REQUIRED_ARG), DEFAULT(""), > + NO_MUTEX_GUARD, NOT_IN_BINLOG, sysvar_validate_redirect_url, 0, 0); By convention, write ON_CHECK(sysvar_validate_redirect_url). And you don't need 0's at the end, so just NO_MUTEX_GUARD, NOT_IN_BINLOG, ON_CHECK(sysvar_validate_redirect_url)); Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 4

Re: [MariaDB commits] [PATCH] MDEV-31872: Add --old_mode=COMPAT_DISCOURAGED
by Sergei Golubchik 12 Sep '23

12 Sep '23

Hi, Kristian, On Sep 12, Kristian Nielsen wrote: > > diff --git a/sql/sys_vars.cc b/sql/sys_vars.cc > index e25d293bbb7..18d08996aa8 100644 > --- a/sql/sys_vars.cc > +++ b/sql/sys_vars.cc > @@ -3863,12 +3863,13 @@ static const char *old_mode_names[]= > "IGNORE_INDEX_ONLY_FOR_JOIN", // deprecated since 11.3 > "COMPAT_5_1_CHECKSUM", // deprecated since 11.3 > "LOCK_ALTER_TABLE_COPY", // deprecated since 11.3 > + "COMPAT_DISCOURAGED", > 0 > }; > > void old_mode_deprecated_warnings(THD *thd, ulonglong v) > { > - v &= ~OLD_MODE_DEFAULT_VALUE; > + v &= ~(OLD_MODE_DEFAULT_VALUE | OLD_MODE_COMPAT_DISCOURAGED); No, this is wrong. old-mode *by definition* is something that is a temporary workaround for something new, old-mode values are by design deprecated from the day one. So, OLD_MODE_COMPAT_DISCOURAGED shouldn't be on the line above, and thus it'll itself be deprecated and will cause a deprecation warning. Which is rather silly, as I wrote in a previous email, and that's why an old-mode is not a good solution for turning deprecated warnings away. > for (uint i=0; old_mode_names[i]; i++) > if ((1ULL<<i) & v) > { > diff --git a/sql/item_strfunc.cc b/sql/item_strfunc.cc > index 4b96271c331..339430a2794 100644 > --- a/sql/item_strfunc.cc > +++ b/sql/item_strfunc.cc > @@ -2720,7 +2720,16 @@ bool Item_func_encode::seed() > hash_password(rand_nr, key->ptr(), key->length()); > sql_crypt.init(rand_nr); > > - warn_deprecated<1103>(current_thd, func_name_cstring().str); > + if (!(current_thd->variables.old_behavior & OLD_MODE_COMPAT_DISCOURAGED)) > + { > + /* > + This function should not be removed despite the deprecation warning. > + That would cause problems for users who can then no longer access > + columns in their database that might have been inserted as > + ENCODE(str,pass_str). > + */ > + warn_deprecated<1103>(current_thd, func_name_cstring().str); > + } No, this is exactly what I wanted to prevent by adding a warn_deprecated<>() wrapper. If something is giving a warning "... is deprecated and will be removed" - it *has to be removed*. We don't want to lie to users to simply scare them. If we promise to do something, we should do it. If we aren't going to remove ENCODE/DECODE functions, we shouldn't deprecate them at all. Which is the result I'm leaning to right now. > > return FALSE; > } Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: 75ea482c4fc: MDEV-30973 GTID in deadlock output
by Sergei Golubchik 11 Sep '23

11 Sep '23

Hi, Aleksey, On Sep 11, Aleksey Midenkov wrote: > revision-id: 75ea482c4fc (mariadb-11.0.1-230-g75ea482c4fc) > parent(s): fe62f70fb4b > author: Aleksey Midenkov > committer: Aleksey Midenkov > timestamp: 2023-09-01 20:30:24 +0300 > message: > > MDEV-30973 GTID in deadlock output > > Example: > > TRANSACTION 40, ACTIVE 0 sec starting index read > mysql tables in use 1, locked 1 > LOCK WAIT 2 lock struct(s), heap size 1152, 1 row lock(s) > MySQL thread id 23, OS thread handle 140201491547840, query id 310, GTID 3-1-1 Updating > delete from tu where a = 9999 and b = 'xxxx' > 2023-04-12 19:53:58 22 [Note] InnoDB: *** (1) WAITING FOR THIS LOCK TO BE GRANTED: > > Deterministic slave_deadlock.test > > Instead of "MDEV-10962 Deadlock with 3 concurrent DELETEs by unique key" > this one uses "MDEV-22698 Deadlock on concurrent acquisition from multiple indexes" > > --- > mysql-test/suite/rpl/t/slave_deadlock.test | 93 ++++++++++++++++++++++++++++++ > sql/sql_show.cc | 10 ++++ please move slave_deadlock.result from the following commit to this one. otherwise ok Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Implementing Binlog GTID Indexes (MDEV-4991)
by Kristian Nielsen 10 Sep '23

10 Sep '23

I am implementing Binlog GTID Indexes, to fix an old performance regression for GTID when slaves connect to the master. I now have a solid design and a working prototype, so I wanted to describe the work to encourage early comments and suggestions. I'm hoping this will make it to the next release (where is the place to actively follow/participate in the release planning?) Ever since I implemented GTID, connecting slaves needed to sequentially scan the leading portion of one binlog file to locate the GTID position to start at. This can be slow as binlog files default to 1GB in size, even worse so when binlog files can now be encrypted. I files MDEV-4991 for this 10 years ago, and it is embarrassing that this has somehow remained unfixed for so long :-/ But now I'm fixing it. The current implementation is in the work-in-progress branch knielsen_mdev4991: https://github.com/MariaDB/server/tree/knielsen_mdev4991 The code is already functional, with the testsuite passing. Still missing is binlog purge, index crash recovery, async path, and general testing and cleanup. The basic idea is for each binlog file master-bin.000001 to write an index file master-bin.000001.idx. The index contains a B+-Tree in which the keys are pairs of (GTID state, binlog offset). A connecting slave's GTID position can be looked up quickly in the tree to find the corresponding binlog offset (and gtid_binlog_state) to start replicating at. Similarly, a non-GTID connecting slave can quickly look up its starting offset (BINLOG_GTID_POS()) to obtain the GTID position corresponding to its starting offset. The index is written out to disk concurrently with the writing of the binlog file. Since the tree is written in sorted order and append-only, the implementation is significantly simplified over a general B+-Tree, and can also be described as a Log-structed Merge Tree with a Btree search structure on top. All GTID state keys (except the first) in the index are delta-compressed, storing only the GTIDs that changed since the last record (which typically will be only one). Additionally the index is sparse, storing only say every 1-in-10 GTIDs in the index (this will be configurable of course). Using a sparse index, the disk space is reduced at the cost only of scanning a few extra events of the binlog file to find the exact position requested. Actual benchmarking of space usage is TBO, but we can expect a typical binlog file of 1 GB containing say 3 million GTIDs to require something like 10 MB of index disk space (1% increase). The file format is page-based (unlike the binlog file), so that it can be written efficiently and easily read using random-access. The index is written in parallel with the binlog file, but no extra fsync()s are needed (except one at the end of the file when the index is closed and synced to disk). The writing of the index will happen asynchroneously from the binlog background thread; this way it will have minimal impact on the performance and scalability of the binlog and the contested LOCK_log. Connecting slaves can read from the currently-being-written ("hot") GTID index by accessing internal memory buffers of pages not yet written to disk. If the server crashes, the existing binlog recovery and binlog checkpoint mechanism will be used to re-create any incomplete indexes as part of the normal recovery scan of binlog files. If somehow a binlog index should be found corrupt or missing (could be eg. old server upgrade or backup script that omits the new .idx files), the code will gracefully fall back to the old (slower) way of sequentially scanning the binlog file to locate the slave starting position. I think this will be a great (if rather late) improvement to one remaining corner of the GTID implementation that currently has sub-optimal performance, and I'm exited to get this completed and reviewed/tested and added to hopefully the next MariaDB release. - Kristian.

1 0

Re: 365faf7229f: MDEV-15935 Clean up string sys_var types.
by Sergei Golubchik 08 Sep '23

08 Sep '23

Hi, Yuchen, On Sep 08, Yuchen Pei wrote: > revision-id: 365faf7229f (mariadb-11.0.1-103-g365faf7229f) > parent(s): 4f39ae75606 > author: Yuchen Pei > committer: Yuchen Pei > timestamp: 2023-09-08 13:12:22 +1000 > message: > > MDEV-15935 Clean up string sys_var types. > > Merge sys_var_charptr with sys_var_charptr_base, as well as merge > Sys_var_session_lexstring into Sys_var_lexstring. Also refactored > update methods of sys_var_charptr accordingly. > > Because the class is more generic, session_update() calls > sys_var_charptr::session_update() which does not assume a buffer field > associated with THD, but instead call strdup/free, we get rid of > THD::default_master_connection_buff accordingly > diff --git a/sql/sql_class.cc b/sql/sql_class.cc > index 18e796655bd..52fa6c27112 100644 > --- a/sql/sql_class.cc > +++ b/sql/sql_class.cc > @@ -1233,10 +1233,12 @@ void THD::init() > avoid temporary tables replication failure. > */ > variables.pseudo_thread_id= thread_id; > - variables.default_master_connection.str= default_master_connection_buff; > - ::strmake(default_master_connection_buff, > - global_system_variables.default_master_connection.str, > - variables.default_master_connection.length); > + variables.default_master_connection.str= > + my_strndup(PSI_INSTRUMENT_ME, may be key_memory_Sys_var_charptr_value instead of PSI_INSTRUMENT_ME? > + global_system_variables.default_master_connection.str, > + global_system_variables.default_master_connection.length, > + MYF(MY_WME | MY_THREAD_SPECIFIC)); > + > mysql_mutex_unlock(&LOCK_global_system_variables); > > user_time.val= start_time= start_time_sec_part= 0; > diff --git a/sql/sql_plugin.cc b/sql/sql_plugin.cc > index d39589ed1ab..4724e2200e6 100644 > --- a/sql/sql_plugin.cc > +++ b/sql/sql_plugin.cc > @@ -3279,6 +3279,9 @@ void plugin_thdvar_init(THD *thd) > #ifndef EMBEDDED_LIBRARY > thd->session_tracker.sysvars.deinit(thd); > #endif > + my_free((char*) thd->variables.default_master_connection.str); > + thd->variables.default_master_connection.str= 0; > + thd->variables.default_master_connection.length= 0; shouldn't this go inside cleanup_variables() ? > > thd->variables= global_system_variables; > > diff --git a/sql/sys_vars.cc b/sql/sys_vars.cc > index 4843450352e..064c60ab2b0 100644 > --- a/sql/sys_vars.cc > +++ b/sql/sys_vars.cc > @@ -1355,12 +1355,11 @@ static bool check_master_connection(sys_var *self, THD *thd, set_var *var) > return false; > } > > -static Sys_var_session_lexstring Sys_default_master_connection( > +static Sys_var_lexstring Sys_default_master_connection( > "default_master_connection", > "Master connection to use for all slave variables and slave commands", > - SESSION_ONLY(default_master_connection), > - NO_CMD_LINE, > - DEFAULT(""), MAX_CONNECTION_NAME, ON_CHECK(check_master_connection)); > + SESSION_ONLY(default_master_connection), NO_CMD_LINE, DEFAULT(""), > + NO_MUTEX_GUARD, NOT_IN_BINLOG, ON_CHECK(check_master_connection)); some length limit would still be nice, I'd say. Doesn't have to be per variable, may be all Sys_var_lexstring could have some common limit, like, arbitrarily, 1K or 10K. > #endif > > static Sys_var_charptr_fscs Sys_init_file( > diff --git a/sql/sys_vars.inl b/sql/sys_vars.inl > index 31ebbae01ca..0f2a07d92fc 100644 > --- a/sql/sys_vars.inl > +++ b/sql/sys_vars.inl > @@ -576,6 +571,21 @@ class Sys_var_charptr_base: public sys_var > new_val= 0; > return new_val; > } > + char *session_update_prepare(set_var *var) do you need this session_update_prepare method? global_update_prepare() and global_update_finish() are used by Sys_var_sesvartrack. Getting rid of them is a task for another day :) > + { > + return update_prepare(var, MYF(MY_WME | MY_THREAD_SPECIFIC)); > + } > + bool session_update(THD *thd, set_var *var) override > + { > + char *new_val= session_update_prepare(var); > + my_free(session_var(thd, char*)); > + session_var(thd, char*)= new_val; > + return (new_val == 0 && var->save_result.string_value.str != 0); > + } > + char *global_update_prepare(set_var *var) > + { > + return update_prepare(var, MYF(MY_WME)); > + } > void global_update_finish(char *new_val) > { > if (flags & ALLOCATED) > @@ -583,50 +593,28 @@ class Sys_var_charptr_base: public sys_var > flags|= ALLOCATED; > global_var(char*)= new_val; > } > - bool global_update(THD *thd, set_var *var) > + bool global_update(THD *thd, set_var *var) override > { > - char *new_val= global_update_prepare(thd, var); > + char *new_val= global_update_prepare(var); > global_update_finish(new_val); > return (new_val == 0 && var->save_result.string_value.str != 0); > } > - void session_save_default(THD *thd, set_var *var)= 0; > - void global_save_default(THD *thd, set_var *var) > + void save_default(set_var *var) > { > char *ptr= (char*)(intptr)option.def_value; > var->save_result.string_value.str= ptr; > var->save_result.string_value.length= ptr ? strlen(ptr) : 0; > } > -}; > - > -class Sys_var_charptr: public Sys_var_charptr_base > -{ > -public: > - Sys_var_charptr(const char *name_arg, > - const char *comment, int flag_args, ptrdiff_t off, size_t size, > - CMD_LINE getopt, > - const char *def_val, PolyLock *lock=0, > - enum binlog_status_enum binlog_status_arg=VARIABLE_NOT_IN_BINLOG, > - on_check_function on_check_func=0, > - on_update_function on_update_func=0, > - const char *substitute=0) : > - Sys_var_charptr_base(name_arg, comment, flag_args, off, size, getopt, > - def_val, lock, binlog_status_arg, > - on_check_func, on_update_func, substitute) > + void session_save_default(THD *, set_var *var) override > { > - SYSVAR_ASSERT(scope() == GLOBAL); > - SYSVAR_ASSERT(size == sizeof(char *)); > + return save_default(var); no, session's default value is global's value. See any other Sys_var_xxx class, e.g. for Sys_var_mybool: void session_save_default(THD *thd, set_var *var) { var->save_result.ulonglong_value= (ulonglong)*(my_bool *)global_value_ptr(thd, 0); } You cannot have a test for that now, but let's have one in the redirect_url commit. > } > - > - bool session_update(THD *thd, set_var *var) > + void global_save_default(THD *, set_var *var) override > { > - DBUG_ASSERT(FALSE); > - return true; > + return save_default(var); > } > - void session_save_default(THD *thd, set_var *var) > - { DBUG_ASSERT(FALSE); } > }; > > - > class Sys_var_charptr_fscs: public Sys_var_charptr > { > using Sys_var_charptr::Sys_var_charptr; > diff --git a/mysql-test/suite/sys_vars/r/mdev_15935.result b/mysql-test/suite/sys_vars/r/mdev_15935.result > new file mode 100644 > index 00000000000..d20c2ad2f19 > --- /dev/null > +++ b/mysql-test/suite/sys_vars/r/mdev_15935.result > @@ -0,0 +1,12 @@ > +# > +# test that the global/session-only enforcement is still in place despite the merge of string sys_var classes > +# > +set session init_connect="whatever"; > +ERROR HY000: Variable 'init_connect' is a GLOBAL variable and should be set with SET GLOBAL > +set session wsrep_auto_increment_control="whatever"; > +ERROR HY000: Variable 'wsrep_auto_increment_control' is a GLOBAL variable and should be set with SET GLOBAL > +set global default_master_connection="whatever"; > +ERROR HY000: Variable 'default_master_connection' is a SESSION variable and can't be used with SET GLOBAL not sure you need that, it's checked in the very base class, you could've have affected that I'd say it's enough if existing tests for Sys_var_session_lexstring and Sys_var_sesvartrack variables succeed and ASAN/MSAN are happy > +# > +# end of test mdev_15935 > +# Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0