maria-developers team mailing list archive

[Nikita Malyavin <nikitamalyavin@xxxxxxxxx>]

>
>
> You forgot to address the following:
>
> >      if (do_commit)
> >      {
> > -      // do not set STMT_END for last event to leave table open in
> altering thd
> > -      error= binlog_flush_pending_rows_event(thd, false, true, binlog,
> &cache);
> > -      if (is_ending_transaction)
> > +      /*
> > +        If the cache wasn't reinited to write, then it remains empty
> after
> > +        the last write.
> > +      */
> > +      if (cache.cache_log.type != READ_CACHE && !error)
>
> this is a confusing new condition. are you trying to avoid locking a
> mutex for an empty cache? If yes, you can check my_b_bytes_in_cache(),
> that'd be more clear.
>

Pardon. My subconscious ignores uncomfortable questions:)

No, i didn't. reinit_io_cache is called inside
>          error= binlog->write_cache(thd, &cache.cache_log);

This reinit function behaves differently when it reinits  WRITE_CACHE ->
READ_CACHE
vs READ_CACHE -> READ_CACHE. The latter happens when we issue COMMIT when
non-trans trable was involved. This simply leads to a crash.

I didn't investigate for something smarter, so yes, checking
my_b_bytes_in_cache() may also work,
I suppose. Let's see...

Anyway, calling Event_log::write_cache with cache.cache_log.type ==
READ_CACHE is error-prone,
so I'll leave it as an assertion.


Sergei, once I reiterated to this MDEV, found out that I was wrong about
this:

>  then we'll have to clear the pending event.

The pending event is deleted during truncate() (called
from restore_prev_position()).

Also I noticed another reinit_io_cache there. I think it's not vulnerable
to the problem above, but
it may end up in a real OS ftell. Even though our file is temporary, I will
add at least an assertion to
the zero reinit_io_cache result.

The commit to see is 4747e9cd584ee
<https://github.com/MariaDB/server/commit/4747e9cd584ee982b0033061957d76eb09b13554>
.
-- 
Yours truly,
Nikita Malyavin