From 5f5ddfd1a1ff39617277ac0a4302bee1ef94b492 Mon Sep 17 00:00:00 2001 From: Antoni Dikov Date: Mon, 30 Mar 2026 09:07:07 +0300 Subject: [PATCH] Fix COMMAND GETKEYS for PFMERGE with no source keys (#14942) PFMERGE's second key spec (source keys) produces an empty range when called with only a dest key (e.g. PFMERGE dest). getKeysUsingKeySpecs treats that as invalid_spec, which discards all previously found keys and returns an error. Add pfmergeGetKeys as a getkeys callback so the command correctly falls back to it when key specs fail on the edge case. --- src/commands.def | 2 +- src/commands/pfmerge.json | 1 + src/db.c | 23 +++++++++++++++++++++++ src/server.h | 1 + tests/unit/introspection-2.tcl | 11 +++++++++++ 5 files changed, 37 insertions(+), 1 deletion(-) diff --git a/src/commands.def b/src/commands.def index 179c16b9a..d9d4ca577 100644 --- a/src/commands.def +++ b/src/commands.def @@ -11839,7 +11839,7 @@ struct COMMAND_STRUCT redisCommandTable[] = { {MAKE_CMD("pfadd","Adds elements to a HyperLogLog key. Creates the key if it doesn't exist.","O(1) to add every element.","2.8.9",CMD_DOC_NONE,NULL,NULL,"hyperloglog",COMMAND_GROUP_HYPERLOGLOG,PFADD_History,0,PFADD_Tips,0,pfaddCommand,-2,CMD_WRITE|CMD_DENYOOM|CMD_FAST,ACL_CATEGORY_HYPERLOGLOG,PFADD_Keyspecs,1,NULL,2),.args=PFADD_Args}, {MAKE_CMD("pfcount","Returns the approximated cardinality of the set(s) observed by the HyperLogLog key(s).","O(1) with a very small average constant time when called with a single key. O(N) with N being the number of keys, and much bigger constant times, when called with multiple keys.","2.8.9",CMD_DOC_NONE,NULL,NULL,"hyperloglog",COMMAND_GROUP_HYPERLOGLOG,PFCOUNT_History,0,PFCOUNT_Tips,0,pfcountCommand,-2,CMD_READONLY|CMD_MAY_REPLICATE,ACL_CATEGORY_HYPERLOGLOG,PFCOUNT_Keyspecs,1,NULL,1),.args=PFCOUNT_Args}, {MAKE_CMD("pfdebug","Internal commands for debugging HyperLogLog values.","N/A","2.8.9",CMD_DOC_SYSCMD,NULL,NULL,"hyperloglog",COMMAND_GROUP_HYPERLOGLOG,PFDEBUG_History,0,PFDEBUG_Tips,0,pfdebugCommand,3,CMD_WRITE|CMD_DENYOOM|CMD_ADMIN,ACL_CATEGORY_HYPERLOGLOG,PFDEBUG_Keyspecs,1,NULL,2),.args=PFDEBUG_Args}, -{MAKE_CMD("pfmerge","Merges one or more HyperLogLog values into a single key.","O(N) to merge N HyperLogLogs, but with high constant times.","2.8.9",CMD_DOC_NONE,NULL,NULL,"hyperloglog",COMMAND_GROUP_HYPERLOGLOG,PFMERGE_History,0,PFMERGE_Tips,0,pfmergeCommand,-2,CMD_WRITE|CMD_DENYOOM,ACL_CATEGORY_HYPERLOGLOG,PFMERGE_Keyspecs,2,NULL,2),.args=PFMERGE_Args}, +{MAKE_CMD("pfmerge","Merges one or more HyperLogLog values into a single key.","O(N) to merge N HyperLogLogs, but with high constant times.","2.8.9",CMD_DOC_NONE,NULL,NULL,"hyperloglog",COMMAND_GROUP_HYPERLOGLOG,PFMERGE_History,0,PFMERGE_Tips,0,pfmergeCommand,-2,CMD_WRITE|CMD_DENYOOM,ACL_CATEGORY_HYPERLOGLOG,PFMERGE_Keyspecs,2,pfmergeGetKeys,2),.args=PFMERGE_Args}, {MAKE_CMD("pfselftest","An internal command for testing HyperLogLog values.","N/A","2.8.9",CMD_DOC_SYSCMD,NULL,NULL,"hyperloglog",COMMAND_GROUP_HYPERLOGLOG,PFSELFTEST_History,0,PFSELFTEST_Tips,0,pfselftestCommand,1,CMD_ADMIN,ACL_CATEGORY_HYPERLOGLOG,PFSELFTEST_Keyspecs,0,NULL,0)}, /* list */ {MAKE_CMD("blmove","Pops an element from a list, pushes it to another list and returns it. Blocks until an element is available otherwise. Deletes the list if the last element was moved.","O(1)","6.2.0",CMD_DOC_NONE,NULL,NULL,"list",COMMAND_GROUP_LIST,BLMOVE_History,0,BLMOVE_Tips,0,blmoveCommand,6,CMD_WRITE|CMD_DENYOOM|CMD_BLOCKING,ACL_CATEGORY_LIST,BLMOVE_Keyspecs,2,NULL,5),.args=BLMOVE_Args}, diff --git a/src/commands/pfmerge.json b/src/commands/pfmerge.json index c93070f11..e3e26fabd 100644 --- a/src/commands/pfmerge.json +++ b/src/commands/pfmerge.json @@ -6,6 +6,7 @@ "since": "2.8.9", "arity": -2, "function": "pfmergeCommand", + "get_keys_function": "pfmergeGetKeys", "command_flags": [ "WRITE", "DENYOOM" diff --git a/src/db.c b/src/db.c index 4d8f60fba..1565b7bef 100644 --- a/src/db.c +++ b/src/db.c @@ -3674,6 +3674,29 @@ int sortGetKeys(struct redisCommand *cmd, robj **argv, int argc, getKeysResult * return result->numkeys; } +int pfmergeGetKeys(struct redisCommand *cmd, robj **argv, int argc, getKeysResult *result) { + int i, numkeys; + keyReference *keys; + UNUSED(cmd); + UNUSED(argv); + + numkeys = argc - 1; /* destkey + all sourcekeys */ + keys = getKeysPrepareResult(result, numkeys); + + /* destkey at argv[1] */ + keys[0].pos = 1; + keys[0].flags = CMD_KEY_RW | CMD_KEY_ACCESS | CMD_KEY_INSERT; + + /* sourcekeys at argv[2..argc-1], may be zero */ + for (i = 2; i < argc; i++) { + keys[i - 1].pos = i; + keys[i - 1].flags = CMD_KEY_RO | CMD_KEY_ACCESS; + } + + result->numkeys = numkeys; + return result->numkeys; +} + /* This command declares incomplete keys, so the flags are correctly set for this function */ int migrateGetKeys(struct redisCommand *cmd, robj **argv, int argc, getKeysResult *result) { int i, j, num, first; diff --git a/src/server.h b/src/server.h index 6848230ec..72036f6b6 100644 --- a/src/server.h +++ b/src/server.h @@ -4026,6 +4026,7 @@ int zunionInterDiffStoreGetKeys(struct redisCommand *cmd,robj **argv, int argc, int evalGetKeys(struct redisCommand *cmd, robj **argv, int argc, getKeysResult *result); int functionGetKeys(struct redisCommand *cmd, robj **argv, int argc, getKeysResult *result); int sortGetKeys(struct redisCommand *cmd, robj **argv, int argc, getKeysResult *result); +int pfmergeGetKeys(struct redisCommand *cmd, robj **argv, int argc, getKeysResult *result); int sortROGetKeys(struct redisCommand *cmd, robj **argv, int argc, getKeysResult *result); int migrateGetKeys(struct redisCommand *cmd, robj **argv, int argc, getKeysResult *result); int georadiusGetKeys(struct redisCommand *cmd, robj **argv, int argc, getKeysResult *result); diff --git a/tests/unit/introspection-2.tcl b/tests/unit/introspection-2.tcl index 1e98fdb6a..32932ef1a 100644 --- a/tests/unit/introspection-2.tcl +++ b/tests/unit/introspection-2.tcl @@ -189,6 +189,17 @@ start_server {tags {"introspection"}} { assert_equal {key1 key2} [r command getkeys lcs key1 key2] } + test {COMMAND GETKEYS PFMERGE with and without source keys} { + # dest + sources: both key specs yield keys + assert_equal {dest src1 src2} [r command getkeys PFMERGE dest src1 src2] + + # dest only, no source keys: spec[1] yields empty range (last < first). + # Without pfmergeGetKeys this returned "Invalid arguments" because + # getKeysUsingKeySpecs treated the empty range as invalid_spec, + # discarding the dest key found by spec[0]. + assert_equal {dest} [r command getkeys PFMERGE dest] + } + test {COMMAND GETKEYS MORE THAN 256 KEYS} { set all_keys [list] set numkeys 260