November 2023 - developers - lists.mariadb.org

[Maria-developers] MariaDB // Temporal tables
by Robert Palm 02 Oct '24

02 Oct '24

2 2

PR#2779: Fix merge commit 5ea5291: No test file or result files should be executable
by Otto Kekäläinen 27 Jan '24

27 Jan '24

Hi Sanja! Did you notice my PR https://github.com/MariaDB/server/pull/2779? I tagged you about a minor bug in you previous merge. I was hoping you would notice it and review that PR, but Daniel Black jumped on it. I am asking now because it seems the exact same thing happened for mysql-test/main/lowercase_table2.result in your commit fecd78b8378 from November 8th, so I thought I'd send an extra email now to ensure you are aware of this happening. - Otto

2 3

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: 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 20 Nov '23

20 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 12 Nov '23

12 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 09 Nov '23

09 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