Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support empty set #1444

Open
wants to merge 1 commit into
base: unstable
Choose a base branch
from
Open

Conversation

sungming2
Copy link
Contributor

@sungming2 sungming2 commented Dec 16, 2024

Issue: #68

Summary

This pull request introduces support for empty sets in Valkey, controlled by a new configuration option, allow-empty-set which is a strong demand in the Redis community (redis/redis#6048). With this change, Valkey can preserve keys for sets even after all their elements have been removed, rather than deleting the key immediately. This feature provides additional flexibility for use cases where an empty set has semantic meaning

Why This Change is Needed

In Valkey, when a set becomes empty (all its elements are removed), the key representing that set is automatically deleted from the database. While this behavior works for most use cases, it introduces limitations for certain scenarios where the distinction between an empty set (existent key) and a non-existent key is important.
For example:
Key: active_users

SADD active_users sungming 
SREM active_users sungming
EXISTS active_users 
> (integer) 0

Now in this case, the absence of the key (active_users) could mean either:

  • The query hasn't been executed yet.
  • The query was executed but returned no users.

The application would need to query its persistent database (e.g., SQL) to determine whether the key represents an empty set or has not been created yet. Alternatively, they could add a dummy value, such as an empty string (""), to the set to ensure the key persists. However, this approach is not ideal and could lead to unintended behavior or safety issues.

Key changes

  1. New Configuration Option (allow-empty-set):
    Added the allow-empty-set configuration to valkey.conf.
    Default: no (empty sets are deleted by default, preserving backward compatibility).
    When enabled, keys for empty sets are retained in the database until explicitly deleted.

  2. Database Behavior:
    Empty sets are represented as valid keys with zero cardinality (SCARD returns 0).
    These keys are preserved in memory and are only removed with the DEL command.

  3. Persistence:
    Empty sets are serialized and saved in the RDB file when allow-empty-set is enabled.
    During RDB/AOF loading, empty sets are restored only if allow-empty-set is enabled. Otherwise, they are skipped.

Test

Added unit tests and integration tests (TCL) to verify:

  1. Persistence and RDB reloading of empty sets.
  2. Correct behavior of set commands when allow-empty-set is enabled/disabled.
  3. Compatibility with default behavior when the option is disabled.

Backward Compatibility

The default behavior (allow-empty-set = no) ensures backward compatibility with existing applications.

Signed-off-by: Seungmin Lee <[email protected]>
Copy link

codecov bot commented Dec 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.79%. Comparing base (ad24220) to head (d856660).
Report is 2 commits behind head on unstable.

Additional details and impacted files
@@             Coverage Diff              @@
##           unstable    #1444      +/-   ##
============================================
- Coverage     70.84%   70.79%   -0.06%     
============================================
  Files           119      119              
  Lines         64688    64698      +10     
============================================
- Hits          45828    45801      -27     
- Misses        18860    18897      +37     
Files with missing lines Coverage Δ
src/config.c 77.59% <ø> (ø)
src/intset.c 96.75% <ø> (-0.03%) ⬇️
src/rdb.c 76.86% <100.00%> (+0.30%) ⬆️
src/server.h 100.00% <ø> (ø)
src/t_set.c 97.27% <100.00%> (-0.57%) ⬇️

... and 14 files with indirect coverage changes

@ranshid ranshid added release-notes This issue should get a line item in the release notes needs-doc-pr This change needs to update a documentation page. Remove label once doc PR is open. labels Dec 16, 2024
@@ -3206,6 +3206,7 @@ standardConfig static_configs[] = {
createBoolConfig("cluster-slot-stats-enabled", NULL, MODIFIABLE_CONFIG, server.cluster_slot_stats_enabled, 0, NULL, NULL),
createBoolConfig("hide-user-data-from-log", NULL, MODIFIABLE_CONFIG, server.hide_user_data_from_log, 1, NULL, NULL),
createBoolConfig("import-mode", NULL, DEBUG_CONFIG | MODIFIABLE_CONFIG, server.import_mode, 0, NULL, NULL),
createBoolConfig("allow-empty-set", NULL, MODIFIABLE_CONFIG, server.allow_empty_set, 0, NULL, NULL),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we want this as a configuration. Application developers should be the ones that get to decided what behavior they want related to empty sets.

}
proc create_emptyset {key members} {
r del $key
foreach member $members { r sadd $key $member }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't personally like that in order to create an empty set, you first need to add an arbitrary element then delete that element. The primary use case for this feature seems to be for query caching, where really your goal is to empty the set (or create a new empty set)

I left a comment in the issue proposing a new API - SEMPTY or something similar (maybe SCREATE). Even if we decide on a different approach than what I was suggesting there - I think an API like this will be needed to make it user friendly

@PingXie PingXie added the major-decision-pending Major decision pending by TSC team label Dec 17, 2024
@PingXie
Copy link
Member

PingXie commented Dec 17, 2024

I marked this as "major-decision-needed" and left some comments on #68.

@valkey-io/core-team FYI

@hwware
Copy link
Member

hwware commented Dec 19, 2024

I marked this as "major-decision-needed" and left some comments on #68.

@valkey-io/core-team FYI

I just left comment there as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
major-decision-pending Major decision pending by TSC team needs-doc-pr This change needs to update a documentation page. Remove label once doc PR is open. release-notes This issue should get a line item in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants