- developers - lists.mariadb.org

Re2: Starting on review of XA patches in bb-10.6-MDEV-31949
by andrei.elkin＠pp.inet.fi 12 Feb '24

12 Feb '24

Salve Kristian, this is the 2nd follow-up replay to your 1st review, to cover few deferred notes in particular. I mark with [x] any addressed [todo] note. > Kristian Nielsen <knielsen(a)knielsen-hq.org> writes: > >> Hi Andrei, > So you're in this context > > 1 innobase_commit_by_xid( > 2 /*===================*/ > 3 handlerton* hton, > 4 XID* xid) /*!< in: X/Open XA transaction > identification */ > 5 { > 6 DBUG_ASSERT(hton == innodb_hton_ptr); > 7 > 8 DBUG_EXECUTE_IF("innobase_xa_fail", > 9 return XAER_RMFAIL;); > 10 > 11 if (high_level_read_only) { > 12 return(XAER_RMFAIL); > 13 } > 14 > 15 if (trx_t* trx = trx_get_trx_by_xid(xid)) { > 16 THD *thd = current_thd; > 17 if (thd) > 18 thd_binlog_pos(thd, &trx->mysql_log_file_name, > 19 &trx->mysql_log_offset); > 20 if (trx->mysql_log_offset > 0) > 21 trx->flush_log_later = true; > > and actually considering `trx->mysql_log_offset` as the return value? > ... I could not grasp your way of thinking in this place. > Could you please expand on? In another mail yesterday concerning with this part I suggested to replace if (trx->mysql_log_offset > 0) with if (thd_binlog_format(thd) != UNSPEC) > > >> >> It would be much better if this could be done similar to how normal >> commits >> are done. We have the commit_ordered handlerton which does the fast >> part of >> commit-to-memory. And then the slow part which does the rest in commit >> handlerton. > > Notice that the *slow* part of the XA commit is the same as in the > normal > case: > > trx_commit_complete_for_mysql > *innobase_commit* > commit_one_phase_2 > ha_commit_one_phase > > The fast parts of the two - the XA and normal - could be unified - into > > innobase_commit_ordered() > > but I did not think (had time for) towards that. > >> >> So we could have commit_by_xid_ordered() and commit_by_xid(). The >> problem is >> that commit_by_xid_ordered() needs to release the xid, so it becomes >> harder >> to get hold of the transaction (which the commit handlerton just finds >> from >> the thd). Maybe commit_by_xid_ordered() would need to return some trx >> handle. >> >> What part of the commit is skipped by this in InnoDB? Is it _only_ >> flushing >> the log up to the LSN of the commit record, or are there more things? > > > So the xa's fast innobase_commit_by_xid() skips a "slow" > trx_commit_complete_for_mysql() > and the same does the normal's innobase_commit_ordered(). > >> >> How is another storage engine supposed to handle this? This also needs >> to be >> documented somewhere, eg. in comments on the handlerton calls. >> >> This also means that the --innodb-flush-log-at-trx-commit=3 is not >> supported >> for XA COMMIT when it's done from a different session that the XA >> PREPARE. >> For example, with --innodb-flush-log-at-trx-commit=3 but >> --sync-binlog=0, >> normal transactions are still durable in InnoDB. But external XA >> transactions will not be. This could be solved if the commit_by_xid() >> was >> split into a fast and slow part similar to normal commit. > > [todo] Let me process that carefully later. [x] My 1st reply already said, just missed to underscore that, that the XA also the slow and fast commit paths, therefore iflatc=3 is good. The past paths are commit_one_phase_2 and innobase_commit_by_xid functionally equivalent. One would vote to unify them, and I'd accept that. > >> >> Probably you'll say that this works for now, and perhaps that's true. >> But at >> least it really _really_ needs some detailed comments explaining what >> is >> going on, and what should be kept in mind if modifying/improving this >> code >> in the future, otherwise it will be impossible for the next developer >> to >> understand. > > [todo] Agreed. > >> >> >>> diff --git a/sql/log.cc b/sql/log.cc >>> index e54d4087d46..7e3dbeaef29 100644 >>> --- a/sql/log.cc >>> +++ b/sql/log.cc >> >>> @@ -9365,18 +9381,84 @@ TC_LOG::run_commit_ordered(THD *thd, bool >>> all) >>> all ? thd->transaction->all.ha_list : >>> thd->transaction->stmt.ha_list; >>> >>> mysql_mutex_assert_owner(&LOCK_commit_ordered); >>> - for (; ha_info; ha_info= ha_info->next()) >>> + >>> + if (!ha_info) >> >> Your patch puts a lot of XA/binlog logic into run_commit_ordered(). >> But >> that's not appropriate. The run_commit_ordered() function has the >> purpose >> solely to call the commit_ordered() handlerton in each engine, and >> it's also >> called from TC_LOG_MMAP (that's why it's TC_LOG::, not >> MYSQL_BIN_LOG::). >> >> Instead make a new function that handles the part of binlog commit >> that >> needs to be done under LOCK_commit_ordered. Be sure to comment it that >> this >> runs under this highly contended mutex, and needs to do as little work >> as >> possible. Agreed. [todo] >> >> This function can then call run_commit_ordered(). It can also be used >> to >> share a few bits of code that are now duplicated between the >> opt_optimize_thread_scheduling and !opt_optimize_thread_scheduling >> cases, >> like: >> >> ++num_commits; >> run_commit_ordered(...) >> entry->thd->wakeup_subsequent_commits() >> if (entry->queued_by_other) { ... } >> >> (if it makes sense to do so in the actual code, only if makes things >> simpler, that's easier to decide when working with the actual code). Will see. [todo] >> >> There sesems to be 3 new cases: >> >> - XA PREPARE >> - XA COMMIT from same session as XA PREPARE >> - XA COMMIT of detached XA transaction >> >> The logic to decide this was already done earlier, when the event >> group was >> queued for group commit, eg. to decide which end_event to use. For >> example >> there is no need to call xid_cache_search() here, that can be done >> earlier >> (and it's also wrong to call my_error() here, as we're in the wrong >> thread >> context). Indeed. The error's thread context was overlooked. Thanks for catching it. >> >> So the logic about what to do should be placed higher up in the call >> stack, >> ideally where we already know, eg. binlog_commit_by_xid(). And errors >> such >> as ER_XAER_NOTA can be thrown already there, and the decision on what >> to do >> put consisely as a flag into the struct group_commit_entry, so the >> logic >> during binlog group commit becomes as simple as possible. >> >> (It might even make sense to simply store a function pointer to the >> method >> that should be called to handle this; normal, attached XA COMMIT, >> detached >> XA COMMIT, XA PREPARE. But again it's easier when actually working >> with the >> code to decide if this is simpler than a couple flags or case on enum >> value). > > > [todo] Let me process the above suggestion block a bit later. [x] To the need of xid_cache_search() etc, let me combine this part to reply to the 2nd review mail's notes around a Dummy XID though ... (read on) > >> >>> + if (thd->rgi_slave && thd->rgi_slave->is_parallel_exec) >>> + { >>> + DBUG_ASSERT(thd->transaction->xid_state.is_dummy_XA()); >>> + >>> + auto xs= xid_cache_search(thd, xid); >>> + if(!xs) >>> + { >>> + my_error(ER_XAER_NOTA, MYF(0)); >>> + return; >>> + } >>> + else >>> + { >>> + thd->transaction->xid_state.xid_cache_element= xs; >>> + } >>> + DBUG_ASSERT(thd->transaction->xid_state.is_recovered()); >>> + } >>> + >>> plugin_foreach(NULL, commit ? xacommit_handlerton : >>> xarollback_handlerton, >>> MYSQL_STORAGE_ENGINE_PLUGIN, &xaop); >>> xid_cache_delete(thd); >> >> Why this check for thd->rgi_slave->is_parallel_exec here deep in >> binlog >> group commit? That seems very wrong, and again no comment whatsoever >> explaining what is going on, so again I have to try to guess... >> >> I'm guessing that it's something with calling xid_cache_delete() which >> needs >> to find thd->transaction->xid_state.xid_cache_element ? > > Right. > >> >> But this could be done much better in a higher-level part of the code >> that's >> specific to xa and (parallel) replication, not here deep down in >> binlog >> group commit code. >> >> And why should this be needed only for parallel replication, not for >> non-parellel replication or non-replication SQL? I don't understand. > > > It's a kind of optimization for the slave side, in that `xid` of XAP > (Prepare) gets released for a XAC (Commit) the soonest. > ... the reason's said here. >> >> >> A couple remarks about the tests I pushed to >> knielsen_mdev31949_review: >> > >> >> I think the failures in rpl.rpl_recovery_xa I linked are from >> duplicate XID >> in XA PREPARE ; XA COMMIT ; XA PREPARE. There comes an ambiguity >> between >> whether the transaction prepared in the engine at recovery is from the >> first >> XA PREPARE (so it should be committed) or the second (so it should be >> rolled >> back). This happens when the second XA PREPARE is in the engine but >> not >> binlogged. >> >> One way to fix could actually be to use the binlog position stored >> inside >> InnoDB. That's a neat reminder on the Innodb durably committed binlog position! Indeed with the duplicate xid case there exists this trouble of identification and the Engine is able and must come to rescue. >> If the XA COMMIT is before this binlog position, then we know the >> first transaction became durable in InnoDB, so we know we have to roll >> back >> the current prepared transaction in the engine. Otherwise we should >> commit >> it. I see the recovery code already uses binlog positions to decide >> the fate >> of pending XA transactions in the engines, but I did not yet fully >> understand what is going on there. >> >> Another way could be to binlog an extra event before the second XA >> PREPARE >> with the duplicate XID. Well, this does not cover the case of prior to crash XA PREPARE only made to Engine. The above consulting with Innodb resolves the ambiguity generally. >> This event would mean "This xid is now durable >> committed in the engine", so we would know not to commit any prepared >> trx in >> the engine with the same xid. This event would only be needed in case >> of XA >> PREPARE with a duplicate xid. This though requires the prior XA COMMIT >> to >> fsync inside InnoDB first, I think. > > Thanks for this piece of analysis! > [todo] I am processing this a bit later. [x] > >> >> >> Another test failure in rpl.slave_provision_xa I think happens because >> the >> XA PREPARE does not update the binlog position inside innodb. >> Maybe the way >> to solve this problem is to have the slave be idempotent, and silently >> ignore any duplicate XA PREPARE if the xid is already prepared? Indeed, the fact that XAP does not update the innodb's binlog position must cause few concerns. I still need to have a close look. [todo] > > [todo] I am processing this a bit later. [x] > >> >> >> About the "ToDo" of using some Xid_list_log_event. I think this is a >> way >> during recovery to get the state of the XA PREPARE/COMMIT's in earlier >> binlog files before the last binlog checkpoint, if I understood things >> correctly? > > True. > >> >> Another way to do this is to delay the binlog checkpoints until all XA >> PREPARE in a binlog file have been XA COMMIT'ted (or XA ROLLBACK'ed), >> so >> that they will all be scanned during recovery. I actually implemented >> this >> in my branch knielsen_mdev32020. The advantage of this is that we may >> anyway >> need this to provide the XA PREPARE events to a slave set up from a >> logical >> backup (like mysqldump), to avoid purging XA PREPARE events too early. >> And >> then it's a bit simpler to simply scan an extra file rather than both >> scan >> and read Xid_list_log_event. But I think Xid_list_log_event could work >> fine >> as well, just mentioning it for you to consider. > > Xlle can't be generally avoided so we have to take conservatively > first. > To add up to a separate thread discussion over this problematics, I've grown more inclined with the first implementation basing on your idea to defer Binlog-CheckPoint. That might work for a majority of XA users, Xid list to be added later if time permits or will be convincingly :-) requested. Now I am turning to account the rest - 2,3 reviews you sent that only glanced through - to reply within few hours. Cheers, Andrei

