dragonflydb / dragonfly

A modern replacement for Redis and Memcached
https://www.dragonflydb.io/
Other
25.36k stars 916 forks source link

FT.SEARCH Not Supported in Lua Scripts of DragonflyDB (Incompatible with Redis) #3704

Open CodeToFreedom opened 1 week ago

CodeToFreedom commented 1 week ago

Unfortunately FT.SEARCH (from within lua scripts) works in redis but not in dragonflydb. Is there a setting in df that I need to enable manually first for it to work (f.e. some atomicity setting) or is it a bug?

Steps to reproduce:

-Create test data index: FT.CREATE idx:testindex ON JSON PREFIX 1 testindex: SCHEMA $.pk AS pk TAG $.color AS color TAG

-Insert test data (optional) JSON.SET testindex:abc1 . "{\"pk\":\"abc1\",\"color\":\"blue\"}"

-Run search query outside lua script (works in redis + dragonflydb) FT.SEARCH idx:testindex "(@color:{blue})"

-Run search query from within lua script (works in redis, but fails in dragonfly): EVAL "return redis.call('FT.SEARCH', 'idx:testindex', '((@color:{blue}))', 'LIMIT', '0', '1')" 0

Error message:

Error: "This Redis command is not allowed from script" Here is the full error message: "ERR Error running script (call to b051a0cdb6086107db75e8f9313672a0e21c6d82): @user_script:2: This Redis command is not allowed from script"

Side note: For hierarchical data we rely heavily on this command to work from within lua scripts in order to traverse data directly on the server (avoiding network roundtrips). Therefore it is crucial for us to work before we can migrate to dragonflydb and really appreciate your assistance.

Many thanks in advance for your efforts.

romange commented 1 week ago

it's not a bug per seh, but it is very hard to implement in Dragonfly. I am not optimistic we will have bandwidth to do it in 2024.

CodeToFreedom commented 1 week ago

Hi @romange and @BagritsevichStepan, thanks for your quick reply. May I ask why exactly this specific command is disallowed or considered hard to implement in df?

In my (probably naive) understanding it seems the lua script engine could "just" forward the command to the already implemented dragonflydb command "FT.SEARCH" to make it work.

What would happen if we just allowed this command by removing the CO::NOSCRIPT tag from it? What would break?

I found the following comment in the source code:

The key functions are RedisCallCommand, RedisPCallCommand, RedisACallCommand, and RedisAPCallCommand. 
These functions are defined in the interpreter.cc file and are responsible 
for handling the Lua calls to Redis commands.

Why is FT.SEARCH not forwarded to the df engine from lua?

In interpreter.cc :

int Interpreter::RedisCallCommand(lua_State* lua) {
  void** ptr = static_cast<void**>(lua_getextraspace(lua));
  return reinterpret_cast<Interpreter*>(*ptr)->RedisGenericCommand(true, false);
}

So why is this command disallowed? Would it be accepted when atomicity is disabled or what exactly is the concern your team had to allow it? It seems like a normal command to me like GET or other commands returning data that are allowed.

if (under_script && (cid->opt_mask() & CO::NOSCRIPT))
    return ErrorReply{"This Redis command is not allowed from script"};

Many thanks again for your efforts and this amazing project.

CodeToFreedom commented 1 week ago

PS: After looking more through the code I found that setting disable-atomicity flag makes it work. I don't really understand why yet, but at least that circumvents the problem for now. Thanks for your efforts.

dranikpg commented 1 week ago

PS: After looking more through the code I found that setting disable-atomicity flag makes it work. I don't really understand why yet, but at least that circumvents the problem for now. Thanks for your efforts.

IIRC we messaged on discord and I've sent you an example script with disable-atomicity. It is because search indices are stored on all shards, which requires the script to be scheduled on all shards as well. If you want your script to preserve atomicity, you can enable allow-undeclared-keys

romange commented 1 week ago

@dranikpg but FT.SEARCH does not declare keys, it's a transaction without keys. Do you still think it will work with allow-undeclared-keys ?

CodeToFreedom commented 5 days ago

@dranikpg Sorry for the late reply. (had a lot of mails in my inbox the last days and didn't see the notification.)

Concerning your question: Yes, we wrote on discord and you made very interesting points, but concerning this bug the command you mentioned there was different. You were using redis.call('JSON.GET' in the example script.

The error message "This Redis command is not allowed from script" only raised when I used FT.SEARCH though.

@dranikpg @romange Is there a reason why dragonflydb requires such strict default behaviour? In my opinion FT.SEARCh is "just" a read-only command where it is quite little risk to allow a less strict flag/ default behaviour. Of course the returned keys could get overwritten or changed in the meantime but that it could be argumented that this is also the case for every GET command as well.

How does redis mitigated the risk of having changed FT.SEARCH results? Me personally I'd just accept the risk as it can't be totally avoided anyway. Even if we stopped the whole index from being altered during executing the search command (which would be bad of course), in the meantime of processing the result from FT.SEARCH in the client script, the data could be changed as well.

Many thanks