@redis/core-team please approve, and state if you think this should be mentioned in the release notes as a breaking change.

Change makes sense to me and should be noted as breaking despite not being documented. Should it be documented now (i.e. commands that can't be run from multi) somewhere (e.g. https://redis.io/topics/transactions)?

I believe we should ban config set appendonly too.

And some other commands I discussed in #5666.

About bgsave bgrewriteaof and config set appendonly yes, how about that we just set server.rdb_bgsave_scheduled or server.aof_rewrite_scheduled to 1 in transaction, then we can trigger it in serverCron(), just like what BLPOP does.

I thought it's a valid way out of the problem, i was about to suggest it for config set appendonly (since i didn't want to add a new config flag), but now that i think of it, and remember some discussion with Salvatore, i think it's wrong.

The contract with the user is that when append only is enabled (during runtime in this case), as soon as the config command replies with OK, the data is guaranteed to be persisted. this is why redis will refuse to shutdown in the initial AOFRW is in progress.
So this is not a normal rewrite, in which we can use the schedule flag, it's a special state, and i don't like to complicate things for this problem and add another state where there is a scheduled initial rewrite.

instead, i think we can just modify updateAppendonly in config.c to return 0 (error) if we're in a transaction.

Regarding BGSAVE and BGAOFREWRITE, it may be slightly misleading for the user.
if someone was bold enough to do

multi
set x y
bgsave
set x z
exec

then i think their intention might have been to actually break the transaction and start the fork (snapshot) half way.
or maybe for some reason they meant that he wants to guarantee it will fork before the next client (not at serverCron or beforeSleep).
So my point is, that i think i'd rather just say it's an invalid use and block it.
but maybe i'm not aware of how many users may be relying on this.

So to sum up, i think we can change this PR and add the new flag to SYNC, PSYNC, SLAVEOF, REPLICAOF, SHUTDOWN.
and modify updateAppendonly in config.c to return 0 (an error) when we're in a transaction.

I just wanna try our best to make it compatible with user's old behavior, because I'm not sure if some scripts are doing like that, but I'm OK if we all think a breaking change is better.

i just found this test, which is apparently working, so although i think it's an invalid case, it's currently solve, so we can keep it this way.

    test {MULTI / EXEC with REPLICAOF} {
        # This test verifies that if we demote a master to replica inside a transaction, the
        # entire transaction is not propagated to the already-connected replica
        set repl [attach_to_replication_stream]
        r set foo bar
        r multi
        r set foo2 bar
        r replicaof localhost 9999
        r set foo3 bar
        r exec
        catch {r set foo4 bar} e
        assert_match {READONLY*} $e
        assert_replication_stream $repl {
            {select *}
            {set foo bar}
        }
        r replicaof no one
    } {OK} {needs:repl cluster:skip}

i just found this test, which is apparently working, so although i think it's an invalid case, it's currently solve, so we can keep it this way.

    test {MULTI / EXEC with REPLICAOF} {
        # This test verifies that if we demote a master to replica inside a transaction, the
        # entire transaction is not propagated to the already-connected replica
        set repl [attach_to_replication_stream]
        r set foo bar
        r multi
        r set foo2 bar
        r replicaof localhost 9999
        r set foo3 bar
        r exec
        catch {r set foo4 bar} e
        assert_match {READONLY*} $e
        assert_replication_stream $repl {
            {select *}
            {set foo bar}
        }
        r replicaof no one
    } {OK} {needs:repl cluster:skip}

It was introduced in #9890 , and I think it's an invalid case we should remove it, WDYT @guybe7 ?

ohh, i have a short memory.. seems like i asked for that test.
but still there was an attempt to handle it in the past (inside multi.c, so that can be an argument to keep it).
i'm leaning towards blocking it now, but i wanna check if there are not too many other cases in the test suite that rely on it.

Let's block it

The contract with the user is that when append only is enabled (during runtime in this case), as soon as the config command replies with OK, the data is guaranteed to be persisted. this is why redis will refuse to shutdown in the initial AOFRW is in progress.
So this is not a normal rewrite, in which we can use the schedule flag, it's a special state, and i don't like to complicate things for this problem and add another state where there is a scheduled initial rewrite.

i realize i was wrong.
setting the appendonly config already has a case where it responds with ok, and just set the server.aof_rewrite_scheduled flag

@oranagra @soloestoy so we'll just make sure to add "transaction" to the list of cases in which CONFIG SET avoids starting a fork.

sound good?

we discussed this in a core-team meeting.
our conclusions were:

• save, sync, psync, shutdown - just reject in a transaction
• bgsave, bgrewrite - delay the fork (set the scheduled flag)
• replicaof, slaveof - keep them working in a transaction (we see some possible use cases for sending anther command just before or after demoting / promoting)
• config set appendonly - delay the fork (set the scheduled flag)

@redis/core-team i made the changes we discussed, please take a quick look at the top comment and approve.