Kristian, Using the chance to forwarding the reply that did not make to the developers list, let me comment on binlog_commit() that I might hastily rate as a big subject.

Hi Andrei,

Here is the last part of my review of the changes in branch bb-10.6-MDEV-31949:

...

--- a/sql/log_event_server.cc +++ b/sql/log_event_server.cc @@ -3324,6 +3324,11 @@ Gtid_log_event::Gtid_log_event(THD *thd_arg, uint64 seq_no_arg, flags2|= thd->lex->sql_command == SQLCOM_XA_PREPARE ? FL_PREPARED_XA : FL_COMPLETED_XA; xid.set(xid_state.get_xid()); + + // multi-engine external completion is unaware of the prepared engine #. + extra_engines= thd->transaction->all.ha_list ? + ha_count_rw_2pc(thd_arg, + thd_arg->in_multi_stmt_transaction_mode()) - 1 : 0;

I cannot understand the comment. I assume it's about that an XA COMMIT of a detached xid (different session that the XA PREPARE, or replication slave) - right?

Correct.

But what is the consequences of not knowing extra_engines in this case? Is the extra_engines used for anything than the optional truncate of the binlog after master crash?

In the multi-engine case when the # of prepared in Engine is greater than in the value in GTID event then the trx is partially committed, so is decided for to be completed.

...

@@ -4175,7 +4173,8 @@ int XA_prepare_log_event::do_commit() thd->lex->xid= &xid; if (!one_phase) { - if ((res= thd->wait_for_prior_commit())) + if (thd->is_current_stmt_binlog_disabled() && + (res= thd->wait_for_prior_commit()))

Why?000 Is it to allow XA_PREPARE_LOG_EVENT to group commit with other event groups? That makes sense, but then shouldn't the wait_for_prior_commit() be somewhere in handler.cc (or maybe xa.cc)? Look at Xid_log_event::do_commit(), it doesn't have any wait_for_prior_commit(), that is in ha_commit_one_phase().

Right.

(The wait-for-prior-commit facility is designed as a server-level feature, not a replication-level, it could in principle be used outside of replication, even though it not currently (IIRC).)

I think I understood what you mean which is follow the ha_commit_one_phase() pattern. [todo]

...

diff --git a/sql/sql_array.h b/sql/sql_array.h index 8610e971016..94c6d86ac05 100644 --- a/sql/sql_array.h +++ b/sql/sql_array.h @@ -172,6 +172,8 @@ template <class Elem> class Dynamic_array

size_t size() const { return array.elements; }

+ size_t max_size() const { return array.max_element; } +

Not that it hurts that much, but I think you can avoid adding this.

Left over of a cleanup from the previous version. [todo]

...

--- a/sql/log.cc +++ b/sql/log.cc @@ -85,6 +85,7 @@ const char *log_bin_index= 0; const char *log_bin_basename= 0;

MYSQL_BIN_LOG mysql_bin_log(&sync_binlog_period); +XID_cache_element *xid_cache_search(THD *thd, XID *xid);

This duplicates the same declaration in sql/xa.h So probably this can be removed now?

[todo]

...

@@ -2186,16 +2202,13 @@ int binlog_commit(THD *thd, bool all, bool ro_1pc) }

if (cache_mngr->trx_cache.empty() && - (thd->transaction->xid_state.get_state_code() != XA_PREPARED || - !(thd->ha_data[binlog_hton->slot].ha_info[1].is_started() && - thd->ha_data[binlog_hton->slot].ha_info[1].is_trx_read_write()))) + (!(is_preparing_xa(thd) || is_prepared_xa(thd)) || + !is_binlog_read_write(thd)))

This binlog_commit function started out as the "commit" method for the binlog handlerton. But that is not how it is used anymore, now it is called directly from several different places, including some places specific to external user XA.

A large part of the function has become complex conditions like this that are basically impossible for the developer (or reviewer) to understand exactly why this set of conditions is the correct one. Again, the function is trying to guess what the caller wants, depending on the state of various thd properties etc.

Instead, the function should be split into multiple functions. One that is called to handle XA PREPARE; one for binlog_commit_by_xid(), etc. Then each version should be able to greatly reduce the number of conditions needed.

And if any logic is shared between the different versions, this can then be put in smaller functions that are called from each version.

