Revised VSS RFC

[allenss-amazon]

Replaces and extends the previous PR for VSS module.

[madolson]

Other open questions that would document for completeness:

1. Will ACLs be supported?
2. Is active defragmentation supported?
3. Is the RDB compatible with redis search.
4. I assume this a subset of redis search functionality. but would be good to document that along with what is missing.
5. How will slot migration work, if at all. Will we also copy indexes between nodes when using the cli?
6. Are any core changes required? We talked about memory sharing and some new keysspace notifications.
7. How does redirecting read requests to the primary work in cluster mode? Can you force strongly consistent reads? By the consistency model do we care?
8. Are we allowing searcg in Lua and multi-exec? Will it cause latency issues? Does redis allow it?

[madolson]

It's a mystery?

[yairgott]

Suggested changes:

On success, the first entry in the response array represents the total number of qualified matching elements, followed by one array entry for each matching element. Note that the amount of response entries may differ from the total number of response elements, captured in the response first entry, just if the LIMIT option is specified.

When NOCONTENT is specified, each entry in the response contains only the matching keys. Otherwise, each entry includes the matching key, followed by an array of the returned fields.

[madolson]

The protocol has n built in, why is it returned as the first argument?

[yairgott]

Right, see my suggested changes above.

[allenss-amazon]

compatibility with Redisearch

[madolson]

So, we have no idea why RedisSearch added it?

[madolson]

There isn't any discussion of the coordinator here, is that in scope for the initial version?

[yairgott]

Yes, it's part of the initial version and it's the way to enable cluster mode.

[allenss-amazon]

yes. will add.

[madolson]

Followup, is there a reason to disable the coordinator for cluster mode? Should it just automatically get enabled for cluster mode?

[madolson]

What is the definition of a failed request? One that errors?

[yairgott]

This captures any ft.search runtime errors including invalid syntax.

[madolson]

Yeah, I think typically those are called errors throughout the codebase.

[yairgott]

Right but this ones is specific to ft.search

[madolson]

that's not my point. My point is that at least in Valkey we call errors equivalent to 400s (You send a bad request) and failure equivalent to a 500 (we couldn't handle it). Are these errors or failures? Secondly, do we really need errors for each search request? Why are these requests special?

I ask because none of the other modules do this (or the core). If we are going to be a singular project, we should have some consistently between core commands and module commands.

[madolson]

Just making one comment, I don't really understand what most of these info fields are supposed to tell end users. Maybe document them like it's for our public documentation?

[madolson]

Still not sure what these subscriptions are, they are only mentioned here.

[yairgott]

Suggested changes:

On success, the first entry in the response array represents the total number of qualified matching elements, followed by one array entry for each matching element. Note that the amount of response entries may differ from the total number of response elements, captured in the response first entry, just if the LIMIT option is specified.

When NOCONTENT is specified, each entry in the response contains only the matching keys. Otherwise, each entry includes the matching key, followed by an array of the returned fields.

[yairgott]

Missing:
RETURN (optional): Specifies the fields you want to retrieve from your documents, along with any aliases for the returned values. By default, all fields are returned unless the NOCONTENT option is set, in which case no fields are returned. If num is set to 0, it behaves the same as NOCONTENT.

[yairgott]

Right, see my suggested changes above.

[yairgott]

Yes, it's part of the initial version and it's the way to enable cluster mode.

[yairgott]

This captures any ft.search runtime errors including invalid syntax.

[yairgott]

Other open questions that would document for completeness:

1. Will ACLs be supported?

ACLs are not supported in the current version.

2. Is active defragmentation supported?

Memory allocated by the module is not subject to active defragmentation.

3. Is the RDB compatible with redis search.

No, the RDB format is not compatible with RediSearch. The module utilizes a proprietary RDB format based on protobuf, which serializes both index metadata and content. In contrast, RediSearch's RDB format only serializes the index metadata.

4. I assume this a subset of redis search functionality. but would be good to document that along with what is missing.

Yes, we should have a section which enumerates key missing functionality as bullet points to provide clarity.

