Hash Field Expiration - listpack support #13209

tezc · 2024-04-15T03:45:46Z

Changes:

Adds listpack support to hash field expiration
Implements hgetf/hsetf commands

Listpack support for hash field expiration

We keep field name and value pairs in listpack for the hash type. With this PR, if one of hash field expiration command is called on the key for the first time, it converts listpack layout to triplets to hold field name, value and ttl per field. If a field does not have a TTL, we store zero as the ttl value. Zero is encoded as two bytes in the listpack. So, once we convert listpack to hold triplets, for the fields that don't have a TTL, it will be consuming those extra 2 bytes per item. Fields are ordered by ttl in the listpack to find the field with minimum expiry time efficiently.

New command implementations as part of this PR:

HGETF command

For each specified field get its value and optionally set the field's expiration time in sec/msec /unix-sec/unix-msec:

HGETF key 
  [NX | XX | GT | LT]
  [EX seconds | PX milliseconds | EXAT unix-time-seconds | PXAT unix-time-milliseconds | PERSIST] 
  <FIELDS count field [field ...]>

HSETF command

For each specified field value pair: set field to value and optionally set the field's expiration time in sec/msec /unix-sec/unix-msec:

HSETF key 
  [DC] 
  [DCF | DOF] 
  [NX | XX | GT | LT] 
  [GETNEW | GETOLD] 
  [EX seconds | PX milliseconds | EXAT unix-time-seconds | PXAT unix-time-milliseconds | KEEPTTL] 
  <FVS count field value [field value …]>

Todo:

Performance improvement.
rdb load/save
aof
defrag

src/t_hash.c

tezc · 2024-04-15T03:48:20Z

src/t_hash.c

+        if (val != HASH_LP_NO_TTL && (uint64_t) val < minExpire)
+            minExpire = val;
+
+        fptr = lpNext(lpt->lp, fptr);


sun: here could be a bottleneck, maybe we can add lpForEach with a callback method and use lpSkip to traverse lp internally, it will be much faster than three lpNext().

src/listpack.h

tezc · 2024-04-15T03:53:22Z

tests/unit/type/hash-field-expire.tcl

@@ -0,0 +1,629 @@
+######## HEXPIRE family commands


Github got confused and shows a huge diff for my commit here. Maybe looking it on git might help. It is better there.

src/t_hash.c

src/server.h

src/object.c

src/t_hash.c

src/db.c

src/listpack.h

src/listpack.c

tezc · 2024-04-27T00:11:31Z

@moticless I'm stuck adding hsetf to new setex api. It becomes too complex. I just realized I got setex api impl wrong for listpack even without hsetf (lost track of return values etc.). I started to feel like we would do better without an unified api (maybe it'd better to have some other smaller abstractions).

I pushed hgetf, I'll prepare and add hsetf without using setex api. There will be duplicated code for these but otherwise I cannot make progress. After that, maybe we'll find a way to reuse some code or you may have a better idea how to combine it to setex api. Meanwhile, please feel free to push changes to integration branch. I won't be able to do my work quickly anyway.

src/db.c

src/listpack.h

src/t_hash.c

moticless

Impressive job.

src/listpack.c

src/server.h

src/t_hash.c

sundb · 2024-05-07T06:30:21Z

src/t_hash.c

+    s = lpGetValue(vptr, &slen, &val);
+    if (s) {
+        /* Normally, item length in the listpack is limited by
+         * 'hash-max-listpack-value' config. It is unlikely, but it might be
+         * larger than sizeof(tmp). */
+        if (slen > sizeof(tmp))
+            p = sdsnewlen(s, slen);
+        else
+            memcpy(tmp, s, slen);
+    }


maybe we can use memmove to move these three entries in the future improvement.

I was thinking about lpBatchInsert() which can be used appending to the tail as well but probably moving is good enough.

src/t_hash.c

sundb · 2024-05-08T01:26:02Z

forget to handle the LISTPACK_EX encoding in rioWriteHashIteratorCursor(), dismissHashObject().
should we add LISTAPCK_EX support to debug listpack?

tezc · 2024-05-08T01:56:02Z

As we discussed, I think we can address rioWriteHashIteratorCursor() and dismissHashObject() when we are dealing with AOF and RDB save.

Added support for debug listpack.

src/t_hash.c

src/listpack.c

src/t_hash.c

src/listpack.h

src/object.c

src/t_hash.c

moticless · 2024-05-08T04:59:49Z

src/object.c

-        serverPanic("Unknown hash encoding type");
-        break;
-    }
+    hashTypeFree(o);
 }


What I missed here is that there are flows that might release a hash-fields with TTLs not in the path of dbGenericDelete() and in turn an invalid item will be leftover dangling in global HFE DS. The following sequenece of commands is such example that release a hash and avoid calling dbGenericDelete():

HSET mykey f v

HEXPIRE mykey 1 1 f

SET mykey f

I tried to avoid adding dbid into listpackEx or dictExpireMetadata and placed a hook in dbGenericDelete() instead. But looks like we cannot escape it.

We can fix it on a distinct ticket for hash and listpack, add dbid to metadata and add the logic here.

Good catch. Looks like we don't have a test case doing something similar.

As part of the fix we can add the test as well.

let's handle that in a follow up pr

tezc · 2024-05-08T16:17:44Z

@sundb @moticless @ronen-kalish Please approve if you don't have any additional comments. I'll merge it if daily run passes fine.

sundb · 2024-05-16T03:47:08Z

src/t_hash.c

+                h = lpGetValue(tptr, NULL, &expire);
+                serverAssert(h == NULL);


ref #13243 (comment)

this assertiong will never be triggered.
you passed NULL to the second parameter of lpGetValue(), if the field is string, it will crash in *slen = ele_len;.

vstr = lpGet(p, &ele_len, NULL); if (vstr) { *slen = ele_len; <- here } else { *lval = ele_len; }

crash log:

=== REDIS BUG REPORT START: Cut & paste starting from here === 64716:M 16 May 2024 11:55:33.528 # Redis 255.255.255 crashed by signal: 11, si_code: 1 64716:M 16 May 2024 11:55:33.528 # Accessing address: (nil) 64716:M 16 May 2024 11:55:33.528 # Crashed running the instruction at: 0x55c8922a2fa9 ------ STACK TRACE ------ EIP: ./src/redis-server *:6379(lpGetValue+0x54)[0x55c8922a2fa9] 64716 redis-server * /lib/x86_64-linux-gnu/libc.so.6(+0x45320)[0x701fc8a45320] ./src/redis-server *:6379(lpGetValue+0x54)[0x55c8922a2fa9] ./src/redis-server *:6379(+0x11140f)[0x55c89220b40f] ./src/redis-server *:6379(hgetfCommand+0x1a3)[0x55c89220b987] ./src/redis-server *:6379(call+0x143)[0x55c89219cb67] ./src/redis-server *:6379(processCommand+0xea1)[0x55c89219e6e1] ./src/redis-server *:6379(processCommandAndResetClient+0x39)[0x55c8921bbcc9] ./src/redis-server *:6379(processInputBuffer+0x1a0)[0x55c8921bbf4e] ./src/redis-server *:6379(readQueryFromClient+0x4b5)[0x55c8921bc516] ./src/redis-server *:6379(+0x1b836f)[0x55c8922b236f] ./src/redis-server *:6379(+0x1b8a98)[0x55c8922b2a98] ./src/redis-server *:6379(aeProcessEvents+0x24d)[0x55c892186438] ./src/redis-server *:6379(aeMain+0x2e)[0x55c892186676] ./src/redis-server *:6379(main+0xd5a)[0x55c8921a7ebe] /lib/x86_64-linux-gnu/libc.so.6(+0x2a1ca)[0x701fc8a2a1ca] /lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0x8b)[0x701fc8a2a28b] ./src/redis-server *:6379(_start+0x25)[0x55c892180155]

what about adding a new method like int lpGetInteger(unsigned char *lp, long long *lval), return 1 if it's a integer.

how did you trigger this crash?
Edit: Ok, I see with corrupt restore, it is possible.

lpGetInteger() sounds good.

sundb@17b544f
it's easy to reproduce the corrupt by using the patch.
then run ./runtest --single integration/corrupt-dump-fuzzer --accurate
@tezc

tezc requested review from sundb and moticless April 15, 2024 03:45

tezc commented Apr 15, 2024

View reviewed changes

src/t_hash.c Show resolved Hide resolved

tezc commented Apr 15, 2024

View reviewed changes

src/listpack.h Show resolved Hide resolved

tezc commented Apr 15, 2024

View reviewed changes

sundb reviewed Apr 17, 2024

View reviewed changes

src/t_hash.c Show resolved Hide resolved

tezc force-pushed the hfe-listpack-v1 branch from 83ba789 to b0be597 Compare April 20, 2024 00:34

moticless reviewed Apr 22, 2024

