refactor: Add DelMutable() helper to unify post_updater.Run() + Del() pattern (#5890)

* refactor: Add DelMutable() helper to unify post_updater.Run() + Del() pattern

* fix: review comments

Author: vyavdoshenko

src/server/db_slice.cc

@@ -816,6 +816,11 @@ void DbSlice::Del(Context cntx, Iterator it) {
   PerformDeletion(it, db.get());
 }
 
+void DbSlice::DelMutable(Context cntx, ItAndUpdater it_updater) {
+  it_updater.post_updater.Run();
+  Del(cntx, it_updater.it);
+}
+
 void DbSlice::FlushSlotsFb(const cluster::SlotSet& slot_ids) {
   VLOG(1) << "Start FlushSlotsFb";
   // Slot deletion can take time as it traverses all the database, hence it runs in fiber.

src/server/db_slice.h

@@ -328,6 +328,11 @@ class DbSlice {
   // Deletes the iterator. The iterator must be valid.
   void Del(Context cntx, Iterator it);
 
+  // Deletes a key after FindMutable(). Runs post_updater before deletion
+  // to update memory accounting while the key is still valid.
+  // Takes ownership of it_updater (pass by value with move semantics).
+  void DelMutable(Context cntx, ItAndUpdater it_updater);
+
   constexpr static DbIndex kDbAll = 0xFFFF;
 
   // Flushes db_ind or all databases if kDbAll is passed

src/server/generic_family.cc

@@ -458,8 +458,7 @@ OpStatus Renamer::DelSrc(Transaction* t, EngineShard* shard) {
 
   DVLOG(1) << "Rename: removing the key '" << src_key_;
 
-  res.post_updater.Run();
-  db_slice.Del(t->GetDbContext(), it);
+  db_slice.DelMutable(t->GetDbContext(), std::move(res));
   if (shard->journal()) {
     RecordJournal(t->GetOpArgs(shard), "DEL"sv, ArgSlice{src_key_}, 2);
   }
@@ -480,8 +479,7 @@ OpStatus Renamer::DeserializeDest(Transaction* t, EngineShard* shard) {
 
   if (dest_found_) {
     DVLOG(1) << "Rename: deleting the destiny key '" << dest_key_;
-    dest_res.post_updater.Run();
-    db_slice.Del(op_args.db_cntx, dest_res.it);
+    db_slice.DelMutable(op_args.db_cntx, std::move(dest_res));
   }
 
   if (restore_args.Expired()) {
@@ -588,8 +586,7 @@ OpStatus OpRestore(const OpArgs& op_args, std::string_view key, std::string_view
       if (restore_args.Replace()) {
         VLOG(1) << "restore command is running with replace, found old key '" << key
                 << "' and removing it";
-        res.post_updater.Run();
-        db_slice.Del(op_args.db_cntx, res.it);
+        db_slice.DelMutable(op_args.db_cntx, std::move(res));
       } else {
         // we are not allowed to replace it.
         return OpStatus::KEY_EXISTS;
@@ -938,14 +935,12 @@ OpResult<void> OpRen(const OpArgs& op_args, string_view from_key, string_view to
     to_res.it->first.SetSticky(sticky);
     to_res.post_updater.Run();
 
-    from_res.post_updater.Run();
-    db_slice.Del(op_args.db_cntx, from_res.it);
+    db_slice.DelMutable(op_args.db_cntx, std::move(from_res));
   } else {
     // Here we first delete from_it because AddNew below could invalidate from_it.
     // On the other hand, AddNew does not rely on the iterators - this is why we keep
     // the value in `from_obj`.
-    from_res.post_updater.Run();
-    db_slice.Del(op_args.db_cntx, from_res.it);
+    db_slice.DelMutable(op_args.db_cntx, std::move(from_res));
     auto op_result = db_slice.AddNew(op_args.db_cntx, to_key, std::move(from_obj), exp_ts);
     RETURN_ON_BAD_STATUS(op_result);
     to_res = std::move(*op_result);

src/server/hset_family.cc

@@ -281,8 +281,7 @@ struct KeyCleanup {
 
 void DeleteKey(DbSlice& db_slice, const OpArgs& op_args, std::string_view key) {
   if (auto del_it = db_slice.FindMutable(op_args.db_cntx, key, OBJ_HASH); del_it) {
-    del_it->post_updater.Run();
-    db_slice.Del(op_args.db_cntx, del_it->it);
+    db_slice.DelMutable(op_args.db_cntx, std::move(*del_it));
     if (op_args.shard->journal()) {
       RecordJournal(op_args, "DEL"sv, {key});
     }
@@ -1166,8 +1165,7 @@ void HSetFamily::HRandField(CmdArgList args, const CommandContext& cmd_cntx) {
       if (string_map->Empty()) {  // Can happen if we use a TTL on hash members.
         auto res_it = db_slice.FindMutable(db_context, key, OBJ_HASH);
         if (res_it) {
-          res_it->post_updater.Run();
-          db_slice.Del(db_context, res_it->it);
+          db_slice.DelMutable(db_context, std::move(*res_it));
         }
         return facade::OpStatus::KEY_NOTFOUND;
       }

src/server/json_family.cc

@@ -1117,8 +1117,7 @@ OpResult<long> OpDel(const OpArgs& op_args, string_view key, string_view path,
     RETURN_ON_BAD_STATUS(res_it);
 
     if (IsValid(res_it->it)) {
-      res_it->post_updater.Run();
-      db_slice.Del(op_args.db_cntx, res_it->it);
+      db_slice.DelMutable(op_args.db_cntx, std::move(*res_it));
       return 1;
     }
     return 0;

src/server/set_family.cc

@@ -483,8 +483,7 @@ OpResult<uint32_t> OpAdd(const OpArgs& op_args, std::string_view key, const NewE
   if (overwrite && (vals_it.begin() == vals_it.end())) {
     auto res_it = db_slice.FindMutable(op_args.db_cntx, key, OBJ_SET);
     if (res_it) {
-      res_it->post_updater.Run();
-      db_slice.Del(op_args.db_cntx, res_it->it);
+      db_slice.DelMutable(op_args.db_cntx, std::move(*res_it));
       if (journal_update && op_args.shard->journal()) {
         RecordJournal(op_args, "DEL"sv, ArgSlice{key});
       }
@@ -927,8 +926,7 @@ OpResult<StringVec> OpPop(const OpArgs& op_args, string_view key, unsigned count
     });
 
     // Delete the set as it is now empty
-    find_res->post_updater.Run();
-    db_slice.Del(op_args.db_cntx, find_res->it);
+    db_slice.DelMutable(op_args.db_cntx, std::move(*find_res));
 
     // Replicate as DEL.
     if (op_args.shard->journal()) {

src/server/string_family.cc

@@ -1149,8 +1149,7 @@ void StringFamily::GetDel(CmdArgList args, const CommandContext& cmnd_cntx) {
       return it_res.status();
 
     auto value = ReadString(tx->GetDbIndex(), key, it_res->it->second, es);
-    it_res->post_updater.Run();  // Run manually before delete
-    db_slice.Del(tx->GetDbContext(), it_res->it);
+    db_slice.DelMutable(tx->GetDbContext(), std::move(*it_res));
     return value;
   };
 

src/server/zset_family.cc

@@ -1969,8 +1969,7 @@ OpResult<ZSetFamily::AddResult> ZSetFamily::OpAdd(const OpArgs& op_args,
   if (zparams.override && members.empty()) {
     auto res_it = db_slice.FindMutable(op_args.db_cntx, key, OBJ_ZSET);
     if (res_it && IsValid(res_it->it)) {
-      res_it->post_updater.Run();
-      db_slice.Del(op_args.db_cntx, res_it->it);
+      db_slice.DelMutable(op_args.db_cntx, std::move(*res_it));
       if (zparams.journal_update && op_args.shard->journal()) {
         RecordJournal(op_args, "DEL"sv, ArgSlice{key});
       }