slipenkomk/valkey
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Nested MULTI or WATCH in MULTI now will abort the transaction (#723)

Browse Source 
Currently, for nested MULTI or executing WATCH in MULTI, we will return
an error but we will not abort the transaction.

```
127.0.0.1:6379> multi
OK
127.0.0.1:6379(TX)> multi
(error) ERR MULTI calls can not be nested
127.0.0.1:6379(TX)> set key value
QUEUED
127.0.0.1:6379(TX)> exec
1) OK

127.0.0.1:6379> multi
OK
127.0.0.1:6379(TX)> watch key
(error) ERR WATCH inside MULTI is not allowed
127.0.0.1:6379(TX)> set key value
QUEUED
127.0.0.1:6379(TX)> exec
1) OK
```

This is an unexpected behavior that should abort the transaction.
The number of elements returned by EXEC also doesn't match the number
of commands in MULTI.
Add the NO_MULTI flag to them so that they will
be rejected in processCommand and rejectCommand will abort the
transaction.

So there are two visible changes:

- Different words in the error messages. (Command not allowed inside a
transaction)
- Exec returns error.

Signed-off-by: Binbin <binloveplay1314@qq.com>
This commit is contained in:
Binbin 2024-07-04 03:27:45 +08:00 committed by GitHub
parent 2d6791bb11
commit 6bf1d02edf
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
5 changed files with 10 additions and 21 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
src/commands.def
View File

@ -11022,8 +11022,8 @@ struct COMMAND_STRUCT serverCommandTable[] = {
/* transactions */
{MAKE_CMD("discard","Discards a transaction.","O(N), when N is the number of queued commands","2.0.0",CMD_DOC_NONE,NULL,NULL,"transactions",COMMAND_GROUP_TRANSACTIONS,DISCARD_History,0,DISCARD_Tips,0,discardCommand,1,CMD_NOSCRIPT|CMD_LOADING|CMD_STALE|CMD_FAST|CMD_ALLOW_BUSY,ACL_CATEGORY_TRANSACTION,DISCARD_Keyspecs,0,NULL,0)},
{MAKE_CMD("exec","Executes all commands in a transaction.","Depends on commands in the transaction","1.2.0",CMD_DOC_NONE,NULL,NULL,"transactions",COMMAND_GROUP_TRANSACTIONS,EXEC_History,0,EXEC_Tips,0,execCommand,1,CMD_NOSCRIPT|CMD_LOADING|CMD_STALE|CMD_SKIP_SLOWLOG,ACL_CATEGORY_TRANSACTION,EXEC_Keyspecs,0,NULL,0)},
{MAKE_CMD("multi","Starts a transaction.","O(1)","1.2.0",CMD_DOC_NONE,NULL,NULL,"transactions",COMMAND_GROUP_TRANSACTIONS,MULTI_History,0,MULTI_Tips,0,multiCommand,1,CMD_NOSCRIPT|CMD_LOADING|CMD_STALE|CMD_FAST|CMD_ALLOW_BUSY,ACL_CATEGORY_TRANSACTION,MULTI_Keyspecs,0,NULL,0)},
{MAKE_CMD("multi","Starts a transaction.","O(1)","1.2.0",CMD_DOC_NONE,NULL,NULL,"transactions",COMMAND_GROUP_TRANSACTIONS,MULTI_History,0,MULTI_Tips,0,multiCommand,1,CMD_NOSCRIPT|CMD_LOADING|CMD_STALE|CMD_FAST|CMD_NO_MULTI|CMD_ALLOW_BUSY,ACL_CATEGORY_TRANSACTION,MULTI_Keyspecs,0,NULL,0)},
{MAKE_CMD("unwatch","Forgets about watched keys of a transaction.","O(1)","2.2.0",CMD_DOC_NONE,NULL,NULL,"transactions",COMMAND_GROUP_TRANSACTIONS,UNWATCH_History,0,UNWATCH_Tips,0,unwatchCommand,1,CMD_NOSCRIPT|CMD_LOADING|CMD_STALE|CMD_FAST|CMD_ALLOW_BUSY,ACL_CATEGORY_TRANSACTION,UNWATCH_Keyspecs,0,NULL,0)},
{MAKE_CMD("watch","Monitors changes to keys to determine the execution of a transaction.","O(1) for every key.","2.2.0",CMD_DOC_NONE,NULL,NULL,"transactions",COMMAND_GROUP_TRANSACTIONS,WATCH_History,0,WATCH_Tips,0,watchCommand,-2,CMD_NOSCRIPT|CMD_LOADING|CMD_STALE|CMD_FAST|CMD_ALLOW_BUSY,ACL_CATEGORY_TRANSACTION,WATCH_Keyspecs,1,NULL,1),.args=WATCH_Args},
{MAKE_CMD("watch","Monitors changes to keys to determine the execution of a transaction.","O(1) for every key.","2.2.0",CMD_DOC_NONE,NULL,NULL,"transactions",COMMAND_GROUP_TRANSACTIONS,WATCH_History,0,WATCH_Tips,0,watchCommand,-2,CMD_NOSCRIPT|CMD_LOADING|CMD_STALE|CMD_FAST|CMD_NO_MULTI|CMD_ALLOW_BUSY,ACL_CATEGORY_TRANSACTION,WATCH_Keyspecs,1,NULL,1),.args=WATCH_Args},
{0}
};

1
src/commands/multi.json
View File

@ -11,6 +11,7 @@
"LOADING",
"STALE",
"FAST",
"NO_MULTI",
"ALLOW_BUSY"
],
"acl_categories": [

1
src/commands/watch.json
View File

@ -11,6 +11,7 @@
"LOADING",
"STALE",
"FAST",
"NO_MULTI",
"ALLOW_BUSY"
],
"acl_categories": [

9
src/multi.c
View File

@ -109,12 +109,7 @@ void flagTransaction(client *c) {
}
void multiCommand(client *c) {
if (c->flag.multi) {
addReplyError(c, "MULTI calls can not be nested");
return;
}
c->flag.multi = 1;
addReply(c, shared.ok);
}
@ -459,10 +454,6 @@ void touchAllWatchedKeysInDb(serverDb *emptied, serverDb *replaced_with) {
void watchCommand(client *c) {
int j;
if (c->flag.multi) {
addReplyError(c, "WATCH inside MULTI is not allowed");
return;
}
/* No point in watching if the client is already dirty. */
if (c->flag.dirty_cas) {
addReply(c, shared.ok);

16
tests/unit/multi.tcl
View File

@ -34,12 +34,10 @@ start_server {tags {"multi"}} {
} {QUEUED OK {a b c}}
test {Nested MULTI are not allowed} {
set err {}
r multi
catch {[r multi]} err
r exec
set _ $err
} {*ERR MULTI*}
assert_error "ERR*" {r multi}
assert_error "EXECABORT*" {r exec}
}
test {MULTI where commands alter argc/argv} {
r sadd myset a
@ -49,12 +47,10 @@ start_server {tags {"multi"}} {
} {a 0}
test {WATCH inside MULTI is not allowed} {
set err {}
r multi
catch {[r watch x]} err
r exec
set _ $err
} {*ERR WATCH*}
assert_error "ERR*" {r watch}
assert_error "EXECABORT*" {r exec}
}
test {EXEC fails if there are errors while queueing commands #1} {
r del foo1{t} foo2{t}