MDEV-38891 fixed THD read access violation

[itzanway]

No description provided.

[vaintroub]

Ok, here some points.

• there is a lot of unrelated whitespace changes. Please do not include them into the patch.

• if THD can't be created, at startup, write a warning to error log (@dr-m can tell how to do it best for Innodb, but there is sql_print_warning), but do not create the timer, so that dict_stats_func is not going to be periodically called, if it can't do anything. You do not need to check if thd is NULL, you can DBUG_ASSERT() that it is not NULL.

• I think , and ask @dr-m to confirm, that whatever Innodb is doing with THDVAR(thd, background_thread) = true; is unnecessary. While harmless at startup, and can bite at shutdown, in innodb_reset_background_thd() in ut_ad(THDVAR(thd, background_thread));. To check if it is not a foreground thread, thd->thread_id == 0 check is entirely sufficient, yes , server ensures that, yes, even then thread_ids wrap around, when 4 bytes ints are exhausted. I would remove that thing in fact, too. together with thd_proc_info(thd, name);, I have no idea what purpose does it serve, the background THDs are invisible from outside. I'd say, these wrappers around create_background_thd() and reset_thds() are obsolete, but would prefer to have @dr-m to confirm it.

• While bug was reported in the main branch, bug it mostly likely exists in all previous versions. Which means, fix could need to be rebased to 10.6 or 10.11, or whatever @gkodinov will decide. As far as I can see it, this bug might not be of a huge importance. From my limited my understanding and very short investigation, is that it only happens when server was not going to start anyway, otherwise, in case of successful startup, dict_stats cleanup would happen earlier, at ha_preshutdown/innodb_preshutdown stage, but I'd again let @dr-m confirm that. UPDATE: ok, this can happen at any bootstrap, unlike normal startup, it does not run ha_preshutdown currently.

[dr-m]

I believe that MDEV-38891 affects all currently maintained MariaDB Server branches. Therefore, a fix of this should target the 10.6 branch.

The real challenge is to reproduce the issue, possibly by injecting some sleep within DBUG_EXECUTE_IF. I would like to see a test case (even a nondeterministic one that fails once in N attempts) and some analysis of what is going on and how the fix is supposed to prevent the problem.

[dr-m]

By convention, our source code files end in a line terminator.

[itzanway]

My editor stripped it out by accident. I've added the missing newline terminator in the latest commit

[itzanway]

Thanks for the review! I've updated the PR to address these points:

Cleaned up the unrelated whitespace changes.

Added a check in dict_stats_start(): if THD creation fails, it now logs a warning via sql_print_warning and skips creating the timer.

Replaced the if (!dict_stats_thd) check in dict_stats_func() with DBUG_ASSERT(dict_stats_thd != nullptr).

[itzanway]

@vaintroub
I have added a test case using DBUG_EXECUTE_IF to reliably trigger the shutdown race condition, and verified it passes cleanly with this fix." Let me know if Buildbot gives you any trouble!

[gkodinov]

Thank you for working on this! This is a preliminary review.

There's already a very active review discussion: please keep working on that.

These are the extra points I see:

• Since this is a crashing bug, I'd agree with Marko that this should be rebased to 10.6. Please rebase.
• Please use a single commit with a commit message conforming to CODING_STANDARDS.md
• Please make sure that buildbot runs pass. Right now some platforms don't even compile

[dr-m]

@vaintroub, I think that you are right about THDVAR(thd, background_thread). First and foremost, I’d like to see a test case for reproducing the problem, ideally in mysql-test, such that without the fix, the test would at least occasionally fail. There are a few special cases around server startup and shutdown, depending on the value of innodb_fast_shutdown and whether InnoDB was started successfully or in some special mode, such as innodb_force_recovery or innodb_read_only or bootstrap.

[vaintroub]

I have seen it once after running test suite hundreds of times, and luckily got it a the debugger. it is innodb_bootstrap test, and I can't say anything more about it. I reported the error as community service, the rest, as for reproducibility, is up to Innodb team. The thing that is failing is bootstrap "mariadbd --bootstrap"

[gkodinov]

Any specific reason you've re-requested review from me? I have nothing new to add and nothing in the PR has changed. So please work on the remarks in my previous review.