Hi! On Fri, May 22, 2020 at 3:27 PM Andrei Elkin <andrei.elkin@mariadb.com> wrote: <cut> CORNER CASES: read-only, pure myisam, binlog-*, @@skip_log_bin, etc

Aria just makes yet another previously unknown use case of an engine that produces THD::ha_info but does not support 2pc, which the assert implied.

To explain more, the original block

#ifndef DBUG_OFF for (ha_info= thd->transaction.all.ha_list; rw_count > 1 && ha_info; ha_info= ha_info->next()) DBUG_ASSERT(ha_info->ht() != binlog_hton); #endif

claims there most be no binlog hton in a transaction consisting of more than 1 hton:s, *when* (at this point) this transaction has not been binlogged yet. So combination of binlog + Innodb hton would be raise the assert, to question "why the heck binlogging has not been done yet?!".

Aria is different from Innodb in this context in that binlogging was done at the end of the statement, so to miss `cache_mngr->need_unlog' flagging (which is at xa prepare time logging).

Thanks for the explanation, we should have had that in the code.

I think we should fix the assert rather than to remove. This way:

cat > assert.diff <<. diff --git a/sql/log.cc b/sql/log.cc index 792c6bb1a99..aaf1fae1cd6 100644 --- a/sql/log.cc +++ b/sql/log.cc @@ -10124,6 +10124,16 @@ int TC_LOG_BINLOG::unlog_xa_prepare(THD *thd, bool all) Ha_trx_info *ha_info; uint rw_count= ha_count_rw_all(thd, &ha_info); bool rc= false; +#ifndef DBUG_OFF + bool no_binlog= true, exist_no_2pc= false; + for (ha_info= thd->transaction->all.ha_list; rw_count > 1 && ha_info; + ha_info= ha_info->next()) + { + no_binlog= no_binlog && ha_info->ht() != binlog_hton; + exist_no_2pc= exist_no_2pc || !ha_info->ht()->prepare; + } + DBUG_ASSERT(no_binlog || exist_no_2pc); +#endif

if (rw_count > 0) { .

I tested it briefly with running XA:s on combination of engines including.

On which tree did you test? bb-10.5-aria? In the end, after discussions on slack, we ended with: #ifndef DBUG_OFF if (rw_count > 1) { /* There must be no binlog_hton used in a transaction consisting of more than 1 engine, *when* (at this point) this transaction has not been binlogged. The one exception is if there is an engine without a prepare method, as in this case the engine doesn't support XA and we have to ignore this check. */ bool binlog= false, exist_hton_without_prepare= false; for (ha_info= thd->transaction->all.ha_list; ha_info; ha_info= ha_info->next()) { if (ha_info->ht() == binlog_hton) binlog= true; if (!ha_info->ht()->prepare) exist_hton_without_prepare= true; } DBUG_ASSERT(!binlog || exist_hton_without_prepare); } #endif

...

The reason it was not hit is that before Aria was not treated as transactional we could only come here in case of errors and that code was was apparently not tested, at least with binary logging on.

...

With Aria we could come here in case of rollback and we got assert for cases that was perfectly ok.

Well, what 'rollback' do you mean? The function is invoked only for ha_prepare().

As far as I remember, I come into this code in some edge case when something failed that normally never fails. I think the issue was that Aria doesn't have a prepare handler, which caused issues in ha_prepare(), but not sure. Anyway, we now have a solution for this. Regards, Monty