-
-
Notifications
You must be signed in to change notification settings - Fork 2k
MDEV-18386: Add server_audit_timestamp_format to customize audit log timestamps #4633
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
gkodinov
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a preliminary review. Thank you for your contribution!
plugin/server_audit/server_audit.cc
Outdated
| PLUGIN_VAR_OPCMDARG, "Limit on the length of the query string in a record", | ||
| NULL, NULL, 1024, 0, 0x7FFFFFFF, 1); | ||
| static MYSQL_SYSVAR_STR(timestamp_format, timestamp_format, | ||
| PLUGIN_VAR_RQCMDARG | PLUGIN_VAR_MEMALLOC, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not a thread safe access to a global variable of type string pointer and the data it points to!
While doing it this way is fine for simple types (up to a native CPU word aligned: 2 or 4 usually). It's definitely not ok to do unsynchronized reads and especially updates. Concurrent updates on themselves are mostly fine due to the way SET GLOBAL works (takes a mutex while running SET for each variable). But unsynchronized reads are definitely not OK as you can get to all kinds of side effects: reading free memory, catching updates "mid-flight" (half old half new etc).
I'd suggest looking into how file_path sysvar works. It's not 100% safe as is, but it comes close.
The idea is as follows:
- you define a lock of some sort (mutex, rwlock, RCU lock, make your pick) local to the plugin to synchronize access to the variable's value.
- You do not use the value the variable points to for anything. Instead you have another local value that will be guarded by your lock.
- You add an update method to the sysvar that will take the value as passed by the server, take your lock in write mode and store the value into the separate synchronized local value.
- at plugin initialization time you take the value configured by the server into the designated variable and store it into your synchronized copy while holding your lock.
- when reading/writing on your own in your plugin (you do not need to write currently, just read) you always use your value and your lock in the relevant mode.
This will make a thread-safe global variable of string type.
Note that there's still the matter of having to synchronize access to your variable degrading performance. Especially if done very frequently. So be careful!
I know we unfortunately do not have boiler-plate code for all this that you can just reuse. It would have been very nice if somebody did something like that. But we take contributions, you know :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the detailed feedback. I have implemented the thread safety improvements by mirroring the implementation of file_path variable.
I just have one question regarding the take_lock parameter I introduced in log_header(). This was necessary to resolve a re-entrant deadlock identified in CI, where update_logging() holds a write lock while calling the logger. While the current implementation works, I’m considering if it’s better to remove locking from log_header() entirely and instead have each calling function (like log_proxy or log_table) wrap the entire operation in a single rd_lock() / rd_unlock() cycle. This would reduce the overhead by requiring only one lock acquisition instead of two, though it holds the lock slightly longer across the formatting and write calls. Which approach do you prefer?
Regarding the lack of boilerplate code for this pattern, I agree and looked into it. I am thinking of writing a small SyncStrVar utility class that encapsulates the buffer, pointer, and validation logic, while leaving domain-specific code in the update callback itself (we can't generalize this I think). This would reduce boilerplate code. Would something like this be welcome as a follow-up contribution? I still need to work out the exact API details or did you have something else in your mind?
This PR adds a new global system variable, server_audit_timestamp_format, to the MariaDB Audit Plugin.
Key changes: