A set STORE command doesn't properly overwrite an existing key

[wdscxsj]

Describe the bug

SINTERSTORE doesn't overwrite an existing string. SUNIONSTORE and SDIFFSTORE are also affected.

Steps to reproduce the bug

Please see the following Python snippet. Commands executed in the CLI have the same results. The "expected" values come from Redis 7.4.1.

import redis

r = redis.Redis()
r.set('foo', 'bar')
r.sadd('s1', *range(10))
r.sadd('s2', *range(5, 15))
r.sinterstore('foo', ['s1', 's2'])

print(r.keys('*'))    # [b'foo', b's1', b's2', b'foo']; expected: [b's2', b's1', b'foo']
print(r.type('foo'))  # 'string'; expected: 'set'

Expected behavior

No response

Screenshots

No response

Release version

v1.0.35

IDE

No response

OS version

Windows 11

Additional context

No response

[nightroman]

It's not about SINTERSTORE as such, see #358 (comment)

Garnet uses two stores internally: a main store to hold raw strings and an object store to hold objects. Thus it's not unexpected to allow the same key in both stores. Changing this will require us to check both stores every time, which is too expensive. We are working on unifying the two indexes but that's not ready yet.

Some fun with this in PowerShell using FarNet.Redis module with Garnet server

$db = Open-Redis

# set same key as String and Set
Set-RedisString test:dupe-key v1
Set-RedisSet test:dupe-key v2, v3

# 2 same keys exist in 2 stores
Search-RedisKey test:dupe-key
<# 2 keys
test:dupe-key
test:dupe-key
#>

# one is String
Get-RedisString test:dupe-key
<# String value
v1
#>

# another is Set
Get-RedisSet test:dupe-key
<# Set values
v2
v3
#>

[wdscxsj]

Thanks! I understand this issue better now.

[badrishc]

How important is it that the same key be prevented by the system from being used for the main and object store? Seems users can easily work around this. cc @TedHartMS

[nightroman]

Stating the obvious perhaps, for a key-value system uniqueness of keys is very important. Users may work around the current feature (not sure about "easily", depends). But users will be tripped over this anyway. Workarounds will come with some price. If the server does not want to pay it, then clients have to pay, arguably higher overall price.

NB If the actual plan is to move to the single index and hence fix this issue then importance is probably low. It would be useful to mention the current feature somewhere in the docs, to minimize surprises.