[my initial response was: Your proposal leads to a significant refactoring, which I add only one hunk. I'd really consider that as a separate exercise I think I've understood. Sure, binlog_prepare() that logs XA-PREPARE can call a common function with binlog_commit() that originally intended to log only normal trx:s. I can look into that. [todo]

...

@@ -11050,10 +11124,30 @@ bool Recovery_context::decide_or_assess(xid_recovery_member *member, int round, } else if (member->in_engine_prepare < last_gtid_engines) { - DBUG_ASSERT(member->in_engine_prepare > 0); + DBUG_ASSERT(member->in_engine_prepare > 0 || + /* XA event over xid that was completed in engines */ + (member->xid == 0 && + member->xa_binlog_state >= xid_recovery_member::XA_PREPARE)); + if (member->in_engine_prepare > 0 && + member->xa_binlog_state == xid_recovery_member::XA_PREPARE) + {

The function decide_or_assess() is hard to understand. I can see that it's called in 3 cases: XID_LOG_EVENT, XA PREPARE, XA COMMIT/ROLLBACK. So again here, I think this should be split into 3 different functions, one for each case (or maybe two, one for internal XID and one for external XA, whatever makes the more sense).

I don't understand the comment "XA event over xid that was completed in engines". But I remember seeing something in log_event_server.cc that sets extra_engines to 0 in the external user XA case.

So is that the case here, that last_gtid_engines==0 marks this is user XA? That would be much clearer if the logic for internal and external XA was not mixed in the same function.

...

+ /* + the first of the two options below can occur when round 1 turns to 2 + so event sequences across binlog files are like the following + g_current(B_0, xid), ... g_last(B^*, xid), + where `g`:s are xa event groups associated with the same member via + common `xid`. As elsewhere B_0 is the 2nd round's first and, + B^* the 1st round binlogs. + */ if (reset_truncate_coord(member->binlog_coord < last_gtid_coord ? member->binlog_coord.second : pos))

I'm trying to understand what is going on here. I think this is refering to the way binlog files are scanned? First we scan the most recent binlog file. At the end of it, we may find that the latest binlog checkpoint is in an earlier file, then we continue to scan those older files. So for example we may scan in order:

master-bin.000004, master-bin.000002, master-bin.000003

This means that we may see the XA PREPARE and XA COMMIT out-of-order as we go from the first scan to the remaining - "round 1 turns to 2".

But I don't understand that this is "the first of the two options below":

member->binlog_coord < last_gtid_coord ? member->binlog_coord.second : pos

If last_gtid_coord is in master-bin.000002, but member->binlog_coord is in master-bin.000004 ("round 1 turns to 2"), then I would think we would have

member->binlog_coord > last_gtid_coord

So it would be the *second* of the options below? What am I missing here?

Also, I would suggest a simplification here. After we do the initial scan in round 1, if we find that we need to scan more files, then reset all the state and hashes. And then do a full scan of all necessary binlog files in-order. In my example, that would be:

master-bin.000004 <reset state> master-bin.000002 master-bin.000003 master-bin.000004

This way, the logic becomes simpler, as we don't need logic to handle seeing events out-of-order.

This will mean that occasionally we will need to scan one more binlog file, but I think that is a good trade-off. The recovery code is notoriously difficult to test and get correct, because it's an unusual occurence to need recovery in the first place, and very hard to reproduce any problems. And robust and correct recovery is the whole point of having XA in the first palce. So making the code as simple as possible is crucial.

The extra cost is small:

- For one, recovery is a rare event - Re-scanning only costs CPU, the file is already read from storage into OS buffers. - Normally, the binlog checkpoint completes shortly after rotation, so the last binlog file that must be rescanned will be short.

So simply redo the scan from the correct point when a "round 2" is needed, to avoid any logic to handle seeing events out-of-order during recovery.

...

@@ -11182,7 +11311,9 @@ void Recovery_context::process_gtid(int round, Gtid_log_event *gev, last_gtid_no2pc= false; last_gtid_standalone= (gev->flags2 & Gtid_log_event::FL_STANDALONE) ? true : false; - if (do_truncate && last_gtid_standalone) + /* a standalone XA-COMPLETE is truncation-safe */ + if (do_truncate && last_gtid_standalone + && !(gev->flags2 & Gtid_log_event::FL_COMPLETED_XA)) update_binlog_unsafe_coord_if_needed(linfo);

I don't understand the comment here. What is "a standalone XA-COMPLETE" ? What does it mean to be "truncation-safe"? Please reword the comment to be comprehensible.

...

+ /* + the user XA events are decided similary to the normal transaction, + difference is just in the timing which is Gtid event read now. + */

I don't understand this comment. Please reformulate to make it understandable. What does normal transactions do, and what does user XA do? How does the timing differ, and why?

...

+ sql_print_warning("GTID %u-%u-%s XA transaction state % " + "in binlog file:%s pos:%llu is inconsistent " + "with the former same state; " + "consider filtered binary loggig.", + ctx.last_gtid.domain_id, ctx.last_gtid.server_id, + buf, is_xac ? "'COMPLETE'" : "'PREPARE'", + linfo->log_file_name, ctx.last_gtid_coord.second);

Typo: s/loggig/logging/

Also, the phrase "the former same state" is not understandable (at least to me), what does it mean? Please reformulate.

I also have to be short now in replying to the rest, which I'll expand on tomorrow morning. Now just this.

...

@@ -909,14 +910,16 @@ ha_commit_checkpoint_request(void *cookie, void (*pre_hook)(void *)) there's no need to rollback here as all transactions must be rolled back already */ -void ha_close_connection(THD* thd) +void ha_close_connection(THD* thd, bool skip_binlog)

I tried to understand what the purpose of this change is. ha_close_connection() is now called from two new places related to replication of XA PREPARE. And in one place with this mysterious skip_binlog=true condition.

My guess is that this is to detach the XA PREPARE from the current THD?

The reason is described in 6cbb63f592 commit name II.1. ``` OTHER CHANGES, to ha_close_connection() are done to keep the binlog hton connection alive for the end of XA-prepare's binlogging on the parallel slave. Other than the binlog hton:s are disconnected from the slave transaction handler in TC_LOG::run_commit_ordered() which makes the prepared transaction engine objects available to binlog-commit-order followers. ``` In other words XAP_k in run_commit_ordered() releases with ha_close_connection(thd, true) all resources that the following XAC_k+i need. The latter does not need XAP_k to release binlog which it still need while being is this stack of TC_LOG::run_commit_ordered MYSQL_BIN_LOG::trx_group_commit_leader MYSQL_BIN_LOG::write_transaction_to_binlog_events MYSQL_BIN_LOG::write_transaction_to_binlog binlog_flush_cache binlog_commit_flush_xa_prepare binlog_commit binlog_prepare prepare_or_error ha_prepare

So that it becomes visible in XA RECOVER to other threads, and (maybe more important) to make the THD available to run another transaction? One of the things InnoDB does in close_connection is: trx_disconnect_prepared().

But I think the purpose of the close_connection handlerton call is different

And you must have meant 'ha_close_connection', right?

- it is to free things at the end of a client connection?

Well, with the ha- prefix, it's to disconnect THD from all transaction branches/htons.

So it can do more than detach the current external XA - which is presumably the reason for the need to hack it to not call the specific binlog_close_connection()?

So this shouldn't hijack the close_connection to mean "detach_xa_trx". It looks like we need an explicit handlerton call for detaching an xa transaction, that can then be used instead of this hack in ha_close_connection().

I need to digest that in the morning. Generally I don't see a problem to conduct hton release flexibly.

...

-static my_bool xacommit_handlerton(THD *unused1, plugin_ref plugin, - void *arg) +my_bool xacommit_handlerton(THD *unused1, plugin_ref plugin, void *arg) { handlerton *hton= plugin_hton(plugin); - if (hton->recover) + if (hton->recover && hton != binlog_hton) { hton->commit_by_xid(hton, ((struct xahton_st *)arg)->xid);

...

-int ha_commit_or_rollback_by_xid(XID *xid, bool commit) +int ha_commit_or_rollback_by_xid(XID *xid, bool commit, THD *thd) {

...

+ if (!skip_binlog) + { + // when binlog is ON start from its transaction branch if (commit) binlog_commit_by_xid(binlog_hton, xid); else binlog_rollback_by_xid(binlog_hton, xid);

I think the purpose if this is to ensure that commit/rollback of XA calls into the binlog code first (to binlog before doing it in the engines).

But as I think Serg also remarked already, this should not be needed. The binlog is the (internal) transaction coordinator when it is enabled, we have a dedicated interface for that, class TC_LOG. This is used elsewhere in handler.cc to avoid special cases for whether the binlog is enabled or not.

So it looks like there needs to be extra virtual functions in TC_LOG for the XA commit/rollback, and these can then be called on the tc_log object and do what is required.

I need to process that tomorrow.

Then some logic in handler.cc can be omitted/simplified.

----

I think with this, I got through all of the code changes in branch bb-10.6-MDEV-31949 as of commit 1bdc9bc8077c79e55cb4dbfc7cc47a193d17d54f from January 20, 2024. So this concludes the current review. I have not looked at the test cases in this round of review.

Thanks again! Cheers, Andrei