2 2

Re: 64-bit time_t support in MariaDB?
by Otto Kekäläinen 12 Feb '24

12 Feb '24

> it's not related to time, it's just an assert that's verified at compile > time, that is, during compilation. Like static_assert in C++. > > I just need to find 32-bit arm somewhere. Unfortunately I don't have any with SSH access to give. Seems the reported of MDEV-33429 has armhf hardware and is helping out. Once you have a patch, I can push it to Launchpad builders (or you can do it yourself, anyone with an Launchpad can push code to their own PPA). Related, Cfarm is a project that gives free accounts to open source developers in case you want to have access to play around with multiple architectures: https://portal.cfarm.net/machines/list/ Unfortunately they don't seem to have armhf/armel.

2 1

Re2: thd_binlog_pos / Re: Starting on review of XA patches in bb-10.6-MDEV-31949
by andrei.elkin＠pp.inet.fi 08 Feb '24

08 Feb '24

>> So you're in this context >> >> 1 innobase_commit_by_xid( > >> 18 thd_binlog_pos(thd, &trx->mysql_log_file_name, >> 19 &trx->mysql_log_offset); >> 20 if (trx->mysql_log_offset > 0) >> 21 trx->flush_log_later = true; >> >> and actually considering `trx->mysql_log_offset` as the return value? >> Could you please expand on? > > My point was just, that what we want to do here is for the (internal) > transaction coordinator to tell the storage engine that the engine does > not > need to fsync() for the commit to be durable, another fsync was already > done > to ensure this. This is what we in the normal commit path do by calling > commit_ordered(), as documented sql/handler.h. > > The thd_binlog_pos() is completely unrelated, it's a way for the > storage... I see your point. My patch's block of if (trx_t* trx = trx_get_trx_by_xid(xid)) { + THD *thd = current_thd; + if (thd) + thd_binlog_pos(thd, &trx->mysql_log_file_name, + &trx->mysql_log_offset); + if (trx->mysql_log_offset > 0) + trx->flush_log_later = true; + /* use cases are: disconnected xa, slave xa, recovery */ innobase_commit_low(trx); + trx->mysql_log_file_name = NULL; should not indeed rely on anything that thd_binlog_pos() computes. I think I wanted to spare --skip-log-bin setups from the flush_log_later policy. I see now that alternatively thd_binlog_format(thd) could be used which will always return BINLOG_FORMAT_UNSPEC when that's the case (or @@skip_log_bin=1). You think it's better to covert the block to use that?

