- developers - lists.mariadb.org

Re: fixes for your commits to make buildbot green
by Kristian Nielsen 09 Jan '24

09 Jan '24

Sergei Golubchik <serg(a)mariadb.org> writes: > as you know, I've cherry-picked all your 11.4 features into one branch > to assemble a preview release 11.4.0 Thanks, Serg! > This has required some fixes to make buildbot greener. Thanks. I think buildbot was not active on 11.4 branches when I tried earlier, but now they seem to run. Was something changed in buildbot around 11.4 branches recently? > There's nothing specific you need to do right now. But eventually you'll > rebase on top of the latest 11.4. Somewhere around that time, please > take a look at bb-11.4-release or bb-11.4-serg branch, see commits > related to your MDEV's and consider them. > a40dcb3da6a MDEV-4991 don't use ulong for sysvars > 723ebcb5df7 MDEV-4991 ASAN bug > f3d2d4609c3 MDEV-4991 compilation failure on fulltest2 > bd295d0919b MDEV-4991 update forgotten test results These are now applied (with a few modifications) and rebased on latest 11.4, in bb-11.4-knielsen-mdev4991 > I suggest you squash them with your commits, no need to keep indivial Sure, will do before final merge to 11.4. When is the deadline for pushing into the 11.4 release? - Kristian.

2 1

Re: [PATCH] MDEV-33031 Assertion failure upon reading from performance
by Yuchen Pei 09 Jan '24

09 Jan '24

Hi Sergei, Regarding 3be269542f4d18eaee0ad8fbeffa55708557879f, I have one comment (see below), otherwise ok to push. > [... 62 lines elided] > diff --git a/storage/spider/spd_param.cc b/storage/spider/spd_param.cc > index 2da262cd2bc..308857d153a 100644 > --- a/storage/spider/spd_param.cc > +++ b/storage/spider/spd_param.cc > @@ -116,16 +116,20 @@ extern volatile ulonglong spider_mon_table_cache_version_req; > MYSQL_SYSVAR_NAME(param_name).def_val; \ > } > +extern handlerton *spider_hton_ptr; > static int spider_trx_status_var(THD *thd, SHOW_VAR *var, char *buff, > ulonglong SPIDER_TRX::*counter) > { > - int error_num = 0; > - SPIDER_TRX *trx; > DBUG_ENTER("spider_direct_update"); > var->type = SHOW_LONGLONG; > - if ((trx = spider_get_trx(thd, TRUE, &error_num))) > - var->value = (char *) &(trx->*counter); > - DBUG_RETURN(error_num); > + var->value= buff; > + if (thd != current_thd) > + mysql_mutex_lock(&thd->LOCK_thd_data); > + SPIDER_TRX *trx = (SPIDER_TRX*)thd_get_ha_data(thd, spider_hton_ptr); > + *(ulonglong*)buff= trx ? trx->*counter : 0; > + if (thd != current_thd) > + mysql_mutex_unlock(&thd->LOCK_thd_data); > + DBUG_RETURN(0); If it always returns 0, how about we change the signature of this function to return void? > } Best, Yuchen

2 1

Re: [PATCH] cleanup: spider status variables
by Yuchen Pei 09 Jan '24

09 Jan '24

Hi Sergei, > reduce code duplication > --- > storage/spider/spd_param.cc | 49 ++++++++++++------------------------- > 1 file changed, 16 insertions(+), 33 deletions(-) b7bcbda853947a5be8307eda7026ca1ff567a885 looks ok to push. > [... 93 lines elided] Best, Yuchen

1 0

Re: dc50afee924: MDEV-32501 KEY_PERIOD_USAGE reveals information to unprivileged user
by Sergei Golubchik 08 Jan '24

08 Jan '24

Hi, Nikita, ok to push under condition that you do the change as below, if it's applicable. On Jan 05, Nikita Malyavin wrote: > revision-id: dc50afee924 (mariadb-11.3.1-13-gdc50afee924) > parent(s): 080d8c3bd8f > author: Nikita Malyavin > committer: Nikita Malyavin > timestamp: 2024-01-05 00:07:06 +0100 > message: > > MDEV-32501 KEY_PERIOD_USAGE reveals information to unprivileged user > > Restrict access to KEY_PERIOD_USAGE: show the constraint record iff any > non-select privilege on any table column is granted. > > Also drop the unprivileged user in the end of test and add merge anchor. > > diff --git a/sql/sql_show.cc b/sql/sql_show.cc > index 95a9d3647b0..e5516466e20 100644 > --- a/sql/sql_show.cc > +++ b/sql/sql_show.cc > @@ -7798,6 +7798,12 @@ int get_schema_key_period_usage_record(THD *thd, TABLE_LIST *tables, > if (!period_name) > return 0; > > +#ifndef NO_EMBEDDED_ACCESS_CHECKS > + check_grant(thd, TABLE_ACLS, tables, 1, 1, 1); > + if ((tables->grant.all_privilege() & ~SELECT_ACL & TABLE_ACLS) == NO_ACL) > + return 0; > +#endif Compare with get_schema_key_column_usage_record() above your get_schema_key_period_usage_record(). Perhaps you can also check `thd->col_access` and avoid check_grant()? > bool err= false; > for (uint k= 0; !err && k < keys_total; k++) > { > Regards, Sergei Chief Architect, MariaDB Server and security(a)mariadb.org

2 1

Re: c50eed1e904: MDEV-32645 CAST(AS UNSIGNED) fails with --view-protocol
by Sergei Golubchik 06 Jan '24

06 Jan '24

