Skip to content
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

before_command() wait for ongoing rollbacks leaks #23

Open
sjaakola opened this issue Nov 23, 2018 · 0 comments
Open

before_command() wait for ongoing rollbacks leaks #23

sjaakola opened this issue Nov 23, 2018 · 0 comments
Assignees
Labels
bug Something isn't working

Comments

@sjaakola
Copy link
Contributor

wsrep::client_state::before_command() has a wait loop to make the client execution to pause until external rollbacking of client's transaction has completed.

External rollbacker, sets clients transaction state to aborting, and the wait loop checks if transaction state is aborting:
while (transaction_.state() == wsrep::transaction::s_aborting)
{
cond_.wait(lock);
}
rollbacker sets the transaction state to aborted, in transaction::after_rollback(). This happens first, and later in rollbacker execution client_state::sync_rollback_complete() is called and the cond_ signal is sent then.
This sequence has a race condition: if rollbacker has called for transaction::after_rollback(), but not yet called client_state::sync_rollback_complete(), incoming client will pass the before_command() wait loop, and after that both client and rollbacker will operate on same client state until rollbacker completes.
This race condition accounts for some sporadic failures with multi-master conflict testing, e.g. with galera.galera_FK_duplicate_client_insert

@sjaakola sjaakola self-assigned this Nov 23, 2018
@sjaakola sjaakola added the bug Something isn't working label Nov 23, 2018
sjaakola added a commit that referenced this issue Nov 24, 2018
Storing background rollbacker thread's ID in client state.
Tis can be used for detecting if there is ongoing background rollback,
and client should keep waiting in before_command() entry to avoid conflicts
in accessing client state during background rollbacking.

There is new public method for client_state: prepare_for_background_rollback()
Background rollbacker thread should call this, before backround rollbacking
starts, to assign himself as the rollbacker for the client.

sync_rollback_complete() method has been modified to release the backround
rollbacker thread from client_state
sjaakola added a commit that referenced this issue Nov 26, 2018
Storing background rollbacker thread's ID in client state.
Tis can be used for detecting if there is ongoing background rollback,
and client should keep waiting in before_command() entry to avoid conflicts
in accessing client state during background rollbacking.

There is new public method for client_state: prepare_for_background_rollback()
Background rollbacker thread should call this, before backround rollbacking
starts, to assign himself as the rollbacker for the client.

sync_rollback_complete() method has been modified to release the backround
rollbacker thread from client_state
sjaakola added a commit that referenced this issue Nov 26, 2018
Fix accordng to Teemu's review. There remained a race condition, where
victim client could enter before_command(), if it arrived just after
s_aborting state was set in transaction::bf_abort()
sjaakola added a commit that referenced this issue Nov 26, 2018
Fixes acording to review, changed client_state's rollbacker_thread_id to be boolean flag: has_rollback_,
which is now set directly from transaction::bf_abort()

With this, there is no need for public method to designate when background rollbacking is fired.
The changed behavior of transction:bf_abort(), triggered many failures for unit testing (mostly assert for
unexpected client state). This caused need for unit test tuning, which is kept to minimal in this commit.
sjaakola added a commit that referenced this issue Nov 27, 2018
Storing information that background rollbacker in ongoing in client state has_rollback_
This can be used for detecting if there is ongoing background rollback,
and client should keep waiting in before_command() entry to avoid conflicts
in accessing client state during background rollbacking.

transaction::bf_abort() is modified to set has_rollback_ flag when
backgroung rollbacking has been assigned for the client

sync_rollback_complete() method has been modified to reset the backround
rollbacker flag
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

1 participant