From afe5179dd4135f8f08eabaf71996dcf74bf0b62c 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 7e6b81527c169a59ea7a2c4d840c0ce82405e9e2 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 22c571795c747b6121962518aede2025fbec8afc 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 3ab91ff2060dfc4511c2ce90ecb85622cd34401f 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 64700cca616e62342b4795ec5972ec1d7b34217a 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 ae97c31644ad6c06a066d59d1a703fefef18018c 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 3af14cfa3dd4d381018f3e8ddbef60383d389e00 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 310a80764d8b92e737fb4a3fa6f1509dedd6e0f2 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 b977f8743847b03bcafddb1d609a2f108383d587 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 9fb8903adbf5c54fe3f2d631f3e9b0049c35b184 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 446618dd9766e55c39bfccde7c3190447ce48e07 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 c1951137bb471701626ada3bffebc37d255f73f0 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 f14bc2799e32b78fdfbed5dd117aa09fb5858fc4 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 bec15e6534d003ed5d71fc76aec2f1095f73064e 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 4ca389bb60b7b0bf2e00d956ed11dd3e0611109e 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 470de731e99b4ac7e537c096dee1c163ecd4dba0 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 0693ac196136f38301833895f3335fb171d81c84 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 76cfe21ff74fdc270ebca122b42798f6bd1efd87 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 8a7e1fe3b074774643d5a15c3c670c7c4e5d7c80 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 328b6e473c0783b653e5824606b39b830c9fd064 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 dcafd2d77ef2a5c8ea47a029c6548989bf8d9bcf 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 e91ca9fee9c56ef319b407b104afc435be0c53cb 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. */