maria-developers team mailing list archive

[Michael Widenius <michael.widenius@xxxxxxxxx>]

Hi!

On Fri, May 22, 2020 at 3:27 PM Andrei Elkin <andrei.elkin@xxxxxxxxxxx> 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