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
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/config.c
Original file line number Diff line number Diff line change
Expand Up @@ -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.


/* String Configs */
createStringConfig("aclfile", NULL, IMMUTABLE_CONFIG, ALLOW_EMPTY_STRING, server.acl_filename, "", NULL, NULL),
Expand Down
3 changes: 0 additions & 3 deletions src/intset.c
Original file line number Diff line number Diff line change
Expand Up @@ -320,9 +320,6 @@ int intsetValidateIntegrity(const unsigned char *p, size_t size, int deep) {
uint32_t count = intrev32ifbe(is->length);
if (sizeof(*is) + count * record_size != size) return 0;

/* check that the set is not empty. */
if (count == 0) return 0;

if (!deep) return 1;

/* check that there are no dup or out of order records. */
Expand Down
25 changes: 21 additions & 4 deletions src/rdb.c
Original file line number Diff line number Diff line change
Expand Up @@ -1900,7 +1900,7 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, int dbid, int *error) {
} else if (rdbtype == RDB_TYPE_SET) {
/* Read Set value */
if ((len = rdbLoadLen(rdb, NULL)) == RDB_LENERR) return NULL;
if (len == 0) goto emptykey;
if (len == 0 && !server.allow_empty_set) goto emptykey;

/* Use a regular set when there are too many entries. */
size_t max_entries = server.set_max_intset_entries;
Expand Down Expand Up @@ -2356,6 +2356,18 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, int dbid, int *error) {
}
o->type = OBJ_SET;
o->encoding = OBJ_ENCODING_INTSET;

if (intsetLen((intset *)encoded) == 0) {
if (!server.allow_empty_set) {
zfree(encoded);
o->ptr = NULL;
decrRefCount(o);
goto emptykey;
}

zfree(encoded);
o->ptr = intsetNew();
}
if (intsetLen(o->ptr) > server.set_max_intset_entries) setTypeConvert(o, OBJ_ENCODING_HASHTABLE);
break;
case RDB_TYPE_SET_LISTPACK:
Expand All @@ -2371,10 +2383,15 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, int dbid, int *error) {
o->encoding = OBJ_ENCODING_LISTPACK;

if (setTypeSize(o) == 0) {
if (!server.allow_empty_set) {
zfree(encoded);
o->ptr = NULL;
decrRefCount(o);
goto emptykey;
}

zfree(encoded);
o->ptr = NULL;
decrRefCount(o);
goto emptykey;
o->ptr = lpNew(0);
}
if (setTypeSize(o) > server.set_max_listpack_entries) setTypeConvert(o, OBJ_ENCODING_HASHTABLE);
break;
Expand Down
4 changes: 4 additions & 0 deletions src/server.h
Original file line number Diff line number Diff line change
Expand Up @@ -1930,6 +1930,10 @@ struct valkeyServer {
invocation of the event loop. */
unsigned int max_new_conns_per_cycle; /* The maximum number of tcp connections that will be accepted during each
invocation of the event loop. */
int allow_empty_set; /* Flag to control whether empty set is allowed in the database
1 empty sets are preserved in database even after all elements are removed
0 by default, key for the set is deleted when it becomes empty */

/* AOF persistence */
int aof_enabled; /* AOF configuration */
int aof_state; /* AOF_(ON|OFF|WAIT_REWRITE) */
Expand Down
14 changes: 8 additions & 6 deletions src/t_set.c
Original file line number Diff line number Diff line change
Expand Up @@ -624,8 +624,10 @@ void sremCommand(client *c) {
if (setTypeRemove(set, c->argv[j]->ptr)) {
deleted++;
if (setTypeSize(set) == 0) {
dbDelete(c->db, c->argv[1]);
keyremoved = 1;
if (!server.allow_empty_set) {
dbDelete(c->db, c->argv[1]);
keyremoved = 1;
}
break;
}
}
Expand Down Expand Up @@ -668,8 +670,8 @@ void smoveCommand(client *c) {
}
notifyKeyspaceEvent(NOTIFY_SET, "srem", c->argv[1], c->db->id);

/* Remove the src set from the database when empty */
if (setTypeSize(srcset) == 0) {
/* Remove the src set from the database when empty and allow-empty-set is disabled */
if (!server.allow_empty_set && setTypeSize(srcset) == 0) {
dbDelete(c->db, c->argv[1]);
notifyKeyspaceEvent(NOTIFY_GENERIC, "del", c->argv[1], c->db->id);
}
Expand Down Expand Up @@ -971,8 +973,8 @@ void spopCommand(client *c) {
addReplyBulk(c, ele);
decrRefCount(ele);

/* Delete the set if it's empty */
if (setTypeSize(set) == 0) {
/* Delete the set if it's empty and allow-empty-set is disabled */
if (!server.allow_empty_set && setTypeSize(set) == 0) {
dbDelete(c->db, c->argv[1]);
notifyKeyspaceEvent(NOTIFY_GENERIC, "del", c->argv[1], c->db->id);
}
Expand Down
11 changes: 10 additions & 1 deletion tests/support/server.tcl
Original file line number Diff line number Diff line change
Expand Up @@ -769,7 +769,7 @@ proc start_multiple_servers {num options code} {
uplevel 1 $code
}

proc restart_server {level wait_ready rotate_logs {reconnect 1} {shutdown sigterm}} {
proc restart_server {level wait_ready rotate_logs {reconnect 1} {shutdown sigterm} {config_override {}}} {
set srv [lindex $::servers end+$level]
if {$shutdown ne {sigterm}} {
catch {[dict get $srv "client"] shutdown $shutdown}
Expand Down Expand Up @@ -798,6 +798,15 @@ proc restart_server {level wait_ready rotate_logs {reconnect 1} {shutdown sigter

set config_file [dict get $srv "config_file"]

# Apply config updates if provided
if {[llength $config_override] > 0} {
set fd [open $config_file "a"]
foreach {key value} $config_override {
puts $fd "$key $value"
}
close $fd
}

set pid [spawn_server $config_file $stdout $stderr {}]

# check that the server actually started
Expand Down
105 changes: 105 additions & 0 deletions tests/unit/type/set.tcl
Original file line number Diff line number Diff line change
Expand Up @@ -1168,6 +1168,111 @@ foreach type {single multiple single_multiple} {
}
}

start_server {
tags {"set external:skip"}
overrides {
"set-max-intset-entries" 1
"allow-empty-set" "yes"
}
} {
proc save_load_rdb {{config_override {}}} {
r save
restart_server 0 true false true now $config_override
wait_done_loading r
}
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

foreach member $members { r srem $key $member }
}
proc verify_emptyset {key} {
assert_equal 0 [r scard $key]
assert_equal 1 [r exists $key]
}

test {EMTPYSET save and reload empty listset from RDB} {
r flushdb
create_emptyset listset {a}
verify_emptyset listset
assert_equal listpack [r object encoding listset]
save_load_rdb
verify_emptyset listset
}

test {EMTPYSET save and reload empty intset from RDB} {
r flushdb
create_emptyset intset {1}
verify_emptyset intset
assert_equal intset [r object encoding intset]
save_load_rdb
verify_emptyset intset
}

test {EMTPYSET save and reload empty hashset from RDB} {
r flushdb
create_emptyset hashset {1 2}
verify_emptyset hashset
assert_equal hashtable [r object encoding hashset]
save_load_rdb
verify_emptyset hashset
}

test {EMTPYSET smove to make empty set} {
r flushdb
assert_equal 1 [r sadd myset a]
assert_equal 1 [r sadd myset2 b]
assert_equal 1 [r smove myset myset2 a]
assert_equal 2 [r scard myset2]
verify_emptyset myset
save_load_rdb
verify_emptyset myset
}

test {EMTPYSET spop to make empty set} {
r flushdb
assert_equal 1 [r sadd myset a]
assert_equal a [r spop myset]
verify_emptyset myset
save_load_rdb
verify_emptyset myset
}

test {EMTPYSET sunion with empty set} {
r flushdb
create_emptyset myset {a}
verify_emptyset myset
assert_equal 1 [r sadd myset2 b]
assert_equal b [r sunion myset myset2]
save_load_rdb
verify_emptyset myset
}

test {EMTPYSET skip loading empty sets when allow-empty-set is no} {
r flushdb
create_emptyset intset {1}
verify_emptyset intset
create_emptyset listset {a}
verify_emptyset listset
create_emptyset hashset {1 2}
verify_emptyset hashset
set config_override {
"allow-empty-set" "no"
}
save_load_rdb $config_override
wait_for_log_messages 0 {"*rdbLoadObject skipping empty key: intset*"} 0 1000 50
wait_for_log_messages 0 {"*rdbLoadObject skipping empty key: listset*"} 0 1000 50
wait_for_log_messages 0 {"*rdbLoadObject skipping empty key: hashset*"} 0 1000 50
wait_for_log_messages 0 {"*Done loading RDB, keys loaded: 0, keys expired: 0, empty keys skipped: 3.*"} 0 1000 50

assert_equal 0 [r exists intset]
assert_equal 0 [r scard intset]
assert_equal 0 [r exists listset]
assert_equal 0 [r scard listset]
assert_equal 0 [r exists hashset]
assert_equal 0 [r scard hashset]
}
}

run_solo {set-large-memory} {
start_server [list overrides [list save ""] ] {

Expand Down
25 changes: 25 additions & 0 deletions valkey.conf
Original file line number Diff line number Diff line change
Expand Up @@ -496,6 +496,31 @@ locale-collate ""
#
# extended-redis-compatibility no

# This configuration controls whether Valkey allows the representation of empty sets in the database.
# By default, when the last element of a set is removed, the key associated with the set is deleted.
# Enabling this option allows Valkey to retain the key and store an empty set instead.
#
# This can be useful for scenarios where an empty set has semantic meaning in the application,
# such as indicating that a query was executed but returned no results.
#
# When this option is enabled:
# - Empty sets are preserved in the database, and their key will not be automatically deleted.
# - The `SCARD` command will return 0 for these keys.
# - These keys will remain in the database until explicitly deleted using the `DEL` command.
#
# Note:
# - This setting may result in additional memory usage due to the retention of keys for empty sets.
# - Ensure client applications handle empty sets correctly, as their presence differs from the absence of a key.
# - When loading RDB files, empty sets are skipped by default unless this option is enabled.
# This ensures backward compatibility with existing RDB files created in versions where empty
# sets were not explicitly supported.
# - Ensure that this setting is consistently enabled if your application relies on the presence of
# empty sets during RDB restores.
#
# Default: no
#
# allow-empty-set no

################################ SNAPSHOTTING ################################

# Save the DB to disk.
Expand Down
Loading