MDEV-38494 .mariadb_history rename race condition#4699
MDEV-38494 .mariadb_history rename race condition#4699MooSayed1 wants to merge 1 commit intoMariaDB:10.11from
Conversation
|
I used a Python helper script (mariadb_history_race.py) alongside the .test file to create a PTY session. |
MooSayed1
force-pushed
the
MDEV-38494-history-race
branch
from
a1f4504 to
f4d2ba2
Compare
gkodinov
added
the
External Contribution
we usually use perl. mtr can run perl scripts directly |
|
There's The fix does silence error message, but doesn't solve loss of history records from concurrent sessions. OTOH bug report didn't request it. |
gkodinov
left a comment
gkodinov
left a comment
There was a problem hiding this comment.
Thank you for your contribution. This is a preliminary review.
client/mysql.cc
Outdated
| read_history(histfile); | ||
| if (!(histfile_tmp= (char*) my_malloc(PSI_NOT_INSTRUMENTED, | ||
| strlen(histfile) + 5, MYF(MY_WME)))) | ||
| strlen(histfile) + 20, MYF(MY_WME)))) |
There was a problem hiding this comment.
I'd add a comment stating why it's 20. Ideally I'd use some sizeof()
client/mysql.cc
Outdated
| } | ||
| sprintf(histfile_tmp, "%s.TMP", histfile); | ||
| // Include PID in temp file name to prevent concurrent-session rename races. | ||
| sprintf(histfile_tmp, "%s.TMP%lu", histfile, (unsigned long) getpid()); |
There was a problem hiding this comment.
please change to snprintf().
Also, I'd rather prefer %s.%lu.TMP myself. Extensions are how the OS recognizes file types. Also, pid_t is a signed type: https://man.archlinux.org/man/pid_t.3type.en. I wonder what does this do to your naming scheme. I'd take the absolute value.
| @@ -0,0 +1,112 @@ | |||
| #!/usr/bin/env python3 | |||
There was a problem hiding this comment.
please use perl. This is what mtr uses and understands. I'd personally avoid the whole thing and make sure mysql's --skip-batch works as one would expect and sets batch to 0.
When multiple mariadb sessions exit at the same time, they all write to the same $HISTFILE.TMP then rename it to $HISTFILE. The second rename fails with Errcode 2 because the first already moved the file. Fix: include the process PID in the temp file name so each session uses its own unique path and no rename collision can occur.
MooSayed1
force-pushed
the
MDEV-38494-history-race
branch
from
f4d2ba2 to
2d206ed
Compare
|
Thanks for helping and reviewing @svoj @gkodinov |
It's probably best done in a separate PR I guess. Since, as a change in behavior, it needs to go to main. |
gkodinov
left a comment
gkodinov
left a comment
There was a problem hiding this comment.
LGTM. Thank you! Stay tuned for the final review please.
4 participants
MDEV-38494:
.mariadb_historyrename race conditionThe Jira issue number for this PR is: MDEV-38494
Description
When multiple
mariadbCLI sessions exit at the same time (e.g. multipleterminal tabs), all sessions write their history to the same temp file path
~/.mariadb_history.TMPand then rename it. The second rename fails becausethe first session already moved the file:
The root cause is in
client/mysql.cc. The temp filename is constructed as:All concurrent sessions use the same
.TMPpath. When session A renamesit to the final history file, session B's subsequent rename fails with ENOENT
because the file is already gone.
The fix includes the process PID in the temp filename so each session has its
own unique rename target:
Release Notes
mariadbCLI no longer showsError on rename of '.mariadb_history.TMP'when multiple sessions exit concurrently.
How can this PR be tested?
The test pre-creates
$HISTFILE.TMPas a sentinel, then runs an interactivemariadb session via a Python PTY wrapper (so
isatty()returns true andhistory is actually written). With the fix, the client uses
$HISTFILE.TMP<PID>and the sentinel survives. Without the fix, the client overwrites and renames
the sentinel → test fails.
Basing the PR against the correct MariaDB version
This is a bug fix. The earliest affected branch is 10.11, so this PR targets
upstream/10.11.PR quality check