Extend Transaction::GetForUpdate with do_validate #4680

maysamyabandeh · 2018-11-14T19:35:04Z

Transaction::GetForUpdate is extended with a do_validate parameter with default value of true. If false it skips validating the snapshot (if there is any) before doing the read. After the read it also returns the latest value (expects the ReadOptions::snapshot to be nullptr). This allows RocksDB applications to use GetForUpdate similarly to how InnoDB does. Similarly ::Merge, ::Put, ::Delete, and ::SingleDelete are extended with assume_exclusive_tracked with default value of false. It true it indicates that call is assumed to be after a ::GetForUpdate(do_validate=false).
The Java APIs are accordingly updated.

facebook-github-bot

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

facebook-github-bot · 2018-11-14T22:48:05Z

@maysamyabandeh has updated the pull request. Re-import the pull request

facebook-github-bot · 2018-11-15T00:45:33Z

@maysamyabandeh has updated the pull request. Re-import the pull request

facebook-github-bot · 2018-11-15T22:54:05Z

@maysamyabandeh has updated the pull request. Re-import the pull request

facebook-github-bot · 2018-11-16T20:15:57Z

@maysamyabandeh has updated the pull request. Re-import the pull request

facebook-github-bot · 2018-12-04T00:28:28Z

@maysamyabandeh has updated the pull request. Re-import the pull request

facebook-github-bot · 2018-12-04T00:33:02Z

@maysamyabandeh has updated the pull request. Re-import the pull request

facebook-github-bot

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

siying · 2018-12-05T18:27:30Z

utilities/transactions/transaction_base.cc

-    s = Get(read_options, column_family, key, &pinnable_val);
+    if (skip_validate && read_options.snapshot != nullptr) {
+      ReadOptions ro(read_options);
+      ro.snapshot = nullptr;


Can you explain why we need to do it?

skip_validate has two functions: skip validating snapshot, which is done by passing it to TryLock, read the latest committed value independent of what the snapshot is. The latter is missing in the inline documentation. If the explanation above is satisfaction, I go ahead with improving the inline docs with that.

If the caller expects to read the latest, not from the snapshot, should they not set read_options.snapshot? Users passing in a snapshot but we have an argument to skip it is too complicated to me.

@hermanlee @lth would it work if we return error if snapshot and skip_validate both are set?

Yes, that would be fine. If skip_validate is set, then snapshot should be null.

siying · 2018-12-05T18:27:48Z

utilities/transactions/transaction_base.cc

-    s = Get(read_options, column_family, key, pinnable_val);
+    if (skip_validate && read_options.snapshot != nullptr) {
+      ReadOptions ro(read_options);
+      ro.snapshot = nullptr;


Same question here. Why do we need to do this.

same as above

siying · 2018-12-05T18:31:04Z

include/rocksdb/utilities/transaction.h


  // An overload of the above method that receives a PinnableSlice
  // For backward compatibility a default implementation is provided
  virtual Status GetForUpdate(const ReadOptions& options,
                              ColumnFamilyHandle* column_family,
                              const Slice& key, PinnableSlice* pinnable_val,
-                              bool exclusive = true) {
+                              bool exclusive = true,
+                              bool skip_validate = false) {


Callaghan had a suggestion which might loosely apply here: avoid argument/option name which contains negative in it, because if it is false, it is double negative and makes it difficulty for users to tune. "Skip" is kind of such thing. skip_validate = false is harder to understand than something like do_validation = false.

facebook-github-bot · 2018-12-05T23:22:23Z

@maysamyabandeh has updated the pull request. Re-import the pull request

facebook-github-bot

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

siying · 2018-12-06T00:02:13Z

utilities/transactions/transaction_base.cc

+                                         bool exclusive,
+                                         const bool do_validate) {
+  assert(do_validate || read_options.snapshot == nullptr);
+  if (UNLIKELY(!do_validate && read_options.snapshot != nullptr)) {


As always, I'm not a fun of UNLIKELY hint. Do we really need it?

I have seen cases that it improves performance and here it might too. I do not see it to be hurtful in anyway.

It is hurtful because it tries to dictate compiler behavior unnecessarily. It's just like that we don't encourage people to deep tune RocksDB behavior unless it's needed, I am not a fan of doing it to compiler either.

Furthermore, just have this macro inside the code for no reason makes it less readable. Imagine someone grabs RocksDB source code and read this function, and sees a "UNLIKELY()".

I believe it is a "hint" rather than "dictation". And it is not "unnecessary" as the previous benchmarks has shown. I actually find them to make the code more readable if they are used properly. Here is a proper use case as the if-then branch is not taken unless there is a user bug.

I even feel that we should take the hit even we see some minor regression. "Previous benchmark" is even weaker argument. It's more readable to you because you wrote so many UNLIKELY() in the code base so that you are so familiar with what it is for when you see it. It's not the case for someone new to the code base. In fact, I'm always reluctant to the use of macros in general. This is also the rule written in the Google C++ Style we are following: https://google.github.io/styleguide/cppguide.html#Preprocessor_Macros

I am going to remove UNLIKELY here as I do not want this minor point to become a blocker for this PR.

@maysamyabandeh I appreciate it.

siying · 2018-12-06T00:03:07Z

utilities/transactions/transaction_base.cc

-  Status s = TryLock(column_family, key, true /* read_only */, exclusive);
+                                         bool exclusive,
+                                         const bool do_validate) {
+  assert(do_validate || read_options.snapshot == nullptr);


I don't think we should assert user input. Crashing user program (while running in debug mode) when passing invalid argument doesn't feel like something we should do.

This is undefined behavior in the API and assert is a way to specify the assumption on the input. We leave the runtime error return just in case there was a corner case that we did not anticipate but during the test we would prefer a highly visible crash over a probably silent error return.
I let @hermanlee , @lth, and @yoshinorim chime in if they prefer it differently.

Assuming assert is debug only, then this is fine. I would prefer it return an error on non-debug builds though.

Correct. The next line returns the error.

We can't assume that RocksDB users never run production with debug build. It has happened and is very likely to be happening somewhere too. Turning user input validation into application crashing will prevent users to do it.

@maysamyabandeh why is it an undefined behavior? The behavior is deterministic: return error to users.

Why is it easier for users to find the corner case by crashing the program, than return them an error message telling them what mistake they have made?

I let MyRocks team as a user of rocksdb chime in of what behavior would make their life easier.

It's not about this concrete case or concrete user. It's about the general principle we follow in this code base. It needs to be all consistent in the code base.

The PR is prepared for MyRocks primarily. I think it is fair to hear out their experience as the user before making strong opinions about carved-in-stone principles.

We can hear it as an input for revisiting the model of crashing application while users provide invalid arguments, but I won't change opinion in here, even if they say they love it a lot.

facebook-github-bot · 2018-12-06T04:05:21Z

@maysamyabandeh has updated the pull request. Re-import the pull request

facebook-github-bot

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

lth · 2018-12-06T17:30:00Z

utilities/transactions/pessimistic_transaction.cc

+        s = Status::InvalidArgument(
+            "assume_exclusive_tracked is set but it is not tracked yet");
+      } else if (lock_upgrade) {
+        s = Status::InvalidArgument(


Discussed offline. Shared locking should be enough. The goal of validation is just to check whether writes have happened, and a shared lock would be able to prevent that.

hermanlee · 2018-12-06T17:30:41Z

HISTORY.md

@@ -3,6 +3,7 @@
 ### New Features

 ### Public API Change
+* Transaction::GetForUpdate is extended with a do_validate parameter with default value of true. If false it skips validating the snapshot before doing the read. Similar ::Merge, ::Put, ::Delete, and ::SingleDelete are extended with assume_exclusive_tracked with default value of false. It true it indicates that call is assumed to be after a ::GetForUpdate(do_validate=false).


Nit: typo It -> If

hermanlee · 2018-12-06T17:35:22Z

utilities/transactions/transaction_base.cc

-  Status s = TryLock(column_family, key, true /* read_only */, exclusive);
+                                         bool exclusive,
+                                         const bool do_validate) {
+  assert(do_validate || read_options.snapshot == nullptr);


Assuming assert is debug only, then this is fine. I would prefer it return an error on non-debug builds though.

hermanlee · 2018-12-06T17:52:18Z

utilities/transactions/pessimistic_transaction.cc

@@ -559,7 +561,18 @@ Status PessimisticTransaction::TryLock(ColumnFamilyHandle* column_family,
  // any writes since this transaction's snapshot.
  // TODO(agiardullo): could optimize by supporting shared txn locks in the
  // future
-  if (skip_validate || snapshot_ == nullptr) {
+  if (!do_validate || snapshot_ == nullptr) {
+    assert(!assume_exclusive_tracked || (previously_locked && !lock_upgrade));


Seems like this assert could just be in an additional else clause after "else if (lock_upgrade)" below.

I updated the if-then clause. Let me know if the update has addressed your comment. I kept assert before the if-then clause so that we crash during the tests and the corner cases that have violated our assumptions would show up in tests.

hermanlee · 2018-12-06T17:55:21Z

utilities/transactions/pessimistic_transaction.cc

-                                       bool exclusive, bool skip_validate) {
+                                       bool exclusive, const bool do_validate,
+                                       const bool assume_exclusive_tracked) {
+  assert(!assume_exclusive_tracked || (!do_validate && exclusive));


I think Manuel mentioned acquiring read locks should be able to set do_validate = false if we want to retrieve the value with snapshot set to nullptr.

Correct. The latest PR is updated with manuel's comment. Let me know if the latest is correct.

facebook-github-bot · 2018-12-06T18:51:26Z

@maysamyabandeh has updated the pull request. Re-import the pull request

siying

This logic makes sense by itself. I'll defer to @lth @yoshinorim @hermanlee to decide whether it makes sense to MyRocks and accept.

facebook-github-bot · 2018-12-06T19:43:14Z

@maysamyabandeh has updated the pull request. Re-import the pull request

hermanlee

Changes seems fine with me. I'll let Manuel have the final approval.

hermanlee · 2018-12-06T19:45:25Z

utilities/transactions/pessimistic_transaction.cc

-  if (skip_validate || snapshot_ == nullptr) {
+  if (!do_validate || snapshot_ == nullptr) {
+    assert(!assume_tracked || previously_locked);
+    if (assume_tracked) {


Minor suggestion. Seems like this could be rewritten as:

if (assume_tracked && !previous_locked) {
s = Status::InvalidArgument();
}

facebook-github-bot · 2018-12-06T20:17:15Z

@maysamyabandeh has updated the pull request. Re-import the pull request

facebook-github-bot · 2018-12-06T22:07:05Z

@maysamyabandeh has updated the pull request. Re-import the pull request

siying

LGTM

lth · 2018-12-06T23:09:32Z

HISTORY.md

@@ -3,6 +3,7 @@
 ### New Features

 ### Public API Change
+* Transaction::GetForUpdate is extended with a do_validate parameter with default value of true. If false it skips validating the snapshot before doing the read. Similarly ::Merge, ::Put, ::Delete, and ::SingleDelete are extended with assume_tracked with default value of false. If true it indicates that call is assumed to be after a ::GetForUpdate(do_validate=false).


I think it's more accurate to say that assume_tracked means the call is assumed to be called after any GetForUpdate call right?

lth · 2018-12-06T23:12:56Z

utilities/transactions/transaction_base.cc

-  Status s = TryLock(column_family, key, true /* read_only */, exclusive);
+                                         bool exclusive,
+                                         const bool do_validate) {
+  if (UNLIKELY(!do_validate && read_options.snapshot != nullptr)) {


Why do we have UNLIKELY only in one place?

lth · 2018-12-06T23:13:40Z

utilities/transactions/transaction_base.cc

-                                  const Slice& key, const Slice& value) {
+                                  const Slice& key, const Slice& value,
+                                  const bool assume_exclusive_tracked) {
+  const bool do_validate = !assume_exclusive_tracked;


Can we rename to just assume_tracked, if we agree that shared locks also imply that we can skip validation?

facebook-github-bot · 2018-12-06T23:30:08Z

@maysamyabandeh has updated the pull request. Re-import the pull request

facebook-github-bot · 2018-12-06T23:34:33Z

@maysamyabandeh has updated the pull request. Re-import the pull request

lth

LGTM

facebook-github-bot

@maysamyabandeh is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

yoshinorim · 2018-12-10T11:59:55Z

include/rocksdb/utilities/transaction.h

@@ -287,6 +293,9 @@ class Transaction {
  // functions in WriteBatch, but will also do conflict checking on the
  // keys being written.
  //
+  // assume_tracked=false expects the key be already tracked. If valid then it


I think you meant assume_tracked=true here.

oops. will fix it.

include/rocksdb/utilities/transaction.h

Summary: Fix the definition of assumed_tracked in Transaction that was introduced in #4680 Pull Request resolved: #4762 Differential Revision: D13399150 Pulled By: maysamyabandeh fbshipit-source-id: 2a30fe49e3c44adacd7e45cd48eae95023ca9dca

adamretter · 2018-12-11T03:39:23Z

java/src/main/java/org/rocksdb/Transaction.java

@@ -834,11 +909,23 @@ public void delete(final byte[] key) throws RocksDBException {
   * @throws RocksDBException when one of the TransactionalDB conditions
   *     described above occurs, or in the case of an unexpected error
   */
+  public void delete(final ColumnFamilyHandle columnFamilyHandle, final byte[][] keyParts,


Could you please rename assume_tracked to assumeTracked so it follows the Java naming conventions.

Could you please add the javadoc for the additional parameter assumeTracked.

This occurs in several places, could you correct each please?

adamretter · 2018-12-11T03:42:38Z

java/src/main/java/org/rocksdb/Transaction.java

@@ -433,6 +433,33 @@ public void rollbackToSavePoint() throws RocksDBException {
   * @param key the key to retrieve the value for.
   * @param exclusive true if the transaction should have exclusive access to
   *     the key, otherwise false for shared access.
+   * @param do_validate true if it should validate the snapshot before doing the read


Could you please rename do_validate to doValidate so it follows the Java naming conventions.

Could you please add the javadoc for the additional parameter doValidate on the other methods which are missing it?

This occurs in several places, could you correct each please?

maysamyabandeh requested review from yoshinorim, lth, yiwu-arbug and siying November 14, 2018 19:35

facebook-github-bot added the CLA Signed label Nov 14, 2018

facebook-github-bot reviewed Nov 14, 2018

View reviewed changes

maysamyabandeh force-pushed the getforupdate-skipvalidate branch from 22ba753 to 6f0ab51 Compare December 4, 2018 00:32

facebook-github-bot reviewed Dec 4, 2018

View reviewed changes

siying reviewed Dec 5, 2018

View reviewed changes

facebook-github-bot reviewed Dec 5, 2018

View reviewed changes

siying reviewed Dec 6, 2018

View reviewed changes

maysamyabandeh added 11 commits December 5, 2018 20:06

extend GetForUpdate with skip_validate

4590fed

the rest of the changes

b1e221c

update history

8f5e290

updata java

39cd1dd

return latest values if skip_validate

9c22a0b

minor fix

9ba8b00

extend writes with assume_exclusive_tracked

bf046b2

make format

e7d4c49

add unit test

fcb4dc7

fix java failures

7fbdad7

return error if skip_validate and snapshot are both set

9898410

facebook-github-bot reviewed Dec 6, 2018

View reviewed changes

maysamyabandeh changed the title ~~Extend Transaction::GetForUpdate with skip_validate~~ Extend Transaction::GetForUpdate with do_validate Dec 6, 2018

lth reviewed Dec 6, 2018

View reviewed changes

hermanlee reviewed Dec 6, 2018

View reviewed changes

assume_exclusive_tracked -> assume_tracked

1ccc46a

siying reviewed Dec 6, 2018

View reviewed changes

apply comments

fa56e6c

hermanlee reviewed Dec 6, 2018

View reviewed changes

remove UNLIKELY

cef4c72

remove assert

b17c7b4

siying reviewed Dec 6, 2018

View reviewed changes

lth reviewed Dec 6, 2018

View reviewed changes

maysamyabandeh added 2 commits December 6, 2018 15:27

rename the remaining assume_exclusive_tracked

386fc22

remove another UNLIKELY

68894ee

Update HISTORY

b3e2c3c

lth approved these changes Dec 7, 2018

View reviewed changes

facebook-github-bot reviewed Dec 7, 2018

View reviewed changes

facebook-github-bot closed this in b878f93 Dec 7, 2018

yoshinorim reviewed Dec 10, 2018

View reviewed changes

include/rocksdb/utilities/transaction.h Show resolved Hide resolved

maysamyabandeh mentioned this pull request Dec 10, 2018

Fix inline comments for assumed_tracked #4762

Closed

adamretter reviewed Dec 11, 2018

View reviewed changes

Extend Transaction::GetForUpdate with do_validate #4680

Extend Transaction::GetForUpdate with do_validate #4680

Conversation

maysamyabandeh commented Nov 14, 2018 • edited

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Nov 14, 2018

facebook-github-bot commented Nov 15, 2018

facebook-github-bot commented Nov 15, 2018

facebook-github-bot commented Nov 16, 2018

facebook-github-bot commented Dec 4, 2018

facebook-github-bot commented Dec 4, 2018

facebook-github-bot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Dec 5, 2018

facebook-github-bot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

siying Dec 6, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Dec 6, 2018

facebook-github-bot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Dec 6, 2018

siying left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Dec 6, 2018

hermanlee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Dec 6, 2018

facebook-github-bot commented Dec 6, 2018

siying left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Dec 6, 2018

facebook-github-bot commented Dec 6, 2018

lth left a comment

Choose a reason for hiding this comment

facebook-github-bot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maysamyabandeh commented Nov 14, 2018 •

edited

siying Dec 6, 2018 •

edited