View reviewed changes

src/t_hash.c Outdated Show resolved Hide resolved

moticless reviewed Apr 22, 2024

View reviewed changes

src/t_hash.c Outdated Show resolved Hide resolved

moticless reviewed Apr 24, 2024

View reviewed changes

add listpack support (todo: fix rebase issues)

5fe4b02

tezc force-pushed the hfe-listpack-v1 branch from c36381c to 5fe4b02 Compare April 26, 2024 09:05

test fix

1318272

tezc requested a review from ronen-kalish April 26, 2024 09:45

tezc added 3 commits April 27, 2024 00:33

add hgetf

cd7396c

silence compiler warning

3b5b33b

fix

eada402

sundb reviewed Apr 28, 2024

View reviewed changes

src/db.c Outdated Show resolved Hide resolved

src/db.c Outdated Show resolved Hide resolved

src/db.c Outdated Show resolved Hide resolved

src/listpack.h Show resolved Hide resolved

src/t_hash.c Outdated Show resolved Hide resolved

hsetf

2d40d61

vladvildanov mentioned this pull request Apr 29, 2024

Added support for hash field expiration commands predis/predis#1456

Draft

tezc added 6 commits May 1, 2024 09:27

simply hsetf

7e8cd04

comment

aa096d1

expose struct listpackTTL

2296d05

minor

920d784

minor

d0b523c

rename listpackttl to listpackex

6c6c6a4

sundb mentioned this pull request May 3, 2024

Add notification support for HFE #13237

Merged

moticless reviewed May 4, 2024

View reviewed changes

sundb reviewed May 7, 2024

View reviewed changes

src/t_hash.c Outdated Show resolved Hide resolved

sundb reviewed May 7, 2024

View reviewed changes

tezc added 3 commits May 7, 2024 16:47

use getRangeLongFromObjectOrReply()

f51900a

Use large num in lp test

6fe761a

try to stabilize test

169070e

sundb reviewed May 8, 2024

View reviewed changes

src/t_hash.c Outdated Show resolved Hide resolved

src/t_hash.c Outdated Show resolved Hide resolved

src/t_hash.c Outdated Show resolved Hide resolved

test change

975b5ad

comments

2ddb307

sundb reviewed May 8, 2024

View reviewed changes

src/t_hash.c Outdated Show resolved Hide resolved

sundb reviewed May 8, 2024

View reviewed changes

src/t_hash.c Outdated Show resolved Hide resolved

moticless reviewed May 8, 2024

View reviewed changes

tezc added 7 commits May 8, 2024 12:17

some comments

84d061d

comments and fixes

4a861c1

comments and fixes

002fba2

handle DCF on non existing object

7b253f3

stabilize test

1649a2e

Merge

a0be4ad

stabilize test

0755068

sundb previously approved these changes May 8, 2024

View reviewed changes

moticless previously approved these changes May 8, 2024

View reviewed changes

fix pointer cast

4bc8ade

tezc dismissed stale reviews from moticless and sundb via 4bc8ade May 8, 2024 16:46

sundb approved these changes May 8, 2024

View reviewed changes

moticless approved these changes May 8, 2024

View reviewed changes

tezc merged commit ca4ed48 into redis:hash-field-expiry-integ May 8, 2024
14 checks passed

sundb reviewed May 16, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hash Field Expiration - listpack support #13209

Hash Field Expiration - listpack support #13209

tezc commented Apr 15, 2024 •

edited

tezc Apr 15, 2024

tezc Apr 15, 2024

tezc commented Apr 27, 2024

moticless left a comment

sundb May 7, 2024

tezc May 7, 2024

sundb commented May 8, 2024

tezc commented May 8, 2024 •

edited

moticless May 8, 2024

tezc May 8, 2024

moticless May 8, 2024

tezc May 8, 2024

tezc commented May 8, 2024

sundb May 16, 2024

tezc May 16, 2024 •

edited

sundb May 16, 2024

		h = lpGetValue(tptr, NULL, &expire);
		serverAssert(h == NULL);

Hash Field Expiration - listpack support #13209

Hash Field Expiration - listpack support #13209

Conversation

tezc commented Apr 15, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tezc commented Apr 27, 2024

moticless left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sundb commented May 8, 2024

tezc commented May 8, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tezc commented May 8, 2024

Choose a reason for hiding this comment

tezc May 16, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tezc commented Apr 15, 2024 •

edited

tezc commented May 8, 2024 •

edited

tezc May 16, 2024 •

edited