fix(hset_family): Ensure empty hash sets are removed #4873

abhijat · 2025-04-01T09:13:14Z

When a search operation is performed on a hash set, expired fields are removed as a side effect.
If at the end of such an operation the hash set becomes empty, its key is removed from the database.

fixes #4856

src/server/hset_family.cc

adiholden · 2025-04-03T11:00:42Z

src/server/hset_family.cc

      }
+
+      // If the set is empty, FindWithCleanup will have removed the key
+      if (sm->Empty()) {


If FindWithCleanup removed the key, is sm still valid or was it deallocated?

Good point, oddly enough in the debugger the pointer is still valid although its size is 0 and entries_ table is empty although the block of memory seems to have been freed. It doesn't seem safe though.

I will look into this further.

I think in tests the pointer is valid because of UB and the OS just hasn't reused the memory.

In the latest version of the code, the map is not accessed after delete because delete is performed during scope exit of the function.

adiholden · 2025-04-03T11:02:26Z

src/server/hset_family.cc

+
+bool ContainsWithCleanup(HSetCleanupCtx ctx, const std::string_view field) {
+  const bool found = ctx.sm->Contains(field);
+  ctx.DelKeyIfEmpty();


This is a blocking function as RecordJournal can preempt

Lets rename this function to have suffix Blocking

If the key was deleted we should not access sm anymore

if I am not wrong with number 2, we need to find a way to enforce that callers will not access sm after calling this functions

Now we set sm to nullptr if the key is deleted.

Another approach could be to consume the input pointer (by always setting to null) and return the string map pointer wrapped in a type such as variant or optional to signify that the string map is no longer valid, but nothing guarantees that the caller will not attempt to use the returned pointer without a check.

When a search operation is performed on a hash set, expired fields are removed as a side effect. If at the end of such an operation the hash set becomes empty, its key is removed from the database. Signed-off-by: Abhijat Malviya <[email protected]>

romange · 2025-04-03T13:23:41Z

src/server/hset_family.cc

  return OpStatus::OK;
+}
+
+struct HSetCleanupCtx {


I think this code needs more improvement.

We pass too many variables into struct, some of them can be deduced from others

The interface a bit awkward - why sm_ptr is double pointer?

It is a double pointer to signal the user that the pointer may be invalidated as discussed in #4873 (comment)

If the key is deleted, then we set the pointer to null. As mentioned in #4873 (comment) though, it is tricky to avoid accidental usage.

romange · 2025-04-03T13:29:50Z

src/server/hset_family.cc

-
-  if (it == sm->end())
-    return OpStatus::KEY_NOTFOUND;
+  if (const auto it = FindWithCleanupBlocking(


I appreciate the attempt to wrap everything into a single call but I think it makes things more confusing on the caller side.
If we found something - we for sure do not need to clean up.
if not - we should check if sm->Empty() and perform the deletion. So the deletion - can be a separate function
with its arguments but pushing this code down into FindWithCleanupBlocking makes the code here more confusing.

Initially in this PR I had the approach to do an empty check + delete using RAII as in f72ff33 - but we discussed later and to make sure the key is always deleted for future commands I combined the check and deletion into one.

I will look into changing to make things simpler.

I tried to simplify it a bit, it can be made simpler by arming the delete object by hand but that leaves the possibility of new code not ensuring delete of empty keys cc @adiholden

Signed-off-by: Abhijat Malviya <[email protected]>

kostasrim · 2025-04-08T10:53:06Z

src/server/hset_family.cc


+struct KeyCleanup {
+  using CleanupFuncT = std::function<void(std::string_view)>;
+  explicit KeyCleanup(CleanupFuncT func, const std::string_view key_view)


In general, string_view is not mutable so const is redundant here

I try to use const with views when I remember because even string_view can be made to point to some other string in the function body and const protects against that.

It's probably not something that happens often so I am fine to remove it, but I used const deliberately here.

ohhh yeah that makes sense! nvm

kostasrim · 2025-04-08T10:59:18Z

src/server/hset_family.cc

+  bool armed{false};
+};
+
+void DeleteKey(DbSlice& db_slice, const OpArgs& op_args, std::string_view key) {


You can move this function inside KeyCleanup. You only call it once as "freestanding" and I think we might even have a bug there

I kept it as a function because it is used twice.

kostasrim · 2025-04-08T11:02:59Z

src/server/hset_family.cc

-    auto it = db_slice.FindMutable(op_args.db_cntx, key).it;
-
-    db_slice.Del(op_args.db_cntx, it);
+    DeleteKey(db_slice, op_args, key);


DeleteKey also writes to the journal right ? Why didn't we do that before ?

Not sure but earlier in the PR discussion it was mentioned we should write to journal when we delete the key. Should we not do this in this case?

This is in the HGetGeneric -> OpGetAll code path. Will it be replicated by itself, considering it is a read command?

Not sure but earlier in the PR discussion it was mentioned we should write to journal when we delete the key. Should we not do this in this case?

Yes we should but I am skeptical before those changes it seems that we did not write explicitly and we do now.

I will need to check at the flow and get back to you

Wow that was an actual bug. Mind if we also add a replication test for this ?

kostasrim

lgtm. Plz follow up with:

Change the log for snapshot as per Adi's comment

abhijat changed the title ~~fix(hset_family): Ensure empty hash sets are removed~~ [wip] fix(hset_family): Ensure empty hash sets are removed Apr 1, 2025

abhijat force-pushed the abhijat/fix/delete-empty-hashset branch 12 times, most recently from f72ff33 to 1b3f3cd Compare April 2, 2025 07:12

adiholden reviewed Apr 2, 2025

View reviewed changes

src/server/hset_family.cc Outdated Show resolved Hide resolved

abhijat force-pushed the abhijat/fix/delete-empty-hashset branch 3 times, most recently from e860e4e to 8b3c8e7 Compare April 3, 2025 06:06

abhijat requested a review from adiholden April 3, 2025 09:18

abhijat changed the title ~~[wip] fix(hset_family): Ensure empty hash sets are removed~~ fix(hset_family): Ensure empty hash sets are removed Apr 3, 2025

adiholden reviewed Apr 3, 2025

View reviewed changes

abhijat force-pushed the abhijat/fix/delete-empty-hashset branch from 8b3c8e7 to c91cea7 Compare April 3, 2025 13:04

romange reviewed Apr 3, 2025

View reviewed changes

abhijat force-pushed the abhijat/fix/delete-empty-hashset branch 4 times, most recently from 1d93270 to b418619 Compare April 4, 2025 11:50

Use RAII with wrappers

5381f48

Signed-off-by: Abhijat Malviya <[email protected]>

abhijat force-pushed the abhijat/fix/delete-empty-hashset branch from b418619 to 5381f48 Compare April 4, 2025 11:51

abhijat requested review from romange and adiholden April 6, 2025 12:37

adiholden approved these changes Apr 8, 2025

View reviewed changes

kostasrim reviewed Apr 8, 2025

View reviewed changes

kostasrim approved these changes Apr 9, 2025

View reviewed changes

abhijat merged commit c129834 into main Apr 9, 2025
10 checks passed

abhijat deleted the abhijat/fix/delete-empty-hashset branch April 9, 2025 06:55

fix(hset_family): Ensure empty hash sets are removed #4873

fix(hset_family): Ensure empty hash sets are removed #4873

Conversation

abhijat commented Apr 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kostasrim left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

abhijat commented Apr 1, 2025 •

edited

Loading