Hi, Alexander, Just one comment below On Jan 06, Alexander Barkov wrote: > revision-id: c50eed1e904 (mariadb-10.4.30-318-gc50eed1e904) > parent(s): 28cdbab1637 > author: Alexander Barkov > committer: Alexander Barkov > timestamp: 2023-11-13 11:18:16 +0400 > message: > > MDEV-32645 CAST(AS UNSIGNED) fails with --view-protocol > > Item_float::neg() did not preserve the "presentation" from "this". > So > CAST(-1e0 AS UNSIGNED) -- cast from double to unsigned > changes its meaning to: > CAST(-1 AS UNSIGNED) -- cast signed to undigned > > Fixing Item_float::neg() to construct the new value for > Item_float::presentation as minus sign followed by its old value: > '1e0' -> '-1e0' > > diff --git a/sql/item.cc b/sql/item.cc > index 259c4bf8f4d..b88f78fbb59 100644 > --- a/sql/item.cc > +++ b/sql/item.cc > @@ -6896,7 +6896,17 @@ Item *Item_float::neg(THD *thd) > else if (value < 0 && max_length) > max_length--; > value= -value; > - presentation= 0; > + if (presentation) > + { may be, still if (*presentation == '-') presentation++ else > + size_t presentation_length= strlen(presentation); > + if (char *tmp= (char*) thd->alloc(presentation_length + 2)) > + { > + tmp[0]= '-'; > + // Copy with the trailing '\0' > + memcpy(tmp + 1, presentation, presentation_length + 1); > + presentation= tmp; > + } > + } > name= null_clex_str; > return this; > } > Regards, Sergei Chief Architect, MariaDB Server and security(a)mariadb.org

1 0

Re: 9e7c80f8006: MDEV-32803 Assertion `total == 0' failed in Event_log::write_cache_raw
by Sergei Golubchik 05 Jan '24

05 Jan '24

Hi, Nikita, First, I'd suggest this change: --- a/sql/online_alter.cc +++ b/sql/online_alter.cc @@ -257,8 +257,7 @@ int online_alter_end_trans(Online_alter_cache_list { my_error(ER_ERROR_ON_WRITE, MYF(ME_ERROR_LOG), binlog->get_name(), errno); - cleanup_cache_list(cache_list, is_ending_transaction); - DBUG_RETURN(error); + break; } } and also I'd suggest to move if (ending_trans) from cleanup_cache_list() to the caller (there's only one anyway). It'll make the code clearer, in particular your statement below that "the cache ... wasn't destroyed". Better do these changes in a separate commit, they aren't part of the bug fix. The fix itself is ok to push. On Jan 05, Nikita Malyavin wrote: > revision-id: 9e7c80f8006 (mariadb-11.2.1-27-g9e7c80f8006) > parent(s): 0c5176b7744 > author: Nikita Malyavin > committer: Nikita Malyavin > timestamp: 2023-11-17 23:40:10 +0100 > message: > > MDEV-32803 Assertion `total == 0' failed in Event_log::write_cache_raw > > A second DML in a transaction for a table of non-rollbackable engine > leads to a cache corruption, because the cache wasn't reset after a > statement end, but also wasn't destroyed. > > This patch resets the cache for a reuse by subsequent statements in > current transaction. > > diff --git a/sql/online_alter.cc b/sql/online_alter.cc > index e0afe602ca8..5f418d8ec29 100644 > --- a/sql/online_alter.cc > +++ b/sql/online_alter.cc > @@ -152,6 +152,7 @@ int online_alter_log_row(TABLE* table, const uchar *before_record, > if (!table->online_alter_cache) > { > table->online_alter_cache= get_cache_data(thd, table); > + DBUG_ASSERT(table->online_alter_cache->cache_log.type == WRITE_CACHE); > trans_register_ha(thd, false, online_alter_hton, 0); > if (thd->in_multi_stmt_transaction_mode()) > trans_register_ha(thd, true, online_alter_hton, 0); > @@ -237,6 +238,8 @@ int online_alter_end_trans(Online_alter_cache_list &cache_list, THD *thd, > mysql_mutex_lock(binlog->get_log_lock()); > error= binlog->write_cache_raw(thd, &cache.cache_log); > mysql_mutex_unlock(binlog->get_log_lock()); > + if (!is_ending_transaction) > + cache.reset(); > } > } > else if (!commit) // rollback > Regards, Sergei Chief Architect, MariaDB Server and security(a)mariadb.org

1 0

Re: 1c9edc72fc2: MDEV-32830 I. refactor XA binlogging for better integration with BGC/replication/recovery
by Sergei Golubchik 05 Jan '24

05 Jan '24

Hi, Andrei, On Jan 05, Andrei wrote: > commit 1c9edc72fc2 > Author: Andrei <andrei.elkin(a)mariadb.com> > Date: Wed Nov 1 20:32:45 2023 +0200 > > MDEV-32830 I. refactor XA binlogging for better integration with BGC/replication/recovery > > This commit is the part I in the series of three that addresses MDEV-31949. > > This part improves upon MDEV-742 design of the XA binlogging. > When binlog is ON, to execute XA-prepare first in engines to write > the xa-prepare replication event last. > MYSQL_BIN_LOG::run_ordered_commit() is made to execute XA commit and > rollback in engines, as well as > it completes with the XA transaction state, so that its > XID has become available for next use, e.g by another slave parallel > worker, that e.g could be waiting for its order to commit a being > prepared XID transaction. Is it intentional that line length in the paragraph above varies randomly from line to line? And I don't understand why MYSQL_BIN_LOG::run_ordered_commit() has to do the complete XA commit in all engines? it's not its job. And it won't make XID available sooner, it'll only make XID available in a different function. > Notable changes: > > sql/handler.cc > - ha_prepare() now is ensured to execute binlog_hton::prepare() as > the last branch to prepare; recovery concerns are addressed > separately (not by MDEV-21469, but rather along the plan of bug#76233) > - read-only cases are handled more uniformly with extending > has_binlog_hton() that registers the binlog branch at ha_prepare() > when necessary > - ha_rollback_trans() executes binlog_hton::rollback() as first > branch, as a part of the 76233 planned recovery; of <what> planned recovery? > - ditto to the external completion of XA via ha_commit_or_rollback_by_xid(). > sql/log.cc > - binlog_commit,rollback() simplified/clarified on the condition of an empty > statement/trx > - MYSQL_BIN_LOG::run_commit_ordered() is extended to cover > XA-{commit, rollback} respective actions in Engines > - XID unlogging by XA is converted to employ the standard > binlog-checkpoint mechanism (cache_mngr->using_xa= TRUE) > sql/xa.cc > - trans_xa_commit,rollback() are refactored to invoke a common function > for external (connection) completion > - adapting the above two and slave_applier_reset_xa_trans() to > possible (normal to slave) XID release, inside run_ordered_commit(). > > diff --git a/sql/handler.cc b/sql/handler.cc > --- a/sql/handler.cc > +++ b/sql/handler.cc > @@ -1472,6 +1472,48 @@ static int prepare_or_error(handlerton *ht, THD *thd, bool all) > return err; > } > > +/** > + Search in @c ha_info transaction list branches for binlog. > + When the 2nd argument is not NULL and the modified engine number > + is non-zero the binlog branch is activated. > + > + @param ha_info The first node in the transaction descriptor list. > + @param thd The client thread that executes the transaction. > + > + @return true when the binlog branch is registered or set up for transaction, > + false otherwise. > +*/ > +static bool has_binlog_hton(Ha_trx_info *ha_info, THD* thd= NULL) > +{ > + bool rc; > + uint rw_count; > + > + for (rc= false, rw_count= 0; ha_info && !rc; ha_info= ha_info->next()) > + { > + rc= ha_info->ht() == binlog_hton; > + rw_count += (ha_info->is_trx_read_write()); > + } > + > + if (!rc && thd && !thd->is_current_stmt_binlog_disabled() && > + unlikely(rw_count > 0)) > + { > + /* > + In case an engine is modified but binlog is not part of xa-prepared > + transaction (thd is non-NULL) set out binlogging of an empty xa-prepare. > + */ when can we land here? non-trans (or non-XA) table modified? > + DBUG_ASSERT(thd->transaction->xid_state.get_state_code() == XA_IDLE); > + > + THD_TRANS *ptr_trans= &thd->transaction->all; > + thd->ha_data[binlog_hton->slot].ha_info[1].register_ha(ptr_trans, > + binlog_hton); > + thd->ha_data[binlog_hton->slot].ha_info[1].set_trx_read_write(); > + thd->binlog_setup_trx_data(); > + > + rc= true; > + } > + > + return rc; > +} > > /** > @retval > @@ -1488,9 +1530,16 @@ int ha_prepare(THD *thd) > > if (ha_info) > { > + int err; > + bool has_binlog= has_binlog_hton(ha_info, thd); > + > for (; ha_info; ha_info= ha_info->next()) > { > handlerton *ht= ha_info->ht(); > + > + if (ht == binlog_hton) > + continue; Does binlog need a handlerton anymore? It was an easy way to get it registered for participating in 2pc. But if you always filter it out, then there's no need to add it to ha_info list in the first place, is there? > + > if (ht->prepare) > { > if (unlikely(prepare_or_error(ht, thd, all))) > diff --git a/sql/log.cc b/sql/log.cc > --- a/sql/log.cc > +++ b/sql/log.cc > @@ -1895,6 +1896,11 @@ binlog_rollback_flush_trx_cache(THD *thd, bool all, > if (thd->transaction->xid_state.get_state_code() == XA_PREPARED) > buflen= serialize_with_xid(thd->transaction->xid_state.get_xid(), > buf, query, q_len); > + cache_mngr->using_xa= TRUE; > +#ifndef DBUG_OFF > + if (thd->killed != NOT_KILLED) > + thd->lex->sql_command= SQLCOM_XA_ROLLBACK; // for run_commit_ord asserts 1. don't abbrev, commits are for others not for you 2. what other values can sql_command have here? > +#endif > } > Query_log_event end_evt(thd, buf, buflen, TRUE, TRUE, TRUE, 0); > > @@ -2186,16 +2200,13 @@ 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->ha_data[binlog_hton->slot].ha_info[1].is_started() && > - thd->ha_data[binlog_hton->slot].ha_info[1].is_trx_read_write()))) > + (!(is_preparing_xa(thd) || is_prepared_xa(thd)) || > + !is_binlog_read_write(thd))) > { > /* > - This is an empty transaction commit (both the regular and xa), > - or such transaction xa-prepare or > - either one's statement having no effect on the transactional cache > - as any prior to it. > - The empty xa-prepare sinks in only when binlog is read-only. > + This is an empty transaction/statement commit (both the regular and xa), > + or the xa transaction is running xa-prepare or xa-commit while > + its binlog branch is not read-write. weird. how and why do you get into binlog_commit on XA PREPARE? > */ > cache_mngr->reset(false, true); > THD_STAGE_INFO(thd, org_stage); > @@ -7752,7 +7763,7 @@ MYSQL_BIN_LOG::write_transaction_to_binlog(THD *thd, > entry.all= all; > entry.using_stmt_cache= using_stmt_cache; > entry.using_trx_cache= using_trx_cache; > - entry.need_unlog= is_preparing_xa(thd); > + entry.need_unlog= 0; is `need_unlog` always 0 now? > ha_info= all ? thd->transaction->all.ha_list : thd->transaction->stmt.ha_list; > entry.ro_1pc= is_ro_1pc; > entry.end_event= end_ev; > @@ -9365,18 +9381,102 @@ TC_LOG::run_commit_ordered(THD *thd, bool all) > all ? thd->transaction->all.ha_list : thd->transaction->stmt.ha_list; > > mysql_mutex_assert_owner(&LOCK_commit_ordered); > + > + if (!ha_info) > + { > + // slave or master's xa-"complete" external what does it mean? > + DBUG_ASSERT(thd->lex->sql_command == SQLCOM_XA_COMMIT || > + thd->lex->sql_command == SQLCOM_XA_ROLLBACK); > + > + XID *xid= thd->lex->xid; > + bool commit= thd->lex->sql_command == SQLCOM_XA_COMMIT; > + struct xahton_st xaop= { xid, 1 }; > + > + plugin_foreach(NULL, commit ? xacommit_handlerton : xarollback_handlerton, > + MYSQL_STORAGE_ENGINE_PLUGIN, &xaop); is it for commit/rollback by xid? > + xid_cache_delete(thd); > + > + DBUG_ASSERT(!thd->transaction->xid_state.is_explicit_XA()); > + > + return; > + } > + > + // else > + if (is_preparing_xa(thd)) > + { > + for (; ha_info; ha_info= ha_info->next()) > + { > + DBUG_ASSERT(all); > + DBUG_ASSERT(thd->lex->sql_command == SQLCOM_XA_PREPARE); > + DBUG_ASSERT(thd->transaction->xid_state.is_explicit_XA()); > + > + handlerton *ht= ha_info->ht(); > + if (!ht->prepare) > + { > + push_warning_printf(thd, Sql_condition::WARN_LEVEL_WARN, > + ER_GET_ERRNO, ER_THD(thd, ER_GET_ERRNO), > + HA_ERR_WRONG_COMMAND, > + ha_resolve_storage_engine_name(ht)); > + continue; > + } > + else if (ht != binlog_hton) > + { > + /* leader only */ > + //if (thd->binlog_setup_trx_data()->last_commit_pos_offset == 0) > + // ht->prepare(ht, thd, all); wait, what? why do you need a loop over all engines if you don't do anything there? > + } > + } > + if (thd->rgi_slave && thd->rgi_slave->is_parallel_exec) > + { > + thd->transaction->xid_state.set_state_code(XA_PREPARED); > + thd->transaction->xid_state.set_cache_element_to_recovered(); > + thd->transaction->xid_state.xid_cache_element= 0; when are you here? > + } > + } > + else if (thd->transaction->xid_state.is_explicit_XA()) > + { > + for (; ha_info; ha_info= ha_info->next()) > + { > + DBUG_ASSERT(all); > + DBUG_ASSERT(thd->lex->sql_command == SQLCOM_XA_COMMIT || > + thd->lex->sql_command == SQLCOM_XA_ROLLBACK); > + DBUG_ASSERT(is_prepared_xa(thd) || > + ((thd->lex->sql_command == SQLCOM_XA_ROLLBACK && > + thd->transaction->xid_state.get_state_code() == XA_IDLE) || > + thd->lex->xa_opt == XA_ONE_PHASE)); > + DBUG_ASSERT(!thd->transaction->xid_state.is_recovered()); > + > + handlerton *ht= ha_info->ht(); > + if (thd->lex->sql_command == SQLCOM_XA_COMMIT) > + { > + if (ht->commit_ordered) > + ht->commit_ordered(ht, thd, all); > + } > + else if (ht != binlog_hton) why not? > + { > + ht->rollback(ht, thd, all); > + } > + } > + xid_cache_delete(thd); // trx is completed now > + > + DBUG_ASSERT(!thd->transaction->xid_state.is_explicit_XA()); > + } > + else > + { when are you here? > for (; ha_info; ha_info= ha_info->next()) > { > handlerton *ht= ha_info->ht(); > if (!ht->commit_ordered) > continue; > ht->commit_ordered(ht, thd, all); > DBUG_EXECUTE_IF("enable_log_write_upto_crash", > { > DBUG_SET_INITIAL("+d,crash_after_log_write_upto"); > sleep(1000); > }); > DEBUG_SYNC(thd, "commit_after_run_commit_ordered"); > } > + } > } > > diff --git a/sql/xa.cc b/sql/xa.cc > --- a/sql/xa.cc > +++ b/sql/xa.cc > @@ -319,9 +342,16 @@ static void xid_cache_delete(THD *thd, XID_cache_element *&element) > > void xid_cache_delete(THD *thd, XID_STATE *xid_state) > { > - DBUG_ASSERT(xid_state->is_explicit_XA()); > - xid_cache_delete(thd, xid_state->xid_cache_element); > - xid_state->xid_cache_element= 0; > + if (xid_state->is_explicit_XA()) why it should be done here and not in the caller? > + { > + xid_cache_delete(thd, xid_state->xid_cache_element); > + xid_state->xid_cache_element= 0; > + } > +} > + > +void xid_cache_delete(THD *thd) > +{ > + xid_cache_delete(thd, &thd->transaction->xid_state); > } > > > @@ -547,7 +577,14 @@ bool trans_xa_prepare(THD *thd) > } > else > { > - thd->transaction->xid_state.xid_cache_element->xa_state= XA_PREPARED; > + if (thd->transaction->xid_state.xid_cache_element) > + { > + thd->transaction->xid_state.xid_cache_element->xa_state= XA_PREPARED; > + } > + else > + { > + DBUG_ASSERT(thd->rgi_slave->is_parallel_exec); what do you mean by that? That parallel slave doesn't add XA transactions to the xid_cache? > + } > MYSQL_SET_TRANSACTION_XA_STATE(thd->m_transaction_psi, XA_PREPARED); > res= thd->variables.pseudo_slave_mode || thd->slave_thread ? > slave_applier_reset_xa_trans(thd) : 0; Regards, Sergei Chief Architect, MariaDB Server and security(a)mariadb.org

1 0

Re: d2de9a89d80: MDEV-31531 Remove my_casedn_str() and my_caseup_str()
by Sergei Golubchik 19 Dec '23

19 Dec '23

Hi, Alexander, Looks good. A couple of minor comments below On Dec 19, Alexander Barkov wrote: > commit d2de9a89d80 > Author: Alexander Barkov <bar(a)mariadb.com> > Date: Fri Jun 23 13:24:02 2023 +0400 > > diff --git a/include/m_ctype.h b/include/m_ctype.h > index 9d13989d1fe..65ccf869992 100644 > --- a/include/m_ctype.h > +++ b/include/m_ctype.h > @@ -1512,6 +1540,14 @@ size_t my_copy_fix_mb(CHARSET_INFO *cs, > /* Functions for 8bit */ > extern size_t my_caseup_str_8bit(CHARSET_INFO *, char *); > extern size_t my_casedn_str_8bit(CHARSET_INFO *, char *); > +static inline size_t my_caseup_str_latin1(char *str) caseup variant doesn't seem to be used anywhere > +{ > + return my_caseup_str_8bit(&my_charset_latin1, str); > +} > +static inline size_t my_casedn_str_latin1(char *str) > +{ > + return my_casedn_str_8bit(&my_charset_latin1, str); > +} > extern size_t my_caseup_8bit(CHARSET_INFO *, > const char *src, size_t srclen, > char *dst, size_t dstlen); > diff --git a/sql/sql_acl.cc b/sql/sql_acl.cc > index 09af5523c76..3166c9e56cf 100644 > --- a/sql/sql_acl.cc > +++ b/sql/sql_acl.cc > @@ -2543,6 +2541,37 @@ static void push_new_user(const ACL_USER &user) > } > > > +/** > + Make a database name on mem_root from a String, > + apply lower-case conversion if lower_case_table_names says so. > + Perform database name length limit validation. > + > + @param thd - the THD, to get the warning text from > + @param mem_root - allocate the result on this memory root > + @param dbstr - the String, e.g. with Field::val_str() result > + > + @return - {NULL,0} in case of EOM or a bad database name, > + or a good database name otherwise. > +*/ > +static LEX_STRING make_and_check_db_name(MEM_ROOT *mem_root, > + const String &dbstr) > +{ > + LEX_STRING dbls= lower_case_table_names ? > + lex_string_casedn_root(mem_root, files_charset_info, > + dbstr.ptr(), dbstr.length()) : > + lex_string_strmake_root(mem_root, > + dbstr.ptr(), dbstr.length()); > + if (!dbls.str) > + return LEX_STRING{NULL, 0}; // EOM what's the difference between this and return {NULL, 0}; > + if (dbls.length > SAFE_NAME_LEN) > + { > + sql_print_warning(ER_DEFAULT(ER_WRONG_DB_NAME), dbls.str); > + return LEX_STRING{NULL, 0}; // Bad name > + } > + return dbls; // Good name > +} > + > + > /* > Initialize structures responsible for user/db-level privilege checking > and load information about grants from open privilege tables. > diff --git a/sql/sql_lex.cc b/sql/sql_lex.cc > index a84fd29affc..f61e5085e47 100644 > --- a/sql/sql_lex.cc > +++ b/sql/sql_lex.cc > @@ -4186,6 +4187,18 @@ bool LEX::copy_db_to(LEX_CSTRING *to) > return thd->copy_db_to(to); > } > > + > +Lex_ident_db_normalized LEX::copy_db_normalized() > +{ > + if (sphead && sphead->m_name.str) > + { > + DBUG_ASSERT(sphead->m_db.str && sphead->m_db.length); you missed that DBUG_ASSERT(sphead->m_db.str); DBUG_ASSERT(sphead->m_db.length); > + return thd->to_ident_db_normalized_with_error(sphead->m_db); > + } > + return thd->copy_db_normalized(); > +} > + > + > /** > Initialize offset and limit counters. > > diff --git a/storage/perfschema/pfs_program.cc b/storage/perfschema/pfs_program.cc > index de456610519..f3734cb383b 100644 > --- a/storage/perfschema/pfs_program.cc > +++ b/storage/perfschema/pfs_program.cc > @@ -118,31 +118,21 @@ static void set_program_key(PFS_program_key *key, > */ > > char *ptr= &key->m_hash_key[0]; > + const char *end= ptr + sizeof(key->m_hash_key) - 1; > > ptr[0]= object_type; > ptr++; > > if (object_name_length > 0) > - { > - char tmp_object_name[COL_OBJECT_NAME_SIZE + 1]; > - memcpy(tmp_object_name, object_name, object_name_length); > - tmp_object_name[object_name_length]= '\0'; > - my_casedn_str(system_charset_info, tmp_object_name); > - memcpy(ptr, tmp_object_name, object_name_length); > - ptr+= object_name_length; > - } > + ptr+= system_charset_info->casedn(object_name, object_name_length, > + ptr, end - ptr); > ptr[0]= 0; > ptr++; > > if (schema_name_length > 0) > - { > - char tmp_schema_name[COL_OBJECT_SCHEMA_SIZE + 1]; > - memcpy(tmp_schema_name, schema_name, schema_name_length); > - tmp_schema_name[schema_name_length]='\0'; > - my_casedn_str(system_charset_info, tmp_schema_name); > - memcpy(ptr, tmp_schema_name, schema_name_length); > - ptr+= schema_name_length; > - } > + ptr+= system_charset_info->opt_casedn(schema_name, schema_name_length, > + ptr, end - ptr, > + lower_case_table_names); why system_charset_info and not files_charset_info ? > ptr[0]= 0; > ptr++; > Regards, Sergei Chief Architect, MariaDB Server and security(a)mariadb.org

1 0

MTR failures on official Debian buildd after MariaDB 1:10.11.6-1 upload
by Otto Kekäläinen 17 Dec '23

17 Dec '23

Hi! I just wanted to let you know there are a bunch of MTR failures/regressions at https://buildd.debian.org/status/package.php?p=mariadb which I need to fix or add to skip-lists. The previous upload of MariaDB 1:10.11.5-3 had more 'green' than ever before, and now there are regressions on 6 archs. [image: image.png]

3 6

Re: 2821cf816a4: MDEV-31531 Remove my_casedn_str() and my_caseup_str()
by Sergei Golubchik 16 Dec '23

16 Dec '23

Hi, Alexander, > commit 2821cf816a4 > Author: Alexander Barkov <bar(a)mariadb.com> > Date: Fri Jun 23 13:24:02 2023 +0400 > > MDEV-31531 Remove my_casedn_str() and my_caseup_str() please, explain here why > > diff --git a/include/m_ctype.h b/include/m_ctype.h > index 9d13989d1fe..d2629121cfe 100644 > --- a/include/m_ctype.h > +++ b/include/m_ctype.h > @@ -848,6 +844,26 @@ struct charset_info_st > return (cset->casedn)(this, src, srclen, dst, dstlen); > } > > + /* Convert to a lower-cased 0-terminated string */ > + size_t casedn_z(const char *src, size_t srclen, > + char *dst, size_t dstlen) const > + { > + DBUG_ASSERT(dstlen); DBUG_ASSERT(src != dst) ? > + size_t len= casedn(src, srclen, dst, dstlen - 1); > + dst[len]= '\0'; > + return len; > + } > + > + /* Convert to a upper-cased 0-terminated string */ > + size_t caseup_z(const char *src, size_t srclen, > + char *dst, size_t dstlen) const > + { > + DBUG_ASSERT(dstlen); > + size_t len= caseup(src, srclen, dst, dstlen - 1); > + dst[len]= '\0'; > + return len; > + } > + > uint caseup_multiply() const > { > return (cset->caseup_multiply)(this); > @@ -1512,6 +1528,14 @@ size_t my_copy_fix_mb(CHARSET_INFO *cs, > /* Functions for 8bit */ > extern size_t my_caseup_str_8bit(CHARSET_INFO *, char *); > extern size_t my_casedn_str_8bit(CHARSET_INFO *, char *); > +static inline size_t my_caseup_str_latin1(char *str) > +{ > + return my_caseup_str_8bit(&my_charset_latin1, str); > +} > +static inline size_t my_casedn_str_latin1(char *str) > +{ > + return my_casedn_str_8bit(&my_charset_latin1, str); > +} may be my_charset_ascii? with an assert that it's indeed ascii? and a comment that it's used for generated temp table names and such > extern size_t my_caseup_8bit(CHARSET_INFO *, > const char *src, size_t srclen, > char *dst, size_t dstlen); > diff --git a/sql/char_buffer.h b/sql/char_buffer.h > index 7b855f5401c..1091775259d 100644 > --- a/sql/char_buffer.h > +++ b/sql/char_buffer.h > @@ -71,25 +76,92 @@ class CharBuffer > m_buff[m_length]= '\0'; > return *this; > } > + CharBuffer<buff_sz> & copy_caseup(CHARSET_INFO *cs, const LEX_CSTRING &str) > + { > + DBUG_ASSERT(!buffer_overlaps(str)); > + m_length= cs->cset->caseup(cs, str.str, str.length, m_buff, buff_sz); > + DBUG_ASSERT(is_sane()); > + m_buff[m_length]= '\0'; // See comments in copy_casedn() > + return *this; > + } > CharBuffer<buff_sz> & copy_casedn(CHARSET_INFO *cs, const LEX_CSTRING &str, > bool casedn) > { > casedn ? copy_casedn(cs, str) : copy_bin(str); > return *this; > } > + > + // Append one character > + CharBuffer<buff_sz> & append_char(char ch) > + { > + DBUG_ASSERT(is_sane()); > + if (available_size()) > + { > + m_buff[m_length++]= ch; > + m_buff[m_length]= '\0'; > + } > + DBUG_ASSERT(is_sane()); > + return *this; > + } > + > + // Append a string > + CharBuffer<buff_sz> & append_bin(const LEX_CSTRING &str) why _bin ? and why even have a suffix, and not just append() here and above? > + { > + DBUG_ASSERT(is_sane()); > + DBUG_ASSERT(!buffer_overlaps(str)); > + size_t len= MY_MIN(available_size(), str.length); > + memcpy(m_buff + m_length, str.str, len); > + m_length+= len; > + DBUG_ASSERT(is_sane()); > + m_buff[m_length]= '\0'; > + return *this; > + } > + > // Append a string with casedn conversion > CharBuffer<buff_sz> & append_casedn(CHARSET_INFO *cs, const LEX_CSTRING &str) > { > DBUG_ASSERT(is_sane()); > DBUG_ASSERT(!buffer_overlaps(str)); > size_t casedn_length= cs->casedn(str.str, str.length, > - m_buff + m_length, buff_sz - m_length); > + m_buff + m_length, available_size()); > + m_length+= casedn_length; > + DBUG_ASSERT(is_sane()); > + m_buff[m_length]= '\0'; > + return *this; > + } > + > + CharBuffer<buff_sz> & append_opt_casedn(CHARSET_INFO *cs, > + const LEX_CSTRING &str, > + bool casedn) > + { > + return casedn ? append_casedn(cs, str) : append_bin(str); > + } > + > + // Append a string with caseup conversion > + CharBuffer<buff_sz> & append_caseup(CHARSET_INFO *cs, const LEX_CSTRING &str) > + { > + DBUG_ASSERT(is_sane()); > + DBUG_ASSERT(!buffer_overlaps(str)); > + size_t casedn_length= cs->caseup(str.str, str.length, > + m_buff + m_length, available_size()); > m_length+= casedn_length; > DBUG_ASSERT(is_sane()); > m_buff[m_length]= '\0'; > return *this; > } > > + CharBuffer<buff_sz> & truncate(size_t length) > + { > + DBUG_ASSERT(is_sane()); > + if (m_length > length) > + { > + m_length= length; > + m_buff[m_length]= '\0'; > + DBUG_ASSERT(is_sane()); > + } > + return *this; > + } > + > LEX_CSTRING to_lex_cstring() const > { > return LEX_CSTRING{m_buff, m_length}; > diff --git a/sql/item_sum.cc b/sql/item_sum.cc > index bbd09a59267..824ed42ec91 100644 > --- a/sql/item_sum.cc > +++ b/sql/item_sum.cc > @@ -3769,20 +3769,15 @@ int group_concat_key_cmp_with_order_with_nulls(void *arg, const void *key1_arg, > } > > > -static void report_cut_value_error(THD *thd, uint row_count, const char *fname) > +static void report_cut_value_error(THD *thd, uint row_count, > + const LEX_CSTRING &fname) > { > - size_t fn_len= strlen(fname); > - char *fname_upper= (char *) my_alloca(fn_len + 1); > - if (!fname_upper) > - fname_upper= (char*) fname; // Out of memory > - else > - memcpy(fname_upper, fname, fn_len+1); > - my_caseup_str(&my_charset_latin1, fname_upper); > + CharBuffer<NAME_LEN> fname_upper; > + fname_upper.copy_caseup(&my_charset_latin1, fname); why bother? we don't do it in other error cases > push_warning_printf(thd, Sql_condition::WARN_LEVEL_WARN, > ER_CUT_VALUE_GROUP_CONCAT, > ER_THD(thd, ER_CUT_VALUE_GROUP_CONCAT), > - row_count, fname_upper); > + row_count, fname_upper.ptr()); > - my_afree(fname_upper); > } > > > diff --git a/sql/lex_ident.h b/sql/lex_ident.h > index f39273b7da5..db96e40cab8 100644 > --- a/sql/lex_ident.h > +++ b/sql/lex_ident.h > @@ -45,6 +46,10 @@ class Lex_ident_fs: public LEX_CSTRING > bool is_in_lower_case() const; > bool ok_for_lower_case_names() const; > #endif > + bool check_db_name_quick() const please, use a name that makes it clear what the return value is Not if (str.check_db_name_quick()) but if (str.is_invalid_db_name()) > + { > + return !length || length > NAME_LEN || str[length-1] == ' '; > + } > }; > > > @@ -160,4 +171,95 @@ class DBNameBuffer: public CharBuffer<SAFE_NAME_LEN + MY_CS_MBMAXLEN> > }; > > > +class Identifier_chain2 > +{ > + LEX_CSTRING m_name[2]; > +public: > + Identifier_chain2() > + :m_name{Lex_cstring(), Lex_cstring()} > + { } > + Identifier_chain2(const LEX_CSTRING &a, const LEX_CSTRING &b) > + :m_name{a, b} > + { } > + > + const LEX_CSTRING& operator [] (size_t i) const > + { > + return m_name[i]; > + } > + > + static Identifier_chain2 split(const LEX_CSTRING &txt) > + { > + DBUG_ASSERT(txt.str[txt.length] == '\0'); // Expect 0-terminated input > + const char *dot= strchr(txt.str, '.'); > + if (!dot) > + return Identifier_chain2(Lex_cstring(), txt); > + size_t length0= dot - txt.str; > + Lex_cstring name0(txt.str, length0); > + Lex_cstring name1(txt.str + length0 + 1, txt.length - length0 - 1); > + return Identifier_chain2(name0, name1); > + } > + > + // Export as a qualified name string: 'db.name' > + size_t make_qname(char *dst, size_t dstlen) const > + { > + return my_snprintf(dst, dstlen, "%.*s.%.*s", > + (int) m_name[0].length, m_name[0].str, > + (int) m_name[1].length, m_name[1].str); > + } > + > + // Export as a qualified name string, allocate on mem_root. > + bool make_qname(MEM_ROOT *mem_root, LEX_CSTRING *dst) const > + { > + const uint dot= !!m_name[0].length; > + char *tmp; > + /* format: [pkg + dot] + name + '\0' */ > + dst->length= m_name[0].length + dot + m_name[1].length; > + if (unlikely(!(dst->str= tmp= (char*) alloc_root(mem_root, > + dst->length + 1)))) > + return true; > + snprintf(tmp, dst->length + 1, "%.*s%.*s%.*s", > + (int) m_name[0].length, (m_name[0].length ? m_name[0].str : ""), > + dot, ".", > + (int) m_name[1].length, m_name[1].str); > + return false; > + } > + > + /* > + Build a separated two step name, e.g. "ident1/ident2", 0-terminated. > + with an optional lower-case conversion. may be should be inside InnoDB? > + @param [OUT] dst - the destination > + @param dst_size - number of bytes available in dst > + @param sep - the separator character > + @param casedn - whether to convert components to lower case > + @return - number of bytes written to "dst", not counting > + the trailing '\0' byte. > + */ > + size_t make_sep_name_opt_casedn(char *dst, size_t dst_size, > + int sep, bool casedn) const > + { > + /* > + Minimal possible buffer is 4 bytes: 'd/t\0' > + where 'd' is the database name, 't' is the table name. > + Callers should never pass a smaller buffer. > + */ > + DBUG_ASSERT(dst_size > 4); > + DBUG_ASSERT(m_name[0].length + m_name[1].length + 2 < FN_REFLEN - 1); > + if (casedn) > + { > + CHARSET_INFO *cs= &my_charset_utf8mb3_general_ci; > + char *ptr= dst, *end= dst + dst_size; > + ptr+= cs->casedn(m_name[0].str, m_name[0].length, ptr, end - ptr - 2); > + *ptr++= (char) sep; > + ptr+= cs->casedn_z(m_name[1].str, m_name[1].length, ptr, end - ptr); > + return ptr - dst; > + } > + memcpy(dst, m_name[0].str, m_name[0].length); > + dst[m_name[0].length] = (char) sep; > + /* Copy the name and null-byte. */ > + memcpy(dst + m_name[0].length + 1, m_name[1].str, m_name[1].length + 1); > + return m_name[0].length + 1/*slash*/ + m_name[1].length; s/slash/sep/ > + } > +}; > + > + > #endif // LEX_IDENT_INCLUDED > diff --git a/sql/log_event_server.cc b/sql/log_event_server.cc > index 6030202adc7..0fc1d4ce962 100644 > --- a/sql/log_event_server.cc > +++ b/sql/log_event_server.cc > @@ -5777,19 +5778,29 @@ int Table_map_log_event::do_apply_event(rpl_group_info *rgi) > /* Step the query id to mark what columns that are actually used. */ > thd->set_query_id(next_query_id()); > > + db_mem_alloced= tname_mem_alloced= NAME_LEN * > + (lower_case_table_names ? 1 : > + files_charset_info->casedn_multiply()) > + + 1 /* for '\0' */; 1. may be db_mem_length/tname_mem_length instead of NAME_LEN? 2. is the ?: condition right? Looks like it's inverted > if (!(memory= my_multi_malloc(PSI_INSTRUMENT_ME, MYF(MY_WME), > &table_list, (uint) sizeof(RPL_TABLE_LIST), > - &db_mem, (uint) NAME_LEN + 1, > - &tname_mem, (uint) NAME_LEN + 1, > + &db_mem, (uint) db_mem_alloced, > + &tname_mem, (uint) tname_mem_alloced, > NullS))) > DBUG_RETURN(HA_ERR_OUT_OF_MEM); > > - db_mem_length= strmov(db_mem, m_dbnam) - db_mem; > - tname_mem_length= strmov(tname_mem, m_tblnam) - tname_mem; > if (lower_case_table_names) > { > - my_casedn_str(files_charset_info, (char*)tname_mem); > - my_casedn_str(files_charset_info, (char*)db_mem); > + db_mem_length= files_charset_info->casedn_z(m_dbnam, m_dblen, > + db_mem, db_mem_alloced); > + tname_mem_length= files_charset_info->casedn_z(m_tblnam, m_tbllen, > + tname_mem, > + tname_mem_alloced); > + } > + else > + { > + db_mem_length= strmov(db_mem, m_dbnam) - db_mem; > + tname_mem_length= strmov(tname_mem, m_tblnam) - tname_mem; > } > > /* call from mysql_client_binlog_statement() will not set rli->mi */ > diff --git a/sql/sp.cc b/sql/sp.cc > index 0a8aa24789b..537d29fb67e 100644 > --- a/sql/sp.cc > +++ b/sql/sp.cc > @@ -2281,12 +2281,20 @@ Sp_handler::sp_exist_routines(THD *thd, TABLE_LIST *routines) const > for (routine= routines; routine; routine= routine->next_global) > { > sp_name *name; > - LEX_CSTRING lex_db; > + LEX_CSTRING lex_db= thd->make_ident_opt_casedn(routine->db, > + lower_case_table_names); > + if (!lex_db.str) > + DBUG_RETURN(TRUE); // EOM // EOM, error was already sent right? (also, in other places) > LEX_CSTRING lex_name; > - thd->make_lex_string(&lex_db, routine->db.str, routine->db.length); > thd->make_lex_string(&lex_name, routine->table_name.str, > routine->table_name.length); Rather inconsistent :( May be, makes sense to create LEX_CSTRING make_lex_string(const char* str, size_t length) const ? > + /* > + routine->db was earlier tested with check_db_name(). > + Now it's lower-cased according to lower_case_table_names. > + It's safe to make a Lex_ident_db_normalized. > + */ > - name= new sp_name(&lex_db, &lex_name, true); > + name= new (thd->mem_root) sp_name(Lex_ident_db_normalized(lex_db), > + lex_name, true); > sp_object_found= sp_find_routine(thd, name, false) != NULL; > thd->get_stmt_da()->clear_warning_info(thd->query_id); > if (! sp_object_found) > diff --git a/sql/sql_acl.cc b/sql/sql_acl.cc > index 09af5523c76..70dde7dc0ed 100644 > --- a/sql/sql_acl.cc > +++ b/sql/sql_acl.cc > @@ -2543,6 +2543,37 @@ static void push_new_user(const ACL_USER &user) > } > > > +/** > + Make a database name on mem_root from a String, > + apply lower-case conversion if lower_case_table_names says so. > + Perform database name length limit validation. > + > + @param thd - the THD, to get the warning text from > + @param mem_root - allocate the result on this memory root > + @param dbstr - the String, e.g. with Field::val_str() result > + > + @return - {NULL,0} in case of EOM or a bad database name, > + or a good database name otherwise. > +*/ > +static LEX_STRING make_and_check_db_name(THD *thd, MEM_ROOT *mem_root, > + const String &dbstr) > +{ > + LEX_STRING dbls= lower_case_table_names ? > + lex_string_casedn_root(mem_root, files_charset_info, > + dbstr.ptr(), dbstr.length()) : > + lex_string_strmake_root(mem_root, > + dbstr.ptr(), dbstr.length()); > + if (!dbls.str) > + return LEX_STRING{NULL, 0}; // EOM > + if (dbls.length > SAFE_NAME_LEN) > + { > + sql_print_warning(ER_THD(thd, ER_WRONG_DB_NAME), dbls.str); It's an error log, should every message be localized according to connection-specific locale? > + return LEX_STRING{NULL, 0}; // Bad name > + } > + return dbls; // Good name > +} > + > + > /* > Initialize structures responsible for user/db-level privilege checking > and load information about grants from open privilege tables. > @@ -3680,26 +3702,27 @@ privilege_t acl_get(const char *host, const char *ip, > { > privilege_t host_access(ALL_KNOWN_ACL), db_access(NO_ACL); > uint i; > - size_t key_length; > - char key[ACL_KEY_LENGTH],*tmp_db,*end; > + const char *tmp_db; > acl_entry *entry; > DBUG_ENTER("acl_get"); > > - tmp_db= strmov(strmov(key, safe_str(ip)) + 1, user) + 1; > - end= strnmov(tmp_db, db, key + sizeof(key) - tmp_db); > - > - if (end >= key + sizeof(key)) // db name was truncated > + constexpr size_t key_data_size= ACL_KEY_LENGTH + 2; > + CharBuffer<key_data_size + MY_CS_MBMAXLEN> key; why ACL_KEY_LENGTH + 2 + MY_CS_MBMAXLEN ? > + key.append_bin(Lex_cstring_strlen(safe_str(ip))); > + key.append_char('\0'); > + key.append_bin(Lex_cstring_strlen(user)); > + key.append_char('\0'); there was strmov(strmov(...)) pattern here before. as far as I can see, you can chain too: key.append_bin().append_char() .append_bin().append_char(); let's use that > + tmp_db= key.end(); > + key.append_opt_casedn(files_charset_info, Lex_cstring_strlen(db), > + lower_case_table_names); > + db= tmp_db; > + > + if (key.length() >= key_data_size) // db name was truncated > DBUG_RETURN(NO_ACL); // no privileges for an invalid db name > > - if (lower_case_table_names) > - { > - my_casedn_str(files_charset_info, tmp_db); > - db=tmp_db; > - } > - key_length= (size_t) (end-key); > - > mysql_mutex_lock(&acl_cache->lock); > - if (!db_is_pattern && (entry=acl_cache->search((uchar*) key, key_length))) > + if (!db_is_pattern && > + (entry= acl_cache->search((uchar*) key.ptr(), key.length()))) > { > db_access=entry->access; > mysql_mutex_unlock(&acl_cache->lock); > @@ -5630,28 +5662,26 @@ static GRANT_NAME *name_hash_search(HASH *name_hash, > const char *user, const char *tname, > bool exact, bool name_tolower) > { > - char helping[SAFE_NAME_LEN*2+USERNAME_LENGTH+3]; > - char *hend = helping + sizeof(helping); > - uint len; > + constexpr size_t key_data_size= SAFE_NAME_LEN * 2 + USERNAME_LENGTH + 3; > + CharBuffer<key_data_size + MY_CS_MBMAXLEN> key; > GRANT_NAME *grant_name,*found=0; > HASH_SEARCH_STATE state; > > - char *db_ptr= strmov(helping, user) + 1; > - char *tname_ptr= strnmov(db_ptr, db, hend - db_ptr) + 1; > - if (tname_ptr > hend) > - return 0; // invalid name = not found > - char *end= strnmov(tname_ptr, tname, hend - tname_ptr) + 1; > - if (end > hend) > + key.append_bin(Lex_cstring_strlen(user)); > + key.append_char('\0'); > + key.append_bin(Lex_cstring_strlen(db)); is db already normalized? If yes, then, please, document it with DBUG_ASSERT(ok_for_lower_case_names()); > + key.append_char('\0'); > + key.append_opt_casedn(files_charset_info, Lex_cstring_strlen(tname), > + name_tolower); > + key.append_char('\0'); > + if (key.length() >= key_data_size) > return 0; // invalid name = not found > > - len = (uint) (end - helping); > - if (name_tolower) > - my_casedn_str(files_charset_info, tname_ptr); > - for (grant_name= (GRANT_NAME*) my_hash_first(name_hash, (uchar*) helping, > - len, &state); > + for (grant_name= (GRANT_NAME*) my_hash_first(name_hash, (uchar*) key.ptr(), > + key.length(), &state); > grant_name ; > - grant_name= (GRANT_NAME*) my_hash_next(name_hash,(uchar*) helping, > - len, &state)) > + grant_name= (GRANT_NAME*) my_hash_next(name_hash, (uchar*) key.ptr(), > + key.length(), &state)) > { > if (exact) > { > @@ -8826,33 +8856,28 @@ static bool check_grant_db_routine(THD *thd, const char *db, HASH *hash) > bool check_grant_db(THD *thd, const char *db) > { > Security_context *sctx= thd->security_ctx; > - char helping [SAFE_NAME_LEN + USERNAME_LENGTH+2], *end; > - char helping2 [SAFE_NAME_LEN + USERNAME_LENGTH+2], *tmp_db; > - uint len, UNINIT_VAR(len2); > + constexpr size_t key_data_size= SAFE_NAME_LEN + USERNAME_LENGTH + 2; > + CharBuffer<key_data_size + MY_CS_MBMAXLEN> key, key2; > bool error= TRUE; > > - tmp_db= strmov(helping, sctx->priv_user) + 1; > - end= strnmov(tmp_db, db, helping + sizeof(helping) - tmp_db); > - > - if (end >= helping + sizeof(helping)) // db name was truncated > - return 1; // no privileges for an invalid db name > - > - if (lower_case_table_names) > - { > - end = tmp_db + my_casedn_str(files_charset_info, tmp_db); > - db=tmp_db; > - } > + key.append_bin(Lex_cstring_strlen(sctx->priv_user)); > + key.append_char('\0'); > + key.append_opt_casedn(files_charset_info, Lex_cstring_strlen(db), > + lower_case_table_names); > + key.append_char('\0'); > > - len= (uint) (end - helping) + 1; > + if (key.length() >= key_data_size) // db name was truncated > + return 1; // no privileges for an invalid db name > > /* > If a role is set, we need to check for privileges here as well. > */ > if (sctx->priv_role[0]) > { > - end= strmov(helping2, sctx->priv_role) + 1; > - end= strnmov(end, db, helping2 + sizeof(helping2) - end); > - len2= (uint) (end - helping2) + 1; > + key2.append_bin(Lex_cstring_strlen(sctx->priv_role)); > + key2.append_char('\0'); > + key2.append_bin(Lex_cstring_strlen(db)); No append_opt_casedn() here? Looks wrong. > + key2.append_char('\0'); > } > > > diff --git a/sql/sql_class.h b/sql/sql_class.h > index 214118d8431..decb9a9064e 100644 > --- a/sql/sql_class.h > +++ b/sql/sql_class.h > @@ -4918,11 +4948,46 @@ class THD: public THD_count, /* this must be first */ > my_message(ER_NO_DB_ERROR, ER(ER_NO_DB_ERROR), MYF(0)); > return TRUE; > } > + return false; > + } > + > + /* > + Copy the current database to the argument. Use the current arena to > + allocate memory for a deep copy: current database may be freed after > + a statement is parsed but before it's executed. > + > + Can only be called by owner of thd (no mutex protection) > + */ > + bool copy_db_to(LEX_CSTRING *to) > + { > + if (check_if_current_db_is_set_with_error()) > + return true; > > to->str= strmake(db.str, db.length); > to->length= db.length; > return to->str == NULL; /* True on error */ > } > + > + /* > + Make a normalized copy of the current database. > + Raise an error if no current database is set. > + Note, in case of lower_case_table_names==2, thd->db can contain the > + name in arbitrary case typed by the user, so it must be lower-cased. > + For other lower_case_table_names values the name is already in > + its normalized case, so it's copied as is. > + */ > + Lex_ident_db_normalized copy_db_normalized() > + { > + if (check_if_current_db_is_set_with_error()) > + return Lex_ident_db_normalized(); > + LEX_STRING ident= make_ident_opt_casedn(db, lower_case_table_names == 2); LEX_STRING or LEX_CSTRING ? > + /* > + A non-empty thd->db is always known to satisfy check_db_name(). > + So after optional lower-casing above it's safe to > + make Lex_ident_db_normalized. > + */ > + return Lex_ident_db_normalized(ident); > + } > /* Get db name or "". Use for printing current db */ > const char *get_db() > { return safe_str(db.str); } > diff --git a/sql/sql_lex.cc b/sql/sql_lex.cc > index a84fd29affc..8cafad65538 100644 > --- a/sql/sql_lex.cc > +++ b/sql/sql_lex.cc > @@ -4186,6 +4186,18 @@ bool LEX::copy_db_to(LEX_CSTRING *to) > return thd->copy_db_to(to); > } > > + > +Lex_ident_db_normalized LEX::copy_db_normalized() > +{ > + if (sphead && sphead->m_name.str) > + { > + DBUG_ASSERT(sphead->m_db.str && sphead->m_db.length); Please, DBUG_ASSERT(sphead->m_db.str); DBUG_ASSERT(sphead->m_db.length); > + return thd->to_ident_db_normalized_with_error(sphead->m_db); > + } > + return thd->copy_db_normalized(); > +} > + > + > /** > Initialize offset and limit counters. > > diff --git a/sql/sql_show.cc b/sql/sql_show.cc > index 1fcafd6ec08..0f39a13632a 100644 > --- a/sql/sql_show.cc > +++ b/sql/sql_show.cc > @@ -3856,30 +3859,31 @@ static bool show_status_array(THD *thd, const char *wild, > names until lp:1306875 has been fixed. > TODO: remove once lp:1306875 has been addressed. > */ > - if (!(*prefix) && !strncasecmp(name_buffer, "wsrep", strlen("wsrep"))) > + if (!prefix.length && > + !strncasecmp(name_buffer.ptr(), "wsrep", strlen("wsrep"))) > { > is_wsrep_var= TRUE; > } > #endif /* WITH_WSREP */ > > if (ucase_names) > - my_caseup_str(system_charset_info, name_buffer); > + name_buffer.append_caseup(system_charset_info, var_name); > else > { > - my_casedn_str(system_charset_info, name_buffer); > - DBUG_ASSERT(name_buffer[0] >= 'a'); > - DBUG_ASSERT(name_buffer[0] <= 'z'); > - > + name_buffer.append_casedn(system_charset_info, var_name); > // WSREP_TODO: remove once lp:1306875 has been addressed. > if (IF_WSREP(is_wsrep_var == FALSE, 1) && > status_var) > - name_buffer[0]-= 'a' - 'A'; > + { > + char *ptr= (char*) name_buffer.ptr(); > + if (ptr[0] >= 'a' && ptr[0] <= 'z') > + ptr[0]-= 'a' - 'A'; > + } is it still needed? > } > > > restore_record(table, s->default_values); > - table->field[0]->store(name_buffer, strlen(name_buffer), > - system_charset_info); > + table->field[0]->store(name_buffer.to_lex_cstring(), system_charset_info); > > /* > Compare name for types that can't return arrays. We do this to not > @@ -4355,18 +4361,15 @@ bool get_lookup_field_values(THD *thd, COND *cond, bool fix_table_name_case, > > if (lower_case_table_names && !rc) > { > - /* > - We can safely do in-place upgrades here since all of the above cases > - are allocating a new memory buffer for these strings. > - */ > if (lookup_field_values->db_value.str && lookup_field_values->db_value.str[0]) > - my_casedn_str(system_charset_info, > - (char*) lookup_field_values->db_value.str); > + lookup_field_values->db_value= thd->make_ident_casedn( > + lookup_field_values->db_value); > + > if (fix_table_name_case && > lookup_field_values->table_value.str && > lookup_field_values->table_value.str[0]) > - my_casedn_str(system_charset_info, > - (char*) lookup_field_values->table_value.str); > + lookup_field_values->table_value= thd->make_ident_casedn( > + lookup_field_values->table_value); Why is it even needed? If db/table names are compared case insensitively, it shouldn't matter if they're lowercased or not, should it? > } > > return rc; > diff --git a/sql/sql_type_fixedbin.h b/sql/sql_type_fixedbin.h > index 8aaa7582cd5..e9728b78a3d 100644 > --- a/sql/sql_type_fixedbin.h > +++ b/sql/sql_type_fixedbin.h > @@ -267,8 +267,7 @@ class Type_handler_fbt: public Type_handler > void print(String *str, enum_query_type query_type) override > { > StringBuffer<FbtImpl::max_char_length()+64> tmp; > - tmp.append(singleton()->name().lex_cstring()); > - my_caseup_str(&my_charset_latin1, tmp.c_ptr()); > + tmp.copy_caseup(&my_charset_latin1, singleton()->name().lex_cstring()); why bother? > str->append(tmp); > str->append('\''); > m_value.to_string(&tmp); > diff --git a/storage/innobase/dict/dict0load.cc b/storage/innobase/dict/dict0load.cc > index bf59fed9ee4..104a8dc8780 100644 > --- a/storage/innobase/dict/dict0load.cc > +++ b/storage/innobase/dict/dict0load.cc > @@ -2955,7 +2955,7 @@ dict_load_foreign( > > foreign->foreign_table_name = mem_heap_strdupl( > foreign->heap, (char*) field, len); > - dict_mem_foreign_table_name_lookup_set(foreign, TRUE); > + foreign->foreign_table_name_lookup_set(); please, ask Marko to review InnoDB changes > > const size_t foreign_table_name_len = len; > const size_t table_name_len = strlen(table_name); > diff --git a/storage/perfschema/pfs_instr_class.h b/storage/perfschema/pfs_instr_class.h > index f353c410d4c..d083db63968 100644 > --- a/storage/perfschema/pfs_instr_class.h > +++ b/storage/perfschema/pfs_instr_class.h > @@ -265,6 +265,40 @@ struct PFS_table_share_key > char m_hash_key[PFS_TABLESHARE_HASHKEY_SIZE]; > /** Length in bytes of @c m_hash_key. */ > uint m_key_length; > + > + size_t available_length() const > + { > + return sizeof(m_hash_key) - m_key_length; > + } > + char *end() > + { > + return m_hash_key + m_key_length; > + } > + // Append and 0-terminate a string with an optional lower-case conversion > + void append_opt_casedn_z(CHARSET_INFO *cs, > + const char *str, size_t length, > + bool casedn) > + { > + DBUG_ASSERT(length <= sizeof(m_hash_key)); // Expect valid db/tbl names > + MY_STRCOPY_STATUS st; > + size_t dst_length= available_length(); > + if (dst_length > 0) > + { > + dst_length--; > + /* > + The code below uses copy_fix() instead of memcpy() to make > + sure we don't break a multi-byte character in the middle. > + */ > + m_key_length+= (uint) (casedn ? > + cs->casedn(str, length, end(), dst_length) : > + cs->copy_fix(end(), dst_length, > + str, length, length, &st)); may be let's not do copy_fix here? there are table names and in a perfschema, for PFS_table_share_key. memcpy would be enough. > + m_hash_key[m_key_length++]= '\0'; > + } > + } > + void set(bool temporary, > + const char *schema_name, size_t schema_name_length, > + const char *table_name, size_t table_name_length); > }; > > /** Table index or 'key' */ > diff --git a/storage/perfschema/pfs_program.cc b/storage/perfschema/pfs_program.cc > index de456610519..6c35a745d30 100644 > --- a/storage/perfschema/pfs_program.cc > +++ b/storage/perfschema/pfs_program.cc > @@ -118,31 +118,20 @@ static void set_program_key(PFS_program_key *key, > */ > > char *ptr= &key->m_hash_key[0]; > + const char *end= ptr + sizeof(key->m_hash_key) - 1; > > ptr[0]= object_type; > ptr++; > > if (object_name_length > 0) > - { > - char tmp_object_name[COL_OBJECT_NAME_SIZE + 1]; > - memcpy(tmp_object_name, object_name, object_name_length); > - tmp_object_name[object_name_length]= '\0'; > - my_casedn_str(system_charset_info, tmp_object_name); > - memcpy(ptr, tmp_object_name, object_name_length); > - ptr+= object_name_length; > - } > + ptr+= system_charset_info->casedn(object_name, object_name_length, > + ptr, end - ptr); > ptr[0]= 0; > ptr++; > > if (schema_name_length > 0) > - { > - char tmp_schema_name[COL_OBJECT_SCHEMA_SIZE + 1]; > - memcpy(tmp_schema_name, schema_name, schema_name_length); > - tmp_schema_name[schema_name_length]='\0'; > - my_casedn_str(system_charset_info, tmp_schema_name); > - memcpy(ptr, tmp_schema_name, schema_name_length); > - ptr+= schema_name_length; > - } > + ptr+= system_charset_info->casedn(schema_name, schema_name_length, > + ptr, end - ptr); That's strange. casedn of schema when lower_case_table_names==0 ? > ptr[0]= 0; > ptr++; > Regards, Sergei Chief Architect, MariaDB Server and security(a)mariadb.org

2 4

review for MDEV-32839 LONG UNIQUE gives error when used with REPLACE
by Alexander Barkov 12 Dec '23

12 Dec '23

Hello Sergei, - This patch: https://github.com/MariaDB/server/commit/4f2d0541b501a7a27bf44aa16821d09395… cleanup: remove innodb-specific code around update_auto_increment() is OK to push. Please write in the commit comments that it was a dead code and that InnoDB team does know why this code was there. - This patch https://github.com/MariaDB/server/commit/c8d0f3370b7d77ecb07f8f1357da6bb1f0… MDEV-32839 LONG UNIQUE gives error when used with REPLACE is OK. I have only a proposal about tests. You added tests above the "# End of 10.5 tests" line, but the patch is for 10.6. Also please add tests for at lease these engines: - MyISAM - InnoDB - Spider as well as partitioned tables for the same engines, because your patch for ha_partition.cc changes the code related to need_info_for_auto_inc(), which has a special implementation in ha_partition and ha_spider. - This patch: https://github.com/MariaDB/server/commit/94f26086663b7c8dbcd6b26930b64de36a… cleanup: remove partition-specific code around update_auto_increment() > diff --git a/sql/ha_partition.cc b/sql/ha_partition.cc > index 25bbcfdd75ce0..e174ef8d21940 100644 > --- a/sql/ha_partition.cc > +++ b/sql/ha_partition.cc > @@ -4308,6 +4308,7 @@ int ha_partition::external_lock(THD *thd, int lock_type) > m_part_info->part_expr->walk(&Item::register_field_in_read_map, 1, 0); > if ((error= m_part_info->vers_set_hist_part(thd))) > goto err_handler; > + need_info_for_auto_inc(); auto-increment code is often done under a condition like this: if (table->next_number_field && buf == table->record[0]) { some_auto_increment code; } This includes the code that you are removing from ha_partition::write_row() Is it ok to call need_info_for_auto_inc() unconditionally here? > } > DBUG_RETURN(0); > > @@ -4623,33 +4624,8 @@ int ha_partition::write_row(const uchar * buf) > */ > if (have_auto_increment) > { > - if (!table_share->next_number_keypart) > - if (unlikely(error= update_next_auto_inc_val())) > - goto exit; Here there was an extra condition !table_share->next_number_keypart before calling update_next_auto_inc_val(). Why was it needed? Why it's not needed in the new version of external_lock() before the call for need_info_for_auto_inc()? Also, the old code checked the return value. The new code int external_code() does not check the result of need_info_for_auto_inc(). Can't the behavior change because of this change? > - > - /* > - If we have failed to set the auto-increment value for this row, > - it is highly likely that we will not be able to insert it into > - the correct partition. We must check and fail if necessary. > - */ > if (unlikely(error= update_auto_increment())) > goto exit; > - > - /* > - Don't allow generation of auto_increment value the partitions handler. > - If a partitions handler would change the value, then it might not > - match the partition any longer. > - This can occur if 'SET INSERT_ID = 0; INSERT (NULL)', > - So allow this by adding 'MODE_NO_AUTO_VALUE_ON_ZERO' to sql_mode. > - The partitions handler::next_insert_id must always be 0. Otherwise > - we need to forward release_auto_increment, or reset it for all > - partitions. > - */ > - if (table->next_number_field->val_int() == 0) > - { > - table->auto_increment_field_not_null= TRUE; > - thd->variables.sql_mode|= MODE_NO_AUTO_VALUE_ON_ZERO; > - } Why is it safe to remove this code? > } > old_map= dbug_tmp_use_all_columns(table, &table->read_set); > error= m_part_info->get_partition_id(m_part_info, &part_id, &func_value); > @@ -11012,10 +10988,7 @@ void ha_partition::get_auto_increment(ulonglong offset, ulonglong increment, > else > { > THD *thd= ha_thd(); > - /* > - This is initialized in the beginning of the first write_row call. > - */ > - DBUG_ASSERT(part_share->auto_inc_initialized); > + update_next_auto_inc_val(); > /* > Get a lock for handling the auto_increment in part_share > for avoiding two concurrent statements getting the same number. > diff --git a/sql/ha_partition.h b/sql/ha_partition.h > index d75e38ec272a5..0ac8f696fcf55 100644 > --- a/sql/ha_partition.h > +++ b/sql/ha_partition.h > @@ -1408,9 +1408,8 @@ class ha_partition final :public handler > { > ulonglong nr= (((Field_num*) field)->unsigned_flag || > field->val_int() > 0) ? field->val_int() : 0; > + update_next_auto_inc_val(); > lock_auto_increment(); > - DBUG_ASSERT(part_share->auto_inc_initialized || > - !can_use_for_auto_inc_init()); > /* must check when the mutex is taken */ > if (nr >= part_share->next_auto_inc_val) > part_share->next_auto_inc_val= nr + 1;

2 2

MDEV-31404 Review
by Kristian Nielsen 11 Dec '23

11 Dec '23

Hi Monty, here is my review of the patch for MDEV-31404: > commit d51086a9f1b1b6a66c7ba8eb6a6876b433fa118f (origin/bb-11.4-MDEV-31404) > Author: Monty <monty(a)mariadb.org> > Date: Sun Dec 3 21:42:44 2023 +0200 > > MDEV-31404 Implement binlog_space_limit > > binlog_space_limit is a variable in Percona server used to limit the total > size of all binary logs. > > This implementation is based on code from Percona server 5.7. General comment: The --slave-connections-needed-for-purge defaults to 1. I think this means that --log-expire-days will have no effect on systems that have binlog enabled but no slaves configured. This is something to be aware of for backwards compatibility. What I do not like about this is the consequence it will have for all systems that have binlog enabled with some --expire-log-days set but no slaves configured. After upgrade, these systems will silently start accumulating binlogs without limit, possibly filling up disk eventually. On the other hand, it seems good to have --slave-connections-needed-for-purge enabled for the new --max-binlog-total-size option; as you pointed out, otherwise a single large transaction could immediately cause deletion of all binlogs when no slaves are connected. And having --slave-connections-needed-for-purge affect purge from --max-binlog-total-size but not for --expire-log-days seems inconsistent. In the end, maybe it isn't too serious. Many small and default-configured systems will probably be able to run for ages without binlogs getting excessive due to low traffic; and it will be easy for DBAs to just delete the binlogs if needed, or google the --slave-connections-needed-for-purge if they care enough. Two suggestions to make it easier for the DBAs to know how to handle this: 1. How about emitting a note or warning in the error log, if purge from --log-expire-days=N gets delayed for more than 2*N because no slaves are connected and --slave-connections-needed-for-purge=1 ? Note: Purge of binlogs prevented for <seconds> because no slaves are connected. To allow purge of binlogs with no slaves connected, set --slave-connections-needed-for-purge=0 2. Mention in the changelogs for 11.4 that binlogs will by default no longer be purged when no slaves are configured, and point to some documentation about how to handle this for the DBA. I think with those two suggestions it should be fine. Detailed comments on the patch below. - Kristian. > diff --git a/mysql-test/suite/binlog_encryption/binlog_index-master.opt b/mysql-test/suite/binlog_encryption/binlog_index-master.opt > new file mode 100644 > index 00000000000..f2673d08020 > --- /dev/null > +++ b/mysql-test/suite/binlog_encryption/binlog_index-master.opt > @@ -0,0 +1 @@ > +--slave_connections_needed_for_purge=0 Is this needed? If so, would it be better to put this in a general my.cnf for the entire binlog_encryption suite, like you did for mysql-test/suite/binlog/my.cnf ? So that a similar .opt will not need to be added manually for each new binlog_encryption test that will need it. > --- a/mysql-test/suite/perfschema/t/show_sanity.test > +++ b/mysql-test/suite/perfschema/t/show_sanity.test > @@ -533,6 +533,7 @@ insert into test.sanity values > ("JUNK: GLOBAL-ONLY", "I_S.SESSION_VARIABLES", "MAX_BINLOG_CACHE_SIZE"), > ("JUNK: GLOBAL-ONLY", "I_S.SESSION_VARIABLES", "MAX_BINLOG_SIZE"), > ("JUNK: GLOBAL-ONLY", "I_S.SESSION_VARIABLES", "MAX_BINLOG_STMT_CACHE_SIZE"), > + ("JUNK: GLOBAL-ONLY", "I_S.SESSION_VARIABLES", "MAX_BINLOGS_SIZE"), I think this is a left-over from previous naming, should probably be "MAX_BINLOG_TOTAL_SIZE ? > +FLUSH LOGS; > +FLUSH LOGS; > +FLUSH LOGS; > +source include/show_binary_logs.inc; > +show status like "binlog_disk_use"; I think you will need --source include/wait_for_binlog_checkpoint.inc after each of these FLUSH LOGS. Otherwise random delay of binlog checkpoint could cause the binlog_disk_use value to fluctuate and get rare random test failures. (Or alternatively consider if you could omit the select of binlog_disk_use completely, since it will probably differ between versions as event formats change.) Adding a small sleep in binlog_background_thread() just before calling mysql_bin_log.mark_xid_done() can usually reproduce the random failures from delayed binlog checkpoint. > +INSERT INTO t1 VALUES (0,repeat("a",3000)); > +show status like "binlog_disk_use"; > +Variable_name Value > +Binlog_disk_use 3848 > +# First binary should be binary.000004 > +show binary logs; > +Log_name File_size > +binary.000004 # > +INSERT INTO t1 VALUES (2,repeat("b",10)); > +# First binary should be binary.000004 > +show binary logs; > +Log_name File_size > +binary.000004 # > +binary.000005 # Here, I don't understand why binary.000004 is not purged. The binary.000004 contains the large transaction 'repeat("a", 3000)', so the binlog total size is too large. And the transaction 'repeat("n",10)' caused a binlog rotation. So why does that not cause binary.000004 to be purged due to --max-binlog-total-size=1500 ? > +show status like "binlog_disk_use"; > +Variable_name Value > +Binlog_disk_use 3647 > +INSERT INTO t1 VALUES (7,repeat("g",3000)); > +# binary.000001 should be deleted now > +show binary logs; > +Log_name File_size > +binary.000002 # > +binary.000003 # Similarly here, why binary.000002 is not purged? > +INSERT INTO t1 VALUES (4,repeat("d",3000)); > +--echo # First binary should be binary.000011 > +source include/show_binary_logs.inc; > + > +RESET MASTER; Idea: Here you could (or ask one of the other devs) add a test that the SHOW STATUS LIKE "binlog_disk_use" is equal to the size of the files on disk, to check that the running calculation of disk use doesn't miss the odd byte or two. > + if ((longlong) (binlog_space_total + binlog_pos - found_space <= > + binlog_space_limit)) > + break; // Found enough The (longlong) cast here looks misplaced. It's casting the result of the (X <= Y) comparison. I think the cast can just be removed, all the variables seem to be ulonglong type. > +static bool waiting_for_slave_to_change_binlog= 0; > +static ulonglong purge_sending_new_binlog_file= 0; I think a short comment here on these variables would be good, explaining their purpose. And same on the declaration of Atomic_counter<ulonglong> sending_new_binlog_file. > + DBUG_ASSERT(!is_relay_log || binlog_xid_count_list.is_empty()); > + if (!is_relay_log) > { > - I_List_iterator<xid_count_per_binlog> it(binlog_xid_count_list); > - while ((b= it++) && > - 0 != strncmp(log_file_name_arg+dirname_length(log_file_name_arg), > - b->binlog_name, b->binlog_name_len)) > - ; > + xid_count_per_binlog *b; > + mysql_mutex_lock(&LOCK_xid_list); Good catch that we were uselessly doing xid_count operations on the relay log, thanks! > +int MYSQL_BIN_LOG::count_binlog_space() > +{ > + int error; > + LOG_INFO log_info; > + DBUG_ENTER("count_binlog_space"); > + if (is_relay_log || !binlog_space_limit) > + DBUG_RETURN(0); Maybe this if (is_relay_log || !binlog_space_limit) could be an assert? We probably should not be calling this function at all in that case. It looks like that binlog_space_limit is already checked in all callers, not sure about relay log. (A small point, just something to consider as you like). > @@ -7608,7 +7807,8 @@ int MYSQL_BIN_LOG::rotate(bool force_rotate, bool* check_purge) > @retval > nonzero - error in rotating routine. > */ > -void MYSQL_BIN_LOG::purge() > + > +void MYSQL_BIN_LOG::purge(bool all) Would be good with a small comment here explaining the meaning of the new parameter `all`. Minor stuff: Commit message: > Binlog file file sizes are checked on startup Typo (s/file file/file/). > when updating the binary log on on binary log rotation Typo (s/on on/and on/ ?) > Without this option max-binlog-total-size would potentially deleted > binlogs needed by slaves on server startup or when a slave disconnects Typo s/deleted/delete/. > +static Sys_var_ulonglong Sys_max_binlog_total_size( > + "max_binlog_total_size", > + "Maximum space to use for all binary logs. Extra logs are deleted on " > + "server start, log rotation, FLUSH LOGGS or when writing to binlog. " > + "Default is 0, which means no size restrictions." > + "See also slave_connections_needed_for_purge", Missing space at end of line (s/restrictions."/restrictions. "/). > +static Sys_var_ulong Sys_slave_connections_needed_for_purge( > + "slave_connections_needed_for_purge", > + "Number of minimum connected slaves needed for automatic binary " > + "log purge with max_binlog_total_size, binlog_expire_logs_seconds " > + "or binlog_expire_logs_days.", Suggestion for better wording: "Minimum number of connected slaves required for automatic binary " "log purge with max_binlog_total_size, binlog_expire_logs_seconds " "or binlog_expire_logs_days."

2 2

Re: d25ab9fdfa1: MDEV-31608 - Connector/NET fails to connect since 10.10
by Sergei Golubchik 30 Nov '23

30 Nov '23

Hi, Vladislav, On Nov 29, Vladislav Vaintroub wrote: > revision-id: d25ab9fdfa1 (mariadb-10.11.2-62-gd25ab9fdfa1) > parent(s): 034848c6c27 > author: Vladislav Vaintroub > committer: Vladislav Vaintroub > timestamp: 2023-10-08 19:37:20 +0200 > message: > > MDEV-31608 - Connector/NET fails to connect since 10.10 > > Connector/NET does not expect collation IDs returned by "show collations" > to be NULL, runs into an exception. > > The fix is to determine connector/net using its connection attributes, > exclude null-id collations from "show collations".The patch introduces > new old_mode NO_NULL_COLLATION_IDs, that is automatically set, > once MySQL Connector/NET connection is determined. I don't like this hack at all, but I don't have any better ideas, so just one question below > A test was added, that uses MySql.Data from powershell - only works > if MySql.Data is installed into GAC (i.e with C/NET MSI package) please, make sure that MySql.Data is installed on at least one buildbot builder. > diff --git a/sql/sql_show.cc b/sql/sql_show.cc > index bff55f7a431..bea993305be 100644 > --- a/sql/sql_show.cc > +++ b/sql/sql_show.cc > @@ -6386,7 +6386,8 @@ int fill_schema_collation(THD *thd, TABLE_LIST *tables, COND *cond) > character set prefix). > */ > if (is_context && > - cmp(tmp_cl->cs_name, Lex_cstring(STRING_WITH_LEN("utf8mb4")))) > + ((thd->variables.old_behavior & OLD_MODE_NO_NULL_COLLATION_IDS) || > + cmp(tmp_cl->cs_name, Lex_cstring(STRING_WITH_LEN("utf8mb4"))))) wouldn't it be better to do instead - bool is_context= cmp(context_collation_name, full_collation_name); + bool is_context= cmp(context_collation_name, full_collation_name) + && !(thd->variables.old_behavior & OLD_MODE_NO_NULL_COLLATION_IDS); ? > continue; > restore_record(table, s->default_values); > table->field[0]->store(context_collation_name, scs); Regards, Sergei Chief Architect, MariaDB Server and security(a)mariadb.org

2 1

Re: [PATCH] MDEV-32551: "Read semi-sync reply magic number error" warnings on master
by Kristian Nielsen 23 Nov '23

23 Nov '23

Hi Monty, > From: Michael Widenius <monty(a)mariadb.com> > > commit 7abf83f86829ed94de618af4749eb96d5379bd90 (origin/bb-10.6-monty) > Author: Michael Widenius <monty(a)mariadb.com> > Date: Wed Nov 8 16:57:58 2023 -0700 > > MDEV-32551: "Read semi-sync reply magic number error" warnings on master I read through the patch, looks good. A few comments below, mostly minor things and typos that you can consider as you like. - Kristian. > - We should in 11.3 changed the default value for > rpl-semi-sync-master-wait-no-slave from TRUE to FALSE as the TRUE > Sys_semisync_master_wait_no_slave( > "rpl_semi_sync_master_wait_no_slave", > - "Wait until timeout when no semi-synchronous replication slave " > - "available (enabled by default).", > + "Wait until timeout when no semi-synchronous replication slave is " > + "available.", > GLOBAL_VAR(rpl_semi_sync_master_wait_no_slave), > CMD_LINE(OPT_ARG), DEFAULT(TRUE), > - NO_MUTEX_GUARD, NOT_IN_BINLOG, ON_CHECK(0), > - ON_UPDATE(fix_rpl_semi_sync_master_wait_no_slave)); > + NO_MUTEX_GUARD, NOT_IN_BINLOG, ON_CHECK(0)); > -my_bool rpl_semi_sync_master_wait_no_slave = 1; > +my_bool rpl_semi_sync_master_wait_no_slave = 0; There seems to be som inconsistency here: - The commit is based on 10.6, but the commit comment says 11.3 - The option help text is changed in the commit - The "DEFAULT(TRUE)" in the sysvar definition is _not_ changed in the commit - The C initialization values of the my_bool rpl_semi_sync_master_wait_no_slave _is_ changed in the commit. Should it be DEFAULT(FALSE) also in this patch, and the commit message say "10.6"? Or should these changes be removed from the 10.6 patch and put in 11.3 instead? At least this looks inconsistent. > diff --git a/mysql-test/include/wait_for_pattern_in_file.inc b/mysql-test/include/wait_for_pattern_in_file.inc > new file mode 100644 > index 00000000000..d088cb125c2 > --- /dev/null > +++ b/mysql-test/include/wait_for_pattern_in_file.inc > @@ -0,0 +1,54 @@ > +# ==== Purpose ==== > +# > +# Waits until pattern comes into log file or until a timeout is reached. Nice addition, this will be useful for other things! > +--let $keep_include_silent= 1 Why do you set this in the .inc? (Normally this is set in the .test files that want to skip the printout). +let SEARCH_SILENT=1; Maybe the .inc should restore the old value at the end (in case a .test will at some point set it globally for its own calls to include/search_pattern_in_file.inc) > +--let $keep_include_silent= 0 Here, better restore the original value (in case some .test has set it to 1 for itself). > +# Test is binlog format indepdendent, so save resources (Typo s/indepdendent/independent/) > +# The followowing command is likely to cause the slave master is not yet setup > +# for semi-sync (Typo s/followowing/following/) > - strcpy(m_reply_file_name, ""); > - strcpy(m_wait_file_name, ""); > + m_reply_file_name[0]= m_wait_file_name[0]= 0; Interestingly, GCC actually creates identical code for these (it knows to optimize strcpy()). (I'm just mentioning this as I know we both are interested in low-level code details, not asking you to revert the change). Example code: ----------------------------------------------------------------------- #include <string.h> struct X { char m_a[100]; char m_b[10]; void init1(); void init2(); }; void X::init1() { strcpy(m_a, ""); strcpy(m_b, ""); } void X::init2() { m_a[0]= m_b[0]= 0; } ----------------------------------------------------------------------- Result from gcc -O3: ----------------------------------------------------------------------- .type _ZN1X5init1Ev, @function _ZN1X5init1Ev: movb $0, (%rdi) movb $0, 100(%rdi) ret .type _ZN1X5init2Ev, @function _ZN1X5init2Ev: movb $0, 100(%rdi) movb $0, (%rdi) ret ----------------------------------------------------------------------- Identical, except that we now initialize in the opposite order ;-) > +#ifndef DBUG_OFF > +void dbug_verify_no_duplicate_slaves(Slave_ilist *m_slaves, THD *thd) > +{ You could make this (debug-only) function "static". > + Master is not responding (gone away?) or it has turned semi sync > + off. Turning off semi-sync repsonces as there is no point in sending (Typo s/repsonces/responses/)

2 1

Re: dadcf5ed7b1: MDEV-24486 Add `table_privileges` view to the `sys` schema
by Sergei Golubchik 21 Nov '23

21 Nov '23

Hi, Oleg, ok to push. Just one comment below. On Nov 20, Oleg Smirnov wrote: > revision-id: dadcf5ed7b1 (mariadb-11.0.1-259-gdadcf5ed7b1) > parent(s): b8f9f796ff8 > author: Oleg Smirnov > committer: Oleg Smirnov > timestamp: 2023-11-20 19:35:36 +0700 > message: > > MDEV-24486 Add `table_privileges` view to the `sys` schema > > The existing INFORMATION_SCHEMA.TABLE_PRIVILEGES displays only those > privileges that were specifically granted on the table level, > whereas it may be useful to see privileges granted at the database > and global level. > > diff --git a/scripts/sys_schema/NEWS.md b/scripts/sys_schema/NEWS.md > index 7aeefb7edf9..12ce1382b75 100644 > --- a/scripts/sys_schema/NEWS.md > +++ b/scripts/sys_schema/NEWS.md > @@ -1,5 +1,12 @@ > # Change history for the MySQL sys schema > > +## 1.5.2 (11/20/23) > + > +### Improvements > + > +* A new `table_privileges` view was added, which displays privileges to tables granted at all levels (user, schema, table) is it "privileges to tables"? I feel like "privileges on tables" would be more correct. It's GRANT privilege ON object TO user same it two other places. > + > + > ## 1.5.1 (07/07/16) Regards, Sergei Chief Architect, MariaDB Server and security(a)mariadb.org

3 3

Question about STACK_DIRETION fix removal in merge commit 6c05edf
by Otto Kekäläinen 20 Nov '23

20 Nov '23

Hi Marko! I noticed you removed from debian/rules a section that flipped -DSTACK_DIRECTION=-1/1 based on architecture in the merge commit https://github.com/MariaDB/server/commit/6c05edfdcd335b9587ba140ebef5d9b082… Was this intentional? I am not sure, but it looks like a mistake to me, so I wanted to confirm. The code originates from cross-compilation fix in Debian in https://salsa.debian.org/mariadb-team/mariadb-server/-/commit/f0ba31e1565fe…, which was later upstreamed to 10.5 (but not on 10.4) in https://github.com/MariaDB/server/commit/ea56841997995283f04e85e957c8d7f315…. - Otto

2 1

Re: 73097e18b77: MDEV-32771 Server crash upon online alter with concurrent XA
by Sergei Golubchik 14 Nov '23

14 Nov '23

Hi, Nikita, ok to push. spelling and renaming suggestions below On Nov 13, Nikita Malyavin wrote: > revision-id: 73097e18b77 (mariadb-11.2.1-20-g73097e18b77) > parent(s): f7646d890b9 > author: Nikita Malyavin > committer: Nikita Malyavin > timestamp: 2023-11-13 15:28:19 +0100 > message: > > MDEV-32771 Server crash upon online alter with concurrent XA > > In case of a non-recovery XA rollback/commit in the same connection, > thon->rollback is called instead of rollback_by_xid, Though reviously, "previously" > thd_ha_data was moved to thd->transaction->xid_state.xid in hton->prepare. > > Like it wasn't enough, XA PREPARE can be skipped upon user and thus we > can end up in hton->commit/rollback with and unprepared XA, so checking > xid_state.is_explicit_XA is not enough -- we should check > xid_state.get_state_code() == XA_PREPARED, which will also guarantee > is_explicit_XA() == true. > --- a/sql/online_alter.cc > +++ b/sql/online_alter.cc > @@ -315,17 +315,35 @@ int online_alter_savepoint_rollback(handlerton *hton, THD *thd, sv_id_t sv_id) > > static int online_alter_commit(handlerton *hton, THD *thd, bool all) > { > - int res= online_alter_end_trans(get_cache_list(hton, thd), thd, > - ending_trans(thd, all), true); > - cleanup_tables(thd); > + int res; > + bool commit= ending_trans(thd, all); > + if (thd->transaction->xid_state.get_state_code() == XA_PREPARED && commit) > + { > + res= hton->commit_by_xid(hton, thd->transaction->xid_state.get_xid()); > + // cleanup is done by prepare() this is conusing, there's no prepare after commit_by_xid. may be you wanted to say "cleanup was already done by prepare()" ? > + } > + else > + { > + res= online_alter_end_trans(get_cache_list(hton, thd), thd, commit, true); > + cleanup_tables(thd); > + } > return res; > }; > > static int online_alter_rollback(handlerton *hton, THD *thd, bool all) > { > - int res= online_alter_end_trans(get_cache_list(hton, thd), thd, > - ending_trans(thd, all), false); > - cleanup_tables(thd); > + int res; > + bool commit= ending_trans(thd, all); 'commit' is weird here, on the rollback path. may be you rename it to, like "is_ending_tran" or "end_tran" or "is_persistent" or something along these lines. And then 'commit' in online_alter_commit too, for consistency. > + if (thd->transaction->xid_state.get_state_code() == XA_PREPARED && commit) > + { > + res= hton->rollback_by_xid(hton, thd->transaction->xid_state.get_xid()); > + // cleanup is done by prepare() same as in online_alter_commit > + } > + else > + { > + res= online_alter_end_trans(get_cache_list(hton, thd), thd, commit, false); > + cleanup_tables(thd); > + } > return res; > }; Regards, Sergei Chief Architect, MariaDB Server and security(a)mariadb.org

1 0

Re: MDEV-27576 Use reverse index for max/min optimization
by Sergey Petrunia 13 Nov '23

13 Nov '23

Hi Yuchen, > commit c4c4bd8e47e5bfd4d0a650e286ea9daa3e6276f0 > Author: Yuchen Pei <ycp(a)mariadb.com> > Date: Thu Nov 2 11:55:42 2023 +1100 > > MDEV-27576 Use reverse index for max/min optimization > > We add a field in st_table_ref to indicate that the key used is > a descending index, which will flip the functions and flags used in > get_index_max_value() and get_index_min_value(), that allows correct > optimization for max/min for descending index. Generally the patch looks good. I have only a few cosmetic comments. Please rename the test file from mdev_27576.test to something more descriptive like desc_index_min_max.test (following desc_index_range.test) Also please rename the table in the test to t1. > diff --git a/sql/sql_select.h b/sql/sql_select.h > index e0bbadadc69..45d7527280a 100644 > --- a/sql/sql_select.h > +++ b/sql/sql_select.h > @@ -126,6 +126,7 @@ typedef struct st_table_ref > uint key_parts; ///< num of ... > uint key_length; ///< length of key_buff > int key; ///< key no > + bool reverse; ///< key is reverse > uchar *key_buff; ///< value to look for with key > uchar *key_buff2; ///< key_buff+key_length > store_key **key_copy; // It's actually not "key is reverse" but "last used key part is reverse"... also this member is never set for the most common scenario of TABLE_REF use, which is ref access. The member is set in a rather trivial logic in find_key_for_maxmin(), and then it's used in get_index_min_value() and get_index_max_value(). I suggest that we do without this member. get_index_min_value() and get_index_max_value() can check: bool reverse= table->key_info[ref->key].key_part[ref->key_part]->key_part_flag & HA_REVERSE_SORT > diff --git a/sql/opt_sum.cc b/sql/opt_sum.cc > index 794ec40fc66..1f94a4f4dc7 100644 > --- a/sql/opt_sum.cc > +++ b/sql/opt_sum.cc > @@ -114,7 +114,8 @@ static int get_index_min_value(TABLE *table, TABLE_REF *ref, > int error; > > if (!ref->key_length) > - error= table->file->ha_index_first(table->record[0]); > + error= ref->reverse ? table->file->ha_index_last(table->record[0]) : > + table->file->ha_index_first(table->record[0]); Please add { ... } as the if-branch is a multi-line statement now. > else > { > /* > @@ -206,8 +210,12 @@ static int get_index_max_value(TABLE *table, TABLE_REF *ref, uint range_fl) > table->file->ha_index_read_map(table->record[0], ref->key_buff, > make_prev_keypart_map(ref->key_parts), > range_fl & NEAR_MAX ? > - HA_READ_BEFORE_KEY : > - HA_READ_PREFIX_LAST_OR_PREV) : > + (ref->reverse ? HA_READ_AFTER_KEY : > + HA_READ_BEFORE_KEY) : > + (ref->reverse ? HA_READ_KEY_OR_NEXT : > + HA_READ_PREFIX_LAST_OR_PREV)) : > + ref->reverse ? > + table->file->ha_index_first(table->record[0]) : > table->file->ha_index_last(table->record[0])); Multiple levels of x?y:z statements are hard to read. Please change to if (ref->key_length) { ... } else { ... } BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Blog: http://petrunia.net

2 2

Re: 28deba0a611: MDEV-32101 CREATE PACKAGE [BODY] for sql_mode=DEFAULT
by Sergei Golubchik 10 Nov '23

10 Nov '23

Hi, Alexander, Looks great! Just a couple of questions below On Nov 07, Alexander Barkov wrote: > revision-id: 28deba0a611 (mariadb-11.0.1-239-g28deba0a611) > parent(s): e9573c05965 > author: Alexander Barkov > committer: Alexander Barkov > timestamp: 2023-09-22 14:38:52 +0400 > message: > > MDEV-32101 CREATE PACKAGE [BODY] for sql_mode=DEFAULT > > This patch adds PACKAGE support with SQL/PSM dialect for sql_mode=DEFAULT: > > - CREATE PACKAGE > - DROP PACKAGE > - CREATE PACKAGE BODY > - DROP PACKAGE BODY > - Package function and procedure invocation from outside of the package: > -- using two step identifiers > SELECT pkg.f1(); > CALL pkg.p1() > > -- using three step identifiers > SELECT db.pkg.f1(); > CALL db.pkg.p1(); > > This is a non-standard MariaDB extension. > > However, later this code can be used to implement > the SQL Standard and DB2 dialects of CREATE MODULE. > > diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy > index af2ec678daa..fca39b63312 100644 > --- a/sql/sql_yacc.yy > +++ b/sql/sql_yacc.yy > @@ -1285,7 +1285,7 @@ bool my_yyoverflow(short **a, YYSTYPE **b, size_t *yystacksize); > %left TRANSACTION_SYM TIMESTAMP PERIOD_SYM SYSTEM USER COMMENT_SYM > > %left PREC_BELOW_SP_OBJECT_TYPE > -%left FUNCTION_SYM > +%left PACKAGE_MARIADB_SYM FUNCTION_SYM Why PACKAGE_ORACLE_SYM and PACKAGE_ALLMODES_SYM don't need it? (same for BODY_ORACLE_SYM and BODY_ALLMODES_SYM) > > > /* > @@ -3049,6 +3059,8 @@ sp_handler: > | PROCEDURE_SYM { $$= &sp_handler_procedure; } > | PACKAGE_ORACLE_SYM { $$= &sp_handler_package_spec; } > | PACKAGE_ORACLE_SYM BODY_ORACLE_SYM { $$= &sp_handler_package_body; } > + | PACKAGE_MARIADB_SYM { $$= &sp_handler_package_spec; } > + | PACKAGE_MARIADB_SYM BODY_MARIADB_SYM { $$= &sp_handler_package_body; } Why not to use here PACKAGE_ALLMODES_SYM and BODY_ALLMODES_SYM? (also, I'd personally call them simply PACKAGE_SYM and BODY_SYM) > ; > > > @@ -19428,35 +19489,33 @@ create_routine: > (Item_result) $8, $10)) > MYSQL_YYABORT; > } > - | create_or_replace definer_opt PACKAGE_ORACLE_SYM > + | create_or_replace definer_opt PACKAGE_ALLMODES_SYM > opt_if_not_exists sp_name opt_create_package_chistics_init > { > sp_package *pkg; > if (unlikely(!(pkg= Lex-> > - create_package_start(thd, > - SQLCOM_CREATE_PACKAGE, > - &sp_handler_package_spec, > - $5, $1 | $4)))) > + create_package_start(thd, &sp_handler_package_spec, > + $5, $1 | $4, > + Lex->sp_chistics)))) > MYSQL_YYABORT; > pkg->set_c_chistics(Lex->sp_chistics); Looks redundant > Lex->sphead->set_body_start(thd, YYLIP->get_cpp_tok_start()); > } > sp_tail_is > opt_package_specification_element_list END > - remember_end_opt opt_sp_name > + remember_end_opt opt_trailing_sp_name > { > if (unlikely(Lex->create_package_finalize(thd, $5, $12, $11))) > MYSQL_YYABORT; > } > - | create_or_replace definer_opt PACKAGE_ORACLE_SYM BODY_ORACLE_SYM > + | create_or_replace definer_opt PACKAGE_ALLMODES_SYM BODY_ALLMODES_SYM > opt_if_not_exists sp_name opt_create_package_chistics_init > { > sp_package *pkg; > if (unlikely(!(pkg= Lex-> > - create_package_start(thd, > - SQLCOM_CREATE_PACKAGE_BODY, > - &sp_handler_package_body, > - $6, $1 | $5)))) > + create_package_start(thd, &sp_handler_package_body, > + $6, $1 | $5, > + Lex->sp_chistics)))) > MYSQL_YYABORT; > pkg->set_c_chistics(Lex->sp_chistics); same > Lex->sphead->set_body_start(thd, YYLIP->get_cpp_tok_start()); Regards, Sergei Chief Architect, MariaDB Server and security(a)mariadb.org

2 2

Re: 2342aaf46eb: MDEV-30973 Implement debug logging for slave_transaction_retries
by Sergei Golubchik 07 Nov '23

07 Nov '23

Hi, Aleksey, On Nov 07, Aleksey Midenkov wrote: > revision-id: 2342aaf46eb (mariadb-11.0.1-256-g2342aaf46eb) > parent(s): 80bc5df1c80 > author: Aleksey Midenkov > committer: Aleksey Midenkov > timestamp: 2023-10-10 13:11:19 +0300 > message: > > MDEV-30973 Implement debug logging for slave_transaction_retries > > Configuration options: > > log_slave_retries > log_slave_retries_max 1. a commit comment of >100 lines that's a bit too much, I think. 2. comments in MDEV-30973 say that one should be able to have everything in one file, namely in the error log. I don't see how it's possible in your commit. 3. one of the reasons for not simply writing into erorr log was "you get what are relatively large entries in there which can make it hard/annoying to also look for the normal, one-line warning and error output". But as far as I can see your slave retry entries are not large, but normal one-line messages. I'd say they can fit into the error log just fine. I've still commented below in the assumption that the separate log implementation will stay. But I honestly don't see a reason for it anymore. > diff --git a/mysql-test/suite/rpl/t/slave_deadlock.test b/mysql-test/suite/rpl/t/slave_deadlock.test > new file mode 100644 > index 00000000000..9fcda7c9b71 > --- /dev/null > +++ b/mysql-test/suite/rpl/t/slave_deadlock.test > @@ -0,0 +1,194 @@ > +--source include/have_debug.inc > +--source include/have_innodb.inc > +--source include/have_binlog_format_statement.inc > + > +# On --repeat test infinitely increments thread_id and finally fails --sync_with_master at log_pos 1754: > +# > +# 231009 20:58:15 server id 1 end_log_pos 1754 CRC32 0x7e5aecd1 Query thread_id=561 exec_time=0 error_code=0 xid=0 I'm sorry, I don't understand that. Could you elaborate please? > +# > +# force_restart seems to fix this > + > +--source include/force_restart.inc > + > +# Note that test requires big enough --debug-sync-timeout unfair scheduling at high load. Sorry, cannot parse that. Could you elaborate please? > +# slave_deadlock.opt sets this to 1 hour > + > +--let $rpl_topology=1->2 > +--source include/rpl_init.inc > + > +--echo # > +--echo # MDEV-30973 Enhance output of innodb deadlocks (SAMU-124, SAMU-131) > +--echo # > +--connection server_2 > +SET @old_dbug= @@global.debug_dbug; > +set @old_parallel_threads= @@global.slave_parallel_threads; > +set @old_domain_parallel_threads= @@global.slave_domain_parallel_threads; > +set @old_parallel_mode= @@global.slave_parallel_mode; > +set @old_slave_transaction_retries= @@global.slave_transaction_retries; > +set @old_innodb_lock_wait_timeout= @@global.innodb_lock_wait_timeout; > +set @old_log_slave_retries= @@global.log_slave_retries; > +set @old_log_slave_retries_file= @@global.log_slave_retries_file; > +set @old_log_slave_retries_max= @@global.log_slave_retries_max; All this makes little sense, it's simpler to put all settings into the .opt file generally it's better to do your way, to have less server restarts, but you have an .opt file already, you can as well use it. except if you decide to remove the .opt file - then nevermind, the above it good. > +--source include/stop_slave.inc > +change master to master_use_gtid= slave_pos; > +set global slave_parallel_threads= 10; > +set global slave_domain_parallel_threads= 1; > +set global slave_parallel_mode= optimistic; > +set global slave_transaction_retries= 4; > +set global innodb_lock_wait_timeout= 1; ... > diff --git a/mysql-test/suite/versioning/r/rpl_row.result b/mysql-test/suite/versioning/r/rpl_row.result > index c161f68d0bd..6343fbecdec 100644 > --- a/mysql-test/suite/versioning/r/rpl_row.result > +++ b/mysql-test/suite/versioning/r/rpl_row.result > @@ -37,12 +37,7 @@ select *, unix_timestamp(row_start) as row_start, unix_timestamp(row_end) as row > x y row_start row_end > 1 1 12345.000000 2147483647.999999 > set timestamp= default; > -### INSERT INTO `test`.`t1` > -### SET > -### @1=1 > -### @2=1 > -### @3=12345.000000 > -### @4=2147483647.999999 why? > +/*!50530 SET @@SESSION.PSEUDO_SLAVE_MODE=1*/; > connection slave; > select * from t1; > x y > diff --git a/sql/log.cc b/sql/log.cc > index 877315c60de..78e874fda5b 100644 > --- a/sql/log.cc > +++ b/sql/log.cc > @@ -1188,9 +1188,17 @@ bool Log_to_file_event_handler:: > log_error(enum loglevel level, const char *format, > va_list args) > { > - return vprint_msg_to_log(level, format, args); > + return vprint_msg_to_log(stderr, level, format, args); > } > > +#ifdef HAVE_REPLICATION > +bool Log_to_file_event_handler::log_slave_retry(const char *format, va_list args) > +{ > + DBUG_ASSERT(slave_retries_file.get()); > + return vprint_msg_to_log(slave_retries_file.get(), NO_LEVEL, format, args); I don't think you should use vprint_msg_to_log and add a NO_LEVEL hack. vprint_msg_to_log also prints into the windows event log, into wsrep json log, and it locks LOCK_error_log. You don't need any of that, just create your own function, it's just one fprintf() anyway. > +} > +#endif > + > void Log_to_file_event_handler::init_pthread_objects() > { > mysql_log.init_pthread_objects(); > @@ -9752,13 +9787,15 @@ static void print_buffer_to_file(enum loglevel level, const char *buffer, > } > } > > +#ifndef EMBEDDED_LIBRARY > mysql_mutex_lock(&LOCK_error_log); > +#endif > > skr= my_time(0); > localtime_r(&skr, &tm_tmp); > start=&tm_tmp; > > - fprintf(stderr, "%d-%02d-%02d %2d:%02d:%02d %lu [%s] %.*s%.*s\n", > + fprintf(file, "%d-%02d-%02d %2d:%02d:%02d [T%lu] %s%.*s%.*s\n", eh... no. don't change the format of existing log files, please. > start->tm_year + 1900, > start->tm_mon+1, > start->tm_mday, > @@ -9834,6 +9872,22 @@ void sql_print_error(const char *format, ...) > } > > > +#ifdef HAVE_REPLICATION > +void sql_print_error2(const char *format, ...) s/sql_print_error2/print_slave_error/ ? > +{ > + va_list args; > + DBUG_ENTER("sql_print_error"); > + > + va_start(args, format); > + error_log_print(ERROR_LEVEL, format, args); > + slave_retries_print(format, args); > + va_end(args); > + > + DBUG_VOID_RETURN; > +} > +#endif > + > + > void sql_print_warning(const char *format, ...) > { > va_list args; > diff --git a/sql/mysqld.cc b/sql/mysqld.cc > index 8cc3ad51ee9..4f6b2273597 100644 > --- a/sql/mysqld.cc > +++ b/sql/mysqld.cc > @@ -765,6 +765,90 @@ char *opt_relay_logname = 0, *opt_relaylog_index_name=0; > char *opt_logname, *opt_slow_logname, *opt_bin_logname; > char *opt_binlog_index_name=0; > > +#ifdef HAVE_REPLICATION > +char default_slave_retries_path[FN_REFLEN]; > +bool opt_slave_retries= false; > +char *opt_slave_retries_path= NULL; > +uint opt_slave_retries_max_log= 0; > +#endif > +Slave_retries_file slave_retries_file; > + > +bool Slave_retries_file::open() > +{ > +#ifdef HAVE_REPLICATION > + DBUG_ASSERT(opt_slave_retries); > + DBUG_ASSERT(opt_slave_retries_path); > + file= fopen(opt_slave_retries_path, "a"); my_fopen or my_open and anyway, why a new class, why not to use MYSQL_LOG like general and slow logs do? > + if (!file) > + { > + sql_print_error("Open of log_slave_retries '%s' failed: %s (%d)", > + opt_slave_retries_path, strerror(errno), errno); This is inconsistent, the server doesn't print this for other log files. I've just tried with general log and got 2023-11-06 17:58:19 0 [ERROR] mysqld: File '/tmp' not found (Errcode: 21 "Is a directory") 2023-11-06 17:58:19 0 [ERROR] Could not use /tmp for logging (error 21). Turning logging off for the whole duration of the MariaDB server process. To turn it on again: fix the cause, shutdown the MariaDB server and restart it. Please, do the same > + return true; > + } > + else > + { > + slave_retries_print("%s (mariadbd %s) starting as process %lu ...", > + my_progname, server_version, (ulong) getpid()); Any reason, why it shouldn't be the same as in the error log? sql_print_information("Starting MariaDB %s source revision %s as process %lu", server_version, SOURCE_REVISION, (ulong) getpid()); > + } > +#endif > + return false; > +} > + > +void Slave_retries_file::close() > +{ > +#ifdef HAVE_REPLICATION > + if (file) > + { > + acquire(); > + fclose(file); > + release(); > + } > + file= NULL; > +#endif > +} > + > +void Slave_retries_file::init() > +{ > +#ifdef HAVE_REPLICATION > + mysql_mutex_init(key_LOCK_global_system_variables, &mutex, MY_MUTEX_INIT_FAST); > +#endif > +} > + > +void Slave_retries_file::destroy() > +{ > +#ifdef HAVE_REPLICATION > + mysql_mutex_destroy(&mutex); > +#endif > +} > + > +bool Slave_retries_file::acquire() > +{ > +#ifdef HAVE_REPLICATION > + if (!file) > + return false; > + mysql_mutex_lock(&mutex); > + return true; > +#else > + return false; > +#endif > +} > + > +void Slave_retries_file::release() > +{ > +#ifdef HAVE_REPLICATION > + mysql_mutex_unlock(&mutex); > +#endif > +} > + > +void Slave_retries_file::flush() > +{ > + if (acquire()) > + { > + fflush(file); > + release(); > + } > +} > + > /* Static variables */ > > my_bool opt_stack_trace; > @@ -5055,6 +5145,35 @@ static int init_server_components() > "this server. However this will be ignored as the " > "--log-bin option is not defined."); > } > + > + slave_retries_file.init(); > + if (!log_error_file_ptr[0]) > + fn_format(default_slave_retries_path, pidfile_name, mysql_data_home, > + "-retries.err", MY_REPLACE_EXT); > + else > + fn_format(default_slave_retries_path, log_error_file_ptr, mysql_data_home, > + "-retries.err", MY_REPLACE_EXT); that's not how all other log names work, grep mysqld.cc for opt_slow_logname > + if (opt_slave_retries_path && !opt_abort) > + { > + if (!opt_slave_retries_path[0]) > + { > + if (!default_slave_retries_path[0]) > + { > + /* fn_format failed */ > + opt_slave_retries_path= NULL; > + opt_slave_retries= false; > + } > + else > + { > + opt_slave_retries_path= my_strdup(key_memory_Sys_var_charptr_value, > + default_slave_retries_path, MYF(0)); > + } > + } > + > + if (opt_slave_retries && > + slave_retries_file.open()) > + unireg_abort(1); > + } > #endif > > if (opt_bin_log) > diff --git a/sql/mysqld.h b/sql/mysqld.h > index 69cfc0cad56..5f1561ca10d 100644 > --- a/sql/mysqld.h > +++ b/sql/mysqld.h > @@ -822,6 +848,8 @@ enum options_mysqld > OPT_KEY_CACHE_CHANGED_BLOCKS_HASH_SIZE, > OPT_LOG_BASENAME, > OPT_LOG_ERROR, > + OPT_SLAVE_RETRIES, > + OPT_SLAVE_RETRIES_LOG, why do you need them? > OPT_LOWER_CASE_TABLE_NAMES, > OPT_PLUGIN_LOAD, > OPT_PLUGIN_LOAD_ADD, > diff --git a/sql/sql_show.cc b/sql/sql_show.cc > index d742bc6c1c3..c53ca0f0097 100644 > --- a/sql/sql_show.cc > +++ b/sql/sql_show.cc > @@ -10662,6 +10663,15 @@ char *thd_get_error_context_description(THD *thd, char *buffer, > /* Don't wait if LOCK_thd_data is used as this could cause a deadlock */ > if (!mysql_mutex_trylock(&thd->LOCK_thd_data)) > { > + if (thd->rgi_slave) is rgi_slave and its content protected by thd->LOCK_thd_data ? > + { > + rpl_group_info *rgi= thd->rgi_slave; > + len= my_snprintf(header, sizeof(header), > + ", GTID %u-%u-%llu", rgi->current_gtid.domain_id, > + rgi->current_gtid.server_id, rgi->current_gtid.seq_no); > + str.append(header, len); > + } > + > if (const char *info= thread_state_info(thd)) > { > str.append(' '); > diff --git a/sql/sys_vars.cc b/sql/sys_vars.cc > index ebb64b340b0..11c321f06dc 100644 > --- a/sql/sys_vars.cc > +++ b/sql/sys_vars.cc > @@ -5862,6 +5862,61 @@ Sys_slave_trans_retry_interval( > "slave_transaction_retry_errors", > GLOBAL_VAR(slave_trans_retry_interval), CMD_LINE(REQUIRED_ARG), > VALID_RANGE(0, 3600), DEFAULT(0), BLOCK_SIZE(1)); > + > +static bool update_slave_retries_log(sys_var *self, THD* thd, enum_var_type type) > +{ > + if (opt_slave_retries) > + { > + if (!slave_retries_file.get() && slave_retries_file.open()) > + { > + my_error(ER_CANT_OPEN_FILE, MYF(0), opt_slave_retries_path, errno); > + opt_slave_retries= false; > + return true; > + } > + } > + else > + slave_retries_file.close(); > + return false; > +} > + > +static Sys_var_mybool Sys_slave_retries_log( > + "log_slave_retries", "Log transaction retries", > + GLOBAL_VAR(opt_slave_retries), > + CMD_LINE(OPT_ARG, OPT_SLAVE_RETRIES), DEFAULT(false), > + NO_MUTEX_GUARD, NOT_IN_BINLOG, ON_CHECK(0), ON_UPDATE(update_slave_retries_log)); > + > + > +static bool check_slave_retries_file(sys_var *self, THD *thd, set_var *var) > +{ > + if (opt_slave_retries) > + { > + my_error(ER_NOT_SUPPORTED_YET, MYF(0), "modifying while log_slave_retries is ON"); as always, please don't hard-code error messages. you can use my_error(ER_OPTION_PREVENTS_STATEMENT, MYF(0), "log_slave_retries=ON"); > + return true; > + } > + return false; > +} > + > +static bool update_slave_retries_file(sys_var *self, THD* thd, enum_var_type type) > +{ > + if (!opt_slave_retries_path) > + opt_slave_retries_path= my_strdup(key_memory_Sys_var_charptr_value, > + default_slave_retries_path, MYF(0)); sys_var.cc can handle that for you, see PREALLOCATED in e.g. opt_slow_logname also, you don't reopen the log when its name changes, how comes? > + return false; > +} > + > +static Sys_var_charptr Sys_slave_retries_log_path( > + "log_slave_retries_file", "Log file path for transaction retries", > + GLOBAL_VAR(opt_slave_retries_path), > + CMD_LINE(REQUIRED_ARG, OPT_SLAVE_RETRIES_LOG), DEFAULT(NULL), > + NO_MUTEX_GUARD, NOT_IN_BINLOG, ON_CHECK(check_slave_retries_file), > + ON_UPDATE(update_slave_retries_file)); > + > + > +static Sys_var_uint Sys_slave_retries_log_max( > + "log_slave_retries_max", "Log max N transaction retries " > + "(default 0 means infinity)", just "(0 means infinity)". Default is printed separately > + GLOBAL_VAR(opt_slave_retries_max_log), CMD_LINE(REQUIRED_ARG), > + VALID_RANGE(0, UINT_MAX), DEFAULT(0), BLOCK_SIZE(1)); > #endif > > static bool check_locale(sys_var *self, THD *thd, set_var *var) > diff --git a/storage/innobase/lock/lock0lock.cc b/storage/innobase/lock/lock0lock.cc > index 0e9cef669d4..8a8eeaf39f6 100644 > --- a/storage/innobase/lock/lock0lock.cc > +++ b/storage/innobase/lock/lock0lock.cc > @@ -49,6 +49,7 @@ Created 5/7/1996 Heikki Tuuri > #include <debug_sync.h> > > #include <set> > +#include <stdio.h> I presume Marko has reviewed (or will review) InnoDB changes > > #ifdef WITH_WSREP > #include <mysql/service_wsrep.h> > diff --git a/mysql-test/main/mysqld--help.result b/mysql-test/main/mysqld--help.result > index 77d6d10b3e6..274c034d455 100644 > --- a/mysql-test/main/mysqld--help.result > +++ b/mysql-test/main/mysqld--help.result > @@ -1673,6 +1678,9 @@ log-isam myisam.log > log-output FILE > log-queries-not-using-indexes FALSE > log-short-format FALSE > +log-slave-retries FALSE > +log-slave-retries-file (No default value) this is another sign that log-slave-retries-file doesn't behave like other log filename variables. > +log-slave-retries-max 0 > log-slave-updates FALSE > log-slow-admin-statements TRUE > log-slow-disabled-statements sp Regards, Sergei Chief Architect, MariaDB Server and security(a)mariadb.org

