March 2024 - developers - lists.mariadb.org

Re: b657bc75f90: MDEV-25829 Change default collation to utf8mb4_1400_ai_ci
by Sergei Golubchik 13 Mar '24

13 Mar '24

Hi, Alexander, On Mar 08, Alexander Barkov wrote: > revision-id: b657bc75f90 (mariadb-11.0.1-262-gb657bc75f90) > parent(s): 8e5f41b3453 > author: Alexander Barkov > committer: Alexander Barkov > timestamp: 2023-11-15 10:58:03 +0400 > message: > > MDEV-25829 Change default collation to utf8mb4_1400_ai_ci > > Step#2 - Adding a new collation derivation level for CAST and CONVERT. > > Now character string cast functions: > - CAST(string_expr AS CHAR) > - CONVERT(expr USING charset_name) > > have a new collation derivation level between: > > - string literals > - utf8 metadata functions, e.g. user() and database() > > Before the change these cast functions had collation derivation equal > to table columns, which caused more illegal mix of collation conflicts. > > Note, binary string cast functions: > - CAST(string_expr AS BINARY) > - CONVERT(expr USING binary) > did not change their collation derivation. Why not? Regards, Sergei Chief Architect, MariaDB Server and security(a)mariadb.org

2 1

Re: 8e5f41b3453: MDEV-25829 Change default collation to utf8mb4_1400_ai_ci
by Sergei Golubchik 13 Mar '24

13 Mar '24

Hi, Alexander, On Mar 08, Alexander Barkov wrote: > revision-id: 8e5f41b3453 (mariadb-11.0.1-261-g8e5f41b3453) > parent(s): 9e457cbe501 > author: Alexander Barkov > committer: Alexander Barkov > timestamp: 2023-11-15 09:37:26 +0400 > message: > > MDEV-25829 Change default collation to utf8mb4_1400_ai_ci > > Step#1 - Changing collation derivation for string user variables > from EXPLICIT to COERCIBLE. from IMPLICIT, oherwise ok to push. and I still think user variable could remember the derivation. It's definitely not a part of MDEV-25829, but if you agree that it should, let's create an MDEV for that. Regards, Sergei Chief Architect, MariaDB Server and security(a)mariadb.org

2 1

Re: [PATCH 0/2] Suggestion for smaller fix to XA parallel replication performance, MDEV-31949
by Kristian Nielsen 12 Mar '24

12 Mar '24

Andrei Elkin <andrei.elkin(a)mariadb.com> writes: > I reviewed your branch to agree with the idea of xid conflicts > handling and its implementation. > > I however could not understand the need of the refactoring part. > In order to track the xid dependency couple functions and and > rpl_parallel_entry::maybe_active_xid > a sort of a sliding window - that the 2nd commit of your branch > introduced must be sufficient to my analysis. > Of course it needs to hold worker indexes of active xid:s. > > This observation led me to create a review branch > > origin/review__knielsen_xa_sched_minimal_fix > > [where origin git@github.com:MariaDB/server.git] > which is a somewhat light elaboration over the 2nd commit of your branch. The reason for the refactoring part is to allow to distribute the transactions as evenly as possible over the worker threads, given the scheduling constraints imposed by XA dependencies. Let's say we have 5 worker threads, and transactions T1..T15. If there are no scheduling constraints, we can schedule evenly like this: W1: T1 T6 T11 W2: T2 T7 T12 W3: T3 T8 T13 W4: T4 T9 T14 W5: T5 T10 T15 Now suppose we need to schedule T6 on the same worker as T4, and T8 on the same worker as T3. The most even way to distribute the transactions is now this: W1: T1 T7 T12 W2: T2 T9 T14 W3: T3 T8 T13 W4: T4 T6 T11 W5: T5 T10 T15 You see, we try to always have 5 consecutive transactions T_i, T_{i+1}, ..., T+{i+4} scheduled on 5 different worker threads, except when this is not possible due to dependency restrictions. But this most-even scheduling requires to change the scheduling order of threads from the original sequential 1,2,3,4,5. You see, the last transactions T11..T15 are scheduled on worker threads in order: W4, W1, W3, W2, W5. The refactoring patch introduces thread_sched_fifo to hold the current scheduling order of worker threads. Whenever we have to schedule out-of-order due to a scheduling dependency, we put the scheduled worker at the end of this fifo, to preserve even scheduling for the next N transactions. In the original scheduling code, we never scheduled workers out-of-order, so the scheduling order was always cyclic W1, W2, W3, W4, W5, W1, ..., and a simple incrementing counter was sufficient. But this is no longer sufficient when out-of-order scheduling occurs. In the example, if we use a simple counter and try to schedule on worker ((i+1) mod N) after worker (i), then we get the following uneven scheduling: W1: T1 T11 W2: T2 T12 W3: T3 T8 T13 W4: T4 T6 T9 T14 W5: T5 T7 T10 T15 Because T6 and T8 have particular scheduling requirements, they cause the scheduling to skip workers. The result is that W4 and W5 get too many transactions, while W1 and W2 get too few, and parallelism is reduced. I hope this explain the reasoning. > Could you please have a look at it? If I understand your patch correctly, it schedules using a simple incrementing counter when there are no dependency requirements, and so would suffer from the uneven scheduling in the above example. It also looks like there's a mistake in the code: > + if ((idx= check_xa_xid_dependency(&gtid_ev->xid)) < (uint32) -1) > + { > + /* > + A previously scheduled event group with the same XID might still be > + active in a worker, so schedule this event group in the same worker > + to avoid a conflict. > + */ > + } > + else > + { > + /* Record this XID now active. */ > + xid_active_generation *a= > + (xid_active_generation *)alloc_dynamic(&maybe_active_xid); > + if (!a) > + return NULL; > + ++idx; > + if (idx >= rpl_thread_max) > + idx= 0; If check_xa_xid_dependency() returns "no dependency", then idx is set to (uint32)-1 and the else {...} branch is executed. This does idx++, which increments the (uint32)-1 to 0. So it looks to me that _all_ transactions are scheduled on worker 0, or am I missing something? This is a simple mistake, it's an easy fix to not assign idx when check_xa_xid_dependency() returns "no dependency". But the code would still suffer from uneven scheduling as in the example, IIUC. So there's a good reason to introduce the thread_sched_fifo as done in my refactoring patch. This though still leaves the limitation in my minimal patch that XA PREPARE xid1 cannot group commit with XA COMMIT xid1. The impact of this will depend on how many transactions on average appear between an XA PREPARE and its associated XA COMMIT, as well as on how expensive fsync() is relative to the cost of each transaction. - Kristian.

1 0

Re: 9e5d4dfc49b: MDEV-23729 MDEV-32218 INFORMATION_SCHEMA table for user login data
by Sergei Golubchik 11 Mar '24

11 Mar '24

Hi, Nikita, On Mar 10, Nikita Malyavin wrote: > revision-id: 9e5d4dfc49b (mariadb-11.4.1-10-g9e5d4dfc49b) > parent(s): 929c2e06aae > author: Nikita Malyavin > committer: Nikita Malyavin > timestamp: 2024-02-29 17:24:27 +0100 > message: > > MDEV-23729 MDEV-32218 INFORMATION_SCHEMA table for user login data > > * A new table INFORMATION_SCHEMA.LOGON is introduced. There were many cases where we lack an INFORMATION_SCHEMA with all user accounts. Forcing users to select from mysql.user is a shame. Let's use the chance and create a table USERS. It doesn't need more columns now than what you've created, but we'll likely create more in the future. > * Upon idea, it stores auxiliary user data related to login/security/resources > * An unprivileged user can access their own data, and that is the main > difference with what mysql.global_priv provides exactly! > * The fields are currently: USER, WRONG_PASSWORD_ATTEMPTS, EXPIRATION_TIME I wanted to write about splitting USER into USER and HOST, but indeed it's just USER everywhere else in INFORMATION_SCHEMA (e.g. DEFINER and GRANTEE columns). And functions like USER() and CURRENT_USER() don't split either. So, you're right, let's consistently use user@host everywhere. > diff --git a/mysql-test/main/information_schema_stats.result b/mysql-test/main/information_schema_stats.result > index 352bcbab823..e38788872fa 100644 > --- a/mysql-test/main/information_schema_stats.result > +++ b/mysql-test/main/information_schema_stats.result ... > +connect(localhost,naughty_user,wrong_passwd,test,16000,/home/nik/mariadb/bld/mysql-test/var/tmp/mysqld.1.sock); here and below - you forgot to replace the path with MASTER_MYSOCK > diff --git a/mysql-test/main/information_schema_stats.test b/mysql-test/main/information_schema_stats.test > index fd5171c3fb4..dbcabe45965 100644 > --- a/mysql-test/main/information_schema_stats.test > +++ b/mysql-test/main/information_schema_stats.test > @@ -47,3 +47,69 @@ select * from information_schema.index_statistics where table_schema='test' and > select * from information_schema.table_statistics where table_schema='test' and table_name='just_a_test'; > set global userstat=@save_userstat; > --enable_ps2_protocol > + > +--echo # > +--echo # MDEV-23729 INFORMATION_SCHEMA Table info. about user locked due to > +--echo # max_password_errors > +--echo # > +--echo # MDEV-32218 message to notify end-user N-days prior the password get > +--echo # expired > +--echo # I don't see how you test "info about user locked due to max_password_errors". This is the reason for implementing this new table, it has to be tested. At least add select * from information_schema.logon where wrong_password_attemps >= @max_password_errors; and show it it's empty at first and then not empty. > diff --git a/sql/sql_acl.cc b/sql/sql_acl.cc > index 14450a5a610..4d199bf0e61 100644 > --- a/sql/sql_acl.cc > +++ b/sql/sql_acl.cc > @@ -12956,6 +12956,87 @@ int fill_schema_column_privileges(THD *thd, TABLE_LIST *tables, COND *cond) > #endif > } > > +namespace Show > +{ > + ST_FIELD_INFO users_fields_info[] = > + { > + Column("USER", Userhost(), NOT_NULL), > + Column("WRONG_PASSWORD_ATTEMPTS", SLonglong(), NULLABLE), @@max_password_errors is documented as "If there is more than this number of failed connect attempts due to invalid password, user will be blocked from further connections until FLUSH_PRIVILEGES" Let's use call incorrect password consistently everywhere. It could be "bad password", "wrong password", "incorrect password", "invalid password", but it should be the same everywhere. I personally think that "invalid" implies some kind if validity check, e.g. not shorter than 10 characters, not equal to the username, etc and a password that doesn't satisfy these validity rules is "invalid". Trying to login with an incorrect password is a different kind of error, a password can be valid but wrong. May be, ask Ian and/or Daniel about it? And then don't forget to update @@max_password_errors help text to match. Also, you don't have a test case for FLUSH PRIVILEGES. > + Column("EXPIRATION_TIME", SLonglong(), NULLABLE), PASSWORD_EXPIRATION_TIME, the user account itself does not expire. > + CEnd() > + }; > +}; > + > +static bool ignore_max_password_errors(const ACL_USER *acl_user); > + > +static int fill_logon_schema_record(THD *thd, TABLE * table, ACL_USER *user) > +{ > + ulonglong lifetime= user->password_lifetime < 0 > + ? default_password_lifetime > + : user->password_lifetime; > + > + bool ignore_password_errors= ignore_max_password_errors(user); why? I think it's still useful to show how many wrong password attempts were there for an account even if it doesn't get blocked. > + bool ignore_expiration_date= lifetime == 0; > + > + /* Skip user if nothing to show */ > + if (ignore_password_errors && ignore_expiration_date) > + return 0; > + > + Grantee_str grantee(user->user.str, safe_str(user->host.hostname)); > + table->field[0]->store(grantee, strlen(grantee), system_charset_info); > + if (ignore_password_errors) > + { > + table->field[1]->set_null(); > + } > + else > + { > + table->field[1]->set_notnull(); > + table->field[1]->store(user->password_errors); > + } > + if (ignore_expiration_date) > + { > + table->field[2]->set_null(); > + } > + else > + { > + table->field[2]->set_notnull(); > + table->field[2]->store(user->password_last_changed > + + user->password_lifetime * 3600 * 24, true); I think it'd be more generally useful to show password_last_changed and expiration period separately. > + } > + > + return schema_table_store_record(thd, table); > +} > + > +int fill_logon_schema_table(THD *thd, TABLE_LIST *tables, COND *cond) > +{ > + int res= 0; > +#ifndef NO_EMBEDDED_ACCESS_CHECKS > + bool see_whole_table= check_global_access(thd, PROCESS_ACL, true) == 0; I don't think PROCESS_ACL is a very logical choice here. And there's nothing better as far as I can see. May be let's just do if (check_access(thd, SELECT_ACL, "mysql", NULL, NULL, 1, 1)) ? There are many checks like that in e.g. sql_parse.cc > + > + TABLE *table= tables->table; > + > + if (!see_whole_table) > + { > + mysql_mutex_lock(&acl_cache->lock); > + ACL_USER *cur_user= find_user_exact(thd->security_ctx->priv_host, > + thd->security_ctx->priv_user); 1. cur_user can be NULL if someone dropped it while there was an active connection for this user 2. add a test for it > + > + res= fill_logon_schema_record(thd, table, cur_user); > + mysql_mutex_unlock(&acl_cache->lock); > + return res; > + } > + > + mysql_mutex_lock(&acl_cache->lock); > + for (size_t i= 0; res == 0 && i < acl_users.elements; i++) > + { > + ACL_USER *user= dynamic_element(&acl_users, i, ACL_USER*); > + res= fill_logon_schema_record(thd, table, user); > + } > + mysql_mutex_unlock(&acl_cache->lock); > +#endif > + return res; > +} > + Regards, Sergei Chief Architect, MariaDB Server and security(a)mariadb.org

2 2

Backup MariaDB Databases for Suprema BioStar 2 Door Access System
by Turritopsis Dohrnii Teo En Ming 09 Mar '24

09 Mar '24

Subject: Backup MariaDB Databases for Suprema BioStar 2 Door Access System Good day from Singapore, On 7 March 2024 Thursday, when I was installing new self-signed SSL certificate for the door access system for a law firm in Singapore, I notice that Suprema BioStar 2 also uses the open source MariaDB database server. If I did not remember wrongly, there are 3 databases. However, the entire platform is a Windows environment, not Linux. How can I backup all the databases in the MariaDB database server? Thank you. Regards, Mr. Turritopsis Dohrnii Teo En Ming Targeted Individual in Singapore Blogs: https://tdtemcerts.blogspot.com https://tdtemcerts.wordpress.com GIMP also stands for Government-Induced Medical Problems.

1 0

Re: be66e975ecf: MDEV-31340 Remove MY_COLLATION_HANDLER::strcasecmp()
by Sergei Golubchik 08 Mar '24

08 Mar '24

Hi, Alexander, Ok to push, thanks for the great refactoring! I've looked at the diff of this and previous commit excluding InnoDB. On Mar 08, Alexander Barkov wrote: > revision-id: be66e975ecf (mariadb-11.0.1-282-gbe66e975ecf) > parent(s): c0c1c80346b > author: Alexander Barkov > committer: Alexander Barkov > timestamp: 2024-01-19 11:04:02 +0400 > message: > > MDEV-31340 Remove MY_COLLATION_HANDLER::strcasecmp() > Regards, Sergei Chief Architect, MariaDB Server and security(a)mariadb.org

1 0

64-bit time_t support in MariaDB?
by Otto Kekäläinen 07 Mar '24

07 Mar '24

Hello! Does MariaDB already fully support 64-bit time_t (the year 2038 problem)? MariaDB 10.11 was today uploaded to Debian experimental without any 64-bit time_t support changes, just a library name change[1] to test that it works with the Debian 64-bit time_t tooling as Debian is currently undergoing a transition to it[2]. So far MariaDB built successfully and passed the main MTR suite on 11 architectures[3]. Monty mentioned that some work is in progress, and indeed I found his commits in two branches [4,5] but looking at the commits and their link to buildbot it seems they are not passing the CI. I also found one MDEV[6] to make TIMESTAMP use the whole 32-bit unsigned range. Are there any other efforts in progress? Does MTR main suite include tests for 64-bit time support? Should MTR automatically currently fail if toolchain time is 64-bit? [1] https://salsa.debian.org/mariadb-team/mariadb-server/-/commit/063109a306a01… [2] https://lists.debian.org/debian-devel-announce/2024/02/msg00000.html [3] https://buildd.debian.org/status/package.php?p=mariadb&suite=experimental [4] https://github.com/MariaDB/server/commits/bb-11.4-timestamp/ [5] https://github.com/MariaDB/server/commits/bb-11.4-monty/ [6] https://jira.mariadb.org/browse/MDEV-32188

2 6

Re: [PATCH 2/2] MDEV-33551: Semi-sync Wait Point AFTER_COMMIT Slow on Workloads with Heavy Concurrency
by Kristian Nielsen 06 Mar '24

06 Mar '24

> commit c51636f254703602f6f6e2e4a260e607e737b9c1 (origin/10.6-MDEV-33551) > Author: Brandon Nesterenko <brandon.nesterenko(a)mariadb.com> > Date: Mon Feb 26 07:01:47 2024 -0700 > > MDEV-33551: Semi-sync Wait Point AFTER_COMMIT Slow on Workloads with Heavy Concurrency First some overall comments. The approach of waking up each thread on its own condition seems good to me. Monty had a different suggestion, which is to have only a single ack sent per group commit; this could also be a possibility, but it seems orthogonal to your fix (ie. both could make sense). Monty also had the idea to move the AFTER_COMMIT sync point into the group commit code, though that changes the semantics of AFTER_COMMIT somewhat, especially with --innodb-flush-log-at-trx-commit=3 (or non-InnoDB storage engine). So in this review I'll focus on keeping your overall approach the same. Your patch introduces a new wait_queue with the list of waiting threads. But there is already a very similar queue, m_active_tranxs. It looks to me as if the m_active_tranx could easily be extended so it can be used for this, and avoid introducing another queue. Your wait_queue is pushed in write_tranx_in_binlog() which already does m_active_tranxs->insert_tranx_node(), and it is popped in report_reply_binlog() which also does m_active_tranxs->clear_active_tranx_nodes(). So add some info to struct Tranx_node, maybe a THD * is all that is needed? A bit of care must be taken about lifetime of Tranx_node and THD respectively (in case one goes away but not the other). But the whole m_active_tranxs is already protected by a lock, and there is a hash for looking up specific position, so I think a THD that wants to abort its wait can simply clear the THD * from its Tranx_node or something like that to ensure the semisync ack will not try to wake up a THD that no longer exists. You'll need to see how this will affect the code for shutdown (await_all_slave_replies()) where it needs to wait for all acks (IIUC). But I hope we can be pragmatic here and find something simple that works. Another general thing related to this: > 3) Repl_semi_sync_master::commit_trx() no longer loops to await > its specific ACK. It waits once, and will either fail from > timeout, or receive its ACK. We still need the loop to wait. From `man pthread_cond_wait`: "When using condition variables there is always a Boolean predicate in‐ volving shared variables associated with each condition wait that is true if the thread should proceed. Spurious wakeups from the pthread_cond_timedwait() or pthread_cond_wait() functions may occur. Since the return from pthread_cond_timedwait() or pthread_cond_wait() does not imply anything about the value of this predicate, the predi‐ cate should be re-evaluated upon such return." This affects the code in the patch in some places, detailed comments below. > 2) The time when thd::is_awaiting_semi_sync_ack is set is moved > to at binlogging time, to ensure transactions which have been > binlogged and queued up to await an ACK are not killed, > and are still waited on. I don't understand this. Why shouldn't a session waiting for ACK be killable? On the contrary, such a wait can take potentially a long time, seems important that it can be killed? I don't fully understand the existing code for this, maybe the patch is correct and I'm just looking for an explanation why it is correct like this. Or maybe this is a limitation of the existing code, and your patch doesn't change it (so having the ability to kill a waiting thread could be added in a different patch)? > diff --git a/mysql-test/suite/rpl/t/rpl_semi_sync_cond_var_per_thd.test b/mysql-test/suite/rpl/t/rpl_semi_sync_cond_var_per_thd.test > index f8fa0a99d9c..ebe15a4ca32 100644 > --- a/mysql-test/suite/rpl/t/rpl_semi_sync_cond_var_per_thd.test > +++ b/mysql-test/suite/rpl/t/rpl_semi_sync_cond_var_per_thd.test > @@ -22,6 +22,7 @@ > --source include/master-slave.inc > > --connection master > +call mtr.add_suppression("Got an error reading communication packets"); Is this suppression needed? If so, why? > diff --git a/sql/mysqld.cc b/sql/mysqld.cc > index e224871795e..b315edc091c 100644 > --- a/sql/mysqld.cc > +++ b/sql/mysqld.cc > @@ -1750,18 +1750,12 @@ static void close_connections(void) > /* > If we are waiting on any ACKs, delay killing the thread until either an ACK > is received or the timeout is hit. > - while (waiting_threads-- > 0) > - repl_semisync_master.await_slave_reply(); > + repl_semisync_master.await_all_slave_replies(); Just curious here, why did you change this code? Was it just because now there is no general COND_binlog_send to wait on? Or was it fix a bug not directly related to the main issue? I'm asking because if it makes things simpler, I think it's fine to keep the COND_binlog_send and signal it in addition to the individual thread-specific condition (in fact your patch doesn't seem to remove it though it looks as if it's no longer used). On the other hand, this old code in close_connections() that simply calls await_slave_reply() N times (N = sync_get_master_wait_sessions()) looks quite wrong to me, looping for a specific number of times makes no sense. So fixing it is good in any case. > diff --git a/sql/semisync_master.cc b/sql/semisync_master.cc > index 0eaf0f0e0e2..f421e924a61 100644 > --- a/sql/semisync_master.cc > +++ b/sql/semisync_master.cc > @@ -502,16 +502,33 @@ void Repl_semi_sync_master::unlock() > > void Repl_semi_sync_master::cond_broadcast() > { > - mysql_cond_broadcast(&COND_binlog_send); > + while (!wait_queue.empty()) > + { > + semisync_wait_trx_t next_waiter= wait_queue.front(); Maybe this function should now have a better name than cond_broadcast() (wakeup_ready_tranx() or something?). But this code will probably change in any case if the wait_queue is folded into m_active_tranxs. > -int Repl_semi_sync_master::cond_timewait(struct timespec *wait_time) > +int Repl_semi_sync_master::cond_timewait(THD *thd, struct timespec *wait_time) I think this function now makes little sense, clearer just to call directly mysql_cond_wait(thd->COND_wakeup_ready) at the caller, what do you think? > @@ -695,12 +712,36 @@ int Repl_semi_sync_master::report_reply_binlog(uint32 server_id, > + lock(); > + while (!wait_queue.empty()) > + { > + semisync_wait_trx_t next_waiter= wait_queue.front(); > > - cond_broadcast(); > + cmp= Active_tranx::compare(m_reply_file_name, m_reply_file_pos, > + next_waiter.binlog_name, next_waiter.binlog_pos); > + if (cmp >= 0) > + { > + DBUG_PRINT("semisync", ("%s: signal thread %llu.", > + "Repl_semi_sync_master::report_reply_binlog", > + next_waiter.thd->thread_id)); > + mysql_cond_signal(&next_waiter.thd->COND_wakeup_ready); > + wait_queue.pop(); This code is very similar to Active_tranx::clear_active_tranx_nodes(), so hopefully the two can be integrated and wait_queue becomes unnecessary. > @@ -779,7 +821,20 @@ int Repl_semi_sync_master::report_binlog_update(THD* thd, const char *log_file, > strcpy(log_info->log_file, log_file + dirname_length(log_file)); > log_info->log_pos = log_pos; > > - return write_tranx_in_binlog(log_info->log_file, log_pos); > + /* > + THD arg depends on wait point mode. If after storage engine commit, the > + individual connection threads will perform the wait for semi-sync ACKt, > + thd is the thread of the user connection thread. > + If it is after binlog sync, the binlog leader thread will perform the > + semi-sync waits on behalf of the grouped transaction (which at this > + point, we (current_thd) are the leader). If using binlog_group_commit, > + thd is the thread of the user connection thread. > + */ > + thd_to_cond_wait= ((wait_point() == SEMI_SYNC_MASTER_WAIT_POINT_AFTER_STORAGE_COMMIT) > + ? thd > + : current_thd); This condition is complex. It's good that you added the detailed comment explaining what's going on, but I think it would be better to avoid it completely. Why not just pass in the value of thd_to_cond_wait explicitly from the caller? As an aside, we have this code in trx_group_commit_leader(): for (current= queue; current != NULL; current= current->next) { current->error= repl_semisync_master.wait_after_sync(current->cache_mngr-> last_commit_pos_file, current->cache_mngr-> last_commit_pos_offset); It seems silly to wait for every binlog position to be acked her one by one, why not just wait for the last one? repl_semisync_master.wait_after_sync(last_in_queue->cache_mngr->last_commit_pos_file, ...); But while related, it's perhaps not directly part of the problem you're addressing in your patch, so up to you if you want to include this or leave it for a possible later patch. > @@ -852,12 +907,11 @@ int Repl_semi_sync_master::commit_trx(const char* trx_wait_binlog_name, > - if (!get_master_enabled() || !is_on()) > + if (!get_master_enabled() || !is_on() || thd_killed(thd)) > goto l_end; > - while (is_on() && !thd_killed(thd)) > - { > - /* We have to check these again as things may have changed */ > - if (!rpl_semi_sync_master_clients && !rpl_semi_sync_master_wait_no_slave) > - { > - aborted= 1; > - break; > - } As described earlier, because of the (at least theoretical) possibility of spurious wakeup, we should keep the while loop here and the checks. > @@ -1253,6 +1294,8 @@ int Repl_semi_sync_master::write_tranx_in_binlog(const char* log_file_name, > } > else > { > + wait_queue.push({log_file_name, log_file_pos, thd}); Hopefully this can be instead folded into m_active_tranxs->insert_tranx_node() which is called just a bit below. > -void Repl_semi_sync_master::await_slave_reply() > +void Repl_semi_sync_master::await_all_slave_replies() > + while (TRUE) > + { > + lock(); > + if (wait_queue.empty() || !get_master_enabled() || !is_on()) > + { > + unlock(); > + break; > + } > + front= wait_queue.front(); > create_timeout(&abstime, NULL); > - cond_timewait(&abstime); > - > -end: > + wait_result= cond_timewait(front.thd, &abstime); I'm wondering, is it safe here to wait on front.thd->COND_wakeup_ready? Isn't there a possibility that this might go away during the wait? A simple way to avoid this could be just to keep the original global COND_binlog_send and wait for that here (and signal both that and thd->COND_wakeup_ready when waking up). IIUC, this code will only run in a single thread during shutdown, so probably fine to have some extra unnecessary wakeups here. > diff --git a/sql/semisync_master.h b/sql/semisync_master.h > index 99f46869354..6de0333d4ce 100644 > --- a/sql/semisync_master.h > +++ b/sql/semisync_master.h > @@ -21,6 +21,7 @@ > +/* > + Element in Repl_semi_sync_master::wait_queue to preserve the state of a > + transaction waiting for an ACK. > +*/ > +typedef struct _semisync_wait_trx { > + const char *binlog_name; > + my_off_t binlog_pos; > + THD *thd; > +} semisync_wait_trx_t; > + std::queue<semisync_wait_trx_t> wait_queue; Let's see if we can avoid this, and use the existing m_active_tranxs queue instead. diff --git a/sql/semisync_master.cc b/sql/semisync_master.cc index 8cc721e5737..0eaf0f0e0e2 100644 --- a/sql/semisync_master.cc +++ b/sql/semisync_master.cc @@ -979,6 +979,14 @@ int Repl_semi_sync_master::commit_trx(const char* trx_wait_binlog_name, { rpl_semi_sync_master_trx_wait_num++; rpl_semi_sync_master_trx_wait_time += wait_time; + /* + Assert we have either recieved our ACK; or have timed out and are + awoken in an off state. + */ + DBUG_ASSERT(!get_master_enabled() || !is_on() || thd->is_killed() || + 0 <= Active_tranx::compare( + m_reply_file_name, m_reply_file_pos, + trx_wait_binlog_name, trx_wait_binlog_pos)); } } } I think it's a bit much to crash the server here (even if only debug build), because of the possibility of spurious wakeups. It's good to have the test case though, maybe put the assertion inside DBUG_EXECUTE_IF()? I'd make it just log an error in the error log which the test case could flag (to avoid crashing), but that's a matter of style I guess and up to you. - Kristian.

2 2

Re: 95f2a07d323: MDEV-33582 Add more warnings to be able to better diagnose network issues
by Sergei Golubchik 05 Mar '24

05 Mar '24

Hi, Michael, On Mar 04, Michael Widenius wrote: > revision-id: 95f2a07d323 (mariadb-10.6.17-30-g95f2a07d323) > parent(s): 33dcf8155b4 > author: Michael Widenius > committer: Michael Widenius > timestamp: 2024-03-04 14:52:03 +0200 > message: > > MDEV-33582 Add more warnings to be able to better diagnose network issues > > Warnings are added to net_server.cc when > global_system_variables.log_warnings >= 4. > > When the above condition holds then: > - All communication errors from net_serv.cc is also written to the > general log. "to the error log" > - In case of a of not being able to read or write a packet, a more detailed "of a of" ? > error is given. > > Other things: > - vio_close() is now first marking the socket closed before closing it. > The reason for this is to ensure that the connection that gets a read > error can check if the reason was that the socket was closed. > - Added option to NET to avoid calling my_error() when there is no data > to be read on socket. This is only used in semi sync master to speed > up handling zero size packages that seams to happen from time to time. "seems" I don't understand why it happens and whether it's normal or not, and if normal, why it's normal only for semisync and not normal otherwise. > - Add a new state to vio to be able to detect if vio is acive, shutdown or > closed. > - Testing of the new warnings is done in rpl_get_lock.test > - Suppress some of the new warnings in mtr to allow one to run some of > the tests with -mysqld=--log-warnings=4. All test in the 'rpl' suite > can now be run with this option. > - Ensure that global.log_warnings are restored at test end in a way > that allows one to use mtr --mysqld=--log-warnings=4. > > diff --git a/mysql-test/mariadb-test-run.pl b/mysql-test/mariadb-test-run.pl > index 4b3d323760d..eabfe1171cc 100755 > --- a/mysql-test/mariadb-test-run.pl > +++ b/mysql-test/mariadb-test-run.pl > @@ -4486,6 +4486,11 @@ sub extract_warning_lines ($$) { > qr/Slave I\/0: Master command COM_BINLOG_DUMP failed/, > qr/Error reading packet/, > qr/Lost connection to MariaDB server at 'reading initial communication packet'/, > + qr/Could not read packet:.* state: [2-3] /, > + qr/Could not read packet:.* errno: 104 /, > + qr/Could not write packet:.* errno: 32 /, > + qr/Could not write packet:.* errno: 104 /, > + qr/Connection was killed/, why are these errors "normal" and can be ignored? > qr/Failed on request_dump/, > qr/Slave: Can't drop database.* database doesn't exist/, > qr/Slave: Operation DROP USER failed for 'create_rout_db'/, > diff --git a/mysql-test/suite/rpl/include/rpl_extra_col_master.test b/mysql-test/suite/rpl/include/rpl_extra_col_master.test > index 02645a577f5..512118f91cd 100644 > --- a/mysql-test/suite/rpl/include/rpl_extra_col_master.test > +++ b/mysql-test/suite/rpl/include/rpl_extra_col_master.test > @@ -59,6 +59,10 @@ > #VARCHAR(M) > # > > +--disable_query_log > +call mtr.add_suppression("Could not read packet:.* errno: 11"); > +--enable_query_log error 11? "Resource temporarily unavailable"? What does it mean in this context? > + > --let $_saved_conn= $CURRENT_CONNECTION > > let $binformat = `SHOW VARIABLES LIKE '%binlog_format%'`; > diff --git a/sql/semisync_master_ack_receiver.cc b/sql/semisync_master_ack_receiver.cc > index 9320c7fc3e6..1bb462ef934 100644 > --- a/sql/semisync_master_ack_receiver.cc > +++ b/sql/semisync_master_ack_receiver.cc > @@ -256,6 +256,12 @@ void Ack_receiver::run() > thd->security_ctx->skip_grants(); > thd->set_command(COM_DAEMON); > init_net(&net, net_buff, REPLY_MESSAGE_MAX_LENGTH); > + /* > + Don't generate error message in case of no data on socket. Will speed > + up my_net_read() slightly in case of NET_READ_INTERRUPTED. > + */ > + net.no_errors_for_no_data= 1; do you mean, it's for performance reasons? > + Regards, Sergei Chief Architect, MariaDB Server and security(a)mariadb.org

2 1

Re: ea9a05f0821: This commit is about improving repair of tables
by Sergei Golubchik 04 Mar '24

04 Mar '24

Hi, Michael, First, instead of situationally fixing repair code here and there, let's take a step back and see how repair works. I've walked the whole REPAIR code line by line, including all storage engines, and that's a simplified description: * if USE_FRM was specified * < FRM_VER_TRUE_VARCHAR(≈2005) && has varchar -> "too old" * if not MyISAM or Aria or CSV -> silently ignore USE_FRM * rename/truncate/rename * if ALGORITHM=COPY is needed (and not FORCE/USE_FRM), use it * otherwise, per engine: * MyISAM run repair, QUICK or EXTENDED, or QUICK then EXTENDED * Aria, same as MyISAM * S3 -> "Table '%-.192s' is read only" * CSV crashes * Archive, perform OPTIMIZE * all other engines * if needs upgrade: use ALTER ALGORITHM=COPY * otherwise "The storage engine for the table doesn't support REPAIR" * if ALGORITHM=COPY is needed but wasn't used -> "Operation failed" I think it's too complex to explain and not user friendly. Note that MyISAM internally can try one repair method and if that fails fall back to another one. So does Aria. Why cannot the server do the same? So, I'd suggest the following logic instead: * if USE_FRM - do USE_FRM stuff (or an warning) * if marked corrupted or EXTENDED or doesn't need upgrade - run repair * if repair isn't supported = ALTER IGNORE unless needs upgrade * if needs upgrade - run upgrade = ALTER IGNORE COPY * if fails because of corruption - go to step 2 if repair wasn't run yet here USE_FRM is handled as before (except that it adds a warning if USE_FRM isn't supported by the engine). After that the server runs engine's repair if the table is marked corrupted or if EXTENDED was used (so no need for a new FORCE syntax). There is no error "engine does not support REPAIR", the server falls back to ALTER automatically in that case. If the data need to be upgraded and ALTER fails, the server tries repair and then ALTER again - just like MyISAM and Aria try different repair methods. Also, note that ALTER should always use IGNORE to delete duplicates in unique after collation of hash changes - that's what repair in MyISAM/Aria does. Anyway, some detailed comments to your patch are inline, please, see below. On Feb 21, Michael Widenius wrote: > commit ea9a05f0821 > Author: Michael Widenius <monty(a)mariadb.org> > Date: Mon Jan 29 11:52:44 2024 +0200 > > This commit is about improving repair of tables now as there is an MDEV, it should be here in the subject line > > The 3 ways to repair a table are: > 1) ALTER TABLE table_name FORCE" (not other options). > As an alias we allow: "ALTER TABLE table_name ENGINE=original_engine" > 2) "REPAIR TABLE" (without FORCE) > 3) "OPTIMIZE TABLE" > > All of the above commands will optimize row space usage (which means that > space will be needed to hold a temporary copy of the table) and > re-genereate all indexes. They will also try to replicate the original > table definition as exact as possible. > > For ALTER TABLE and "REPAIR TABLE without FORCE", the following will hold: > If the table is from an older MariaDB version and data conversion is > needed (for example for old type HASH columns, MySQL JSON type or new > TIMESTAMP format) "ALTER TABLE table_name FORCE, algorithm=COPY" will be > used. > > The differences between the algoritms are > 1) Will use the fastest algorithm the engine supports to do a full repair > of the table (except if data conversions are is needed). > 2) Will use the storage engine internal REPAIR facility (MyISAM, Aria). > If the engine does not support REPAIR then > "ALTER TABLE FORCE, ALGORITHM=COPY" will be used. > If there was data incompatibilites (which means that FORCE was used) > then there will be a warning after REPAIR that ALTER TABLE FORCE is > still needed. > The reason for this is that REPAIR may be able to go around data > errors (wrong incompatible data, crashed or unreadable sectors) that > ALTER TABLE cannot do. > 3) Will use the storage engine internal OPTIMIZE. If engine does not > support optimize, then "ALTER TABLE FORCE" is used. > > The above will ensure that ALTER TABLE FORCE is able to > correct almost any errors in the row or index data. In case of > corrupted blocks then REPAIR possibile followed by ALTER TABLE is needed. > This is important as mariadb-upgrade executes ALTER TABLE table_name > FORCE for any table that must be re-created. > > Bugs fixed with InnoDB tables: > - No error for INNODB_DEFAULT_ROW_FORMAT=COMPACT even if row length > would be too wide. (Independent of innodb_strict_mode). > - Tables using symlinks will be symlinked after any of the above commands > (Independent of the setting of --symbolic-links) > > If one specifies an algorithm together with ALTER TABLE FORCE, things > will work as before (except if data conversion is required as then > the COPY algoritm is enforced). > > ALTER TABLE .. OPTIMIZE ALL PARTITIONS will work as before. > > Other things: > - FORCE argument added to REPAIR to allow one to first run internal > repair to fix damaged blocks and then follow it with ALTER TABLE. > - REPAIR will not update frm_version if ha_check_for_upgrade() finds > that table is still incompatible with current version. In this case the > REPAIR will end with an error. doesn't look like "the REPAIR will end with an error" > - Default error messages length for %M increased from 128 to 256 to not > cut information from REPAIR. > - Documented HA_ADMIN_XX variables related to repair. > - Added HA_ADMIN_NEEDS_DATA_CONVERSION to signal that we have to > do data conversions when converting the table (and thus ALTER TABLE > copy algorithm is needed). > > diff --git a/include/my_handler_errors.h b/include/my_handler_errors.h > index df414888907..06dde5b3f74 100644 > --- a/include/my_handler_errors.h > +++ b/include/my_handler_errors.h > @@ -101,7 +101,7 @@ static const char *handler_error_messages[]= > "Operation was interrupted by end user (probably kill command?)", > "Disk full", > /* 190 */ > - "Incompatible key or row definition between the MariaDB .frm file and the information in the storage engine. You may have retry or dump and restore the table to fix this", > + "Incompatible key or row definition between the MariaDB .frm file and the information in the storage engine. You can try REPAIR TABLE ... USE_FRM possible followed by ALTER TABLE ... FORCE or dump and restore the table to fix this", 1. typo. "possibly" 2. please, don't recommend USE_FRM, it's a last resort lossy method, designed only for the case when MYI file is lost or corrupted. 3. REPAIR USE_FRM is likey broken for CSV, but I couldn't test it, because REPAIR crashes on CSV tables with or without USE_FRM. > "Too many words in a FTS phrase or proximity search", > "Table encrypted but decryption failed. This could be because correct encryption management plugin is not loaded, used encryption key is not available or encryption method does not match.", > "Foreign key cascade delete/update exceeds max depth", > diff --git a/mysql-test/main/ctype_utf8_def_upgrade.result b/mysql-test/main/ctype_utf8_def_upgrade.result > index d30f8670536..e7c21b76058 100644 > --- a/mysql-test/main/ctype_utf8_def_upgrade.result > +++ b/mysql-test/main/ctype_utf8_def_upgrade.result > @@ -10,24 +10,39 @@ SELECT @@character_set_database; > latin1 > SET @@character_set_database="latin1"; > SELECT COUNT(*) FROM t1; > -ERROR HY000: Got error 190 "Incompatible key or row definition between the MariaDB .frm file and the information in the storage engine. You may have retry " from storage engine MyISAM > +ERROR HY000: Got error 190 "Incompatible key or row definition between the MariaDB .frm file and the information in the storage engine. You can try REPAIR TABLE ... USE_FRM possible followed by ALTER TABLE ... FORCE or dump and restore the table to fix this" from storage engine MyISAM > +ALTER TABLE t1 FORCE; > +ERROR HY000: Got error 190 "Incompatible key or row definition between the MariaDB .frm file and the information in the storage engine. You can try REPAIR TABLE ... USE_FRM possible followed by ALTER TABLE ... FORCE or dump and restore the table to fix this" from storage engine MyISAM > CHECK TABLE t1; > Table Op Msg_type Msg_text > -test.t1 check Error Got error 190 "Incompatible key or row definition between the MariaDB .frm file and the information in the storage engine. You may have retry " from storage engine MyISAM > +test.t1 check Error Got error 190 "Incompatible key or row definition between the MariaDB .frm file and the information in the storage engine. You can try REPAIR TABLE ... USE_FRM possible followed by ALTER TABLE ... FORCE or dump and restore the table to fix this" from storage engine MyISAM > test.t1 check error Corrupt > REPAIR TABLE t1; > Table Op Msg_type Msg_text > -test.t1 repair Error Got error 190 "Incompatible key or row definition between the MariaDB .frm file and the information in the storage engine. You may have retry " from storage engine MyISAM > +test.t1 repair Error Got error 190 "Incompatible key or row definition between the MariaDB .frm file and the information in the storage engine. You can try REPAIR TABLE ... USE_FRM possible followed by ALTER TABLE ... FORCE or dump and restore the table to fix this" from storage engine MyISAM > +test.t1 repair error Corrupt > +REPAIR TABLE t1 FORCE; > +Table Op Msg_type Msg_text > +test.t1 repair Warning Can't open table > test.t1 repair error Corrupt > REPAIR TABLE t1 USE_FRM; > Table Op Msg_type Msg_text > -test.t1 repair status OK > +test.t1 repair note Table data recovered > +t1 repair error Table rebuild required. Please do "ALTER TABLE `test.t1` FORCE" or dump/reload to fix it! better to avoid exclamation marks in the error messages! > +test.t1 repair status Operation failed > SELECT COUNT(*) FROM t1; > COUNT(*) > 0 > CHECK TABLE t1; > Table Op Msg_type Msg_text > +test.t1 check error Table rebuild required. Please do "ALTER TABLE `t1` FORCE" or dump/reload to fix it! > +ALTER TABLE t1 FORCE; > +CHECK TABLE t1; > +Table Op Msg_type Msg_text > test.t1 check status OK > +SELECT COUNT(*) FROM t1; > +COUNT(*) > +0 > SHOW CREATE TABLE t1; > Table Create Table > t1 CREATE TABLE `t1` ( > diff --git a/sql/handler.h b/sql/handler.h > index fe272d841d9..f429cdacc6f 100644 > --- a/sql/handler.h > +++ b/sql/handler.h > @@ -2249,6 +2267,8 @@ struct Table_scope_and_contents_source_pod_st // For trivial members > enum_stats_auto_recalc stats_auto_recalc; > bool varchar; ///< 1 if table has a VARCHAR > bool sequence; // If SEQUENCE=1 was used > + /* True if we are using OPTIMIZE TABLE, REPAIR TABLE or ALTER TABLE FORCE */ it'd be good to clarify here /* this is used to suppress certain warnings that are only used for CREATE/ALTER TABLE */ > + bool recreate_identical_table; > > List<Virtual_column_info> *check_constraint_list; > > diff --git a/sql/sql_admin.cc b/sql/sql_admin.cc > index fcbd8a55c15..d4eabd7c34a 100644 > --- a/sql/sql_admin.cc > +++ b/sql/sql_admin.cc > @@ -48,7 +48,8 @@ const LEX_CSTRING msg_optimize= { STRING_WITH_LEN("optimize") }; > /* Prepare, run and cleanup for mysql_recreate_table() */ > > static bool admin_recreate_table(THD *thd, TABLE_LIST *table_list, > - Recreate_info *recreate_info) > + Recreate_info *recreate_info, > + bool partition_admin, bool table_copy) you don't need to pass partition_admin as an argument, if it's always thd->lex->alter_info.partition_flags & ALTER_PARTITION_ADMIN > { > bool result_code; > DBUG_ENTER("admin_recreate_table"); > @@ -859,20 +867,29 @@ static bool mysql_admin_table(THD* thd, TABLE_LIST* tables, > /* purecov: end */ > } > > - if (operator_func == &handler::ha_repair && > - !(check_opt->sql_flags & TT_USEFRM)) > + if (operator_func == &handler::ha_repair) > { > handler *file= table->table->file; > int check_old_types= file->check_old_types(); > int check_for_upgrade= file->ha_check_for_upgrade(check_opt); why check_old_types is not part of ha_check_for_upgrade? they're always used together, as far as I can see. > + require_data_conversion= > + (check_old_types == HA_ADMIN_NEEDS_DATA_CONVERSION || > + check_for_upgrade == HA_ADMIN_NEEDS_DATA_CONVERSION); > + require_alter_table= > + (check_old_types == HA_ADMIN_NEEDS_ALTER || > + check_for_upgrade == HA_ADMIN_NEEDS_ALTER); > > - if (check_old_types == HA_ADMIN_NEEDS_ALTER || > - check_for_upgrade == HA_ADMIN_NEEDS_ALTER) > + if (!(check_opt->sql_flags & (TT_USEFRM | TT_FORCE))) > + { > + if (require_data_conversion || require_alter_table) > { > /* We use extra_open_options to be able to open crashed tables */ > thd->open_options|= extra_open_options; > - result_code= admin_recreate_table(thd, table, &recreate_info) ? > + result_code= admin_recreate_table(thd, table, &recreate_info, > + (lex->alter_info.partition_flags & > + ALTER_PARTITION_ADMIN), 1) ? > HA_ADMIN_FAILED : HA_ADMIN_OK; > + recreate_used= 1; > thd->open_options&= ~extra_open_options; > goto send_result; > } > if (check_old_types || check_for_upgrade) > { > /* If repair is not implemented for the engine, run ALTER TABLE */ > need_repair_or_alter= 1; > } > } > - > + } > result_code= compl_result_code= HA_ADMIN_OK; > > if (operator_func == &handler::ha_analyze) > @@ -1229,7 +1277,10 @@ static bool mysql_admin_table(THD* thd, TABLE_LIST* tables, > *save_next_global= table->next_global; > table->next_local= table->next_global= 0; > > - result_code= admin_recreate_table(thd, table, &recreate_info); > + result_code= admin_recreate_table(thd, table, &recreate_info, > + (lex->alter_info.partition_flags & > + ALTER_PARTITION_ADMIN), 0); > + recreate_used= 1; that's a bit too late, you check recreate_used earlier > trans_commit_stmt(thd); > trans_commit(thd); > close_thread_tables(thd); > @@ -1641,7 +1696,8 @@ bool Sql_cmd_repair_table::execute(THD *thd) > WSREP_TO_ISOLATION_BEGIN_WRTCHK(NULL, NULL, first_table); > res= mysql_admin_table(thd, first_table, &m_lex->check_opt, &msg_repair, > TL_WRITE, 1, > - MY_TEST(m_lex->check_opt.sql_flags & TT_USEFRM), > + MY_TEST(m_lex->check_opt.sql_flags & > + (TT_USEFRM | TT_FORCE)), better rename repair_table_use_frm argument of mysql_admin_table and open_only_one_table, because it's not only TT_USEFRM anymore. a good name could be no_errors_from_open > HA_OPEN_FOR_REPAIR, &prepare_for_repair, > &handler::ha_repair, &view_repair, true); > > diff --git a/sql/sql_table.cc b/sql/sql_table.cc > index 55319a2242f..7d5a968070a 100644 > --- a/sql/sql_table.cc > +++ b/sql/sql_table.cc > @@ -10868,19 +10869,56 @@ do_continue:; > } > #endif /* WITH_WSREP */ > > + /* > + ALTER TABLE ... ENGINE to the same engine is a common way to > + request table rebuild. Set ALTER_RECREATE flag to force table > + rebuild. > + */ > + if (new_db_type == old_db_type) > + { > + if (create_info->used_fields & HA_CREATE_USED_ENGINE) > + alter_info->flags|= ALTER_RECREATE; > + > + /* > + Check if we are using ALTER TABLE FORCE without any other options > + (except ENGINE == current_engine). > + In this case we will try to recreate an identical to the original. > + This code is also used with REPAIR and OPTIMIZE. > + */ > + if (alter_info->flags == ALTER_RECREATE && > + ((create_info->used_fields & ~HA_CREATE_USED_ENGINE) == 0)) > + create_info->recreate_identical_table= 1; > + } > + > /* > We can use only copy algorithm if one of the following is true: > + - If the table is from an old MariaDB version and requires data > + modifications. In this case we ignore --alter-algorithm as eh, no, you cannot ignore it, if the user specified incompatible set of options, ALTER TABLE has to fail with an error, saying ALGORITHM=INPLACE (for example) is impossible here. The server cannot just silently ignore an explicit user command. > + as we cannot use any other algorithm than COPY (using other > + algorithms could open up the problem that the table .frm version > + is updated without data transformations and the table would be > + corrupted without any way for MariaDB to notice this during > + check/upgrade). > + This logic ensurses that ALTER TABLE ... FORCE (no other > + options) will always be be able to repair a table structure and > + convert data from any old format. > - In-place is impossible for given operation. > - Changes to partitioning which were not handled by fast_alter_part_table() > needs to be handled using table copying algorithm unless the engine > supports auto-partitioning as such engines can do some changes > using in-place API. > */ > + check_opt.init(); > + > + require_copy_algorithm= (table->file->ha_check_for_upgrade(&check_opt) == > + HA_ADMIN_NEEDS_DATA_CONVERSION); > + if (require_copy_algorithm || > - if (is_inplace_alter_impossible(table, create_info, alter_info) > - || IF_PARTITIONING((partition_changed && > + is_inplace_alter_impossible(table, create_info, alter_info) || > + IF_PARTITIONING((partition_changed && > !(old_db_type->partition_flags() & HA_USE_AUTO_PARTITION)), 0)) > { > - if (alter_info->algorithm_is_nocopy(thd)) > + if (alter_info->algorithm_is_nocopy(thd) && > + !(require_copy_algorithm && alter_info->algorithm_not_specified())) > { > my_error(ER_ALTER_OPERATION_NOT_SUPPORTED, MYF(0), > alter_info->algorithm_clause(thd), "ALGORITHM=COPY"); > @@ -12451,10 +12482,14 @@ bool mysql_recreate_table(THD *thd, TABLE_LIST *table_list, > create_info.init(); > create_info.row_type=ROW_TYPE_NOT_USED; > create_info.alter_info= &alter_info; > + create_info.recreate_identical_table= 1; > /* Force alter table to recreate table */ > alter_info.flags= (ALTER_CHANGE_COLUMN | ALTER_RECREATE); > + alter_info.partition_flags= thd->lex->alter_info.partition_flags; > + if (partition_admin) > + alter_info.partition_flags= ALTER_PARTITION_ADMIN; when is this needed? I couldn't find a single code path where alter_info.partition_flags is not ALTER_PARTITION_ADMIN at this point. Neither code analysys nor mtr have found anything. You can safely remove this if() and add DBUG_ASSERT(alter_info.partition_flags & ALTER_PARTITION_ADMIN); > > - if (table_copy) > + if (table_copy && !partition_admin) > alter_info.set_requested_algorithm( > Alter_info::ALTER_TABLE_ALGORITHM_COPY); > > diff --git a/sql/table.cc b/sql/table.cc > index f84eeda297f..b09e9b11f17 100644 > --- a/sql/table.cc > +++ b/sql/table.cc > @@ -1846,6 +1846,8 @@ int TABLE_SHARE::init_from_binary_frm_image(THD *thd, bool write, > */ > if (share->frm_version == FRM_VER_TRUE_VARCHAR -1 && frm_image[33] == 5) > share->frm_version= FRM_VER_TRUE_VARCHAR; > + if (share->frm_version <= FRM_VER_TRUE_VARCHAR) > + share->keep_original_mysql_version= 1; this is wrong. see prepare_frm_header(), frm_version is *always* FRM_VER_TRUE_VARCHAR if there are no vcols. you've basically just disabled update_frm_version() for almost everyone. > > new_field_pack_flag= frm_image[27]; > new_frm_ver= (frm_image[2] - FRM_VER); Regards, Sergei Chief Architect, MariaDB Server and security(a)mariadb.org

2 3