From 792ef10ca85e7525869838065cfe4d3a1558ee7f Mon Sep 17 00:00:00 2001
From: Yi Wu <yiwu@fb.com>
Date: Sat, 28 Oct 2017 21:56:50 -0700
Subject: [PATCH] Return Status::InvalidArgument if user request sync write
 while disabling WAL

Summary:
write_options.sync = true and write_options.disableWAL is incompatible. When WAL is disabled, we are not able to persist the write immediately. Return an error in this case to avoid misuse of the options.
Closes https://github.com/facebook/rocksdb/pull/3086

Differential Revision: D6176822

Pulled By: yiwu-arbug

fbshipit-source-id: 1eb10028c14fe7d7c13c8bc12c0ef659f75aa071
---
 HISTORY.md          |  1 +
 db/db_impl_write.cc |  5 ++++-
 db/db_test.cc       |  5 ++++-
 db/db_write_test.cc | 12 +++++++++++-
 4 files changed, 20 insertions(+), 3 deletions(-)

diff --git a/HISTORY.md b/HISTORY.md
index 351e0fc904..8a500037d2 100644
--- a/HISTORY.md
+++ b/HISTORY.md
@@ -3,6 +3,7 @@
 ### Public API Change
 * `BackupableDBOptions::max_valid_backups_to_open == 0` now means no backups will be opened during BackupEngine initialization. Previously this condition disabled limiting backups opened.
 * Deprecate trash_dir param in NewSstFileManager, right now we will rename deleted files to <name>.trash instead of moving them to trash directory
+* Return an error on write if write_options.sync = true and write_options.disableWAL = true to warn user of inconsistent options. Previously we will not write to WAL and not respecting the sync options in this case.
 
 ### New Features
 * `DBOptions::bytes_per_sync` and `DBOptions::wal_bytes_per_sync` can now be changed dynamically, `DBOptions::wal_bytes_per_sync` will flush all memtables and switch to a new WAL file.
diff --git a/db/db_impl_write.cc b/db/db_impl_write.cc
index 6cb748b4a3..55801e827a 100644
--- a/db/db_impl_write.cc
+++ b/db/db_impl_write.cc
@@ -64,6 +64,9 @@ Status DBImpl::WriteImpl(const WriteOptions& write_options,
   if (my_batch == nullptr) {
     return Status::Corruption("Batch is nullptr!");
   }
+  if (write_options.sync && write_options.disableWAL) {
+    return Status::InvalidArgument("Sync writes has to enable WAL.");
+  }
   if (concurrent_prepare_ && immutable_db_options_.enable_pipelined_write) {
     return Status::NotSupported(
         "pipelined_writes is not compatible with concurrent prepares");
@@ -154,7 +157,7 @@ Status DBImpl::WriteImpl(const WriteOptions& write_options,
 
   mutex_.Lock();
 
-  bool need_log_sync = !write_options.disableWAL && write_options.sync;
+  bool need_log_sync = write_options.sync;
   bool need_log_dir_sync = need_log_sync && !log_dir_synced_;
   if (!concurrent_prepare_ || !disable_memtable) {
     // With concurrent writes we do preprocess only in the write thread that
diff --git a/db/db_test.cc b/db/db_test.cc
index c729582943..94a6379288 100644
--- a/db/db_test.cc
+++ b/db/db_test.cc
@@ -60,7 +60,6 @@
 #include "util/compression.h"
 #include "util/file_reader_writer.h"
 #include "util/filename.h"
-#include "util/hash.h"
 #include "util/mutexlock.h"
 #include "util/rate_limiter.h"
 #include "util/string_util.h"
@@ -223,6 +222,10 @@ TEST_F(DBTest, SkipDelay) {
 
   for (bool sync : {true, false}) {
     for (bool disableWAL : {true, false}) {
+      if (sync && disableWAL) {
+        // sync and disableWAL is incompatible.
+        continue;
+      }
       // Use a small number to ensure a large delay that is still effective
       // when we do Put
       // TODO(myabandeh): this is time dependent and could potentially make
diff --git a/db/db_write_test.cc b/db/db_write_test.cc
index 726f444fa1..b5ea930a03 100644
--- a/db/db_write_test.cc
+++ b/db/db_write_test.cc
@@ -9,7 +9,6 @@
 #include "db/db_test_util.h"
 #include "db/write_batch_internal.h"
 #include "port/stack_trace.h"
-#include "util/sync_point.h"
 
 namespace rocksdb {
 
@@ -21,6 +20,17 @@ class DBWriteTest : public DBTestBase, public testing::WithParamInterface<int> {
   void Open() { DBTestBase::Reopen(GetOptions(GetParam())); }
 };
 
+// It is invalid to do sync write while disabling WAL.
+TEST_P(DBWriteTest, SyncAndDisableWAL) {
+  WriteOptions write_options;
+  write_options.sync = true;
+  write_options.disableWAL = true;
+  ASSERT_TRUE(dbfull()->Put(write_options, "foo", "bar").IsInvalidArgument());
+  WriteBatch batch;
+  ASSERT_OK(batch.Put("foo", "bar"));
+  ASSERT_TRUE(dbfull()->Write(write_options, &batch).IsInvalidArgument());
+}
+
 // Sequence number should be return through input write batch.
 TEST_P(DBWriteTest, ReturnSeuqneceNumber) {
   Random rnd(4422);