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

gh-ost does not define a transaction isolation level #1155

Closed
timvaillancourt opened this issue Jul 30, 2022 · 7 comments · Fixed by #1156
Closed

gh-ost does not define a transaction isolation level #1155

timvaillancourt opened this issue Jul 30, 2022 · 7 comments · Fixed by #1156

Comments

@timvaillancourt
Copy link
Collaborator

gh-ost does not define a transaction isolation level when it sets up new connections in go/mysql/connection.go. This means that gh-ost "trusts" the safety of the default isolation level of the MySQL server

In most situations this is probably a safe assumption, but I believe this could bite us if the server default is READ_UNCOMMITTED. This isolation could cause the min/max queries for row copying to return un-comitted rows. These rows may never actually be committed (due to ROLLBACK) or committed later than when they're read 😱

Luckily, the transaction isolation can be defined at a per-connection level. Setting a transaction isolation at a per-connection level allows us to be certain we're using the right isolation while allowing users to default to more-relaxed isolations. I would like to take that approach

There are 2 x isolations I can see fitting for gh-ost:

  1. REPEATABLE_READ - this is the default for MySQL and is probably the most commonly used. It's snapshot-level isolation and gap-locking may be more strict than gh-ost really needs, however. This isolation can cause more redo/undo logging as well, although it may be negligible in this use-case
  2. READ_COMMITTED - this always returns what has been COMMIT-ed to disk (not a snapshot in time) and disables gap-locking. As gh-ost uses autocommit=true our transactions are a single operation only (BEGIN => a single-operation => COMMIT), so I believe there would be no major difference to "visibility" of rows returned in this use case. If we did several queries per transaction (we don't) there would be a major difference, however. So in practice I think this isolation would just cause less overhead to the server, not a change to what rows are visible. Please correct me if I'm wrong here!

REPEATABLE_READ is the safest bet, but I am curious if the community has feedback on the feasibility of READ_COMMITTED becoming the default isolation for gh-ost, and if there are any gotchas/benefits I missed above 🤔

More on the differences between these two isolations in this great Percona Blog 👍

cc @dm-2 / @rashiq / @shlomi-noach / @ajm188 for thoughts/validation here 🙇

@dm-2
Copy link
Contributor

dm-2 commented Aug 8, 2022

This is a tricky one!

I would be cautious about making the transaction isolation level user-configurable when we don't have a good way to reliably test that gh-ost behaves as expected with READ_COMMITTED, and because all testing that we do today is with the default of REPEATABLE_READ. It's possible that people who choose to use READ_COMMITTED may encounter some subtle data corruption bugs, which would also be really difficult to debug.

I'd be in favour of:

  • explicitly setting the transaction isolation level to REPEATABLE_READ as it's the safest bet
  • not making the transaction isolation level user-configurable (we could always revisit this in the future if there's a need for using other transaction isolation levels)
  • documenting that gh-ost always uses REPEATABLE_READ

@timvaillancourt what do you think of that approach?

@timvaillancourt
Copy link
Collaborator Author

timvaillancourt commented Aug 10, 2022

I would be cautious about making the transaction isolation level user-configurable when we don't have a good way to reliably test that gh-ost behaves as expected with READ_COMMITTED, and because all testing that we do today is with the default of REPEATABLE_READ.

@dm-2 that's a great point 👍. I've added a localtests/ for READ_COMMITTED but I suppose it's possible that it's not telling us the full story with just a few rows/writes

I'd be in favour of:

  • explicitly setting the transaction isolation level to REPEATABLE_READ as it's the safest bet
  • not making the transaction isolation level user-configurable (we could always revisit this in the future if there's a need for using other transaction isolation levels)
  • documenting that gh-ost always uses REPEATABLE_READ

I think that's a reasonable approach for now 👍

Assuming READ_COMMITTED has the same integrity guarantees, I think the only thing we will miss-out on is reduced gap locking other queries will momentarily wait for while row-copy queries execute, but that's nothing new

@timvaillancourt
Copy link
Collaborator Author

@dm-2 the approach discussed above is now in #1156

@EagleEyeJohn
Copy link
Contributor

Hi @timvaillancourt

I've ended up here to see if we can alleviate the pain from gh-ost 1.1.5 starting, and whilst I have the information to hand thought it might be worth documenting some real world experience.

Despite a default isolation of READ-COMMITTED (in production use for many years)

select @@transaction_isolation\G
*************************** 1. row ***************************
@@transaction_isolation: READ-COMMITTED

when gh-ost opts to do

SELECT keyId FROM SomeTable ORDER BY keyId ASC LIMIT 1;
SELECT keyId FROM SomeTable ORDER BY keyId DESC LIMIT 1;
:
2023-11-20 06:47:09 INFO Table found. Engine=InnoDB
2023-11-20 06:47:10 INFO Estimated number of rows via EXPLAIN: 4068373190
2023-11-20 06:47:10 INFO Recursively searching for replication master
:
2023-11-20 06:53:44 INFO Chosen shared unique key is PRIMARY
:
2023-11-20 06:53:51 INFO Intercepted changelog state ReadMigrationRangeValues
2023-11-20 06:53:51 INFO Handled changelog state ReadMigrationRangeValues
2023-11-20 09:52:52 INFO Migration min values: [1]
2023-11-20 13:01:28 INFO Migration max values: [5983227227]
2023-11-20 13:01:28 INFO Waiting for first throttle metrics to be collected
2023-11-20 13:01:28 INFO First throttle metrics collected
:

this took over 6 hours and caused significant impact to history length and purging

image

That makes me think that the default isolation READ-COMMITTED is not being respected?

The same queries could be rewritten as follows, completely avoiding the impact altogether

SELECT MIN(keyId), MAX(keyId) FROM SomeTable;

I had assumed that gh-ost wasn't doing that in order to get accurate rows examined statistic, but the first Copy log line proceeds to start at

Copy: 0/4068373190 0.0%; Applied: 0; ...

which uses the estimated row count anyway.

So in terms of feedback, we'd most definitely support being able to set isolation to READ-COMMITTED.

@timvaillancourt
Copy link
Collaborator Author

@EagleEyeJohn a new --transaction-isolation flag was added recently. This allows READ-COMMITTED to be defined, but no release has been cut yet. Your confirmation would be much appreciated 🙇

@EagleEyeJohn
Copy link
Contributor

Hi @timvaillancourt

Thank you, I will try to find some time to test that out.

We solved our history length issue (#1155 (comment)) when gh-ost obtains the smallest and largest key values using ORDER BY, by:

  1. Executing SET GLOBAL optimizer_switch = "prefer_ordering_index=on" immediately before starting gh-ost on master instance.
  2. Using the hook gh-ost-on-before-row-copy to execute SET GLOBAL optimizer_switch = "prefer_ordering_index=off" to reset it to our default value.

It would be helpful if gh-ost could detect that setting, and if 'off', override it to 'on' within its session.

@timvaillancourt
Copy link
Collaborator Author

@EagleEyeJohn that's pretty interesting. Ordering the index makes a lot of sense. By default this is on correct?

If we want this to apply to just 1-2 queries I suggest a SET_VAR() MySQL optimizer hint is used instead of set/resetting the whole session. Or do you see this benefitting other areas of the code? I'll think on it 🤔

I believe this can be achieved by:
SELECT /*+ SET_VAR(prefer_ordering_index=on) */ * FROM ...

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

Successfully merging a pull request may close this issue.

3 participants