Add an option to toggle timestamp based validation for the whole DB #12857
Conversation
|
@jowlyzhang has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
| if (!enable_udt_validation && kMaxTxnTimestamp != read_timestamp_) { | ||
| return Status::InvalidArgument( | ||
| "read_timestamp is set but timestamp validation is disabled for the " | ||
| "DB"); | ||
| } else if (enable_udt_validation && !do_validate && | ||
| kMaxTxnTimestamp != read_timestamp_) { | ||
| return Status::InvalidArgument( | ||
| "If do_validate is false then GetForUpdate with read_timestamp is not " | ||
| "defined."); | ||
| } else if (do_validate && kMaxTxnTimestamp == read_timestamp_) { | ||
| } else if (enable_udt_validation && do_validate && | ||
| kMaxTxnTimestamp == read_timestamp_) { | ||
| return Status::InvalidArgument("read_timestamp must be set for validation"); | ||
| } |
There was a problem hiding this comment.
Minor but some repeated checks could be eliminated via nesting:
if (!enable_udt_validation) {
if (kMaxTxnTimestamp != read_timestamp_) {
// ...
}
} else {
if (!do_validate) {
if (kMaxTxnTimestamp != read_timestamp_) {
// ...
}
} else {
if (kMaxTxnTimestamp == read_timestamp_) {
// ...
}
}
}
There was a problem hiding this comment.
Also, I feel it would be nice to proactively make the same change in GetEntityForUpdate too
There was a problem hiding this comment.
Thanks a lot for this suggestion! I included this into a SanityCheckReadTimestamp method to be shared by them too.
| // do_validate and set snapshot, execute sequence number based conflict | ||
| // checking and skip timestamp based conflict checking. |
There was a problem hiding this comment.
Would it make sense to test (using parallel txns) that timestamp based conflicts are not caught if the new option is false?
There was a problem hiding this comment.
That's a good idea! I added a test to write some out of order timestamps.
jowlyzhang
force-pushed
the
db_no_udt_validation
branch
from
1e5bbeb
to
34fdd51
Compare
|
@jowlyzhang has updated the pull request. You must reimport the pull request before landing. |
|
@jowlyzhang has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
@ltamasi thanks a lot for the review! Sorry for the delayed response. |
|
@jowlyzhang merged this pull request in 24d86f7. |
…12857) Summary: As titled. This PR adds a `TransactionDBOptions` field `enable_udt_validation` to allow user to toggle the timestamp based validation behavior across the whole DB. When it is true, which is the default value and the existing behavior. A recap of what this behavior is: `GetForUpdate` does timestamp based conflict checking to make sure no other transaction has committed a version of the key tagged with a timestamp equal to or newer than the calling transaction's `read_timestamp_` the user set via `SetReadTimestampForValidation`. When this field is set to false, we disable timestamp based validation for the whole DB. MyRocks find it hard to find a read timestamp for this validation API, so we added this flexibility. Pull Request resolved: #12857 Test Plan: Added unit test Reviewed By: ltamasi Differential Revision: D60194134 Pulled By: jowlyzhang fbshipit-source-id: b8507f8ddc37fc7a2948cf492ce5c599ae646fef
…12857) Summary: As titled. This PR adds a `TransactionDBOptions` field `enable_udt_validation` to allow user to toggle the timestamp based validation behavior across the whole DB. When it is true, which is the default value and the existing behavior. A recap of what this behavior is: `GetForUpdate` does timestamp based conflict checking to make sure no other transaction has committed a version of the key tagged with a timestamp equal to or newer than the calling transaction's `read_timestamp_` the user set via `SetReadTimestampForValidation`. When this field is set to false, we disable timestamp based validation for the whole DB. MyRocks find it hard to find a read timestamp for this validation API, so we added this flexibility. Pull Request resolved: #12857 Test Plan: Added unit test Reviewed By: ltamasi Differential Revision: D60194134 Pulled By: jowlyzhang fbshipit-source-id: b8507f8ddc37fc7a2948cf492ce5c599ae646fef
As titled. This PR adds a
TransactionDBOptionsfieldenable_udt_validationto allow user to toggle the timestamp based validation behavior across the whole DB. When it is true, which is the default value and the existing behavior. A recap of what this behavior is:GetForUpdatedoes timestamp based conflict checking to make sure no other transaction has committed a version of the key tagged with a timestamp equal to or newer than the calling transaction'sread_timestamp_the user set viaSetReadTimestampForValidation. When this field is set to false, we disable timestamp based validation for the whole DB. MyRocks find it hard to find a read timestamp for this validation API, so we added this flexibility.Test plan:
Added unit test