1 0

Starting on review of XA patches in bb-10.6-MDEV-31949
by Kristian Nielsen 07 Feb '24

07 Feb '24

Hi Andrei, It's not an easy review of the XA patches in bb-10.6-MDEV-31949. One thing that would have helped is a clear design document that lists all the different concerns and points that need to be considered around recovery, backup, replication, possible conflicts, etc. This could then be referenced by the reviewer to see if a given concern was considered, and if so what the resolution is. But I'll try as best I can with what we have. As I'm trying to understand the different issues, I found making test cases to check my understanding helped. I have pushed some to the branch knielsen_mdev31949_review . They seem to show some problems in the current patch: mysql-test/suite/rpl/t/rpl_recovery_xa.test - A trx is left in "XA PREPARED" state after crash recovery, even though the binlogged event group is incomplete (it should have been rolled back). - A trx is committed after crash recovery, even though it is not binlogged (it should have been rolled back). - An incomplete XA event group is output by mysqlbinlog with a "ROLLBACK" at the end which gives an error (should probably be XA ROLLBACK). mysql-test/suite/rpl/t/rpl_recovery_xa2.test - An XA PREPAREd transaction is lost after recovery when it was binlogged in an earlier binlog file. (I think this might be the issue that's referenced as a ToDo with Xid_list_log_event in patch?) mysql-test/suite/mariabackup/slave_provision_xa.test - An XA PREPARE does not update the binlog position inside InnoDB. So a slave provisioned with mariabackup from this starting position can fail with duplicate apply of XA PREPARE. Some other initial remarks follow. Since there's a desire to progress with this quickly, I thought it would be useful to send these early, so you can start looking/discussions before I complete building an understanding of the whole design and code picture. > diff --git a/storage/innobase/handler/ha_innodb.cc b/storage/innobase/handler/ha_innodb.cc > index 17e512bf34e..a4cdc72e18c 100644 > --- a/storage/innobase/handler/ha_innodb.cc > +++ b/storage/innobase/handler/ha_innodb.cc > @@ -17144,8 +17144,16 @@ innobase_commit_by_xid( > } > > if (trx_t* trx = trx_get_trx_by_xid(xid)) { > + THD *thd = current_thd; > + if (thd) > + thd_binlog_pos(thd, &trx->mysql_log_file_name, > + &trx->mysql_log_offset); I guess this is necessary to get the correct binlog position stored inside innodb to match the binlogged XA COMMIT event, right? But will it work if called during crash recovery? During crash recovery, we could actually update the position, but only if this is trx is the last event group in the old binlog. Maybe that's better handled elsewhere (there are currently other cases where the binlog position in InnoDB can be wrong immediately after restart until the first new transaction is committed). > + if (trx->mysql_log_offset > 0) > + trx->flush_log_later = true; But I'm guessing this is the motivation. If called from within binlog group commit, you want to disable fsync in InnoDB. But if called from recovery, or when binlog is disabled, this condition will be false, and fsync will be done. If so, that's a _really_ obfuscated way to do it :-/ It surely needs a very clear comment explaining what is going on. And it is very fragile. What if thd_binlog_pos() is changed at some point, to slightly change the conditions under which it returns 0 or not? That could silently break this code. It would be much better if this could be done similar to how normal commits are done. We have the commit_ordered handlerton which does the fast part of commit-to-memory. And then the slow part which does the rest in commit handlerton. So we could have commit_by_xid_ordered() and commit_by_xid(). The problem is that commit_by_xid_ordered() needs to release the xid, so it becomes harder to get hold of the transaction (which the commit handlerton just finds from the thd). Maybe commit_by_xid_ordered() would need to return some trx handle. What part of the commit is skipped by this in InnoDB? Is it _only_ flushing the log up to the LSN of the commit record, or are there more things? How is another storage engine supposed to handle this? This also needs to be documented somewhere, eg. in comments on the handlerton calls. This also means that the --innodb-flush-log-at-trx-commit=3 is not supported for XA COMMIT when it's done from a different session that the XA PREPARE. For example, with --innodb-flush-log-at-trx-commit=3 but --sync-binlog=0, normal transactions are still durable in InnoDB. But external XA transactions will not be. This could be solved if the commit_by_xid() was split into a fast and slow part similar to normal commit. Probably you'll say that this works for now, and perhaps that's true. But at least it really _really_ needs some detailed comments explaining what is going on, and what should be kept in mind if modifying/improving this code in the future, otherwise it will be impossible for the next developer to understand. > diff --git a/sql/log.cc b/sql/log.cc > index e54d4087d46..7e3dbeaef29 100644 > --- a/sql/log.cc > +++ b/sql/log.cc > @@ -9365,18 +9381,84 @@ TC_LOG::run_commit_ordered(THD *thd, bool all) > all ? thd->transaction->all.ha_list : thd->transaction->stmt.ha_list; > > mysql_mutex_assert_owner(&LOCK_commit_ordered); > - for (; ha_info; ha_info= ha_info->next()) > + > + if (!ha_info) Your patch puts a lot of XA/binlog logic into run_commit_ordered(). But that's not appropriate. The run_commit_ordered() function has the purpose solely to call the commit_ordered() handlerton in each engine, and it's also called from TC_LOG_MMAP (that's why it's TC_LOG::, not MYSQL_BIN_LOG::). Instead make a new function that handles the part of binlog commit that needs to be done under LOCK_commit_ordered. Be sure to comment it that this runs under this highly contended mutex, and needs to do as little work as possible. This function can then call run_commit_ordered(). It can also be used to share a few bits of code that are now duplicated between the opt_optimize_thread_scheduling and !opt_optimize_thread_scheduling cases, like: ++num_commits; run_commit_ordered(...) entry->thd->wakeup_subsequent_commits() if (entry->queued_by_other) { ... } (if it makes sense to do so in the actual code, only if makes things simpler, that's easier to decide when working with the actual code). There sesems to be 3 new cases: - XA PREPARE - XA COMMIT from same session as XA PREPARE - XA COMMIT of detached XA transaction The logic to decide this was already done earlier, when the event group was queued for group commit, eg. to decide which end_event to use. For example there is no need to call xid_cache_search() here, that can be done earlier (and it's also wrong to call my_error() here, as we're in the wrong thread context). So the logic about what to do should be placed higher up in the call stack, ideally where we already know, eg. binlog_commit_by_xid(). And errors such as ER_XAER_NOTA can be thrown already there, and the decision on what to do put consisely as a flag into the struct group_commit_entry, so the logic during binlog group commit becomes as simple as possible. (It might even make sense to simply store a function pointer to the method that should be called to handle this; normal, attached XA COMMIT, detached XA COMMIT, XA PREPARE. But again it's easier when actually working with the code to decide if this is simpler than a couple flags or case on enum value). > + if (thd->rgi_slave && thd->rgi_slave->is_parallel_exec) > + { > + DBUG_ASSERT(thd->transaction->xid_state.is_dummy_XA()); > + > + auto xs= xid_cache_search(thd, xid); > + if(!xs) > + { > + my_error(ER_XAER_NOTA, MYF(0)); > + return; > + } > + else > + { > + thd->transaction->xid_state.xid_cache_element= xs; > + } > + DBUG_ASSERT(thd->transaction->xid_state.is_recovered()); > + } > + > plugin_foreach(NULL, commit ? xacommit_handlerton : xarollback_handlerton, > MYSQL_STORAGE_ENGINE_PLUGIN, &xaop); > xid_cache_delete(thd); Why this check for thd->rgi_slave->is_parallel_exec here deep in binlog group commit? That seems very wrong, and again no comment whatsoever explaining what is going on, so again I have to try to guess... I'm guessing that it's something with calling xid_cache_delete() which needs to find thd->transaction->xid_state.xid_cache_element ? But this could be done much better in a higher-level part of the code that's specific to xa and (parallel) replication, not here deep down in binlog group commit code. And why should this be needed only for parallel replication, not for non-parellel replication or non-replication SQL? I don't understand. A couple remarks about the tests I pushed to knielsen_mdev31949_review: I think the failures in rpl.rpl_recovery_xa I linked are from duplicate XID in XA PREPARE ; XA COMMIT ; XA PREPARE. There comes an ambiguity between whether the transaction prepared in the engine at recovery is from the first XA PREPARE (so it should be committed) or the second (so it should be rolled back). This happens when the second XA PREPARE is in the engine but not binlogged. One way to fix could actually be to use the binlog position stored inside InnoDB. If the XA COMMIT is before this binlog position, then we know the first transaction became durable in InnoDB, so we know we have to roll back the current prepared transaction in the engine. Otherwise we should commit it. I see the recovery code already uses binlog positions to decide the fate of pending XA transactions in the engines, but I did not yet fully understand what is going on there. Another way could be to binlog an extra event before the second XA PREPARE with the duplicate XID. This event would mean "This xid is now durable committed in the engine", so we would know not to commit any prepared trx in the engine with the same xid. This event would only be needed in case of XA PREPARE with a duplicate xid. This though requires the prior XA COMMIT to fsync inside InnoDB first, I think. Another test failure in rpl.slave_provision_xa I think happens because the XA PREPARE does not update the binlog position inside innodb. Maybe the way to solve this problem is to have the slave be idempotent, and silently ignore any duplicate XA PREPARE if the xid is already prepared? About the "ToDo" of using some Xid_list_log_event. I think this is a way during recovery to get the state of the XA PREPARE/COMMIT's in earlier binlog files before the last binlog checkpoint, if I understood things correctly? Another way to do this is to delay the binlog checkpoints until all XA PREPARE in a binlog file have been XA COMMIT'ted (or XA ROLLBACK'ed), so that they will all be scanned during recovery. I actually implemented this in my branch knielsen_mdev32020. The advantage of this is that we may anyway need this to provide the XA PREPARE events to a slave set up from a logical backup (like mysqldump), to avoid purging XA PREPARE events too early. And then it's a bit simpler to simply scan an extra file rather than both scan and read Xid_list_log_event. But I think Xid_list_log_event could work fine as well, just mentioning it for you to consider. That's it for now, I will continue trying to understand what is going on and complete the review. - Kristian.

