-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
MDEV-37229: Set proper trx isolation level for Wsrep system threads #4422
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?
MDEV-37229: Set proper trx isolation level for Wsrep system threads #4422
Conversation
|
|
4eff7de to
31f544d
Compare
| } | ||
| } | ||
|
|
||
| /* Statement-based replication requires InnoDB repeatable read |
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.
I think some elaboration is needed in this comment:
- As the transaction is already started in
Wsrep_high_priority_service::start_transaction(), is the change ofthd->tx_isolationfully effective here or does it just suppress some server level checks? - Is it possible that the write set contains both row and query events? In this case a row event could start a transaction with
ISO_READ_COMMITTEDand the following query event would be actually executed with read-committed isolation as thethd->tx_isolationis effective only when the transaction is started. - Should this apply only to DML or also to DDLs (which are replicated as TOI)?
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.
I added a test that shows that mixed replication works for appliers, but I don't know if it's just a suppression of some server level checks or a true isolation level change. I would expect the former as InnoDB transaction is the same throughout the duration of a server transaction, and it seems that the isolation level is assigned to it only once from the beginning.
Regarding TOI, as it uses query events (statement-based replication), now it will run with REPEATABLE_READ.
31f544d to
7543684
Compare
mariadb-TeemuOllakka
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.
Looks good
Every Wsrep system thread should run with READ_COMMITTED transaction isolation level to prevent issues caused by InnoDB gap locks. The exception is statement-based replication for appliers, where REPEATABLE_READ is required by the server code. To account for that, set the isolation level before every applied event. It won't affect an already running transaction, but allows to handle both statement- and row-based replications accordingly. However, the problem might arise with the mixed replication format. Apart from that, there was a separate issue with applier thread vars: 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.
7543684 to
4d089e7
Compare
Description
Every Wsrep system thread should run with READ_COMMITTED transaction isolation level to prevent issues caused by InnoDB gap locks.
The exception is statement-based replication for appliers, where REPEATABLE_READ is required by the server code. To account for that, set the isolation level before every applied event. It won't affect an already running transaction, but allows to handle both statement- and row-based replications accordingly. However, the problem might arise with the mixed replication format.
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.
Release Notes
TODO: What should the release notes say about this change?
Include any changed system variables, status variables or behaviour. Optionally list any https://mariadb.com/kb/ pages that need changing.
How can this PR be tested?
MTR test is updated.
Basing the PR against the correct MariaDB version
mainbranch.PR quality check