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

HA#realize(): avoid deadlocks via exclusive locking, i.e. SELECT ... FOR UPDATE #788

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Al2Klimov
Copy link
Member

@Al2Klimov Al2Klimov commented Aug 12, 2024

In its transaction, this method always performs a SELECT ... <lock clause> and at least one upsert in the same table. If two instances aquire shared locks, those will deadlock each other during the following upsert.

Tests (edited)

  • With LOCK IN SHARE MODE the SELECT results in the row of the other instance and the INSERT results in a deadlock.
  • With FOR UPDATE the SELECT is blocked! Therefore, we would not perform an INSERT if the other instance is considered active.

In my opinion, FOR UPDATE should be used as it locks stricter and prevents the deadlocks of the INSERTs.

Also, I just started 10 Icinga DB instances each for LOCK IN SHARE MODE and FOR UPDATE and got no deadlocks with FOR UPDATE, but all the time with LOCK IN SHARE MODE.

For completeness, the "INSERT" in question contains "ON DUPLICATE KEY UPDATE" which makes it the "upsert" I mentioned.

Anyway, I'd like to properly use locking as recommended per https://dev.mysql.com/doc/refman/5.7/en/innodb-locking-reads.html

…FOR UPDATE

In its transaction, this method always performs a SELECT ... <lock clause>
and at least one upsert in the same table. If two instances aquire shared locks,
those will deadlock each other during the following upsert.
@cla-bot cla-bot bot added the cla/signed label Aug 12, 2024
@julianbrost
Copy link
Contributor

I have a hard time understanding what the goal of this PR is.

avoid deadlocks via exclusive locking, i.e. SELECT ... FOR UPDATE

Before-behavior (i.e. LOCK IN SHARE MODE)

[...]

  • ERROR 1213 (40001): Deadlock found when trying to get lock; try restarting transaction

[...]

After-behavior (i.e. s/LOCK IN SHARE MODE/FOR UPDATE/)

Same result.

Then how does this avoid deadlocks?

Anyway, I'd like to properly use locking as recommended per dev.mysql.com/doc/refman/5.7/en/innodb-locking-reads.html

Where exactly does it recommend that?

@Al2Klimov
Copy link
Member Author

It recommends it if one needs exclusive locking. And IMAO our deadlocks come from shared locking.

@yhabteab
Copy link
Member

yhabteab commented Sep 6, 2024

And IMAO our deadlocks come from shared locking.

You're contradicting yourself! Your PR summary was same result, which to me indicates it's not worth wasting any more time on it.

@oxzi
Copy link
Member

oxzi commented Sep 6, 2024

Based on the summary within the MySQL Locking Reads documentation, this PR makes sense:

  • SELECT ... FOR SHARE
    Sets a shared mode lock on any rows that are read. Other sessions can read the rows, but cannot modify them until your transaction commits. If any of these rows were changed by another transaction that has not yet committed, your query waits until that transaction ends and then uses the latest values.
  • SELECT ... FOR UPDATE
    For index records the search encounters, locks the rows and any associated index entries, the same as if you issued an UPDATE statement for those rows. Other transactions are blocked from updating those rows, from doing SELECT ... FOR SHARE, or from reading the data in certain transaction isolation levels.

But the "After-behavior: Same result." makes me question things.

@oxzi
Copy link
Member

oxzi commented Sep 6, 2024

How about trying to add a NOWAIT, like … FOR UPDATE NOWAIT. Due to the retry logic with the backoff interval, the first query should fail in case of an active lock, wait for a short time period and retry until waiting for some lock to be released.

@yhabteab
Copy link
Member

yhabteab commented Sep 6, 2024

How about trying to add a NOWAIT, like … FOR UPDATE NOWAIT.

Isn't that new MySQL/MariaDB feature?

Server version: 10.2.44-MariaDB-1:10.2.44+maria~bionic mariadb.org binary distribution
...
ERROR 1064 (42000): You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near 'NOWAIT' at line 1

@oxzi
Copy link
Member

oxzi commented Sep 6, 2024

How about trying to add a NOWAIT, like … FOR UPDATE NOWAIT.

Isn't that new MySQL/MariaDB feature?

Seems to be unavailable in MySQL 5.7 and MariaDB prior to version 10.3.

Are there still any supported distributions shipping those versions? AlmaLinux 8 (being my RHEL-reference) ships, for example, some MariaDB 10.3, 10.5 and 10.11 according to repology. I have only found Amazon 2 Linux as a problem child with some ancient MySQL version.

@Al2Klimov
Copy link
Member Author

And IMAO our deadlocks come from shared locking.

You're contradicting yourself! Your PR summary was same result, which to me indicates it's not worth wasting any more time on it.

Admittedly it looks this way, but I hope that I just haven't found a proper test, yet.

FWIW, SLES 12.5 installs mariadb-10.2.44-3.53.1.x86_64 by default, but I guess we can ignore this.

@lippserd
Copy link
Member

lippserd commented Oct 2, 2024

I don't think you have tested the scenario with the non-empty instance table correctly:

  • With LOCK IN SHARE MODE the SELECT results in the row of the other instance and the INSERT results in a deadlock.
  • With FOR UPDATE the SELECT is blocked! Therefore, we would not perform an INSERT if the other instance is considered active.

In my opinion, FOR UPDATE should be used as it locks stricter and prevents the deadlocks of the INSERTs. But please create a new PR with proper tests and a proper description that is actually understandable and explains the state (non-empty vs empty) before and after queries, and also post all queries instead of this s// foo.

Also, I just started 10 Icinga DB instances each for LOCK IN SHARE MODE and FOR UPDATE and got no deadlocks with FOR UPDATE, but all the time with LOCK IN SHARE MODE.

@yhabteab
Copy link
Member

Ping @Al2Klimov

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants