@madolson @yossigo i added a #8820 (comment) in the original PR, and also an "UPDATE" note on the top comment linking to that comment and this PR.
do we wanna take any additional measures of documentation or release notes to discourage users from using the _RO variants?

Adding some documentation is likely a good idea. Maybe just documenting that that EVAL_RO is useful in specific circumstances, and EVAL + no-write flag is just as good in most situations.

yes, i think we need to mention something like:

1. this command mainly exists for the purpose of clients (have different flags than EVAL)
2. preferably scripts should use the shebang way to tell redis how they're gonna be used
3. might get deprecated one day

put some of that in the top comment for the purpose of the release notes, and some in redis-io.
@madolson can you try to draft something?

@madolson I edited the top comment, removed all the doc text about read-only scripts and left just the part about the purpose of the read-only commands. feel free to re-edit.

The logic basically says that if a script executes ANY read-only command, then track the keys DECLARED by the lua script as well as the command, even if they weren't explicitly accessed. I honestly would expect it to just to track the keys accessed by commands

@madolson can you point me to the code that backs that?
from looking at the code you modified in call(), it looks like that would have been the behavior if you didn't adjust the code, and that the one before this PR, and also after it, is that we only track the ones being accessed (not the declared ones)

another side effect is now these commands will let is_read_command in processCommand to be 1, it doesn't have any harm, is_read_command flag is only used here:

    /* Prevent a replica from sending commands that access the keyspace.
     * The main objective here is to prevent abuse of client pause check
     * from which replicas are exempt. */
    if ((c->flags & CLIENT_SLAVE) && (is_may_replicate_command || is_write_command || is_read_command)) {
        rejectCommandFormat(c, "Replica can't interact with the keyspace");
        return C_OK;
    }

the change is in #8868, but I think we should only allow slave (not monitor) client execute REPLCONF, and we can just remove the is_read_command check.

another side effect is now these commands will let is_read_command in processCommand to be 1

yeah, for clients the READONLY flag may mean they can send it to replicas, and for redis that flags has a completely different meaning. i.e. a command without READONLY or WRITE is one that doesn't touch the keyspace, and a command with either one of them is one that does touch the keyspace.
i.e. EVAL is marked as one that doesn't touch the keyspace, and EVAL_RO is one that does.
this is very odd, but i'm willing to live with it (regret we added that command, and wish we'll remove it one day).

@madolson can you point me to the code that backs that?
from looking at the code you modified in call(), it looks like that would have been the behavior if you didn't adjust the code, and that the one before this PR, and also after it, is that we only track the ones being accessed (not the declared ones)

@oranagra It's just right there. server.script_caller is the the client with the EVAL* loaded onto it, so when calling trackingRememberKeys() it will track the declared keys of the EVAL command. The current command being executed, something like a GET by the script_client, is just completely ignored in this context. You can check the test I wrote for EVAL to confirm the behavior. This behavior kind of makes sense because tracking needs to happen in the context of the end client.

Maybe my point didn't come across exactly right. EVAL behaves the way I described, it only tracks "declared" keys. I believe that EVAL_RO should behave the "same" as EVAL when executing the same set of commands.

A question I asked in the top was if we wanted to maintain the current behavior. The more "logical" behavior is that we should track keys based off of the commands being executed by the script client. I'm inclined to do this, and can be done either here or in a follow up CR. Make sense?

ok i see now...
but your text at the top says:

track the keys DECLARED by the lua script as well as the command

is that something i'm still missing or a mistake in the comment?

anyway, could it be that this behavior is an innocent mistake? i.e. the tracking mechanism needs the real client since that's the client that should be reported to, so maybe the fact it takes the wrong command was unintended? do you remember any discussion around that?
p.s for scripts that execute multiple redis commands, it also attempts to track the same set of keys again and again, that's surely unintended, right?

either way, i suggest we merge this PR as is (sets the flags to EVAL_RO and keeps the caching behavior as it was), and consider a change in the caching logic in a followup PR.

track the keys DECLARED by the lua script as well as the command

Oh, yeah, that isn't correct. I think that's how I thought it worked when I started working on the PR, and later learned it was even weirder than I thought. I updated the top comment to also clarify that the behavior actually didn't change.

anyway, could it be that this behavior is an innocent mistake? i.e. the tracking mechanism needs the real client since that's the client that should be reported to, so maybe the fact it takes the wrong command was unintended? do you remember any discussion around that?

I think it's an honest mistake or perhaps just an optimization. I don't remember any conversation around this.

either way, i suggest we merge this PR as is (sets the flags to EVAL_RO and keeps the caching behavior as it was), and consider a change in the caching logic in a followup PR.

Agree, updated the top comment and will merge now.