Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Using IDatabase.Execute or IDatabase.ExecuteAsync causes to next command run after a 'SELECT 0' #2845

Open
atakavci opened this issue Jan 28, 2025 · 6 comments

Comments

@atakavci
Copy link
Contributor

each time an IDatabase.ExecuteAsync or IDatabase.Execute s called, the next command executes after a SELECT 0 is sent.

        var db = ConnectionMultiplexer.Connect("localhost:6379").GetDatabase();

        db.StringSet("key1", "abc");
        db.StringSet("key2", "abc");

        await db.ExecuteAsync("set", "explicitCall1", "abc");

        db.StringSet("key3", "abc");
        db.StringSet("key4", "abc");

        db.Execute("set", "explicitCall2", "abc");

        db.StringSet("key5", "abc");
        db.StringSet("key6", "abc");

output from MONITOR command;

1738053208.322367 [0 192.168.48.1:55570] "SET" "key1" "abc"
1738053208.323767 [0 192.168.48.1:55570] "SET" "key2" "abc"
1738053208.328989 [0 192.168.48.1:55570] "SET" "explicitCall1" "abc"
1738053208.336072 [0 192.168.48.1:55570] "SELECT" "0"
1738053208.336110 [0 192.168.48.1:55570] "SET" "key3" "abc"
1738053208.336622 [0 192.168.48.1:55570] "SET" "key4" "abc"
1738053208.338578 [0 192.168.48.1:55570] "SET" "explicitCall2" "abc"
1738053208.339415 [0 192.168.48.1:55570] "SELECT" "0"
1738053208.339426 [0 192.168.48.1:55570] "SET" "key5" "abc"
1738053208.339852 [0 192.168.48.1:55570] "SET" "key6" "abc"

only way i can find to avoid ths seems to disable the SELECT command, which is not suitable for all cases.

this is caused due to the type of command Message is UNKNOWN when Execute or ExecuteAsync
here 👇

connection.SetUnknownDatabase();

How about making it optional to set it as unknown every time, or introduce an option command type CUSTOM , i am happy to come up with a PR.

@mgravell
Copy link
Collaborator

This is intentional because if you're using Execute*, you're effectively issuing an unknown command - therefore we don't know what the side-effects of that are; it could change the database context, in which case we would be issuing the next command to the wrong database, which would be bad™. Because correctness is the overriding concern, we re-assert the database (via select) before issuing the next command. In reality, this is a tiny command that is processed in the IO loop of the server, rather than the server core, so it should not represent a performance concern.

I would suggest the most appropriate fix in this case would be: use StringSet instead of ExecuteAsync("set", ...) - if the library understands the intent, it won't trigger this behavior.

@atakavci
Copy link
Contributor Author

@mgravell i agree for SET, it is best to stick with IDatabase.StringSet.
On the other hand, NRedisStack leverages both Execute and ExecuteAsync for module commands,
and these SELECT operations show up as an unnecessary overhead for each command, even within a pipeline.
I am looking for the most convenient way to avoid this overhead.

@mgravell
Copy link
Collaborator

Firstly, I'd challenge whether there is a measured and demonstrable impact from a pipelined select. Secondly, I strongly suspect that even if there is, it is drowned by the API overheads of the general-purpose Execute API. Please see the discussion (especially mine) here - we have a plan to add a better API for ad-hoc/unknown commands (not the one Monty proposes - I mean something more fundamental). I think a vast amount of overhead will be saved by using that (currently vapor) API. As a secondary consideration, we can pose

API consumers (declaring custom commands) should be able to declare 'I don't screw with the database' on that API

on the "if you like, the bad things are on you" basis. However, as Monty notes: there's a huge amount of API overhead at the moment, so adding a CommandFlags for the same is unlikely to be hugely useful or beneficial - it is solving the 2% part of the problem (unmeasured).

@atakavci
Copy link
Contributor Author

atakavci commented Jan 29, 2025

TBH i dont have such benchmarks right in my hand, either for client side or server side.
i see the issue and both of these spotting to the same API with kİnd of concerns around performance.
the issue from my POV is, we have an additional whole command that goes all the way to the server and back, which is completely unnecessary for the most wide use case, and there is no way to properly avoid it.

@mgravell
Copy link
Collaborator

we have an additional whole command that goes all the way to the server and back,

and if we were forcing an additional round-trip for that: I would 100% agree with you; however, a pipelined select prefix that doesn't even force a "flush", and that is processed outside of the server core: is extremely unlikely to cause any relevant impact. However, I'm adding this to the set of things we're trying to resolve in the proposed API work.

@atakavci
Copy link
Contributor Author

However, I'm adding this to the set of things we're trying to resolve in the proposed API work.

👍 thank youu!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants