- developers - lists.mariadb.org

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

21 Oct '23

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

3 5

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

17 Oct '23

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

2 1

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

16 Oct '23

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

1 0

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

16 Oct '23

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

1 0

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

16 Oct '23

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

1 0

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

16 Oct '23

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

2 3

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

12 Oct '23

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

2 1

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

10 Oct '23

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

1 0

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

09 Oct '23

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

2 2

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

09 Oct '23

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

2 3