Hi, Kristian, On Oct 20, Kristian Nielsen wrote:
Sergei Golubchik <serg@mariadb.org> writes:
If there is some reason, then the correct fix could be to set current_thd for the duration of run_commit_ordered, which will satisfy the assertion and presumably the actual bug?
Setting current_thd is cheating, it will trick the assert, but won't fix the underlying problem as outlined above.
Nono, it's not cheating, it's in fact setting the correct value. While running commit_ordered(T_i), the current THD _is_ the THD for T_i, not the THD for T_1, which is unrelated to transaction T_i.
okay. but practically, is it cheaper than lock/unlock of an uncontended mutex? UPD: just looked at the assembly, and indeed, it is very cheap. It used to be expensive, but that was fixed years ago. Let's do it. Also, it made me reconsider the old rule of "avoid current_thd, it's expensive, pass THD as an argument". What to avoid, it's just Dump of assembler code for function _current_thd(): 0x000055780130bb21 <+0>: push %rbp 0x000055780130bb22 <+1>: mov %rsp,%rbp 0x000055780130bb25 <+4>: mov %fs:0x0,%rax 0x000055780130bb2e <+13>: lea -0x1c8(%rax),%rax 0x000055780130bb35 <+20>: mov (%rax),%rax 0x000055780130bb38 <+23>: pop %rbp 0x000055780130bb39 <+24>: ret (in a debug build, I don't have an optiimized build handy)
Monty asserted many times that taking an uncontended mutex is cheap. Did you think it's expensive or did you benchmark that?
I did, actually! (in the past). Taking a mutex needs to reserve the associated cache line, which is not free. "Expensive" is relative, of course.
I'm sure it's an implementation detail. But I've just looked on my laptop and the mutex lock was lock cmpxchg so, it's a bus lock, indeed, not cheap.
For the same reason, set_current_thd() was not done in this code, to avoid it if it serves no purpose.
Indeed.
I think it is fine to add set_current_thd() *if it is actually needed*. But we should understand first *why* it is needed, otherwise we may hide the real bug.
That's as a I said, the assert simply verifies that the protocol is properly followed. So, set_current_thd() isn't really needed beyond that. But as it's cheap, let's do it. Regards, Sergei Chief Architect, MariaDB Server and security@mariadb.org