5. How will slot migration work, if at all. Will we also copy indexes between nodes when using the cli?

Slot migration is supported. Indexes are cluster level concepts so they are already replicated across the cluster, and not contained within a slot. Keys from the migrated slot trigger keyspace mutation events, which are processed in the same manner as client-originated mutations, except that they do not block the client during execution.

6. Are any core changes required? We talked about memory sharing and some new keysspace notifications.

While the module can function without engine changes, two engine changes have been introduced to enhance user experience:

1. Blocking client on keyspace mutation events: Ensures visibility of mutations in consecutive queries on the same connection. This change extends the behavior of the module API RedisModule_BlockClient.
2. Memory deduplication: Adds new module APIs to enable memory sharing between the module and the engine, reducing duplication.

7. How does redirecting read requests to the primary work in cluster mode? Can you force strongly consistent reads? By the consistency model do we care?

Valkey doesn't support distributed transactions so I don't see any consistency guarantee with scatter-gather across multiple shards.
Additional context: In cluster mode, a query can be received by any node, which fans out the request to all shards. Each shard processes the query locally and returns results to the initiating node, which selects the top responses across shards. Currently, forcing reads from primary nodes only are not supported. The current logic for instance selection is designed to improve the performance, increase throughput and lower latency, by load balancing between the instances.

8. Are we allowing searcg in Lua and multi-exec? Will it cause latency issues? Does redis allow it?
   Will ACLs be supported?

• Multi-exec: Supported, but it introduces significant performance impacts. Mutation processing cannot be parallelized, and queries inside a multi-exec context must wait for preceding mutations to complete.
• Lua scripts: The main constrain is that the engine doesn't allow the client to be blocked when issued by a LUA script. Long term, it makes sense to address this in the engine but in the meantime the current state is as follow:
  ** Mutation processing: mutations are processed but clients are not blocked.
  ** Queries: issuing ft.search is the context of LUA script would result in an error due to the fact that the client cannot be blocked.

[yairgott]

Made some inline editing to bullet #8 regarding to LUA.

[zhangh43]

RedisSearch API is weird. Pinecone/Milvus API is more natural

# create
pinecone.create_index(name=index_name, dimension=dimension)
# insert
index = pinecone.Index(index_name)
index.upsert(vectors)
# search
results = index.query(queries=[query_vector], top_k=5)

I believe developers prefer the latter.

[madolson]

So to clarify the difference:

1. In the current implementation, you insert keys with other commands and then create an index ontop of a prefix structure.
2. In the pinecone world, you create an index and then insert objects into the index explicitly.

?

[allenss-amazon]

In the proposed implementation, once an index is created, it always reflects the current state of the keys that the index covers. This means that the order of key insertion vs index creation is arbitrary, i.e., you can do them in any order.

[yairgott]

I agree that the API is not intuitive and has room for improvement. However, we've opted for RediSearch compatibility to facilitate easier adoption. In the long run, though, we should explore designing a more user-friendly API.

[madolson]

Should there be a corresponding JSON variant?

[madolson]

Still unclear, what are exceptions? How do these fails and what are end users supposed to be doing about these. Are these just syntax errors?

[madolson]

Still not sure what these subscriptions are, they are only mentioned here.

[madolson]

Followup, is there a reason to disable the coordinator for cluster mode? Should it just automatically get enabled for cluster mode?

[madolson]

Is this on a separate port? Does this need to be configurable so that end users can slot it into their system?

[zuiderkwast]

This communication needs to be explained in some more depth. How do the nodes exchange the grpc-port with each other, over the cluster bus?

Is there already some module API for modules to use the cluster bus to exchange this kind of information or do we have to add it?

[zuiderkwast]

I've read the RFC and it's interesting. I don't know much about vector similarity search, so the introduction and motivation was very useful.

I didn't review the individual commands. I assume those are similar to the redis and other APIs and I don't know enough about it, but I put some comments about the specification, especially the communication with other nodes and data persistence, etc. I think it needs some more details.

I'm also missing some discussion about what happens in case of failover, corner cases like failover during a distributed search, outdated data, etc. whatever can happen. Are there race conditions regarding if keys are modified at the same time as a search is started? What about consistency in a distributed search, are there any guarantees?

Is a global cross-slot search possible or is everything per cluster slot or per shard? Implicitly, it seems to be global search, but I think this should be mentioned because with most of the existing commands, like SUNION, cross-slot operations is not allowed, while in other commands like SCAN, keys from multiple slots can be returned in the same call.

Regarding the gRPC communication, can you explain the RCP calls you're using, if not in detail then at least in some pseudo-code style?

[zuiderkwast]

This should be markdown metadata (frontmatter) and the RFC number should be the PR number.

Suggested change

	## RFC: 8
	## Status: Proposed
	---
	RFC: 15
	Status: Proposed
	---

[zuiderkwast]

This section is about how the data is represented and stored. I miss information about how all of this is stored in RDB files, AOF files, replicated to replicas and stored in slot migration.

What are "keys and distances"?

• Are they temporary data just only produce one response and then discarded?
• Are they as actual keys in the database? If not, it should be clarified, because when the word "keys" is used, it can be confusing what it is.
• Are these keys and distances stored in RDB, AOF and replicated?
• Are they sharded or local to one node?

[zuiderkwast]

This communication needs to be explained in some more depth. How do the nodes exchange the grpc-port with each other, over the cluster bus?

Is there already some module API for modules to use the cluster bus to exchange this kind of information or do we have to add it?

[zuiderkwast]

Does this mean that indexes don't need to be replicated? Please elaborate and mention something about these cases:

• Are indexes replicated and stored in RDB and AOF files?
• Do the indexes not need to be replicated, because the replica builds its own index from the keys it receives from the primary?
• The same questions about slot migration: Do the indexes need to be moved with slot migration or does the importing node just build its own index with the keys it receives from the migrating node?

[allenss-amazon]

Commands that manipulate the index metadata (FT.CREATE, FT.DROP, ...) are replicated like any other mutation command. The contents of the index are not replicated, each node is expected to maintain it's own indexes derived from it's own local copy of the keys. Save/Restore of the per-field indexes is optional as they can be rebuilt at load time. The current implementation elects to save/restore vector indexes because the cost of rebuilding those is so high.

Indexes are not built at the per-slot level, but rather at the per-shard level. Thus slot migration cannot move indexes. Rather slot migration is simply treated as per-key index update operation, i.e., delete from one shard and insert into another shard -- just like standard slot migration.

Because query operations are broadcast to all shards, individual shards will perform their portion of the query at different points in time. This creates the situation where any key that's in the process of being migrated while the shards are being interrogated could be present in 0, 1 or 2 shard results (depending on the timing). The duplication case is easily handled during the merging of the per-shard results. The 0 occurrence case presents as a degradation of recall, not a correctness issue.

[zuiderkwast]

I'm missing some details about ACL.

