From 1bb5ee9c683835c6bb542c31ccb5b3e11f006900 Mon Sep 17 00:00:00 2001 From: hwware Date: Thu, 16 Jan 2020 17:33:23 -0500 Subject: [PATCH 01/22] fix potentical memory leaks --- src/debug.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/debug.c b/src/debug.c index dd96ad416..77d3e97dc 100644 --- a/src/debug.c +++ b/src/debug.c @@ -685,9 +685,12 @@ NULL sds stats = sdsempty(); char buf[4096]; - if (getLongFromObjectOrReply(c, c->argv[2], &dbid, NULL) != C_OK) + if (getLongFromObjectOrReply(c, c->argv[2], &dbid, NULL) != C_OK){ + sdsfree(stats); return; + } if (dbid < 0 || dbid >= server.dbnum) { + sdsfree(stats); addReplyError(c,"Out of range database"); return; } From 7277e5d8a83cb4d8eed94684c26f83c783d9838f Mon Sep 17 00:00:00 2001 From: hwware Date: Thu, 16 Jan 2020 17:35:26 -0500 Subject: [PATCH 02/22] format fix --- src/debug.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/debug.c b/src/debug.c index 77d3e97dc..36af35aec 100644 --- a/src/debug.c +++ b/src/debug.c @@ -685,7 +685,7 @@ NULL sds stats = sdsempty(); char buf[4096]; - if (getLongFromObjectOrReply(c, c->argv[2], &dbid, NULL) != C_OK){ + if (getLongFromObjectOrReply(c, c->argv[2], &dbid, NULL) != C_OK) { sdsfree(stats); return; } From 2966132c520ffd2fc25472f7a202e37a64766639 Mon Sep 17 00:00:00 2001 From: "meir@redislabs.com" Date: Mon, 10 Feb 2020 12:10:32 +0200 Subject: [PATCH 03/22] Changed log level for module fork api from 'notice' to 'verbos'. --- src/module.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/module.c b/src/module.c index 8c61f60e2..cce3d1c11 100644 --- a/src/module.c +++ b/src/module.c @@ -6724,7 +6724,7 @@ int RM_Fork(RedisModuleForkDoneHandler cb, void *user_data) { server.module_child_pid = childpid; moduleForkInfo.done_handler = cb; moduleForkInfo.done_handler_user_data = user_data; - serverLog(LL_NOTICE, "Module fork started pid: %d ", childpid); + serverLog(LL_VERBOSE, "Module fork started pid: %d ", childpid); } return childpid; } @@ -6747,7 +6747,7 @@ int TerminateModuleForkChild(int child_pid, int wait) { server.module_child_pid != child_pid) return C_ERR; int statloc; - serverLog(LL_NOTICE,"Killing running module fork child: %ld", + serverLog(LL_VERBOSE,"Killing running module fork child: %ld", (long) server.module_child_pid); if (kill(server.module_child_pid,SIGUSR1) != -1 && wait) { while(wait4(server.module_child_pid,&statloc,0,NULL) != From ecf3b2ef320f8e9d07ed05634be432d8bd9529db Mon Sep 17 00:00:00 2001 From: srzhao Date: Fri, 17 Jan 2020 11:46:19 +0800 Subject: [PATCH 04/22] fix impl of aof-child whitelist SIGUSR1 feature. --- src/aof.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/aof.c b/src/aof.c index 3682c4568..8ab9349f0 100644 --- a/src/aof.c +++ b/src/aof.c @@ -1798,14 +1798,15 @@ void backgroundRewriteDoneHandler(int exitcode, int bysignal) { serverLog(LL_VERBOSE, "Background AOF rewrite signal handler took %lldus", ustime()-now); } else if (!bysignal && exitcode != 0) { + server.aof_lastbgrewrite_status = C_ERR; + + serverLog(LL_WARNING, + "Background AOF rewrite terminated with error"); + } else { /* SIGUSR1 is whitelisted, so we have a way to kill a child without * tirggering an error condition. */ if (bysignal != SIGUSR1) server.aof_lastbgrewrite_status = C_ERR; - serverLog(LL_WARNING, - "Background AOF rewrite terminated with error"); - } else { - server.aof_lastbgrewrite_status = C_ERR; serverLog(LL_WARNING, "Background AOF rewrite terminated by signal %d", bysignal); From dd4798802c82ce674be00d908a66903f46dda5a6 Mon Sep 17 00:00:00 2001 From: wangyuan21 Date: Tue, 31 Dec 2019 19:53:00 +0800 Subject: [PATCH 05/22] free time event when delete eventloop --- src/ae.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/ae.c b/src/ae.c index 2c1dae512..248096e1f 100644 --- a/src/ae.c +++ b/src/ae.c @@ -135,6 +135,13 @@ void aeDeleteEventLoop(aeEventLoop *eventLoop) { aeApiFree(eventLoop); zfree(eventLoop->events); zfree(eventLoop->fired); + /* Free time event. */ + aeTimeEvent *next_te, *te = eventLoop->timeEventHead; + while (te) { + next_te = te->next; + zfree(te); + te = next_te; + } zfree(eventLoop); } From ea697b6345830f24811a293c9689a65bc438015f Mon Sep 17 00:00:00 2001 From: antirez Date: Thu, 27 Feb 2020 17:41:48 +0100 Subject: [PATCH 06/22] Improve aeDeleteEventLoop() top comment grammar. --- src/ae.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/ae.c b/src/ae.c index 248096e1f..d2248fe5c 100644 --- a/src/ae.c +++ b/src/ae.c @@ -135,7 +135,8 @@ void aeDeleteEventLoop(aeEventLoop *eventLoop) { aeApiFree(eventLoop); zfree(eventLoop->events); zfree(eventLoop->fired); - /* Free time event. */ + + /* Free the time events list. */ aeTimeEvent *next_te, *te = eventLoop->timeEventHead; while (te) { next_te = te->next; From dafb94db7958114cd6c68d30c6e975579566df82 Mon Sep 17 00:00:00 2001 From: Ponnuvel Palaniyappan Date: Tue, 14 Jan 2020 08:10:39 +0000 Subject: [PATCH 07/22] Fix a potential overflow with strncpy --- src/config.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/config.c b/src/config.c index d55d1f8b5..5841ae7a0 100644 --- a/src/config.c +++ b/src/config.c @@ -108,12 +108,12 @@ clientBufferLimitsConfig clientBufferLimitsDefaults[CLIENT_TYPE_OBUF_COUNT] = { /* Generic config infrastructure function pointers * int is_valid_fn(val, err) * Return 1 when val is valid, and 0 when invalid. - * Optionslly set err to a static error string. + * Optionally set err to a static error string. * int update_fn(val, prev, err) * This function is called only for CONFIG SET command (not at config file parsing) * It is called after the actual config is applied, * Return 1 for success, and 0 for failure. - * Optionslly set err to a static error string. + * Optionally set err to a static error string. * On failure the config change will be reverted. */ @@ -729,7 +729,7 @@ void configSetCommand(client *c) { * config_set_memory_field(name,var) */ } config_set_memory_field( "client-query-buffer-limit",server.client_max_querybuf_len) { - /* Everyhing else is an error... */ + /* Everything else is an error... */ } config_set_else { addReplyErrorFormat(c,"Unsupported CONFIG parameter: %s", (char*)c->argv[2]->ptr); @@ -1673,9 +1673,9 @@ static int enumConfigSet(typeData data, sds value, int update, char **err) { enumerr[sdslen(enumerr) - 2] = '\0'; - /* Make sure we don't overrun the fixed buffer */ - enumerr[LOADBUF_SIZE - 1] = '\0'; strncpy(loadbuf, enumerr, LOADBUF_SIZE); + /* strncpy does not if null terminate if source string length is >= destination buffer. */ + loadbuf[LOADBUF_SIZE - 1] = '\0'; sdsfree(enumerr); *err = loadbuf; From 84243064337a60779b4ec14868be30f2c3eab490 Mon Sep 17 00:00:00 2001 From: antirez Date: Thu, 27 Feb 2020 17:45:48 +0100 Subject: [PATCH 08/22] Remove useless comment from enumConfigSet(). --- src/config.c | 1 - 1 file changed, 1 deletion(-) diff --git a/src/config.c b/src/config.c index 5841ae7a0..8d069f8db 100644 --- a/src/config.c +++ b/src/config.c @@ -1674,7 +1674,6 @@ static int enumConfigSet(typeData data, sds value, int update, char **err) { enumerr[sdslen(enumerr) - 2] = '\0'; strncpy(loadbuf, enumerr, LOADBUF_SIZE); - /* strncpy does not if null terminate if source string length is >= destination buffer. */ loadbuf[LOADBUF_SIZE - 1] = '\0'; sdsfree(enumerr); From 9d4219ebac8fe36dc5f8504e1b3ed1dec30bfa84 Mon Sep 17 00:00:00 2001 From: antirez Date: Thu, 27 Feb 2020 17:47:50 +0100 Subject: [PATCH 09/22] Fix SDS misuse in enumConfigSet(). Related to #6778. --- src/config.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/config.c b/src/config.c index 8d069f8db..aeb2fea7e 100644 --- a/src/config.c +++ b/src/config.c @@ -1666,12 +1666,12 @@ static int enumConfigSet(typeData data, sds value, int update, char **err) { sds enumerr = sdsnew("argument must be one of the following: "); configEnum *enumNode = data.enumd.enum_value; while(enumNode->name != NULL) { - enumerr = sdscatlen(enumerr, enumNode->name, strlen(enumNode->name)); + enumerr = sdscatlen(enumerr, enumNode->name, + strlen(enumNode->name)); enumerr = sdscatlen(enumerr, ", ", 2); enumNode++; } - - enumerr[sdslen(enumerr) - 2] = '\0'; + sdsrange(enumerr,0,-3); /* Remove final ", ". */ strncpy(loadbuf, enumerr, LOADBUF_SIZE); loadbuf[LOADBUF_SIZE - 1] = '\0'; From fe902461f467c28de15803cff8455abddab1288b Mon Sep 17 00:00:00 2001 From: "bodong.ybd" Date: Sat, 21 Dec 2019 21:27:38 +0800 Subject: [PATCH 10/22] Fix spop return nil #4709 --- src/t_set.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/t_set.c b/src/t_set.c index abbf82fde..60cf22d8c 100644 --- a/src/t_set.c +++ b/src/t_set.c @@ -415,7 +415,7 @@ void spopWithCountCommand(client *c) { /* Make sure a key with the name inputted exists, and that it's type is * indeed a set. Otherwise, return nil */ - if ((set = lookupKeyWriteOrReply(c,c->argv[1],shared.null[c->resp])) + if ((set = lookupKeyWriteOrReply(c,c->argv[1],shared.emptyset[c->resp])) == NULL || checkType(c,set,OBJ_SET)) return; /* If count is zero, serve an empty set ASAP to avoid special From 141c0679a03795fc395ef8b03818902c8a8fe24a Mon Sep 17 00:00:00 2001 From: antirez Date: Thu, 27 Feb 2020 18:04:34 +0100 Subject: [PATCH 11/22] Changelog: explain Redis 6 SPOP change. --- 00-RELEASENOTES | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/00-RELEASENOTES b/00-RELEASENOTES index 3572bcc54..9d1670536 100644 --- a/00-RELEASENOTES +++ b/00-RELEASENOTES @@ -114,7 +114,10 @@ Redis 6.0 is mostly a strict superset of 5.0, you should not have any problem upgrading your application from 5.0 to 6.0. However this is a list of small non-backward compatible changes introduced in the 6.0 release: -* Nothing found yet. +* The SPOP command no longer returns null when the set key does not + exist. Now it returns the empty set as it should and as happens when it is + called with a 0 argument. This is technically a fix, however it changes the + old behavior. -------------------------------------------------------------------------------- From e3c1f43952e324ffc3c0d940130b1eda696708e2 Mon Sep 17 00:00:00 2001 From: antirez Date: Thu, 27 Feb 2020 18:21:12 +0100 Subject: [PATCH 12/22] Show Redis version when not understanding a config directive. This makes simpler to give people help when posting such kind of errors in the mailing list or other help forums, because sometimes the directive looks well spelled, but the version of Redis they are using is not able to support it. --- src/config.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/config.c b/src/config.c index aeb2fea7e..fd04b7c87 100644 --- a/src/config.c +++ b/src/config.c @@ -518,7 +518,8 @@ void loadServerConfigFromString(char *config) { return; loaderr: - fprintf(stderr, "\n*** FATAL CONFIG FILE ERROR ***\n"); + fprintf(stderr, "\n*** FATAL CONFIG FILE ERROR (Redis %s) ***\n", + REDIS_VERSION); fprintf(stderr, "Reading the configuration file, at line %d\n", linenum); fprintf(stderr, ">>> '%s'\n", lines[i]); fprintf(stderr, "%s\n", err); From df152b0ce7260e1edc80bce5c7e0a853cc66cbab Mon Sep 17 00:00:00 2001 From: Guy Benoish Date: Wed, 18 Dec 2019 12:27:03 +0530 Subject: [PATCH 13/22] streamReplyWithRangeFromConsumerPEL: Redundant streamDecodeID --- src/t_stream.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/t_stream.c b/src/t_stream.c index e1efc6bca..557d1d642 100644 --- a/src/t_stream.c +++ b/src/t_stream.c @@ -1068,9 +1068,7 @@ size_t streamReplyWithRangeFromConsumerPEL(client *c, stream *s, streamID *start * by the user by other means. In that case we signal it emitting * the ID but then a NULL entry for the fields. */ addReplyArrayLen(c,2); - streamID id; - streamDecodeID(ri.key,&id); - addReplyStreamID(c,&id); + addReplyStreamID(c,&thisid); addReplyNullArray(c); } else { streamNACK *nack = ri.data; From afe0b16c02b48ff7af2a616fdfdd54b37f64758d Mon Sep 17 00:00:00 2001 From: Oran Agra Date: Sun, 16 Feb 2020 15:43:19 +0200 Subject: [PATCH 14/22] module api docs for aux_save and aux_load --- src/module.c | 6 ++++++ src/rdb.c | 2 +- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/src/module.c b/src/module.c index cce3d1c11..17af5336e 100644 --- a/src/module.c +++ b/src/module.c @@ -3529,6 +3529,8 @@ void moduleTypeNameByID(char *name, uint64_t moduleid) { * // Optional fields * .digest = myType_DigestCallBack, * .mem_usage = myType_MemUsageCallBack, + * .aux_load = myType_AuxRDBLoadCallBack, + * .aux_save = myType_AuxRDBSaveCallBack, * } * * * **rdb_load**: A callback function pointer that loads data from RDB files. @@ -3536,6 +3538,10 @@ void moduleTypeNameByID(char *name, uint64_t moduleid) { * * **aof_rewrite**: A callback function pointer that rewrites data as commands. * * **digest**: A callback function pointer that is used for `DEBUG DIGEST`. * * **free**: A callback function pointer that can free a type value. + * * **aux_save**: A callback function pointer that saves out of keyspace data to RDB files. + * 'when' argument is either REDISMODULE_AUX_BEFORE_RDB or REDISMODULE_AUX_AFTER_RDB. + * * **aux_load**: A callback function pointer that loads out of keyspace data from RDB files. + * Similar to aux_save, returns REDISMODULE_OK on success, and ERR otherwise. * * The **digest* and **mem_usage** methods should currently be omitted since * they are not yet implemented inside the Redis modules core. diff --git a/src/rdb.c b/src/rdb.c index 61265433d..cbcea96c6 100644 --- a/src/rdb.c +++ b/src/rdb.c @@ -2195,7 +2195,7 @@ int rdbLoadRio(rio *rdb, int rdbflags, rdbSaveInfo *rsi) { io.ver = 2; /* Call the rdb_load method of the module providing the 10 bit * encoding version in the lower 10 bits of the module ID. */ - if (mt->aux_load(&io,moduleid&1023, when) || io.error) { + if (mt->aux_load(&io,moduleid&1023, when) != REDISMODULE_OK || io.error) { moduleTypeNameByID(name,moduleid); serverLog(LL_WARNING,"The RDB file contains module AUX data for the module type '%s', that the responsible module is not able to load. Check for modules log above for additional clues.", name); exit(1); From 3144a278ddcbf86a73ce018bc510b5bc2222758f Mon Sep 17 00:00:00 2001 From: Oran Agra Date: Thu, 6 Feb 2020 14:53:33 +0200 Subject: [PATCH 15/22] add no_auth to COMMAND INFO --- src/server.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/server.c b/src/server.c index 22c81070c..bb8b3b103 100644 --- a/src/server.c +++ b/src/server.c @@ -3773,6 +3773,7 @@ void addReplyCommand(client *c, struct redisCommand *cmd) { flagcount += addReplyCommandFlag(c,cmd,CMD_SKIP_SLOWLOG, "skip_slowlog"); flagcount += addReplyCommandFlag(c,cmd,CMD_ASKING, "asking"); flagcount += addReplyCommandFlag(c,cmd,CMD_FAST, "fast"); + flagcount += addReplyCommandFlag(c,cmd,CMD_NO_AUTH, "no_auth"); if ((cmd->getkeys_proc && !(cmd->flags & CMD_MODULE)) || cmd->flags & CMD_MODULE_GETKEYS) { From 650484604c3e29c9da42c6b8d8a092f46a85e68e Mon Sep 17 00:00:00 2001 From: Guy Benoish Date: Tue, 4 Feb 2020 19:28:09 +0530 Subject: [PATCH 16/22] Add RM_CreateStringFromDouble --- src/module.c | 12 ++++++++++++ src/redismodule.h | 2 ++ 2 files changed, 14 insertions(+) diff --git a/src/module.c b/src/module.c index 17af5336e..b00118681 100644 --- a/src/module.c +++ b/src/module.c @@ -1042,6 +1042,17 @@ RedisModuleString *RM_CreateStringFromLongLong(RedisModuleCtx *ctx, long long ll return RM_CreateString(ctx,buf,len); } +/* Like RedisModule_CreatString(), but creates a string starting from a double + * integer instead of taking a buffer and its length. + * + * The returned string must be released with RedisModule_FreeString() or by + * enabling automatic memory management. */ +RedisModuleString *RM_CreateStringFromDouble(RedisModuleCtx *ctx, double d) { + char buf[128]; + size_t len = d2string(buf,sizeof(buf),d); + return RM_CreateString(ctx,buf,len); +} + /* Like RedisModule_CreatString(), but creates a string starting from a long * double. * @@ -7690,6 +7701,7 @@ void moduleRegisterCoreAPI(void) { REGISTER_API(CreateStringFromCallReply); REGISTER_API(CreateString); REGISTER_API(CreateStringFromLongLong); + REGISTER_API(CreateStringFromDouble); REGISTER_API(CreateStringFromLongDouble); REGISTER_API(CreateStringFromString); REGISTER_API(CreateStringPrintf); diff --git a/src/redismodule.h b/src/redismodule.h index e74611f13..a43443f13 100644 --- a/src/redismodule.h +++ b/src/redismodule.h @@ -467,6 +467,7 @@ size_t REDISMODULE_API_FUNC(RedisModule_CallReplyLength)(RedisModuleCallReply *r RedisModuleCallReply *REDISMODULE_API_FUNC(RedisModule_CallReplyArrayElement)(RedisModuleCallReply *reply, size_t idx); RedisModuleString *REDISMODULE_API_FUNC(RedisModule_CreateString)(RedisModuleCtx *ctx, const char *ptr, size_t len); RedisModuleString *REDISMODULE_API_FUNC(RedisModule_CreateStringFromLongLong)(RedisModuleCtx *ctx, long long ll); +RedisModuleString *REDISMODULE_API_FUNC(RedisModule_CreateStringFromDouble)(RedisModuleCtx *ctx, double d); RedisModuleString *REDISMODULE_API_FUNC(RedisModule_CreateStringFromLongDouble)(RedisModuleCtx *ctx, long double ld, int humanfriendly); RedisModuleString *REDISMODULE_API_FUNC(RedisModule_CreateStringFromString)(RedisModuleCtx *ctx, const RedisModuleString *str); RedisModuleString *REDISMODULE_API_FUNC(RedisModule_CreateStringPrintf)(RedisModuleCtx *ctx, const char *fmt, ...); @@ -726,6 +727,7 @@ static int RedisModule_Init(RedisModuleCtx *ctx, const char *name, int ver, int REDISMODULE_GET_API(CreateStringFromCallReply); REDISMODULE_GET_API(CreateString); REDISMODULE_GET_API(CreateStringFromLongLong); + REDISMODULE_GET_API(CreateStringFromDouble); REDISMODULE_GET_API(CreateStringFromLongDouble); REDISMODULE_GET_API(CreateStringFromString); REDISMODULE_GET_API(CreateStringPrintf); From fff6b26ae3c6144876c5971ce7b292b7da96365a Mon Sep 17 00:00:00 2001 From: Oran Agra Date: Wed, 5 Feb 2020 18:24:14 +0200 Subject: [PATCH 17/22] RM_Scan disable dict rehashing The callback approach we took is very efficient, the module can do any filtering of keys without building any list and cloning strings, it can also read data from the key's value. but if the user tries to re-open the key, or any other key, this can cause dict re-hashing (dictFind does that), and that's very bad to do from inside dictScan. this commit protects the dict from doing any rehashing during scan, but also warns the user not to attempt any writes or command calls from within the callback, for fear of unexpected side effects and crashes. --- src/dict.c | 7 +++++++ src/module.c | 20 ++++++++++++++------ 2 files changed, 21 insertions(+), 6 deletions(-) diff --git a/src/dict.c b/src/dict.c index 106467ef7..93e6c39a7 100644 --- a/src/dict.c +++ b/src/dict.c @@ -871,6 +871,10 @@ unsigned long dictScan(dict *d, if (dictSize(d) == 0) return 0; + /* Having a safe iterator means no rehashing can happen, see _dictRehashStep. + * This is needed in case the scan callback tries to do dictFind or alike. */ + d->iterators++; + if (!dictIsRehashing(d)) { t0 = &(d->ht[0]); m0 = t0->sizemask; @@ -937,6 +941,9 @@ unsigned long dictScan(dict *d, } while (v & (m0 ^ m1)); } + /* undo the ++ at the top */ + d->iterators--; + return v; } diff --git a/src/module.c b/src/module.c index b00118681..b821f0c31 100644 --- a/src/module.c +++ b/src/module.c @@ -6553,9 +6553,13 @@ void RM_ScanCursorDestroy(RedisModuleScanCursor *cursor) { * } * RedisModule_ScanCursorDestroy(c); * - * The function will return 1 if there are more elements to scan and 0 otherwise, - * possibly setting errno if the call failed. - * It is also possible to restart and existing cursor using RM_CursorRestart. */ + * The function will return 1 if there are more elements to scan and 0 otherwise, + * possibly setting errno if the call failed. + * It is also possible to restart and existing cursor using RM_CursorRestart. + * + * NOTE: You must avoid doing any database changes from within the callback, you should avoid any + * RedisModule_OpenKey or RedisModule_Call, if you need to do these, you need to keep the key name + * and do any work you need to do after the call to Scan returns. */ int RM_Scan(RedisModuleCtx *ctx, RedisModuleScanCursor *cursor, RedisModuleScanCB fn, void *privdata) { if (cursor->done) { errno = ENOENT; @@ -6633,9 +6637,13 @@ static void moduleScanKeyCallback(void *privdata, const dictEntry *de) { * RedisModule_CloseKey(key); * RedisModule_ScanCursorDestroy(c); * - * The function will return 1 if there are more elements to scan and 0 otherwise, - * possibly setting errno if the call failed. - * It is also possible to restart and existing cursor using RM_CursorRestart. */ + * The function will return 1 if there are more elements to scan and 0 otherwise, + * possibly setting errno if the call failed. + * It is also possible to restart and existing cursor using RM_CursorRestart. + * + * NOTE: You must avoid doing any database changes from within the callback, you should avoid any + * RedisModule_OpenKey or RedisModule_Call, if you need to do these, you need to keep the field name + * and do any work you need to do after the call to Scan returns. */ int RM_ScanKey(RedisModuleKey *key, RedisModuleScanCursor *cursor, RedisModuleScanKeyCB fn, void *privdata) { if (key == NULL || key->value == NULL) { errno = EINVAL; From c5319612b4879202d9f40b02c895561736f4af92 Mon Sep 17 00:00:00 2001 From: antirez Date: Fri, 28 Feb 2020 18:06:30 +0100 Subject: [PATCH 18/22] Modules: more details in RM_Scan API top comment. --- src/module.c | 28 ++++++++++++++++++++++------ 1 file changed, 22 insertions(+), 6 deletions(-) diff --git a/src/module.c b/src/module.c index b821f0c31..47371fb32 100644 --- a/src/module.c +++ b/src/module.c @@ -6557,9 +6557,21 @@ void RM_ScanCursorDestroy(RedisModuleScanCursor *cursor) { * possibly setting errno if the call failed. * It is also possible to restart and existing cursor using RM_CursorRestart. * - * NOTE: You must avoid doing any database changes from within the callback, you should avoid any - * RedisModule_OpenKey or RedisModule_Call, if you need to do these, you need to keep the key name - * and do any work you need to do after the call to Scan returns. */ + * IMPORTANT: This API is very similar to the Redis SCAN command from the + * point of view of the guarantees it provides. This means that the API + * may report duplicated keys, but guarantees to report at least one time + * every key that was there from the start to the end of the scanning process. + * + * NOTE: If you do database changes within the callback, you should be aware + * that the internal state of the database may change. For instance it is safe + * to delete or modify the current key, but may not be safe to delete any + * other key. + * Moreover playing with the Redis keyspace while iterating may have the + * effect of returning more duplicates. A safe pattern is to store the keys + * names you want to modify elsewhere, and perform the actions on the keys + * later when the iteration is complete. Howerver this can cost a lot of + * memory, so it may make sense to just operate on the current key when + * possible during the iteration, given that this is safe. */ int RM_Scan(RedisModuleCtx *ctx, RedisModuleScanCursor *cursor, RedisModuleScanCB fn, void *privdata) { if (cursor->done) { errno = ENOENT; @@ -6641,9 +6653,13 @@ static void moduleScanKeyCallback(void *privdata, const dictEntry *de) { * possibly setting errno if the call failed. * It is also possible to restart and existing cursor using RM_CursorRestart. * - * NOTE: You must avoid doing any database changes from within the callback, you should avoid any - * RedisModule_OpenKey or RedisModule_Call, if you need to do these, you need to keep the field name - * and do any work you need to do after the call to Scan returns. */ + * NOTE: Certain operations are unsafe while iterating the object. For instance + * while the API guarantees to return at least one time all the elements that + * are present in the data structure consistently from the start to the end + * of the iteration (see HSCAN and similar commands documentation), the more + * you play with the elements, the more duplicates you may get. In general + * deleting the current element of the data structure is safe, while removing + * the key you are iterating is not safe. */ int RM_ScanKey(RedisModuleKey *key, RedisModuleScanCursor *cursor, RedisModuleScanKeyCB fn, void *privdata) { if (key == NULL || key->value == NULL) { errno = EINVAL; From edc0ed141573cfa62fcc890b9f0979de6d7947a5 Mon Sep 17 00:00:00 2001 From: antirez Date: Fri, 28 Feb 2020 18:09:46 +0100 Subject: [PATCH 19/22] Modules: reformat RM_Scan() top comment a bit. --- src/module.c | 33 +++++++++++++++++++++------------ 1 file changed, 21 insertions(+), 12 deletions(-) diff --git a/src/module.c b/src/module.c index 47371fb32..bbd54082c 100644 --- a/src/module.c +++ b/src/module.c @@ -6526,24 +6526,32 @@ void RM_ScanCursorDestroy(RedisModuleScanCursor *cursor) { zfree(cursor); } -/* Scan api that allows a module to scan all the keys and value in the selected db. +/* Scan API that allows a module to scan all the keys and value in + * the selected db. * * Callback for scan implementation. - * void scan_callback(RedisModuleCtx *ctx, RedisModuleString *keyname, RedisModuleKey *key, void *privdata); - * - ctx - the redis module context provided to for the scan. - * - keyname - owned by the caller and need to be retained if used after this function. - * - key - holds info on the key and value, it is provided as best effort, in some cases it might - * be NULL, in which case the user should (can) use RedisModule_OpenKey (and CloseKey too). - * when it is provided, it is owned by the caller and will be free when the callback returns. - * - privdata - the user data provided to RedisModule_Scan. + * void scan_callback(RedisModuleCtx *ctx, RedisModuleString *keyname, + * RedisModuleKey *key, void *privdata); + * ctx - the redis module context provided to for the scan. + * keyname - owned by the caller and need to be retained if used after this + * function. + * + * key - holds info on the key and value, it is provided as best effort, in + * some cases it might be NULL, in which case the user should (can) use + * RedisModule_OpenKey (and CloseKey too). + * when it is provided, it is owned by the caller and will be free when the + * callback returns. + * + * privdata - the user data provided to RedisModule_Scan. * * The way it should be used: * RedisModuleCursor *c = RedisModule_ScanCursorCreate(); * while(RedisModule_Scan(ctx, c, callback, privateData)); * RedisModule_ScanCursorDestroy(c); * - * It is also possible to use this API from another thread while the lock is acquired durring - * the actuall call to RM_Scan: + * It is also possible to use this API from another thread while the lock + * is acquired durring the actuall call to RM_Scan: + * * RedisModuleCursor *c = RedisModule_ScanCursorCreate(); * RedisModule_ThreadSafeContextLock(ctx); * while(RedisModule_Scan(ctx, c, callback, privateData)){ @@ -6553,8 +6561,9 @@ void RM_ScanCursorDestroy(RedisModuleScanCursor *cursor) { * } * RedisModule_ScanCursorDestroy(c); * - * The function will return 1 if there are more elements to scan and 0 otherwise, - * possibly setting errno if the call failed. + * The function will return 1 if there are more elements to scan and + * 0 otherwise, possibly setting errno if the call failed. + * * It is also possible to restart and existing cursor using RM_CursorRestart. * * IMPORTANT: This API is very similar to the Redis SCAN command from the From 10e71b3d01eb2e5d37227ca1f79ee6623fae62f2 Mon Sep 17 00:00:00 2001 From: Oran Agra Date: Wed, 5 Feb 2020 18:06:33 +0200 Subject: [PATCH 20/22] Optimize temporary memory allocations for getKeysFromCommand mechanism now that we may use it more often (ACL), these excessive calls to malloc and free can become an overhead. --- src/db.c | 41 +++++++++++++++++++++++++++++++---------- 1 file changed, 31 insertions(+), 10 deletions(-) diff --git a/src/db.c b/src/db.c index 8a0242d9e..6c5d4b4d4 100644 --- a/src/db.c +++ b/src/db.c @@ -1305,6 +1305,8 @@ int expireIfNeeded(redisDb *db, robj *key) { /* ----------------------------------------------------------------------------- * API to get key arguments from commands * ---------------------------------------------------------------------------*/ +#define MAX_KEYS_BUFFER 65536 +static int getKeysTempBuffer[MAX_KEYS_BUFFER]; /* The base case is to use the keys position as given in the command table * (firstkey, lastkey, step). */ @@ -1319,7 +1321,12 @@ int *getKeysUsingCommandTable(struct redisCommand *cmd,robj **argv, int argc, in last = cmd->lastkey; if (last < 0) last = argc+last; - keys = zmalloc(sizeof(int)*((last - cmd->firstkey)+1)); + + int count = ((last - cmd->firstkey)+1); + keys = getKeysTempBuffer; + if (count > MAX_KEYS_BUFFER) + keys = zmalloc(sizeof(int)*count); + for (j = cmd->firstkey; j <= last; j += cmd->keystep) { if (j >= argc) { /* Modules commands, and standard commands with a not fixed number @@ -1329,7 +1336,7 @@ int *getKeysUsingCommandTable(struct redisCommand *cmd,robj **argv, int argc, in * return no keys and expect the command implementation to report * an arity or syntax error. */ if (cmd->flags & CMD_MODULE || cmd->arity < 0) { - zfree(keys); + getKeysFreeResult(keys); *numkeys = 0; return NULL; } else { @@ -1365,7 +1372,8 @@ int *getKeysFromCommand(struct redisCommand *cmd, robj **argv, int argc, int *nu /* Free the result of getKeysFromCommand. */ void getKeysFreeResult(int *result) { - zfree(result); + if (result != getKeysTempBuffer) + zfree(result); } /* Helper function to extract keys from following commands: @@ -1386,7 +1394,9 @@ int *zunionInterGetKeys(struct redisCommand *cmd, robj **argv, int argc, int *nu /* Keys in z{union,inter}store come from two places: * argv[1] = storage key, * argv[3...n] = keys to intersect */ - keys = zmalloc(sizeof(int)*(num+1)); + keys = getKeysTempBuffer; + if (num+1>MAX_KEYS_BUFFER) + keys = zmalloc(sizeof(int)*(num+1)); /* Add all key positions for argv[3...n] to keys[] */ for (i = 0; i < num; i++) keys[i] = 3+i; @@ -1412,7 +1422,10 @@ int *evalGetKeys(struct redisCommand *cmd, robj **argv, int argc, int *numkeys) return NULL; } - keys = zmalloc(sizeof(int)*num); + keys = getKeysTempBuffer; + if (num>MAX_KEYS_BUFFER) + keys = zmalloc(sizeof(int)*num); + *numkeys = num; /* Add all key positions for argv[3...n] to keys[] */ @@ -1433,7 +1446,7 @@ int *sortGetKeys(struct redisCommand *cmd, robj **argv, int argc, int *numkeys) UNUSED(cmd); num = 0; - keys = zmalloc(sizeof(int)*2); /* Alloc 2 places for the worst case. */ + keys = getKeysTempBuffer; /* Alloc 2 places for the worst case. */ keys[num++] = 1; /* is always present. */ @@ -1491,7 +1504,10 @@ int *migrateGetKeys(struct redisCommand *cmd, robj **argv, int argc, int *numkey } } - keys = zmalloc(sizeof(int)*num); + keys = getKeysTempBuffer; + if (num>MAX_KEYS_BUFFER) + keys = zmalloc(sizeof(int)*num); + for (i = 0; i < num; i++) keys[i] = first+i; *numkeys = num; return keys; @@ -1524,7 +1540,9 @@ int *georadiusGetKeys(struct redisCommand *cmd, robj **argv, int argc, int *numk * argv[1] = key, * argv[5...n] = stored key if present */ - keys = zmalloc(sizeof(int) * num); + keys = getKeysTempBuffer; + if (num>MAX_KEYS_BUFFER) + keys = zmalloc(sizeof(int) * num); /* Add all key positions to keys[] */ keys[0] = 1; @@ -1542,7 +1560,7 @@ int *memoryGetKeys(struct redisCommand *cmd, robj **argv, int argc, int *numkeys UNUSED(cmd); if (argc >= 3 && !strcasecmp(argv[1]->ptr,"usage")) { - keys = zmalloc(sizeof(int) * 1); + keys = getKeysTempBuffer; keys[0] = 2; *numkeys = 1; return keys; @@ -1589,7 +1607,10 @@ int *xreadGetKeys(struct redisCommand *cmd, robj **argv, int argc, int *numkeys) num /= 2; /* We have half the keys as there are arguments because there are also the IDs, one per key. */ - keys = zmalloc(sizeof(int) * num); + keys = getKeysTempBuffer; + if (num>MAX_KEYS_BUFFER) + keys = zmalloc(sizeof(int) * num); + for (i = streams_pos+1; i < argc-num; i++) keys[i-streams_pos-1] = i; *numkeys = num; return keys; From 07dc1b42fb9910bbc9c2c7e22f2feff9183bd0f7 Mon Sep 17 00:00:00 2001 From: antirez Date: Mon, 2 Mar 2020 16:49:11 +0100 Subject: [PATCH 21/22] Use a smaller getkeys global buffer. The idea is that very few commands have a lot of keys, and when this happens the allocation time becomes neglegible. --- src/db.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/db.c b/src/db.c index 6c5d4b4d4..04e26c33b 100644 --- a/src/db.c +++ b/src/db.c @@ -1305,7 +1305,7 @@ int expireIfNeeded(redisDb *db, robj *key) { /* ----------------------------------------------------------------------------- * API to get key arguments from commands * ---------------------------------------------------------------------------*/ -#define MAX_KEYS_BUFFER 65536 +#define MAX_KEYS_BUFFER 256 static int getKeysTempBuffer[MAX_KEYS_BUFFER]; /* The base case is to use the keys position as given in the command table From be4bc1a5be26a7fde2fd05acd8187f5f0ed59f25 Mon Sep 17 00:00:00 2001 From: antirez Date: Tue, 3 Mar 2020 14:58:11 +0100 Subject: [PATCH 22/22] Remove RDB files used for replication in persistence-less instances. --- src/replication.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++- src/server.c | 8 ++++++++ src/server.h | 1 + 3 files changed, 56 insertions(+), 1 deletion(-) diff --git a/src/replication.c b/src/replication.c index c497051c8..e71b269d8 100644 --- a/src/replication.c +++ b/src/replication.c @@ -45,6 +45,11 @@ void replicationSendAck(void); void putSlaveOnline(client *slave); int cancelReplicationHandshake(void); +/* We take a global flag to remember if this instance generated an RDB + * because of replication, so that we can remove the RDB file in case + * the instance is configured to have no persistence. */ +int RDBGeneratedByReplication = 0; + /* --------------------------- Utility functions ---------------------------- */ /* Return the pointer to a string representing the slave ip:listening_port @@ -591,6 +596,10 @@ int startBgsaveForReplication(int mincapa) { retval = C_ERR; } + /* If we succeeded to start a BGSAVE with disk target, let's remember + * this fact, so that we can later delete the file if needed. */ + if (retval == C_OK && !socket_target) RDBGeneratedByReplication = 1; + /* If we failed to BGSAVE, remove the slaves waiting for a full * resynchronization from the list of slaves, inform them with * an error about what happened, close the connection ASAP. */ @@ -883,6 +892,36 @@ void putSlaveOnline(client *slave) { replicationGetSlaveName(slave)); } +/* We call this function periodically to remove an RDB file that was + * generated because of replication, in an instance that is otherwise + * without any persistence. We don't want instances without persistence + * to take RDB files around, this violates certain policies in certain + * environments. */ +void removeRDBUsedToSyncReplicas(void) { + if (allPersistenceDisabled() && RDBGeneratedByReplication) { + client *slave; + listNode *ln; + listIter li; + + int delrdb = 1; + listRewind(server.slaves,&li); + while((ln = listNext(&li))) { + slave = ln->value; + if (slave->replstate == SLAVE_STATE_WAIT_BGSAVE_START || + slave->replstate == SLAVE_STATE_WAIT_BGSAVE_END || + slave->replstate == SLAVE_STATE_SEND_BULK) + { + delrdb = 0; + break; /* No need to check the other replicas. */ + } + } + if (delrdb) { + RDBGeneratedByReplication = 0; + unlink(server.rdb_filename); + } + } +} + void sendBulkToSlave(connection *conn) { client *slave = connGetPrivateData(conn); char buf[PROTO_IOBUF_LEN]; @@ -894,7 +933,8 @@ void sendBulkToSlave(connection *conn) { if (slave->replpreamble) { nwritten = connWrite(conn,slave->replpreamble,sdslen(slave->replpreamble)); if (nwritten == -1) { - serverLog(LL_VERBOSE,"Write error sending RDB preamble to replica: %s", + serverLog(LL_VERBOSE, + "Write error sending RDB preamble to replica: %s", connGetLastError(conn)); freeClient(slave); return; @@ -1639,12 +1679,14 @@ void readSyncBulkPayload(connection *conn) { "Failed trying to load the MASTER synchronization " "DB from disk"); cancelReplicationHandshake(); + if (allPersistenceDisabled()) unlink(server.rdb_filename); /* Note that there's no point in restarting the AOF on sync failure, it'll be restarted when sync succeeds or replica promoted. */ return; } /* Cleanup. */ + if (allPersistenceDisabled()) unlink(server.rdb_filename); zfree(server.repl_transfer_tmpfile); close(server.repl_transfer_fd); server.repl_transfer_fd = -1; @@ -3149,6 +3191,10 @@ void replicationCron(void) { } } + /* Remove the RDB file used for replication if Redis is not running + * with any persistence. */ + removeRDBUsedToSyncReplicas(); + /* Refresh the number of slaves with lag <= min-slaves-max-lag. */ refreshGoodSlavesCount(); replication_cron_loops++; /* Incremented with frequency 1 HZ. */ diff --git a/src/server.c b/src/server.c index bb8b3b103..a6d4b357e 100644 --- a/src/server.c +++ b/src/server.c @@ -1455,12 +1455,20 @@ void updateDictResizePolicy(void) { dictDisableResize(); } +/* Return true if there are no active children processes doing RDB saving, + * AOF rewriting, or some side process spawned by a loaded module. */ int hasActiveChildProcess() { return server.rdb_child_pid != -1 || server.aof_child_pid != -1 || server.module_child_pid != -1; } +/* Return true if this instance has persistence completely turned off: + * both RDB and AOF are disabled. */ +int allPersistenceDisabled(void) { + return server.saveparamslen == 0 && server.aof_state == AOF_OFF; +} + /* ======================= Cron: called every 100 ms ======================== */ /* Add a sample to the operations per second array of samples. */ diff --git a/src/server.h b/src/server.h index 87c293c26..5763671e4 100644 --- a/src/server.h +++ b/src/server.h @@ -1786,6 +1786,7 @@ void loadingProgress(off_t pos); void stopLoading(int success); void startSaving(int rdbflags); void stopSaving(int success); +int allPersistenceDisabled(void); #define DISK_ERROR_TYPE_AOF 1 /* Don't accept writes: AOF errors. */ #define DISK_ERROR_TYPE_RDB 2 /* Don't accept writes: RDB errors. */