Skip to content

Commit

Permalink
Fix SORT STORE quicklist with the right options
Browse files Browse the repository at this point in the history
We forgot to call quicklistSetOptions after createQuicklistObject,
in the sort store scenario, we will create a quicklist with incorrect
fill or compress options.

This PR adds fill and depth parameters to createQuicklistObject to
specify that options need to be set after creating a quicklist.

This closes redis#12871.
  • Loading branch information
enjoy-binbin committed Feb 8, 2024
1 parent 81666a6 commit 3e3e7ce
Show file tree
Hide file tree
Showing 5 changed files with 19 additions and 11 deletions.
3 changes: 2 additions & 1 deletion src/object.c
Original file line number Diff line number Diff line change
Expand Up @@ -232,10 +232,11 @@ robj *dupStringObject(const robj *o) {
}
}

robj *createQuicklistObject(void) {
robj *createQuicklistObject(int fill, int depth) {
quicklist *l = quicklistCreate();
robj *o = createObject(OBJ_LIST,l);
o->encoding = OBJ_ENCODING_QUICKLIST;
quicklistSetOptions(o->ptr, fill, depth);
return o;
}

Expand Down
8 changes: 2 additions & 6 deletions src/rdb.c
Original file line number Diff line number Diff line change
Expand Up @@ -1862,9 +1862,7 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, int dbid, int *error) {
if ((len = rdbLoadLen(rdb,NULL)) == RDB_LENERR) return NULL;
if (len == 0) goto emptykey;

o = createQuicklistObject();
quicklistSetOptions(o->ptr, server.list_max_listpack_size,
server.list_compress_depth);
o = createQuicklistObject(server.list_max_listpack_size, server.list_compress_depth);

/* Load every single element of the list */
while(len--) {
Expand Down Expand Up @@ -2174,9 +2172,7 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, int dbid, int *error) {
if ((len = rdbLoadLen(rdb,NULL)) == RDB_LENERR) return NULL;
if (len == 0) goto emptykey;

o = createQuicklistObject();
quicklistSetOptions(o->ptr, server.list_max_listpack_size,
server.list_compress_depth);
o = createQuicklistObject(server.list_max_listpack_size, server.list_compress_depth);
uint64_t container = QUICKLIST_NODE_CONTAINER_PACKED;
while (len--) {
unsigned char *lp;
Expand Down
2 changes: 1 addition & 1 deletion src/server.h
Original file line number Diff line number Diff line change
Expand Up @@ -2771,7 +2771,7 @@ robj *createStringObjectFromLongLong(long long value);
robj *createStringObjectFromLongLongForValue(long long value);
robj *createStringObjectFromLongLongWithSds(long long value);
robj *createStringObjectFromLongDouble(long double value, int humanfriendly);
robj *createQuicklistObject(void);
robj *createQuicklistObject(int fill, int depth);
robj *createListListpackObject(void);
robj *createSetObject(void);
robj *createIntsetObject(void);
Expand Down
5 changes: 2 additions & 3 deletions src/sort.c
Original file line number Diff line number Diff line change
Expand Up @@ -304,8 +304,7 @@ void sortCommandGeneric(client *c, int readonly) {
if (sortval)
incrRefCount(sortval);
else
sortval = createQuicklistObject();

sortval = createQuicklistObject(server.list_max_listpack_size, server.list_compress_depth);

/* When sorting a set with no sort specified, we must sort the output
* so the result is consistent across scripting and replication.
Expand Down Expand Up @@ -557,7 +556,7 @@ void sortCommandGeneric(client *c, int readonly) {
} else {
/* We can't predict the size and encoding of the stored list, we
* assume it's a large list and then convert it at the end if needed. */
robj *sobj = createQuicklistObject();
robj *sobj = createQuicklistObject(server.list_max_listpack_size, server.list_compress_depth);

/* STORE option specified, set the sorting result as a List object */
for (j = start; j <= end; j++) {
Expand Down
12 changes: 12 additions & 0 deletions tests/unit/sort.tcl
Original file line number Diff line number Diff line change
Expand Up @@ -356,6 +356,18 @@ foreach command {SORT SORT_RO} {
}
}
}

test {SORT STORE quicklist with the right options} {
set origin_config [config_get_set list-max-listpack-size -1]
r del lst lst_dst
r config set list-max-listpack-size -1
r config set list-compress-depth 12
r lpush lst {*}[split [string repeat "1" 6000] ""]
r sort lst store lst_dst
assert_encoding quicklist lst_dst
assert_match "*ql_listpack_max:-1 ql_compressed:1*" [r debug object lst_dst]
config_set list-max-listpack-size $origin_config
} {} {needs:debug}
}

start_cluster 1 0 {tags {"external:skip cluster sort"}} {
Expand Down

0 comments on commit 3e3e7ce

Please sign in to comment.