1 0

Re: [PATCH] MDEV-32673 Handle InnoDB binlog position recovery for RESET MASTER or binlog name change
by Kristian Nielsen 07 Nov '23

07 Nov '23

Hi Marko, Thanks for the patch. It looks good, just a few small comments inline. Approved after fixing the check for !is_relay_log. I have pushed an extra commit on top of bb-10.4-MDEV-32673 with a testcase (feel free to squash it onto your commit). The tests pass with your patch (but fails without it). - Kristian. > diff --git a/sql/handler.h b/sql/handler.h > index 6085111bc25..134212a0ad7 100644 > --- a/sql/handler.h > +++ b/sql/handler.h > @@ -1438,6 +1438,7 @@ struct handlerton > enum_binlog_command binlog_command, > const char *query, uint query_length, > const char *db, const char *table_name); > + void (*reset_binlog_pos)(const char *name, uint64_t pos); I think there should be a comment describing what this new call does. Suggestion: /* Called at server startup or RESET MASTER when the new binlog is opened. Informs the storage engine of the change in binlog file name and position. */ > diff --git a/sql/log.cc b/sql/log.cc > index 59730c3205a..e46f3796b63 100644 > --- a/sql/log.cc > +++ b/sql/log.cc > @@ -4327,6 +4327,8 @@ bool MYSQL_BIN_LOG::reset_logs(THD *thd, bool create_new_log, > err: > if (error == 1) > name= const_cast<char*>(save_name); > + else > + ha_reset_binlog_pos(get_log_fname(), BIN_LOG_HEADER_SIZE); Sorry, I forgot something when we discussed this. This function is also used for resetting the relay logs. So it needs a check for !is_relay_log. For example: --- a/sql/log.cc +++ b/sql/log.cc @@ -4331,6 +4331,10 @@ bool MYSQL_BIN_LOG::reset_logs(THD *thd, bool create_new_log, if (!is_relay_log) { xid_count_per_binlog *b; + + if (!error) + ha_reset_binlog_pos(get_log_fname(), BIN_LOG_HEADER_SIZE); + /* Remove all entries in the xid_count list except the last. Normally we will just be deleting all the entries that we waited for to > diff --git a/storage/innobase/trx/trx0rseg.cc b/storage/innobase/trx/trx0rseg.cc > index be52373d03b..3ffa003588d 100644 > --- a/storage/innobase/trx/trx0rseg.cc > +++ b/storage/innobase/trx/trx0rseg.cc > @@ -826,3 +826,52 @@ trx_rseg_update_binlog_offset(byte* rseg_header, const trx_t* trx, mtr_t* mtr) > mlog_write_string(p, binlog_name, len, mtr); > } > } > + > +void trx_rseg_reset_binlog_pos(const char *name, uint64_t pos) > +{ > + mtr_t mtr; > + /* Invalidate the information on the TRX_SYS page. */ > + mtr.start(); > + if (buf_block_t *sys= trx_sysf_get(&mtr, true)) > + { > + byte *magic= TRX_SYS + TRX_SYS_MYSQL_LOG_INFO + > + TRX_SYS_MYSQL_LOG_MAGIC_N_FLD + sys->frame; > + if (mach_read_from_4(magic)) > + mlog_memset(magic, > + TRX_SYS_MYSQL_LOG_NAME + TRX_SYS_MYSQL_LOG_NAME_LEN, 0, > + &mtr); > + > + const uint32_t page_no= trx_sysf_rseg_get_page_no(sys, 0); > + if (page_no != FIL_NULL) > + { > + const uint32_t space_id= trx_sysf_rseg_get_space(sys, 0); > + ut_ad(space_id == TRX_SYS_SPACE); > + buf_block_t *block= buf_page_get(page_id_t(space_id, page_no), > + 0, RW_X_LATCH, &mtr); > + mlog_write_ull(TRX_RSEG + TRX_RSEG_BINLOG_OFFSET + block->frame, > + pos, &mtr); > + byte *p= TRX_RSEG + TRX_RSEG_BINLOG_NAME + block->frame; > + const size_t len= strlen(name) + 1; > + if (memcmp(name, p, len)) > + mlog_write_string(p, reinterpret_cast<const byte*>(name), len, &mtr); > + } > + > + /* Invalidate the information in the other rollback segment headers. */ > + for (ulint rseg_id= 1; rseg_id < TRX_SYS_N_RSEGS; rseg_id++) > + { > + const uint32_t page_no= trx_sysf_rseg_get_page_no(sys, rseg_id); > + if (page_no != FIL_NULL) > + { > + const uint32_t space_id= trx_sysf_rseg_get_space(sys, rseg_id); > + buf_block_t *block= buf_page_get(page_id_t(space_id, page_no), > + 0, RW_X_LATCH, &mtr); > + byte *name= TRX_RSEG + TRX_RSEG_BINLOG_NAME + block->frame; > + if (*name) > + mlog_write_ulint(name, 0, MLOG_1BYTE, &mtr); > + } > + } > + } > + mtr.commit(); > + /* Durably write the change. */ > + log_write_up_to(mtr.commit_lsn(), true); > +}

1 0

Re: 3aea3bd7422: MDEV-23294 Segfault or assertion upon MyISAM repair
by Sergei Golubchik 06 Nov '23

06 Nov '23

Hi, Aleksey, On Oct 23, Aleksey Midenkov wrote: > revision-id: 3aea3bd7422 (mariadb-10.4.30-176-g3aea3bd7422) > parent(s): 4fa6383c937 > author: Aleksey Midenkov > committer: Aleksey Midenkov > timestamp: 2023-09-26 20:20:46 +0300 > message: > > MDEV-23294 Segfault or assertion upon MyISAM repair > > When computing vcol expression some items use current_thd and that was > not set in MyISAM repair thread. Since all the repair threads belong > to one connection and items should not write into THD we can utilize > table THD for that. > > diff --git a/storage/myisam/ha_myisam.cc b/storage/myisam/ha_myisam.cc > index ddab8bcf741..27151548704 100644 > --- a/storage/myisam/ha_myisam.cc > +++ b/storage/myisam/ha_myisam.cc > @@ -714,6 +714,8 @@ static int compute_vcols(MI_INFO *info, uchar *record, int keynum) > /* This mutex is needed for parallel repair */ > mysql_mutex_lock(&info->s->intern_lock); > TABLE *table= (TABLE*)(info->external_ref); > + if (!current_thd) > + set_current_thd(table->in_use); why not to do it in thr_find_all_keys(), unconditionally? you'd avoid an if() executed per row. Also, I'd suggest a comment, like /* To evaluate vcols we must have current_thd set. This will set current_thd in all threads to the same THD, but it's safe, because vcols are always evaluated under info->s->intern_lock. */ set_current_thd((TABLE*)(info->external_ref)->in_use); > table->move_fields(table->field, record, table->field[0]->record_ptr()); > if (keynum == -1) // update all vcols > { Regards, Sergei Chief Architect, MariaDB Server and security(a)mariadb.org

2 3

Re: 0f0ef93c4d4: MDEV-32444 Data from orphaned XA transaction is lost after online alter
by Sergei Golubchik 02 Nov '23

02 Nov '23

Hi, Nikita, See below. A small suggestion for the test. And a rather larger comment about extending xid_t, as you, likely, expected :) On Oct 19, Nikita Malyavin wrote: > revision-id: 0f0ef93c4d4 (mariadb-11.2.1-15-g0f0ef93c4d4) > parent(s): 7f00853f2f1 > author: Nikita Malyavin > committer: Nikita Malyavin > timestamp: 2023-10-13 13:39:36 +0400 > diff --git a/mysql-test/main/alter_table_online_debug.result b/mysql-test/main/alter_table_online_debug.result > index 1ebb19965eb..154aec003ca 100644 > --- a/mysql-test/main/alter_table_online_debug.result > +++ b/mysql-test/main/alter_table_online_debug.result > @@ -1563,6 +1563,103 @@ connection default; > drop table t1, t2; > set @@binlog_format=default; > set debug_sync= reset; > +# MDEV-32444 Data from orphaned XA transaction is lost after online alter > +create table t (a int primary key) engine=innodb; > +insert into t values (1); > +# XA commit > +set debug_sync= 'alter_table_online_downgraded signal downgraded wait_for go'; > +alter table t force, algorithm=copy, lock=none; > +connection con1; > +set debug_sync= 'now wait_for downgraded'; > +xa begin 'x1'; > +update t set a = 2 where a = 1; > +xa end 'x1'; > +xa prepare 'x1'; > +disconnect con1; > +connection con2; > +xa commit 'x1'; > +set debug_sync= 'now signal go'; > +connection default; > +select * from t; > +a > +2 > +# XA rollback > +set debug_sync= 'alter_table_online_downgraded signal downgraded wait_for go'; > +alter table t force, algorithm=copy, lock=none; > +connect con1, localhost, root,,; > +xa begin 'x2'; > +insert into t values (53); > +xa end 'x2'; > +xa prepare 'x2'; > +disconnect con1; > +connection con2; > +xa rollback 'x2'; > +set debug_sync= 'now signal go'; > +connection default; > +select * from t; > +a > +2 > +# XA transaction is left uncommitted > +# end then is rollbacked after alter fails > +set debug_sync= 'alter_table_online_downgraded signal downgraded wait_for go'; > +set statement innodb_lock_wait_timeout=0, lock_wait_timeout= 0 > +for alter table t force, algorithm=copy, lock=none; > +connect con1, localhost, root,,; > +xa begin 'xuncommitted'; > +insert into t values (3); > +xa end 'xuncommitted'; > +xa prepare 'xuncommitted'; > +set debug_sync= 'now signal go'; > +disconnect con1; > +connection default; > +ERROR HY000: Lock wait timeout exceeded; try restarting transaction > +xa rollback 'xuncommitted'; > +select * from t; > +a > +2 > +# Same, but commit > +set debug_sync= 'alter_table_online_downgraded signal downgraded wait_for go'; > +set statement innodb_lock_wait_timeout=0, lock_wait_timeout= 0 > +for alter table t force, algorithm=copy, lock=none; instead of `alter table t force`, better do something that changes the table, so that you could see whether alter was executed or aborted. For example, you can change the column type int->bigint and back. not only here, but everywhere. Or add/drop column, this has an added benefit that you won't need a separate `show create table`, your already existing `select * from t` will show whether the extra column exists > +connect con1, localhost, root,,; > +xa begin 'committed_later'; > +insert into t values (3); > +xa end 'committed_later'; > +xa prepare 'committed_later'; > +set debug_sync= 'now signal go'; > +disconnect con1; > +connection default; > +ERROR HY000: Lock wait timeout exceeded; try restarting transaction > +xa commit 'committed_later'; > +select * from t; > +a > +2 > +3 > +# Commit, but error in statement, and there is some stmt data to rollback > +set debug_sync= 'alter_table_online_downgraded signal downgraded wait_for go'; > +alter table t force, algorithm=copy, lock=none; > +connect con1, localhost, root,,; > +set debug_sync= 'now wait_for downgraded'; > +xa begin 'x1'; > +insert into t values (4), (3); > +ERROR 23000: Duplicate entry '3' for key 'PRIMARY' > +insert into t values (5); > +xa end 'x1'; > +xa prepare 'x1'; > +disconnect con1; > +connection con2; > +xa commit 'x1'; > +set debug_sync= 'now signal go'; > +connection default; > +select * from t; > +a > +2 > +3 > +5 > +connect con1, localhost, root,,; > +connection default; > +drop table t; > +set debug_sync= reset; > disconnect con1; > disconnect con2; > # > diff --git a/sql/handler.h b/sql/handler.h > index 07b392c010a..4622e49f883 100644 > --- a/sql/handler.h > +++ b/sql/handler.h > @@ -903,6 +905,7 @@ struct xid_t { > long gtrid_length; > long bqual_length; > char data[XIDDATASIZE]; // not \0-terminated ! > + Online_alter_cache_list *online_alter_cache; well, no, we've talked about it. Even the comment above the structure says that you cannot change it. make a separate structure, xid_and_online_alter_cache_t or xid_t_internal or whatever. but xid_t should be defined identically everywhere. For example: https://github.com/berkeleydb/libdb/blob/master/src/dbinc/xa.h === okay, now I've seen how you used it and I'd say it's _such a rare use case_ that you can store your online_alter_cache in the XID_cache_element and look it up there. Or even in a separate data structure, that will be empty most of the time anyway. > > xid_t() = default; /* Remove gcc warning */ > bool eq(struct xid_t *xid) const > diff --git a/sql/online_alter.cc b/sql/online_alter.cc > index 8b8c81319d1..19abe57914d 100644 > --- a/sql/online_alter.cc > +++ b/sql/online_alter.cc > @@ -334,14 +336,57 @@ static int online_alter_log_init(void *p) > online_alter_hton->savepoint_rollback_can_release_mdl= > [](handlerton *hton, THD *thd){ return true; }; > > - online_alter_hton->commit= [](handlerton *hton, THD *thd, bool all) > - { return online_alter_end_trans(hton, thd, all, true); }; > - online_alter_hton->rollback= [](handlerton *hton, THD *thd, bool all) > - { return online_alter_end_trans(hton, thd, all, false); }; > - online_alter_hton->commit_by_xid= [](handlerton *hton, XID *xid) > - { return online_alter_end_trans(hton, current_thd, true, true); }; > - online_alter_hton->rollback_by_xid= [](handlerton *hton, XID *xid) > - { return online_alter_end_trans(hton, current_thd, true, false); }; > + online_alter_hton->commit= [](handlerton *hton, THD *thd, bool all) -> int > + { > + int res= online_alter_end_trans(get_cache_list(hton, thd), thd, > + ending_trans(thd, all), true); > + cleanup_tables(thd); > + return res; > + }; > + online_alter_hton->rollback= [](handlerton *hton, THD *thd, bool all) -> int > + { > + int res= online_alter_end_trans(get_cache_list(hton, thd), thd, > + ending_trans(thd, all), false); > + cleanup_tables(thd); > + return res; > + }; > + > + > + online_alter_hton->recover= [](handlerton*, XID*, uint){ return 0; }; > + online_alter_hton->prepare= [](handlerton *hton, THD *thd, bool all) -> int > + { > + auto &cache_list= get_cache_list(hton, thd); > + int res= 0; > + if (ending_trans(thd, all)) > + { > + thd->transaction->xid_state.set_online_alter_cache(&cache_list); > + thd_set_ha_data(thd, hton, NULL); > + } > + else > + { > + res= online_alter_end_trans(cache_list, thd, false, true); is that possible? > + } > + > + cleanup_tables(thd); > + return res; > + }; > + online_alter_hton->commit_by_xid= [](handlerton *hton, XID *xid) -> int > + { > + int res= online_alter_end_trans(*xid->online_alter_cache, current_thd, > + true, true); > + delete xid->online_alter_cache; > + xid->online_alter_cache= NULL; > + return res; > + }; > + online_alter_hton->rollback_by_xid= [](handlerton *hton, XID *xid) -> int > + { > + int res= online_alter_end_trans(*xid->online_alter_cache, current_thd, > + true, false); > + delete xid->online_alter_cache; > + xid->online_alter_cache= NULL; > + return res; > + }; > + > > online_alter_hton->drop_table= [](handlerton *, const char*) { return -1; }; > online_alter_hton->flags= HTON_NOT_USER_SELECTABLE | HTON_HIDDEN Regards, Sergei Chief Architect, MariaDB Server and security(a)mariadb.org

2 6

Re: 2d937b62c33: MDEV-27744 InnoDB: Failing assertion: !cursor->index->is_committed() in row0ins.cc (from row_ins_sec_index_entry_by_modify) | Assertion `0' failed in row_upd_sec_index_entry (debug) | Corruption
by Sergei Golubchik 02 Nov '23

02 Nov '23

Hi, Alexander, Few questions and suggestions below. Nothing major. On Oct 27, Alexander Barkov wrote: > commit 2d937b62c33 > Author: Alexander Barkov <bar(a)mariadb.com> > Date: Mon Apr 4 14:50:21 2022 +0400 > > MDEV-27744 InnoDB: Failing assertion: !cursor->index->is_committed() in row0ins.cc (from row_ins_sec_index_entry_by_modify) | Assertion `0' failed in row_upd_sec_index_entry (debug) | Corruption > would it make sense to change the MDEV title in Jira to better describe the problem? I sometimes do it with my bugs. Like "LPAD in vcol created in ORACLE mode makes table corrupted in non-ORACLE" (I tried to make it short, appropriate for a title) and, of course, change it in the comment and in the test file to match. > The crash happened with an indexed virtual column whose > value is evaluated using a function that has a different meaning > in sql_mode='' vs sql_mode=ORACLE: > > diff --git a/mysql-test/suite/compat/oracle/r/func_concat.result b/mysql-test/suite/compat/oracle/r/func_concat.result > index 392d579707a..17ca4be078a 100644 > --- a/mysql-test/suite/compat/oracle/r/func_concat.result > +++ b/mysql-test/suite/compat/oracle/r/func_concat.result > @@ -211,14 +211,14 @@ SET sql_mode=ORACLE; > CREATE VIEW v1 AS SELECT 'foo'||NULL||'bar' AS test; > SHOW CREATE VIEW v1; > View Create View character_set_client collation_connection > -v1 CREATE VIEW "v1" AS select concat_operator_oracle(concat_operator_oracle('foo',NULL),'bar') AS "test" latin1 latin1_swedish_ci > +v1 CREATE VIEW "v1" AS select concat(concat('foo',NULL),'bar') AS "test" latin1 latin1_swedish_ci > SELECT * FROM v1; > test > foobar > SET sql_mode=DEFAULT; > SHOW CREATE VIEW v1; > View Create View character_set_client collation_connection > -v1 CREATE ALGORITHM=UNDEFINED DEFINER=`root`@`localhost` SQL SECURITY DEFINER VIEW `v1` AS select concat_operator_oracle(concat_operator_oracle('foo',NULL),'bar') AS `test` latin1 latin1_swedish_ci > +v1 CREATE ALGORITHM=UNDEFINED DEFINER=`root`@`localhost` SQL SECURITY DEFINER VIEW `v1` AS select oracle_schema.concat(oracle_schema.concat('foo',NULL),'bar') AS `test` latin1 latin1_swedish_ci please, add a mysqldump test. Like, create a table with virtual columns, check, defaults in the default sql mode. and create a table in oracle mode. Also, a view in the default mode and a view in oracle mode. and then mysqldump, to see that it dumps and restores everything correctly may be stored routines/triggers/etc, if you'd like, but they aren't directly relevant to this MDEV, as far as I understand. > SELECT * FROM v1; > test > foobar > diff --git a/mysql-test/suite/compat/oracle/r/func_decode.result b/mysql-test/suite/compat/oracle/r/func_decode.result > index 2809e971be3..1870a1ec0d5 100644 > --- a/mysql-test/suite/compat/oracle/r/func_decode.result > +++ b/mysql-test/suite/compat/oracle/r/func_decode.result > @@ -1,8 +1,8 @@ > SET sql_mode=ORACLE; > SELECT DECODE(10); > -ERROR 42000: Incorrect parameter count in the call to native function 'DECODE' > +ERROR 42000: Incorrect parameter count in the call to native function 'oracle_schema.DECODE' Hmm, may be this should say DECODE as before? you know, falls under the case "if you don't change sql_mode back and forth, you won't see schema-qualified names" > SELECT DECODE(10,10); > -ERROR 42000: Incorrect parameter count in the call to native function 'DECODE' > +ERROR 42000: Incorrect parameter count in the call to native function 'oracle_schema.DECODE' > SELECT DECODE(10,10,'x10'); > DECODE(10,10,'x10') > x10 > diff --git a/mysql-test/suite/compat/oracle/r/vcol_innodb.result b/mysql-test/suite/compat/oracle/r/vcol_innodb.result > new file mode 100644 > index 00000000000..9fa97c75c10 > --- /dev/null > +++ b/mysql-test/suite/compat/oracle/r/vcol_innodb.result > @@ -0,0 +1,51 @@ > +SET @table_open_cache=@@GLOBAL.table_open_cache; why do you need to manipulate table_open_cache? to trigger a reopen? Just do flush tables, it's explicit, more readable and more... controllable. > +SET sql_mode=''; > +CREATE TABLE t (d INT,b VARCHAR(1),c CHAR(1),g CHAR(1) GENERATED ALWAYS AS (SUBSTR(b,0,0)) VIRTUAL,PRIMARY KEY(b),KEY g(g)) ENGINE=InnoDB; > +INSERT INTO t VALUES (0); > +ERROR 21S01: Column count doesn't match value count at row 1 > +SET sql_mode='ORACLE'; > +INSERT INTO t SET c=REPEAT (1,0); > +Warnings: > +Warning 1364 Field 'b' doesn't have a default value > +ALTER TABLE t CHANGE COLUMN a b INT; > diff --git a/sql/item_func.h b/sql/item_func.h > index 76a997c33fb..cdbefb82541 100644 > --- a/sql/item_func.h > +++ b/sql/item_func.h > @@ -56,8 +56,40 @@ class Item_func :public Item_func_or_sum, > bool check_argument_types_can_return_date(uint start, uint end) const; > bool check_argument_types_can_return_time(uint start, uint end) const; > void print_cast_temporal(String *str, enum_query_type query_type); > + > + void print_schema_qualified_name(String *to, > + const LEX_CSTRING &schema_name, > + const char *function_name) const I don't see why you'd need this helper. is it something that was used in earlier versions of the patch? > + { > + // e.g. oracle_schema.func() > + to->append(schema_name); > + to->append('.'); > + to->append(function_name); > + } > + > + void print_sql_mode_qualified_name(String *to, > + enum_query_type query_type, > + const char *function_name) const > + { > + const Schema *func_schema= schema(); > + if (!func_schema || func_schema == Schema::find_implied(current_thd)) > + to->append(function_name); > + else > + print_schema_qualified_name(to, func_schema->name(), function_name); > + } > + > + void print_sql_mode_qualified_name(String *to, enum_query_type query_type) > + const > + { > + return print_sql_mode_qualified_name(to, query_type, func_name()); > + } I don't see why you need this helper either, you never use print_sql_mode_qualified_name with the last argument being not func_name(). So you can remove this helper and the third argument of print_sql_mode_qualified_name. > + > public: > > + // Print an error message for a builtin-schema qualified function call > + static void wrong_param_count_error(const LEX_CSTRING &schema_name, > + const LEX_CSTRING &func_name); > + > table_map not_null_tables_cache; > > enum Functype { UNKNOWN_FUNC,EQ_FUNC,EQUAL_FUNC,NE_FUNC,LT_FUNC,LE_FUNC, > diff --git a/sql/item_strfunc.cc b/sql/item_strfunc.cc > index ae078dbb22f..92d5e196da4 100644 > --- a/sql/item_strfunc.cc > +++ b/sql/item_strfunc.cc > @@ -2170,13 +2170,31 @@ bool Item_func_trim::fix_length_and_dec() > > void Item_func_trim::print(String *str, enum_query_type query_type) > { > + LEX_CSTRING suffix= {STRING_WITH_LEN("_oracle")}; > if (arg_count == 1) > { > - Item_func::print(str, query_type); > + if (query_type & QT_FOR_FRM) > + { > + // 10.3 downgrade compatibility for FRM > + str->append(func_name()); > + if (schema() == &oracle_schema_ref) > + str->append(suffix); > + } > + else > + print_sql_mode_qualified_name(str, query_type, func_name()); > + print_args_parenthesized(str, query_type); > return; > } > - str->append(Item_func_trim::func_name()); > - str->append(func_name_ext()); > + > + if (query_type & QT_FOR_FRM) > + { > + // 10.3 downgrade compatibility for FRM > + str->append(Item_func_trim::func_name()); > + if (schema() == &oracle_schema_ref) > + str->append(suffix); > + } > + else > + print_sql_mode_qualified_name(str, query_type, Item_func_trim::func_name()); it'd be simpler if you move the above block that prints the function name before if (arg_count == 1) also you won't need suffix, but can do like in all other functions str->append(STRING_WITH_LEN("trim_oracle"); > str->append('('); > str->append(mode_name()); > str->append(' '); > diff --git a/sql/sql_lex.cc b/sql/sql_lex.cc > index 71f592a3852..bb53d1a510a 100644 > --- a/sql/sql_lex.cc > +++ b/sql/sql_lex.cc > @@ -2084,7 +2100,64 @@ bool Lex_input_stream::get_7bit_or_8bit_ident(THD *thd, uchar *last_char) > } > > > -int Lex_input_stream::scan_ident_sysvar(THD *thd, Lex_ident_cli_st *str) > +/* > + Resolve special SQL functions that have a qualified syntax in sql_yacc.yy. > + These functions are not listed in the native function registry > + because of a special syntax, or a reserved keyword: > + > + mariadb_schema.SUBSTRING('a' FROM 1 FOR 2) -- Special syntax I didn't find it in Oracle's manual, by the way > + mariadb_schema.TRIM(BOTH ' ' FROM 'a') -- Special syntax > + mariadb_schema.REPLACE('a','b','c') -- Verb keyword > +*/ > + > +int Lex_input_stream::find_keyword_qualified_special_func(Lex_ident_cli_st *str, > + uint length) const > +{ > + /* > + There are many other special functions, see the following grammar rules: > + function_call_keyword > + function_call_nonkeyword > + Here we resolve only those that have a qualified syntax to handle > + different behavior in different @@sql_mode settings. > + > + Other special functions do not work in qualified context: > + SELECT mariadb_schema.year(now()); -- Function year is not defined > + SELECT mariadb_schema.now(); -- Function now is not defined > + > + We don't resolve TRIM_ORACLE here, because it does not have > + a qualified syntax yet. Search for "trim_operands" in sql_yacc.yy > + to find more comments. > + */ > diff --git a/sql/sql_schema.h b/sql/sql_schema.h > index 1174bc7a83f..2c52646f2ea 100644 > --- a/sql/sql_schema.h > +++ b/sql/sql_schema.h > @@ -77,5 +98,6 @@ class Schema > > > extern Schema mariadb_schema; > +extern const Schema &oracle_schema_ref; What's the difference between these two definitions. Do you expect someone will need to change mariadb_schema? > > #endif // SQL_SCHEMA_H_INCLUDED Regards, Sergei Chief Architect, MariaDB Server and security(a)mariadb.org

2 2

Eliminating sprintf deprecation warnings (maybe others as well)
by Stein Vidar Hagfors Haugan 27 Oct '23

27 Oct '23

Hi! First time here, so my apologies if I'm violating some spoken or unspoken rules :) I'm developing/maintaining two specialized storage engines(*) and I have one issue: There are numerous uses of sprintf() in the general mariadb codebase, and each one generates a warning during compilation when doing a plain cmake/make build (on MacOS & RHEL7 at least). I am a bit fanatical about hunting down and eliminating warnings if at all possible, no matter how small and insignificant - if you become accustomed to routinely getting warnings during compilation, you can easily overlook an important one. So I am considering to contribute with a patch that will eliminate all sprintf() instances by converting them to snprintf(destination, N, ...). Of course, hunting down the correct N for each instance can be a *lot* of work (I presume this is why this hasn't been done already). So I'm proposing to make a macro UNSAFE_SNPRINTF(destination, ...) which would expand to snprintf(destination, 10000, ...) and replace all current sprintf() calls with that. Yes, 10000 is a ridiculously high number that makes it look like we're introducing a huge security risk, but but the current sprintf() calls have an implicit N = infinity. In some instances, the compiler sniffs out the actual size of the buffer and complains - in that case I would change the code to an appropriate regular snprintf() call. Would doing this be worthwile? I.e., is there a good chance that such a patch would actually be accepted into the codebase? If so, I might also do the same with other warnings (I believe there are some type-punning warnings at least). Sincerely, Stein Haugan *) The engines are for internal use so far, but might make them public at some point

2 2