1 2

Re: Xlle question
by andrei.elkin＠pp.inet.fi 06 Feb '24

06 Feb '24

>> Xlle can't be generally avoided so we have to take conservatively >> first. > > Why it cannot be avoided? What is it needed for? The main purpose is to discern at recovery a prepared XA (XAP) that was never binlogged from one that was. Your case >>> Another way to do this is to delay the binlog checkpoints until all >>> XA >>> PREPARE in a binlog file have been XA COMMIT'ted assumes that XAC must always come in "normal" time. But of course, being a part of the user's decision, it can be late significantly up until a purge from binary logs that contain the incl the total RESET MASTER. In the latter case its eventual post RM XA-COMMIT would be bin-logged with the full awareness.

1 0

Fwd/Re: Starting on review of XA patches in bb-10.6-MDEV-31949
by andrei.elkin＠pp.inet.fi 05 Feb '24

05 Feb '24

From: Andrei Elkin <andrei.elkin(a)mariadb.com> Subject: Re: Starting on review of XA patches in bb-10.6-MDEV-31949 To: Kristian Nielsen <knielsen(a)knielsen-hq.org> Cc: developers(a)lists.mariadb.org, andrei elkin <andrei.elkin(a)mariadb.com>, Michael Widenius <michael.widenius(a)gmail.com> Date: Mon, 05 Feb 2024 14:11:01 +0200 (36 minutes, 34 seconds ago) Organization: MariaDB Kristian Nielsen <knielsen(a)knielsen-hq.org> writes: > Hi Andrei, > > It's not an easy review of the XA patches in bb-10.6-MDEV-31949. > > One thing that would have helped is a clear design document that lists > all > the different concerns and points that need to be considered around > recovery, backup, replication, possible conflicts, etc. This could then > be > referenced by the reviewer to see if a given concern was considered, > and if > so what the resolution is. I am sorry for discomfort the lack of the above caused. Unfortunately the IV recovery part has consumed more time than was expected (which is at the final stage of my local testing). > But I'll try as best I can with what we > have. If you'll need any online discussion to compensate for that I'd be glad to do it. > > As I'm trying to understand the different issues, I found making test > cases > to check my understanding helped. I have pushed some to the branch > knielsen_mdev31949_review. Thank you for streamlining it this way! > They seem to show some problems in the current > patch: > > mysql-test/suite/rpl/t/rpl_recovery_xa.test > - A trx is left in "XA PREPARED" state after crash recovery, even > though > the binlogged event group is incomplete (it should have been > rolled > back). > - A trx is committed after crash recovery, even though it is not > binlogged (it should have been rolled back). > - An incomplete XA event group is output by mysqlbinlog with a > "ROLLBACK" > at the end which gives an error (should probably be XA ROLLBACK). > > mysql-test/suite/rpl/t/rpl_recovery_xa2.test > - An XA PREPAREd transaction is lost after recovery when it was > binlogged > in an earlier binlog file. (I think this might be the issue that's > referenced as a ToDo with Xid_list_log_event in patch?) > > mysql-test/suite/mariabackup/slave_provision_xa.test > - An XA PREPARE does not update the binlog position inside InnoDB. > So a > slave provisioned with mariabackup from this starting position can > fail > with duplicate apply of XA PREPARE. > > Some other initial remarks follow. Since there's a desire to progress > with > this quickly, I thought it would be useful to send these early, so you > can > start looking/discussions before I complete building an understanding > of the > whole design and code picture. Super! > > >> diff --git a/storage/innobase/handler/ha_innodb.cc >> b/storage/innobase/handler/ha_innodb.cc >> index 17e512bf34e..a4cdc72e18c 100644 >> --- a/storage/innobase/handler/ha_innodb.cc >> +++ b/storage/innobase/handler/ha_innodb.cc >> @@ -17144,8 +17144,16 @@ innobase_commit_by_xid( >> } >> >> if (trx_t* trx = trx_get_trx_by_xid(xid)) { >> + THD *thd = current_thd; >> + if (thd) >> + thd_binlog_pos(thd, &trx->mysql_log_file_name, >> + &trx->mysql_log_offset); > > I guess this is necessary to get the correct binlog position stored > inside > innodb to match the binlogged XA COMMIT event, right? Correct. > But will it work if called during crash recovery? Right. The unpushed part IV makes sure of that. > > During crash recovery, we could actually update the position, but only > if > this is trx is the last event group in the old binlog. Maybe that's > better > handled elsewhere (there are currently other cases where the binlog > position > in InnoDB can be wrong immediately after restart until the first new > transaction is committed). > >> + if (trx->mysql_log_offset > 0) >> + trx->flush_log_later = true; > > But I'm guessing this is the motivation. If called from within binlog > group > commit, you want to disable fsync in InnoDB. But if called from > recovery, or > when binlog is disabled, this condition will be false, and fsync will > be > done. Indeed. > > If so, that's a _really_ obfuscated way to do it :-/ I remember it was a very quick decision for myself to miss this sort of reflection. [todo] I'll add comments of course. > It surely needs a very clear comment explaining what is going on. > And it is very fragile. What if thd_binlog_pos() is changed at some > point, > to slightly change the conditions under which it returns 0 or not? That > could silently break this code. So you're in this context 1 innobase_commit_by_xid( 2 /*===================*/ 3 handlerton* hton, 4 XID* xid) /*!< in: X/Open XA transaction identification */ 5 { 6 DBUG_ASSERT(hton == innodb_hton_ptr); 7 8 DBUG_EXECUTE_IF("innobase_xa_fail", 9 return XAER_RMFAIL;); 10 11 if (high_level_read_only) { 12 return(XAER_RMFAIL); 13 } 14 15 if (trx_t* trx = trx_get_trx_by_xid(xid)) { 16 THD *thd = current_thd; 17 if (thd) 18 thd_binlog_pos(thd, &trx->mysql_log_file_name, 19 &trx->mysql_log_offset); 20 if (trx->mysql_log_offset > 0) 21 trx->flush_log_later = true; and actually considering `trx->mysql_log_offset` as the return value? ... I could not grasp your way of thinking in this place. Could you please expand on? > > It would be much better if this could be done similar to how normal > commits > are done. We have the commit_ordered handlerton which does the fast > part of > commit-to-memory. And then the slow part which does the rest in commit > handlerton. Notice that the *slow* part of the XA commit is the same as in the normal case: trx_commit_complete_for_mysql *innobase_commit* commit_one_phase_2 ha_commit_one_phase The fast parts of the two - the XA and normal - could be unified - into innobase_commit_ordered() but I did not think (had time for) towards that. > > So we could have commit_by_xid_ordered() and commit_by_xid(). The > problem is > that commit_by_xid_ordered() needs to release the xid, so it becomes > harder > to get hold of the transaction (which the commit handlerton just finds > from > the thd). Maybe commit_by_xid_ordered() would need to return some trx > handle. > > What part of the commit is skipped by this in InnoDB? Is it _only_ > flushing > the log up to the LSN of the commit record, or are there more things? So the xa's fast innobase_commit_by_xid() skips a "slow" trx_commit_complete_for_mysql() and the same does the normal's innobase_commit_ordered(). > > How is another storage engine supposed to handle this? This also needs > to be > documented somewhere, eg. in comments on the handlerton calls. > > This also means that the --innodb-flush-log-at-trx-commit=3 is not > supported > for XA COMMIT when it's done from a different session that the XA > PREPARE. > For example, with --innodb-flush-log-at-trx-commit=3 but > --sync-binlog=0, > normal transactions are still durable in InnoDB. But external XA > transactions will not be. This could be solved if the commit_by_xid() > was > split into a fast and slow part similar to normal commit. [todo] Let me process that carefully later. > > Probably you'll say that this works for now, and perhaps that's true. > But at > least it really _really_ needs some detailed comments explaining what > is > going on, and what should be kept in mind if modifying/improving this > code > in the future, otherwise it will be impossible for the next developer > to > understand. [todo] Agreed. > > >> diff --git a/sql/log.cc b/sql/log.cc >> index e54d4087d46..7e3dbeaef29 100644 >> --- a/sql/log.cc >> +++ b/sql/log.cc > >> @@ -9365,18 +9381,84 @@ TC_LOG::run_commit_ordered(THD *thd, bool all) >> all ? thd->transaction->all.ha_list : >> thd->transaction->stmt.ha_list; >> >> mysql_mutex_assert_owner(&LOCK_commit_ordered); >> - for (; ha_info; ha_info= ha_info->next()) >> + >> + if (!ha_info) > > Your patch puts a lot of XA/binlog logic into run_commit_ordered(). But > that's not appropriate. The run_commit_ordered() function has the > purpose > solely to call the commit_ordered() handlerton in each engine, and it's > also > called from TC_LOG_MMAP (that's why it's TC_LOG::, not > MYSQL_BIN_LOG::). > > Instead make a new function that handles the part of binlog commit that > needs to be done under LOCK_commit_ordered. Be sure to comment it that > this > runs under this highly contended mutex, and needs to do as little work > as > possible. > > This function can then call run_commit_ordered(). It can also be used > to > share a few bits of code that are now duplicated between the > opt_optimize_thread_scheduling and !opt_optimize_thread_scheduling > cases, > like: > > ++num_commits; > run_commit_ordered(...) > entry->thd->wakeup_subsequent_commits() > if (entry->queued_by_other) { ... } > > (if it makes sense to do so in the actual code, only if makes things > simpler, that's easier to decide when working with the actual code). > > There sesems to be 3 new cases: > > - XA PREPARE > - XA COMMIT from same session as XA PREPARE > - XA COMMIT of detached XA transaction > > The logic to decide this was already done earlier, when the event group > was > queued for group commit, eg. to decide which end_event to use. For > example > there is no need to call xid_cache_search() here, that can be done > earlier > (and it's also wrong to call my_error() here, as we're in the wrong > thread > context). > > So the logic about what to do should be placed higher up in the call > stack, > ideally where we already know, eg. binlog_commit_by_xid(). And errors > such > as ER_XAER_NOTA can be thrown already there, and the decision on what > to do > put consisely as a flag into the struct group_commit_entry, so the > logic > during binlog group commit becomes as simple as possible. > > (It might even make sense to simply store a function pointer to the > method > that should be called to handle this; normal, attached XA COMMIT, > detached > XA COMMIT, XA PREPARE. But again it's easier when actually working with > the > code to decide if this is simpler than a couple flags or case on enum > value). [todo] Let me process the above suggestion block a bit later. > >> + if (thd->rgi_slave && thd->rgi_slave->is_parallel_exec) >> + { >> + DBUG_ASSERT(thd->transaction->xid_state.is_dummy_XA()); >> + >> + auto xs= xid_cache_search(thd, xid); >> + if(!xs) >> + { >> + my_error(ER_XAER_NOTA, MYF(0)); >> + return; >> + } >> + else >> + { >> + thd->transaction->xid_state.xid_cache_element= xs; >> + } >> + DBUG_ASSERT(thd->transaction->xid_state.is_recovered()); >> + } >> + >> plugin_foreach(NULL, commit ? xacommit_handlerton : >> xarollback_handlerton, >> MYSQL_STORAGE_ENGINE_PLUGIN, &xaop); >> xid_cache_delete(thd); > > Why this check for thd->rgi_slave->is_parallel_exec here deep in binlog > group commit? That seems very wrong, and again no comment whatsoever > explaining what is going on, so again I have to try to guess... > > I'm guessing that it's something with calling xid_cache_delete() which > needs > to find thd->transaction->xid_state.xid_cache_element ? Right. > > But this could be done much better in a higher-level part of the code > that's > specific to xa and (parallel) replication, not here deep down in binlog > group commit code. > > And why should this be needed only for parallel replication, not for > non-parellel replication or non-replication SQL? I don't understand. It's a kind of optimization for the slave side, in that `xid` of XAP (Prepare) gets released for a XAC (Commit) the soonest. > > > A couple remarks about the tests I pushed to knielsen_mdev31949_review: > > > I think the failures in rpl.rpl_recovery_xa I linked are from duplicate > XID > in XA PREPARE ; XA COMMIT ; XA PREPARE. There comes an ambiguity > between > whether the transaction prepared in the engine at recovery is from the > first > XA PREPARE (so it should be committed) or the second (so it should be > rolled > back). This happens when the second XA PREPARE is in the engine but not > binlogged. > > One way to fix could actually be to use the binlog position stored > inside > InnoDB. If the XA COMMIT is before this binlog position, then we know > the > first transaction became durable in InnoDB, so we know we have to roll > back > the current prepared transaction in the engine. Otherwise we should > commit > it. I see the recovery code already uses binlog positions to decide the > fate > of pending XA transactions in the engines, but I did not yet fully > understand what is going on there. > > Another way could be to binlog an extra event before the second XA > PREPARE > with the duplicate XID. This event would mean "This xid is now durable > committed in the engine", so we would know not to commit any prepared > trx in > the engine with the same xid. This event would only be needed in case > of XA > PREPARE with a duplicate xid. This though requires the prior XA COMMIT > to > fsync inside InnoDB first, I think. Thanks for this piece of analysis! [todo] I am processing this a bit later. > > > Another test failure in rpl.slave_provision_xa I think happens because > the > XA PREPARE does not update the binlog position inside innodb. Maybe the > way > to solve this problem is to have the slave be idempotent, and silently > ignore any duplicate XA PREPARE if the xid is already prepared? [todo] I am processing this a bit later. > > > About the "ToDo" of using some Xid_list_log_event. I think this is a > way > during recovery to get the state of the XA PREPARE/COMMIT's in earlier > binlog files before the last binlog checkpoint, if I understood things > correctly? True. > > Another way to do this is to delay the binlog checkpoints until all XA > PREPARE in a binlog file have been XA COMMIT'ted (or XA ROLLBACK'ed), > so > that they will all be scanned during recovery. I actually implemented > this > in my branch knielsen_mdev32020. The advantage of this is that we may > anyway > need this to provide the XA PREPARE events to a slave set up from a > logical > backup (like mysqldump), to avoid purging XA PREPARE events too early. > And > then it's a bit simpler to simply scan an extra file rather than both > scan > and read Xid_list_log_event. But I think Xid_list_log_event could work > fine > as well, just mentioning it for you to consider. Xlle can't be generally avoided so we have to take conservatively first. > > > That's it for now, I will continue trying to understand what is going > on and > complete the review. Thank you for so prompt piece of feedback! I'll return with a reply within a work day to [todo] tagged. Cheers, Andrei ----------

2 1

Review of MDEV-7850: Extend GTID Binlog Events with Thread Id
by Kristian Nielsen 31 Jan '24

31 Jan '24

> commit c37b2087b4abe576f1b0391c8d379dba6299dcb5 (origin/bb-11.4-MDEV-7850) > Author: Brandon Nesterenko <brandon.nesterenko(a)mariadb.com> > Date: Mon Jul 10 18:53:19 2023 +0300 > > MDEV-7850: Extend GTID Binlog Events with Thread Id > > This patch augments Gtid_log_event with the user thread-id. > In particular that compensates for the loss of this info in > Rows_log_events. Here's my review of this patch (which was already pushed, but there are some misunderstandings that should be fixed; Monty asked me about it as he had been told that I had reviewed the patch, but that is not the case (until now)): > diff --git a/sql/log_event.cc b/sql/log_event.cc > index 41ddc038594..f167284ad1d 100644 > --- a/sql/log_event.cc > +++ b/sql/log_event.cc > @@ -1901,11 +1904,12 @@ Query_log_event::begin_event(String *packet, ulong ev_offset, > /* > Currently we only need to replace GTID event. > The length of GTID differs depending on whether it contains commit id. > + And/or thread id. > */ > - DBUG_ASSERT(data_len == LOG_EVENT_HEADER_LEN + GTID_HEADER_LEN || > - data_len == LOG_EVENT_HEADER_LEN + GTID_HEADER_LEN + 2); > - if (data_len != LOG_EVENT_HEADER_LEN + GTID_HEADER_LEN && > - data_len != LOG_EVENT_HEADER_LEN + GTID_HEADER_LEN + 2) > + DBUG_ASSERT(data_len >= LOG_EVENT_HEADER_LEN + GTID_HEADER_LEN && > + data_len <= LOG_EVENT_HEADER_LEN + GTID_HEADER_LEN + 2 + 9); > + if (data_len < LOG_EVENT_HEADER_LEN + GTID_HEADER_LEN || > + data_len > LOG_EVENT_HEADER_LEN + GTID_HEADER_LEN + 2 + 9) This change of the asserts/checks doesn't make any sense. They were there originally, because we are hand-crafting a BEGIN query event, and there were only two cases of the length, so two fixed-sized events were crafted by two different code paths. And the asserts are there to safe-guard if the length would change. But now this check was already wrong, as more optional info has been added to the GTID event without updating the code for Query_log_event::begin_event() (XID, flags_extra, extra_engines). But the new code in this patch is generic and can handle any length GTID event, so these checks no longer make any sense, and should just be removed (rather than attempt to correct them). > @@ -1926,9 +1930,19 @@ Query_log_event::begin_event(String *packet, ulong ev_offset, > } > else > { > - DBUG_ASSERT(data_len == LOG_EVENT_HEADER_LEN + GTID_HEADER_LEN + 2); > - /* Put in an empty time_zone_str to take up the extra 2 bytes. */ > - int2store(q + Q_STATUS_VARS_LEN_OFFSET, 2); > + DBUG_ASSERT(data_len <= LOG_EVENT_HEADER_LEN + GTID_HEADER_LEN + 11); This assert should not be there. The length can be larger than 11, and the code below works fine for arbitrary length. > + DBUG_ASSERT(data_len >= LOG_EVENT_HEADER_LEN + GTID_HEADER_LEN + 2); > + > + /* Put in an empty time_zone_str to take up the extra 2 plus the number of > + dummy_bytes. */ > + size_t dummy_bytes= > + data_len - (LOG_EVENT_HEADER_LEN + GTID_HEADER_LEN + 2); > + > + int2store(q + Q_STATUS_VARS_LEN_OFFSET, dummy_bytes + 2); > + for (size_t i= 0; i < dummy_bytes; i++) > + q[Q_DATA_OFFSET + i]= Q_DUMMY; > + q+= dummy_bytes; > + > q[Q_DATA_OFFSET]= Q_TIME_ZONE_CODE; > q[Q_DATA_OFFSET+1]= 0; /* Zero length for empty time_zone_str */ > q[Q_DATA_OFFSET+2]= 0; /* Zero terminator for empty db */ This new code with Q_DUMMY is generic and independent of the length of the GTID event. That's good! But it needs a comment explaining (that the Q_DUMMY is unknown to the old slave, which will make the old slave just skip the rest of the user vars, which are just there to fill up the extra bytes). And now the code that adds the dummy empty time_zone_str is redundant, and should just be removed, as should the code for the case of data_len == LOG_EVENT_HEADER_LEN + GTID_HEADER_LEN. Since now any extra bytes in the GTID can just be replaced by Q_DUMMY. This way, the code will be fixed to work for all the possible GTID lengths, and be shorter and simpler (and a lot less confusing). > @@ -1584,6 +1584,9 @@ Query_log_event::Query_log_event(const uchar *buf, uint event_len, > sa_seq_no = uint8korr(pos); > pos+= 8; > } > + } > + case Q_DUMMY: > + { > break; > } > default: > /* That's why you must write status vars in growing order of code */ > DBUG_PRINT("info",("Query_log_event has unknown status vars (first has code: %u), skipping the rest of them", (uint) *(pos-1))); > pos= (const uchar*) end; // Break loop There's no point in having this code. Q_DUMMY is supposed to be higher than any known status var type, so it will be ignored anyway in the `default` case. And besides, server that knows about Q_DUMMY can never receive it in the first place. (But if this is really wanted, then don't delete the `break` statement in the prior switch case, that's likely to introduce a bug the next time a case is added. And assign Q_DUMMY the next free value, not the value 255). > diff --git a/sql/log_event.h b/sql/log_event.h > index 473506fafed..d324642ad5e 100644 > --- a/sql/log_event.h > +++ b/sql/log_event.h > @@ -324,6 +324,7 @@ class String; > > #define Q_HRNOW 128 > #define Q_XID 129 > +#define Q_DUMMY 255 This needs a good comment, explaining how Q_DUMMY is a code that will be unknown to any server version, and can be used to make the slave skip this (and any following) user vars. diff --git a/mysql-test/suite/rpl/t/rpl_gtid_thread_id.test b/mysql-test/suite/rpl/t/rpl_gtid_thread_id.test new file mode 100644 index 00000000000..b8ed1865da9 --- /dev/null +++ b/mysql-test/suite/rpl/t/rpl_gtid_thread_id.test @@ -0,0 +1,125 @@ +# +# Verify that GTID log events are written into the binary log along with the +# id of the thread which executed the transaction. On the primary, this is the <snip> Disclaimer: I did not review this testcase. - Kristian.

2 3

Slow or hanging parallel replication in 10.6
by Kristian Nielsen 30 Jan '24

30 Jan '24

As we are discussing possible problems with XA parallel replication being slow or hanging in 10.6, I just want to remind of these recently fixed bugs: MDEV-31482: Lock wait timeout with INSERT-SELECT, autoinc, and statement-based replication This can cause slow or hanging parallel replication, as it hangs for --innodb-lock-wait-timeout MDEV-32096 Parallel replication lags because innobase_kill_query() may fail to interrupt a lock wait This can cause show or hanging parallel replication MDEV-31655: Parallel replication deadlock victim preference code errorneously removed This can cause parallel replication to be slow or fail with "too many retries" These bugs were fixed in 10.6.16. These bugs are not related to XA. But the symptoms of these bugs are that optimistic parallel replication occasionally is slow, hangs, or breaks under high load. So it is important not to mistakenly assume that these symptoms are necessarily caused by XA, even though the workload that has problems might be using external XA transactions. In versions prior to 10.6.16, these kinds of problems are likely to be caused by the above known bugs. (I got this list from a quick scan through the 10.6 commit history, there may be one or two more like them I missed. But the conclusion is the same, using optimistic parallel replication under high load in 10.6 requires using at least 10.6.16 to work). - Kristian.

1 0

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

28 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

Suggestions for the problems around replication of XA in 10.5
by Kristian Nielsen 27 Jan '24

27 Jan '24

Hi Monty, As promised, here my thoughts around the issues with the implementation of replication of external XA as implemented since MariaDB 10.5. I see at least two architectural issues with the current implementation. One is that it splits transactions in two separate GTIDs in the binlog (XA PREPARE and XA COMMIT). This breaks with the fundamental principle that replication applies transactions one after the other in strict sequence, and the replication position/GTID is the single transaction last replicated. This for example means that a mysqldump backup can no longer be used to provision a slave, since any XA PREPAREd event at the time of the dump will be missing; a testcase rpl_xa_provision.test in MDEV-32020 demonstrates this. Another architectural issue is that each XA PREPARE keeps row locks around on every slave until commit (or rollback). This means replication will break if _any_ transaction replicated after the XA PREPARE gets blocked on a lock. This can easily happen; surely in many ways in statement-based replication, and even in row-based replication without primary key as demonstrated by testcase rpl_mdev32020.test in MDEV-32020. There are other problems; for example the update of mysql.gtid_slave_pos cannot be committed crash-safe together with the transaction for XA PREPARE (since the transaction is not committed). I believe the root of the problem is architectural: external XA should be replicated only after they commit on the master. Trying to fix individual problems one by one will not address the root problem and will lead to ever increasing complexity without ever being fully successful. The current implementation appears to only address a very specific and rare use-case, where enhanced semi-synchronous replication is used with row-based binlogging to try to fail-over to a slave and preserve any external XA that was in PREPAREd state on the master before the failover. Mixed-mode replication, provisioning slaves with mysqldump, slaves not intended for failover, etc., seem to be not considered and basically broken since 10.5. Here is my idea for a design that solves most of these problems. At XA PREPARE, we can still write the events to the binlog, but without a GTID, and we do not replicate it to slaves by default. Then at XA COMMIT we binlog a commit transaction that can be replicated normally to the slaves without problems. If necessary, the events for XA COMMIT can be read from the PREPARE earlier in the binlog, eg. after server crash/restart. We already have the binlog checkpoint mechanism to ensure that required binlog files are preserved until no longer needed for transaction recovery. This way we make external XA preserved across server restart, and all normal replication features continue to work - mysqldump, mixed-mode, etc. Nice and simple. Then optionally we can support the specific usecase of being able to recover external XA PREPAREd transactions on a slave after failover. When enabled, the slave can receive the XA PREPARE events and binlog them itself, without applying. Then as part of failover, those XA PREPARE in the binlog that are still pending can be applied, leaving them in PREPAREd state on the new master. This way, _only_ the few transactions that need to be failed-over need special handling, the majority can still just replicate normally. There are different refinements and optimizations that can be added on top of this. But the point is that this is a simple implementation that is robust, correct, and crash-safe from the start, without needing to add complexity and fixes on top. I've done some initial proof-of-concept code for this, and continue to work on it on the branch knielsen_mdev32020 on github. - Kristian.

3 6