Fix backup/checkpoint stress test failure

[jowlyzhang]

This PR fixes this type of stress test failure that could happen in either checkpoint or backup. Example failure messages are like this:

Failure in a backup/restore operation with: Corruption: 0x00000000000001D5000000000000012B00000000000000FD exists in original db but not in restore

A checkpoint operation failed with: Corruption: 0x0000000000000365000000000000012B0000000000000067 exists in original db but not in checkpoint /...

The internal task has an example test command to quickly reproduce this type of error.

The common symptom of these test failures are these expected keys do not exist in the original db either. The root cause is TestCheckpoint and TestBackupRestore both use the expected state as a proxy for the state of the original db when it comes to check a key's existence.

rocksdb/db_stress_tool/db_stress_test_base.cc

Line 1838 in 0758271

bool exists = thread->shared->Exists(rand_column_families[i], rand_keys[0]);

This ExpectedState::Exists API returns true if a key has a pending write, such as a pending put. In usual case, this pending put should either soon materialize to an actual write when PendingExpectedValue::Commit is called to reflect a successful write to the DB, or test should be safely terminated if write to DB fails. All of which happens while a key is locked. So checkpoint and backup usually won't see the discrepancy between db and expected state caused by pending writes. However, the external file ingestion test currently has a path that will proceed the test after a failed ingestion caused by injected errors, leaving the pending put in the expected state.

rocksdb/db_stress_tool/no_batched_ops_stress.cc

Lines 1577 to 1589 in 0758271

	if (!s.ok()) {
	if (!s.IsIOError() || !std::strstr(s.getState(), "injected")) {
	fprintf(stderr, "file ingestion error: %s\n", s.ToString().c_str());
	thread->shared->SafeTerminate();
	} else {
	fprintf(stdout, "file ingestion error: %s\n", s.ToString().c_str());
	}
	} else {
	for (size_t i = 0; i < pending_expected_values.size(); ++i) {
	pending_expected_values[i].Commit();
	}
	}
	}

I think a proper and future proof fix for this is to explicitly rollback a pending state when a db write operation failed so that expected state do not diverge from db in the first place. I added a PendingExpectedValue::Rollback API so that we don't implicitly depend on thread termination to prevent test failures. Another place that could cause same divergence as external file ingestion is PreloadDbAndReopenAsReadOnly.

rocksdb/db_stress_tool/db_stress_test_base.cc

Lines 616 to 619 in 0758271

	pending_expected_value.Commit();
	if (!s.ok()) {
	break;
	}

[facebook-github-bot]

@jowlyzhang has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[hx235]

LGTM - I also wonder if it's possible to do some checking in ExpectedValue's destructor to see if Rollback() or Commit() is called. If the check is complicated, it could be left in a follow up PR.

[facebook-github-bot]

@jowlyzhang has updated the pull request. You must reimport the pull request before landing.

[facebook-github-bot]

@jowlyzhang has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[jowlyzhang]

LGTM - I also wonder if it's possible to do some checking in ExpectedValue's destructor to see if Rollback() or Commit() is called. If the check is complicated, it could be left in a follow up PR.

That's a good point. This can help make this more future proof. It looks straightforward from a glance, after looking more into it, I realize some intermediate methods could be creating PendingExpectedValue on other's behalf, for example ExpectedState::PrepareDeleteRange. So I need to look a bit more into other related changes for this, like enforcing a usage pattern for PendingExpectedValue besides the check in the destructor. I will do it in a follow up.

[facebook-github-bot]

@jowlyzhang merged this pull request in c4228ab.