• ACL per command automatically extends to module commands, so we get these automatically, right?
• Does the module create a new ACL category for the VSS commands? (There's module API for that, IIRC.)
• ACL rules per key pattern, do these extend to searches? How/why not?

[allenss-amazon]

None of the search commands actually have keys on the command, so the existing built-in ACL support for modules doesn't apply. Separately AWS has implemented module extensions for ACLs for search. This will be contributed, but under a separate RFC.

[zuiderkwast]

ACL is not only about keys. You can allow and disallow commands:

   • +<command>: Add the command to the list of commands the user can call.  Can be used with | for allowing subcommands (e.g “+config|get”).

   • -<command>:  Remove the command to the list of commands the user can call.  Starting Valkey 7.0, it can be used with | for blocking sub‐
     commands (e.g “-config|set”).

[yairgott]

Right, module-added commands are already supported by ACL. We will flesh out the details of how ACL will work in a separate RFC.

[madolson]

Can you flesh it out here? We want a single RFC for VSS, this is our main point of alignment and it's called out in the template as a section we would like people to address.

[allenss-amazon]

I think this is covered in the next comment. An extension to the existing module interface is required, should that be proposed here or in a separate RFC for the core?

[zuiderkwast]

Is this about the ACL rules for key patterns?

What's the idea here? Should users be able to search only for keys they have access to read?

Technically, how could this work? Search all keys first and then filter before returning the results to the client?

[allenss-amazon]

The security model that AWS implemented directly supports the prefix-based definition of indexes. This implementation operates at the start of a command and checks if the current user has read access to any of the keys that MIGHT be in the index, i.e., current user must have read access to 100% of the keyspace covered by the PREFIX clause of the creation of the index. Once that check passes, no additional security check would be performed.

[yairgott]

hmm, I think that we should discuss this. I do have some reservations about this approach because ACLs might be changed after an index is created and ft.search should still honor the ACLs.

[allenss-amazon]

Perhaps my explanation was poorly worded. The ACL checks operate on every command. My comment about "no additional security check would be performed" meant to say that there's no key-level access checking. An individual user either has 100% or 0% access to an index which is determined right at the start of the command.

[hwware]

Can we close RFC #8?

[hwware]

Can we tolerance this inconsistent?

[yairgott]

I believe the term "consistent" might be misleading in this context. It's important to clarify that there is no consistency issue. Rather, the situation involves search results being omitted or excluded from the search response. This occurs only when a query includes unindexed fields in the specified returned fields, and some search result entries are modified during the background thread's execution, causing them to no longer match the query filter.

[allenss-amazon]

In CMD mode, the application could use LUA or multi/exec to avoid this issue-- with the concurrent reduction in throughput.

For CME, the question is more interesting as there is no attempt to synchronize the per-shard queries. Thus while it may be possible to avoid this issue in the originating shard (i.e., do something like the LUA multi/exec described above), the application will still have to deal with the fact that the returned results might be out-of-date for all of the non-originating shards.

[hwware]

Do we have a chance to support GRAPH_PQ ( combination of the HNSW algorithm and the PQ algorithm), IVF_GRAPH ( combination of IVF and HNSW) and IVF_GRAPH_PQ (combination of the PQ algorithm with the IVF or HNSW). These algorithms could work for huge amount records (from hundreds of millions to more than 1 billion files in shards)

[allenss-amazon]

We've taken the stance of putting into the RFC the functionality that's being contributed -- which doesn't include these particular algorithms. But with that said, there's no reason that this query architecture can't easily be extended to support these algorithms in the future.

[hwware]

Do we have a chance to support hamming: Hamming distance ?

[allenss-amazon]

Currently, there's only support for the 32-bit floating point datatype. Hamming distance has no meaning for this datatype. As described above, when binary-based vectors get supported in some future implementation I would assume that Hamming distance would be supported for those algorithms.

[hwware]

Do we have a chance to implement the Boolean query, ScriptScore Query and Re-Score Query (If the GRAPH_PQ or IVF_GRAPH_PQ index is used)

[allenss-amazon]

Same answer as above.

[hwware]

Thanks for your address.

[hwware]

I just add comments for this RFC, I am not sure if we could implment more index algorithms and query methods, but the link Using Vector Indexes for Data Search in an Elasticsearch Cluster give me more hint https://support.huaweicloud.com/intl/en-us/usermanual-css/css_01_0123.html

[madolson]

Core team discussion: High level consensus on the design of just copying the APIs. In the future we will consider adding our own APIs with our own semantics.

1. Should be able to run on the existing LangChain.
   1. We should be compatible with Redis' langchain code, but long term we should support our own.
2. ACLs?
   1. We should figure out what we should do before version 1.0, add it to the open discussions.
   2. Align with scan behavior (require * permission)
   3. Prefixes
   4. Do the check on each result.
3. Release the source code on January 15th, (Yair, can I post this?)
4. CRC hash is questionable? Why not xxHash or some modern hashing algorithm? Maybe we don't really care.
5. Module APIs needed in the core. (These need to be done for 8.1)
   1. API for reducing the overhead for clusterbus messages.
   2. Need an API so that modules can share memory. Is Memory duplication a requirement for V1?
   3. Need an API for blocking a client from a keyspace notification.