Fix pushed to 5.1, merged to 5.2 and 5.3.
Part of the fix only exists in 10.0 - push that to 10.0-base and merge to 10.0

I am looking at the 5.5 version of this fix (rev 2502.565.20 in lp:maria/5.5) and I am not sure I understand why last_pool was replaced with last_pool_ptr there? Any help would be greatly appreciated.

Thanks,

Sorry, pool_last and pool_last_ptr.

> I am not sure I understand why last_pool was replaced with last_pool_ptr
> there?

It looks like this is to correctly handle the case of an empty list.

TC_LOG_MMAP::sync() links in a page at the end of the linked list. If the list
is non-empty, then this means changing the `next' pointer to point to the new
element. But if the list is empty, then there is no `next' pointer to change;
rather the TC_LOG::pool member must be changed to point to the new element.

The last_pool_ptr is a standard technique for linked lists to handle this
without special cases. If the list is empty, last_pool_ptr points to
TC_LOG::pool, else it points to the `next' pointer of the last element. So
linking in the new page in all cases can be done by assigning to
(*last_pool_ptr).

Note that this usage matches the variable `best_p' in function
TC_LOG_MMAP::get_active_from_pool(), which is probably why I did it this way.

Does this make things clear for you?

Kristian -

Thanks for your explanation, I haven't considered the case of an
empty pool list.

But I don't see how that list can be possibly empty? TC_LOG_MMAP
requires at least three pages, at most one can be active, at most one
can be syncing, thus one page should be present in the pool at all
times, thus TC_LOG_MMAP::sync() should invariantly have at least one
page in the pool in the LOCK_pool critical section.
TC_LOG_MMAP::get_active_from_pool() is called with
active == NULL, thus there should be at least two pages in the pool
there.

> But I don't see how that list can be possibly empty?

Well, there were multiple problems in the pool list handling. I suppose I just
fixed all of them that I found. It could be that it was another problem that
caused the actual bug. If you check the code before the patch, you will see
that if get_active_from_pool() selected the last page in the list, then the
page was not unlinked from the pool.

So maybe the list can not be empty, but it still seems better to not leave
completely wrong code in there, doesn't it?

Another problem in the bug was a race condition that get_active_from_pool()
could select a page with no free slots.

IIRC, this bug was found because of a different bug elsewhere in the code.
The slots in the pages were never freed, so eventually the pool would run out
of free slots, thus exercising this code that was just completely wrong,
having never been tested before. Without the other bug, it may be hard to
reproduce in practice, though I think theoretically it is possible.

Out of curiosity, what makes you interested in this bug? Did you experience it
in practise?

Kristian -

Now I see, thank you. I understood all the other changes by this patch fixing the problems you describe, it's only the empty list handling which I had doubts about.

I haven't experienced this in practice. My interest is caused by preparing Percona Server to work with TokuDB, which obviously requires production-quality TC_LOG_MMAP. Thus I am reviewing MariaDB changes, MySQL 5.7 fix for http://bugs.mysql.com/bug.php?id=47134, etc.

Thanks again