Set logs as getting flushed before releasing lock, race condition fix #1909
Conversation
reidHoruff
changed the title
|
@reidHoruff has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
db/db_impl.cc
Outdated
| @@ -5058,6 +5058,26 @@ void DBImpl::MaybeFlushColumnFamilies() { | |||
| return; | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
This seems equivalent to lines 5050-5079 but shorter and (imo) clearer:
if (allow_2pc() &&
oldest_log_with_uncommited_prep > 0 &&
oldest_log_with_uncommited_prep <= oldest_alive_log) {
if (unable_to_flush_oldest_log_) {
// we already attempted to flush all column families dependent on
// the oldest alive log but the log still contained uncommited transactions.
// the oldest alive log STILL contains uncommited transaction so there
// is still nothing that we can do.
return;
} else {
// the oldes log contains uncommitted transactions, so we won't be able to get rid of it just yet.
// let's flush its memtables anyway (btw, why?).
Log(InfoLogLevel::WARN_LEVEL, immutable_db_options_.info_log,
"Unable to release oldest log due to uncommited transaction");
unable_to_flush_oldest_log_ = true;
}
} else {
alive_log_files_.begin()->getting_flushed = true;
}
There was a problem hiding this comment.
let's flush its memtables anyway (btw, why?).
We flush the memtables we can now and the remaining tables later. It makes sense intuitively to me but maybe its not actually optimal behavior? It also reduces the chance of another transaction coming in and 'preparing' into a memtable that we could have flushed now will be unable to flush in the future. This guarantees that we are making progress on releasing the log.
db/db_impl.cc
Outdated
| @@ -5058,6 +5058,26 @@ void DBImpl::MaybeFlushColumnFamilies() { | |||
| return; | |||
| } | |||
|
|
|||
| // we only mark this log as getting flushed if we have successfully | |||
| // flushed all data in this log. If this log contains outstanding prepred | |||
There was a problem hiding this comment.
"prepred" - did you mean "prepared"?
|
@reidHoruff I already approved the diff. Are you going to find a way to get all the tests run and commit it? |
|
I will see that the sandcastle people have to say again. |
|
@reidHoruff has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
|
@reidHoruff maybe you can try to create another pull request just to trigger the tests. |
|
@reidHoruff updated the pull request - view changes - changes since last import |
|
@reidHoruff has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
Relating to #1903:
In MaybeFlushColumnFamilies() we want to modify the 'getting_flushed' flag before releasing the db mutex when SwitchMemtable() is called.
The following 2 actions need to be atomic in MaybeFlushColumnFamilies()
In the 2pc case:
T1 enters function but is unable to flush all CFs to release log
T1 sets unable_to_flush_oldest_log_ = true
T1 begins flushing all CFs possible
T2 enters function but is unable to flush all CFs to release log
T2 sees unable_to_flush_oldes_log_ has been set so exits
T3 enters function and will be able to flush all CFs to release oldest log
T3 sets getting_flushed = true on oldest log
T3 begins flushing all remaining CFs to release logs
Important piece of atomicity: Once we determine that we will be able to flush sufficient CFs to release the oldest log, we must set getting_flushed on that log BEFORE doing the flushing.