From 813327b231d4abbe2f5f9394b005680b1712f04b Mon Sep 17 00:00:00 2001 From: Binbin Date: Thu, 8 Feb 2024 20:36:11 +0800 Subject: [PATCH] Fix SORT STORE quicklist with the right options (#13042) We forgot to call quicklistSetOptions after createQuicklistObject, in the sort store scenario, we will create a quicklist with default 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 #12871. release notes: > Fix lists created by SORT STORE to respect list compression and packing configs. --- src/object.c | 4 ++-- src/quicklist.c | 4 ++-- src/quicklist.h | 4 ++-- src/rdb.c | 8 ++------ src/server.h | 2 +- src/sort.c | 5 ++--- src/t_list.c | 3 +-- tests/unit/sort.tcl | 12 ++++++++++++ 8 files changed, 24 insertions(+), 18 deletions(-) diff --git a/src/object.c b/src/object.c index 054f7d348..dffb6fab9 100644 --- a/src/object.c +++ b/src/object.c @@ -232,8 +232,8 @@ robj *dupStringObject(const robj *o) { } } -robj *createQuicklistObject(void) { - quicklist *l = quicklistCreate(); +robj *createQuicklistObject(int fill, int compress) { + quicklist *l = quicklistNew(fill, compress); robj *o = createObject(OBJ_LIST,l); o->encoding = OBJ_ENCODING_QUICKLIST; return o; diff --git a/src/quicklist.c b/src/quicklist.c index 3b1187897..a08431bff 100644 --- a/src/quicklist.c +++ b/src/quicklist.c @@ -161,9 +161,9 @@ void quicklistSetFill(quicklist *quicklist, int fill) { quicklist->fill = fill; } -void quicklistSetOptions(quicklist *quicklist, int fill, int depth) { +void quicklistSetOptions(quicklist *quicklist, int fill, int compress) { quicklistSetFill(quicklist, fill); - quicklistSetCompressDepth(quicklist, depth); + quicklistSetCompressDepth(quicklist, compress); } /* Create a new quicklist with some default parameters. */ diff --git a/src/quicklist.h b/src/quicklist.h index 3d33634f5..e17bc1f57 100644 --- a/src/quicklist.h +++ b/src/quicklist.h @@ -155,9 +155,9 @@ typedef struct quicklistEntry { /* Prototypes */ quicklist *quicklistCreate(void); quicklist *quicklistNew(int fill, int compress); -void quicklistSetCompressDepth(quicklist *quicklist, int depth); +void quicklistSetCompressDepth(quicklist *quicklist, int compress); void quicklistSetFill(quicklist *quicklist, int fill); -void quicklistSetOptions(quicklist *quicklist, int fill, int depth); +void quicklistSetOptions(quicklist *quicklist, int fill, int compress); void quicklistRelease(quicklist *quicklist); int quicklistPushHead(quicklist *quicklist, void *value, const size_t sz); int quicklistPushTail(quicklist *quicklist, void *value, const size_t sz); diff --git a/src/rdb.c b/src/rdb.c index 8bd630bf5..48a0c61c1 100644 --- a/src/rdb.c +++ b/src/rdb.c @@ -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--) { @@ -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; diff --git a/src/server.h b/src/server.h index edc930874..067b5df93 100644 --- a/src/server.h +++ b/src/server.h @@ -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 compress); robj *createListListpackObject(void); robj *createSetObject(void); robj *createIntsetObject(void); diff --git a/src/sort.c b/src/sort.c index bef260555..bd1f10064 100644 --- a/src/sort.c +++ b/src/sort.c @@ -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. @@ -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++) { diff --git a/src/t_list.c b/src/t_list.c index 966199e0e..d93193749 100644 --- a/src/t_list.c +++ b/src/t_list.c @@ -62,8 +62,7 @@ static void listTypeTryConvertListpack(robj *o, robj **argv, int start, int end, /* Invoke callback before conversion. */ if (fn) fn(data); - quicklist *ql = quicklistCreate(); - quicklistSetOptions(ql, server.list_max_listpack_size, server.list_compress_depth); + quicklist *ql = quicklistNew(server.list_max_listpack_size, server.list_compress_depth); /* Append listpack to quicklist if it's not empty, otherwise release it. */ if (lpLength(o->ptr)) diff --git a/tests/unit/sort.tcl b/tests/unit/sort.tcl index eade6ea34..a46f77cf9 100644 --- a/tests/unit/sort.tcl +++ b/tests/unit/sort.tcl @@ -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{t} lst_dst{t} + r config set list-max-listpack-size -1 + r config set list-compress-depth 12 + r lpush lst{t} {*}[split [string repeat "1" 6000] ""] + r sort lst{t} store lst_dst{t} + assert_encoding quicklist lst_dst{t} + assert_match "*ql_listpack_max:-1 ql_compressed:1*" [r debug object lst_dst{t}] + config_set list-max-listpack-size $origin_config + } {} {needs:debug} } start_cluster 1 0 {tags {"external:skip cluster sort"}} {