fix: allow SELECT in multi/exec if it's a noop (#4146)

Fixes #4120 Signed-off-by: Roman Gershman <[email protected]>
dragonflydb · Nov 18, 2024 · d467a34 · d467a34
1 parent e16ef83
commit d467a34
Show file tree

Hide file tree

Showing 5 changed files with 17 additions and 6 deletions.
diff --git a/src/server/db_slice.cc b/src/server/db_slice.cc
@@ -1000,6 +1000,7 @@ bool DbSlice::Acquire(IntentLock::Mode mode, const KeyLockArgs& lock_args) {
   if (lock_args.fps.empty()) {  // Can be empty for NO_KEY_TRANSACTIONAL commands.
     return true;
   }
+  DCHECK_LT(lock_args.db_index, db_array_size());
 
   auto& lt = db_arr_[lock_args.db_index]->trans_locks;
   bool lock_acquired = true;

diff --git a/src/server/generic_family.cc b/src/server/generic_family.cc
@@ -1607,6 +1607,16 @@ void GenericFamily::Select(CmdArgList args, Transaction*, SinkReplyBuilder* buil
   if (index < 0 || index >= absl::GetFlag(FLAGS_dbnum)) {
     return builder->SendError(kDbIndOutOfRangeErr);
   }
+
+  if (cntx->conn_state.db_index == index) {
+    // accept a noop.
+    return builder->SendOk();
+  }
+
+  if (cntx->conn_state.exec_info.IsRunning()) {
+    return builder->SendError("SELECT is not allowed in a transaction");
+  }
+
   cntx->conn_state.db_index = index;
   auto cb = [ns = cntx->ns, index](EngineShard* shard) {
     auto& db_slice = ns->GetDbSlice(shard->shard_id());

diff --git a/src/server/journal/executor.cc b/src/server/journal/executor.cc
@@ -76,15 +76,15 @@ void JournalExecutor::Execute(journal::ParsedEntry::CmdData& cmd) {
 }
 
 void JournalExecutor::SelectDb(DbIndex dbid) {
-  conn_context_.conn_state.db_index = dbid;
-
   if (ensured_dbs_.size() <= dbid)
     ensured_dbs_.resize(dbid + 1);
 
   if (!ensured_dbs_[dbid]) {
     auto cmd = BuildFromParts("SELECT", dbid);
     Execute(cmd);
     ensured_dbs_[dbid] = true;
+  } else {
+    conn_context_.conn_state.db_index = dbid;
   }
 }
 

diff --git a/src/server/main_service.cc b/src/server/main_service.cc
@@ -1084,7 +1084,7 @@ std::optional<ErrorReply> Service::VerifyCommandState(const CommandId* cid, CmdA
     return ErrorReply{"-READONLY You can't write against a read only replica."};
 
   if (multi_active) {
-    if (cmd_name == "SELECT" || absl::EndsWith(cmd_name, "SUBSCRIBE"))
+    if (absl::EndsWith(cmd_name, "SUBSCRIBE"))
       return ErrorReply{absl::StrCat("Can not call ", cmd_name, " within a transaction")};
 
     if (cmd_name == "WATCH" || cmd_name == "FLUSHALL" || cmd_name == "FLUSHDB")

diff --git a/tests/dragonfly/replication_test.py b/tests/dragonfly/replication_test.py
@@ -1067,7 +1067,7 @@ async def assert_replica_reconnections(replica_inst, initial_reconnects_count):
 @pytest.mark.asyncio
 async def test_replication_info(df_factory: DflyInstanceFactory, df_seeder_factory, n_keys=2000):
     master = df_factory.create()
-    replica = df_factory.create(logtostdout=True, replication_acks_interval=100)
+    replica = df_factory.create(replication_acks_interval=100)
     df_factory.start_all([master, replica])
     c_master = master.client()
     c_replica = replica.client()
@@ -1157,8 +1157,8 @@ async def test_flushall_in_full_sync(df_factory):
 
 @pytest.mark.asyncio
 async def test_readonly_script(df_factory):
-    master = df_factory.create(proactor_threads=2, logtostdout=True)
-    replica = df_factory.create(proactor_threads=2, logtostdout=True)
+    master = df_factory.create(proactor_threads=2)
+    replica = df_factory.create(proactor_threads=2)
 
     df_factory.start_all([master, replica])