- developers - lists.mariadb.org

Re: 424ee6eac78: MDEV-32837 long unique does not work like unique key when using replace
by Sergei Golubchik 11 Jan '24

11 Jan '24

Hi, Alexander, On Jan 11, Alexander Barkov wrote: > revision-id: 424ee6eac78 (mariadb-10.5.23-57-g424ee6eac78) > parent(s): c9902a20b3a > author: Alexander Barkov > committer: Alexander Barkov > timestamp: 2024-01-10 15:35:25 +0400 > message: > > MDEV-32837 long unique does not work like unique key when using replace > > diff --git a/sql/sql_insert.cc b/sql/sql_insert.cc > index ec9e379768f..edc2be207f3 100644 > --- a/sql/sql_insert.cc > +++ b/sql/sql_insert.cc > @@ -2065,7 +2065,8 @@ int write_record(THD *thd, TABLE *table, COPY_INFO *info, select_result *sink) > we just should not expose this fact to users by invoking > ON UPDATE triggers. > */ > - if (last_uniq_key(table,key_nr) && > + if ((!table->s->long_unique_table || table->versioned()) && > + last_uniq_key(table,key_nr) && > !table->file->referenced_by_foreign_key() && > (!table->triggers || !table->triggers->has_delete_triggers())) > { 1. why table->versioned() ? 2. use show status in your test for handler_update/delete to see what REPLACE actually did 3. technically, you can keep the optimization if the long unique is the last *and there are no in-engine uniques*, this can be checked with && !(table->key_info->flags & HA_NOSAME) 4. but here's the problem that last_uniq_key() doesn't see long uniques, so it'll treat every one of them as the last. So it should be something like bool is_long_unique= table->s->key_info[key_nr].flags & HA_UNIQUE_HASH; if ((is_long_unique ? last_uniq_key(table, table->s->key_info, key_nr) && !(table->key_info->flags & HA_NOSAME) : last_uniq_key(table, table->key_info, key_nr)) && !table->file->referenced_by_foreign_key() && (!table->triggers || !table->triggers->has_delete_triggers())) I suspect this will correctly use the UPDATE optimization in all cases (only long unique / mix of uniques)x(last / not last key conflicting) Regards, Sergei Chief Architect, MariaDB Server and security(a)mariadb.org

1 0

Re: 014167880d2: MDEV-32559 ensure spider alter table init queries are executed after ddl recovery
by Sergei Golubchik 10 Jan '24

10 Jan '24

Hi, Yuchen, On Jan 05, Yuchen Pei wrote: > revision-id: 014167880d2 (mariadb-10.10.4-52-g014167880d2) > parent(s): 21822625aa6 > author: Yuchen Pei > committer: Yuchen Pei > timestamp: 2023-11-08 13:10:38 +1100 > message: > > MDEV-32559 ensure spider alter table init queries are executed after ddl recovery > > If spider was initialised in init_server_components(), i.e. with > --plugin-load-add, then all alter table init queries are to be > executed in the signal_ddl_recovery_done() handlerton callback. Since > this is part of the spider plugin init, we update the callback > signature to return an int, and deinit the plugin when it fails the > callback. > > diff --git a/sql/mysqld.cc b/sql/mysqld.cc > index a3971a00a59..a922562d043 100644 > --- a/sql/mysqld.cc > +++ b/sql/mysqld.cc > @@ -5159,6 +5159,14 @@ static int init_server_components() > if (!opt_abort && ddl_log_initialize()) > unireg_abort(1); > > + /* Plugin initialisation done here should defer any ALTER TABLE > + queries to after the ddl recovery is done, in the > + signal_ddl_recovery_done() callback called by > + ha_signal_ddl_recovery_done(). As such, between the plugin_init() > + call and the ha_signal_ddl_recovery_done() call below only things > + related to prepare for recovery should be done and nothing else, and > + definitely not anything assuming that all plugins have been > + initialised. */ Better put this comment in plugin.h near the (*init)(void*) declaration. This is part of the (*init)() callback documentation it should be there, not in some random .cc file that plugin authors shouldn't be required to look at. > if (plugin_init(&remaining_argc, remaining_argv, > (opt_noacl ? PLUGIN_INIT_SKIP_PLUGIN_TABLE : 0) | > (opt_abort ? PLUGIN_INIT_SKIP_INITIALIZATION : 0))) > diff --git a/sql/sql_plugin.cc b/sql/sql_plugin.cc > index 3576730723e..85a8f14a6da 100644 > --- a/sql/sql_plugin.cc > +++ b/sql/sql_plugin.cc > @@ -2553,9 +2554,15 @@ bool plugin_foreach_with_mask(THD *thd, plugin_foreach_func *func, > > for (idx= 0; idx < total; idx++) > { > - /* It will stop iterating on first engine error when "func" returns TRUE */ > + /* It will stop iterating on first engine error or mark DELETED if > + reap_on_fail is true when "func" returns TRUE. */ > if ((res= func(thd, plugins[idx], arg))) > + { > + if (reap_on_fail) > + plugin_ref_to_int(plugins[idx])->state= PLUGIN_IS_DELETED; may be you don't need a new argument for plugin_foreach_with_mask, and can put this logic into the `func` callback? That is into static my_bool signal_ddl_recovery_done(THD *, plugin_ref plugin, void *) { handlerton *hton= plugin_hton(plugin); if (hton->signal_ddl_recovery_done) if ((hton->signal_ddl_recovery_done)(hton)) plugin_ref_to_int(plugin)->state= PLUGIN_IS_DELETED; return 0; } or something like that. > + else > break; > + } > } > > plugin_unlock_list(0, plugins, total); > diff --git a/storage/spider/spd_table.cc b/storage/spider/spd_table.cc > index a9782a8a822..e50942d449f 100644 > --- a/storage/spider/spd_table.cc > +++ b/storage/spider/spd_table.cc > @@ -6376,15 +6376,13 @@ bool spider_init_system_tables() > DBUG_RETURN(TRUE); > } > > - const int size= sizeof(spider_init_queries) / sizeof(spider_init_queries[0]); > - for (int i= 0; i < size; i++) > + for (uint i= 0; i < size; i++) > { > - const LEX_STRING *query= &spider_init_queries[i]; > + const LEX_STRING *query= &queries[i]; I don't see much of a point, why not to run *all* queries from spider_ddl_recovery_done() ? > if (mysql_real_query(mysql, query->str, query->length)) > { > - fprintf(stderr, > - "[ERROR] SPIDER plugin initialization failed at '%s' by '%s'\n", > - query->str, mysql_error(mysql)); > + sql_print_error("SPIDER: plugin initialization failed at '%s' by '%s'\n", > + query->str, mysql_error(mysql)); > > mysql_close(mysql); > DBUG_RETURN(TRUE); > @@ -6667,10 +6665,20 @@ int spider_db_init( > spider_udf_table_mon_list_hash[roop_count].array.size_of_element); > } > > - if (spider_init_system_tables()) > - { > + if (spider_init_system_tables( > + spider_init_queries, > + sizeof(spider_init_queries) / sizeof(spider_init_queries[0]))) > + goto error_system_table_creation; > + > + /* If plugin_init() has already been called, it implies this > + function is called after the ddl recovery is done, thus we can > + safely execute ALTER TABLE init queries. Otherwise it is deferred to > + the spider_ddl_recovery_done() callback. */ > + if (plugins_are_initialized && > + spider_init_system_tables(spider_init_alter_table_queries, > + sizeof(spider_init_alter_table_queries) / > + sizeof(spider_init_alter_table_queries[0]))) Don't do that, instead, please, take the "ddl_recovery_done" change from the commit 0930eb86cb00edb2a - it makes the storage engine API simpler, the engine doesn't need to implement two code paths and try to detect whether it's loaded at run-time or at startup. > goto error_system_table_creation; > - } > > if (!(spider_table_sts_threads = (SPIDER_THREAD *) > spider_bulk_malloc(NULL, 256, MYF(MY_WME | MY_ZEROFILL), > Regards, Sergei Chief Architect, MariaDB Server and security(a)mariadb.org

2 1

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