Add SCAN, HSCAN, SSCAN and ZSCAN cursor iteration commands#53
Add SCAN, HSCAN, SSCAN and ZSCAN cursor iteration commands#53tchalikanti1705 wants to merge 17 commits into
Conversation
Rohit-Dnath
left a comment
There was a problem hiding this comment.
This needs a design conversation before more code, so I want to be clear up front that the craft here is not the problem. The COUNT overflow guard, the lock discipline in sortedLiveKeys, holding wmu across both reply elements so a pub/sub push cannot interleave, the stateless cursor, 413 lines of tests covering arity, bad cursors, COUNT 0, dangling MATCH, expiry and wrong-type. That is careful work and it all shows.
The problem is underneath it. An offset-into-a-sorted-list cursor over a mutable map cannot deliver SCAN's contract, and no amount of polish around the edges changes that.
SCAN silently skips keys that exist for the whole iteration
The cursor is offset*shardCount + shardIndex, a positional index into a freshly sorted key list. Delete any earlier-sorted key and every later key shifts down a slot, so the cursor now points past them.
I reproduced this against your branch with scan-and-delete, which is the canonical SCAN use case:
scanned/deleted 1652 of 2000; 348 keys left behind
GUARANTEE VIOLATED: scan-and-delete swept only 1652/2000 keys;
348 present throughout were never returned
17% of the keyspace silently skipped. My read-only control over the same 2000 keys returns all 2000, so this is specifically mutation during iteration. Same failure in HSCAN/SSCAN/ZSCAN, since they all route through scanWindow: HSCAN h 0 COUNT 1 then HDEL the field you were just handed, which is exactly what a scan-and-delete client does, and the next field vanishes from the iteration.
No error is surfaced. The client believes it swept the keyspace.
The docs sentence is the part I want to push back on hardest
docs/commands.md:63 says keys can be missed and that "Redis makes a similarly weak promise."
Redis makes the opposite promise, explicitly: "A full iteration always retrieves all the elements that were present in the collection from the start to the end of a full iteration." Redis's weak guarantees are only that elements added or removed during iteration may or may not appear, and that elements may be returned more than once. Missing an element present throughout is never permitted. Redis's reverse-binary cursor exists precisely to make that impossible under concurrent mutation and rehashing.
I am flagging this separately from the bug because the honest disclosure makes the trade-off look considered, and a reviewer who trusted that sentence would have approved this. That is the thing to avoid repeating: when a design cannot meet a contract, the docs need to say so plainly rather than lower the reader's sense of what the contract was.
Also: the typed scans are quadratic and sort under the lock
sort.Strings over every field runs inside defer sh.mu.RUnlock(), on every call:
HSCAN full iteration: n=1000 → 14ms n=2000 → 63ms n=4000 → 303ms n=8000 → 1.42s
Clean quadratic. A 100k-field hash is minutes of CPU for one full HSCAN, each call holding the shard's read lock while sorting all 100k fields and blocking every writer to that shard. HSCAN exists to avoid HGETALL's blocking, and this is worse than HGETALL on both axes. It is also remotely triggerable on any large hash. The keyspace Scan path gets this right by releasing the lock before sorting, so it is only the three typed scans.
This one should be fixed regardless of what we decide about the cursor.
Where I land
Do not iterate on the offset cursor. The real path is Redis's own: a reverse-binary cursor over the bucket space, which is what survives concurrent mutation. That needs a cursor addressing buckets rather than sorted positions, and a per-shard Go map cannot be resumed positionally, so it likely needs an ordered per-shard structure. That is a substantially bigger change than this PR and it touches the core store, so I would rather agree the approach first than have you write it twice.
If we decide that is too much for now, the fallback is to keep this cursor and rewrite docs/commands.md:63 to say plainly that RAMen's SCAN does not implement Redis's guarantee and will skip keys under concurrent writes, with no appeal to Redis being similarly weak. I am genuinely torn on that, because it ships something Redis clients will misuse by default, and scan-and-delete losing 17% of keys with no error is a bad failure to hand someone.
Let's talk about which before you spend more time. Sorry to send back work this solid, but I would rather have that conversation now than after you have built on the cursor.
HSCAN, SSCAN and ZSCAN sorted every field of the hash while holding the shard's read lock, so each page of a large hash blocked all writers to that shard for a full O(n log n) sort, remotely triggerable on any big key. Copy the data under the lock and sort after releasing it, the same way the keyspace Scan's sortedLiveKeys already does.
The old wording claimed Redis makes a similarly weak promise. It does not: Redis guarantees a full iteration returns every element present from start to end, and its weak guarantees only cover elements added or removed mid-iteration or returned twice. Describe our cursor's actual behavior without leaning on Redis.
2f2bdbc to
cc8d46d
Compare
|
Thanks for the repro and for catching this before I built more on it. You're right and I'm not going to defend the cursor — scan-and-delete silently losing 17% of the keyspace is disqualifying, and my docs line about Redis was just wrong. I've read the Redis SCAN guarantee again and it promises exactly the thing my cursor can't deliver. I pushed the two things that made sense under either outcome: the typed scans now copy under the lock and sort after releasing it (the fix-regardless one), and the docs line no longer leans on Redis — it says plainly that this cursor can skip keys under concurrent writes. I haven't touched the cursor itself. On direction: I'd rather build the real thing than ship something Redis clients will misuse by default, so I'm up for the reverse-binary cursor. My understanding is the cursor has to address a stable bucket space, which a plain Go map can't resume into, so each shard needs an ordered structure the cursor can walk. Before writing any code I'd like to post a short design on the issue for you to shoot at first — same as we did for AOF. Two questions to anchor it: do you want the ordered structure to replace the shard map for all operations or sit beside it just for iteration, and is it OK if this PR parks until that design settles rather than shipping the fallback docs version? |
Summary
Adds cursor-based iteration —
SCAN,HSCAN,SSCANandZSCAN— matching Redis semantics. Clients can iterate the keyspace or a single collection incrementally instead of pulling everything at once withKEYS/HGETALL/SMEMBERS.Related Issue
Closes #11
Cursor approach (as requested on the issue)
I went with the packed-integer cursor you proposed rather than last-returned-key, so the cursor stays numeric and needs no per-connection state. For
SCANthe cursor packs the shard index and the offset into that shard's sorted key slice (offset*shardCount + shardIndex);0starts and0terminates. One small deviation, called out for review: a singleSCANcall examines up toCOUNTkeys and rolls across shards as needed, taking each shard's read lock only for its own window (never held across shards). This avoids ~256 empty round-trips on a small keyspace while keeping your exact cursor encoding. The typed scans sort one key's members by name and use the offset as the cursor.The weak guarantee is documented in
docs/commands.md: keys added or removed mid-iteration shift the offsets, so a key can be missed or repeated, exactly like Redis.MATCHis applied after theCOUNTwindow, so a page can come back empty with a non-zero cursor — there is a test asserting a client must loop until the cursor is0.Changes
SCAN <cursor> [MATCH pattern] [COUNT count]— iterate the keyspace; replies[cursor, [keys...]].HSCAN <key> <cursor> [MATCH pattern] [COUNT count]— iterate a hash; flat[field, value, ...].MATCHfilters the field name.SSCAN <key> <cursor> [MATCH pattern] [COUNT count]— iterate a set;[member, ...].ZSCAN <key> <cursor> [MATCH pattern] [COUNT count]— iterate a sorted set; flat[member, score, ...], scores formatted likeZSCORE.COUNTis a hint (default 10); a bad cursor repliesERR invalid cursor,COUNT < 1or an unknown option repliesERR syntax error.internal/store/scan.go, handlers ininternal/server/cmd_scan.go, registrations incommands.go.internal/server/server_test.goand documented indocs/commands.md.A huge
COUNTon the typed scans is bounded so it cannot overflow the slice index and panic the server.Checklist
go test ./...)go fmt ./...)