-
Notifications
You must be signed in to change notification settings - Fork 2
MDEV-38073: Always use tx_read_only=false for Wsrep system threads #545
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: 10.11
Are you sure you want to change the base?
Conversation
Use common variable initialization for all Wsrep schema threads. Simplify the code for replay & recovery of SR transactions.
janlindstrom
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.
In my opinion setting on sql_plugin.cc looks incorrect.
| plugin_thdvar_init(thd); | ||
|
|
||
| // Restore proper transaction read-only context | ||
| thd->variables.tx_read_only= false; |
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.
Can you explain why this needs to be here, assuming that tx_read_only=true is configuration doest this just overwrite it ?
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 nasty one. This is the comment from here (MariaDB#4422):
Apparently, there was a separate issue with applier thread variables: wsrep_plugins_post_init() would overwrite thd->variables for every applier thread and forget to restore proper default isolation level. Then, upon every server transaction termination,
trans_reset_one_shot_statistics() would set thread's isolation level to the one stored in thd->variables, thus spoiling the isolation level value for appliers.
I didn't want to duplicate it, but I can add it to the commit description here as well. Hopefully, I found only two variables tx_read_only and tx_isolation with such a behavior.
| */ | ||
| thd->variables.option_bits|= OPTION_BIN_LOG; | ||
|
|
||
| /* Allow applying in a transaction read-only context */ |
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.
For wsrep, I would save original values and then set these and restore global default after use.
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.
It's not needed. It's a thread created and belonging to the applier. We set all the variables like that.
| { | ||
| Wsrep_schema_impl::wsrep_off wsrep_off(thd); | ||
| Wsrep_schema_impl::binlog_off binlog_off(thd); | ||
| Wsrep_schema_impl::sql_safe_updates sql_safe_updates(thd); |
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.
Did this disapear or it it set somewhere?
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.
These are like this as they are RAII scoped objects restoring original context. If they are really not needed, they should be removed from code or in my opinion there could be just one RAII scoped object.
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.
The function is called only from the method of wsrep schema of the same name. In that method, a new THD object is created, used in the function and destroyed. This THD object is now initialized with wsrep_init_thd_variables() below, so that it has all the needed properties as other schema threads.
All these RAII objects did mostly the same, but they didn't account for future modifications like setting proper isolation (I will do it in another PR) or setting tx_read_only.
To sum up: I de-obfuscated this code and made all interactions explicit.
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.
Also, we cannot remove these RAII classes as they are used by storage service THD calls, which for some reason doesn't have all the values set. But I agree that mostly all these RAII classes are used in conjunction and all together at a time. We might merge them one day if we figure out how to simplify other code parts.
78dc2f1 to
44cd276
Compare
Apparently, there was a separate issue with applier thread variables: wsrep_plugins_post_init() would overwrite thd->variables for every applier thread and forget to restore proper read only context. Then, upon every server transaction termination, trans_reset_one_shot_statistics() would set thread's read only context to the one stored in thd->variables, thus spoiling the read only context value for appliers.
44cd276 to
acecce3
Compare
Unify setting variables for Wsrep schema threads:
Apparently, there was a separate issue with applier thread variables:
wsrep_plugins_post_init() would overwrite thd->variables for every
applier thread and forget to restore proper read only context.
Then, upon every server transaction termination,
trans_reset_one_shot_statistics() would set thread's read only context
to the one stored in thd->variables, thus spoiling the read only
context value for appliers.