SCAN/RANDOMKEY and lazy-expire by guybe7 · Pull Request #11788 · redis/redis

guybe7 · 2023-02-07T19:50:12Z

Starting from Redis 7.0 (#9890) we started wrapping everything a command propagates with MULTI/EXEC.
The problem is that both SCAN and RANDOMKEY can lazy-expire arbitrary keys (similar behavior to active-expire), and put DELs in a transaction.

Fix: When these commands are called without a parent exec-unit (e.g. not in EVAL or MULTI) we avoid wrapping their DELs in a transaction (for the same reasons active-expire and eviction avoids a transaction)

This PR adds a per-command flag that indicates that the command may touch arbitrary keys (not the ones in the arguments), and uses that flag to avoid the MULTI-EXEC.
For now, this flag is internal, since we're considering other solutions for the future.

Note for cluster mode: if SCAN/RANDOMKEY is inside EVAL/MULTI it can still cause the same situation (as it always did), but it won't cause a CROSSSLOT because replicas and AOF do not perform slot checks.
The problem with the above is mainly for 3rd party ecosystem tools that propagate commands from master to master, or feed an AOF file with redis-cli into a master.
This PR aims to fix the regression in redis 7.0, and we opened #11792 to try to handle the bigger problem with lazy expire better for another release.

Starting from Redis 7.0 we wrap everything a command propagates with MULTI/EXEC the problem is that both SCAN and RANDOMKEY can lazy-expire random keys (similar behavior to active-expire) 1. When these commands are called without a parent exec-unit we avoid a transaction (for the same reasons active-expire avoids a transaction) 2. When these commands are called with a parent exec-unit we can't avoid a transaction. In order to prevent CROSSSLOT in cluster mode, we just avoid lazy-expiry in thi scenario.

src/db.c

soloestoy · 2023-02-08T03:50:52Z

It is introduced in #9890. I don't think this PR is a right way, as I know lots of users are using SCAN to delete the expired keys.

What we should do is decouple the expire delete propagation and the command propagation, just as what eviction does. A way to fix it is call propagateNow directly instead of propagateDeletion in deleteExpiredKeyAndPropagate to avoid populate the also propagate array.

oranagra · 2023-02-08T07:04:17Z

as I know lots of users are using SCAN to delete the expired keys

the change here doesn't prevent SCAN from reclaiming expired keys, it just avoids the transaction (similar outcome to what eviction does).
i don't think SCAN can call propagateNow, it could be running inside something else, like a script.
so the approach Guy took is to avoid deleting keys only when it runs inside an existing transaction, and avoid creating one when it doesn't.

i updated the top comment, please have a look and fix.

i think that the outcome we're aiming for here is ok, but i'm not certain about the implementation.
maybe instead of adding a command flag and handling this inside expireIfNeeded, we better just add some explicit flag when it is called in these two commands (i.e. the commands will be aware of this concern rather than the infrastructure)

src/db.c

src/commands/randomkey.json

tests/cluster/tests/16-transactions-on-replica.tcl

src/server.h

guybe7 · 2023-02-08T16:18:34Z

comment regarding my last commit (f71801c):
since both replicas and AOF do not check slots we figured that there's no need to avoid lazy-expiry (the original motivation was the fear of a CROSSSLOT on replica)

guybe7 · 2023-02-08T16:50:07Z

@oranagra @soloestoy there's also the matter of SORT - it can also touch arbitrary keys (BY option)

oranagra · 2023-02-09T06:17:09Z

src/module.c

        else if (!strcasecmp(t,"no-cluster")) flags |= CMD_MODULE_NO_CLUSTER;
        else if (!strcasecmp(t,"no-mandatory-keys")) flags |= CMD_NO_MANDATORY_KEYS;
        else if (!strcasecmp(t,"allow-busy")) flags |= CMD_ALLOW_BUSY;
+        else if (!strcasecmp(t,"access-arbitrary-keys")) flags |= CMD_TOUCHES_ARBITRARY_KEYS;


this needs to get documented.
also, this means the PR became a "major decision" (due to interface change)

oranagra · 2023-02-09T06:19:44Z

not sure we can afford mark SORT with the ARBITRARY_KEYS flag (it does access two declared keys).
although for propagation purposes, it writes into just one key, so propagating it without MULTI is ok.
considering that we don't (yet?) reflect this flag in COMMAND output, i guess there's no actual harm in flagging it.
@soloestoy WDYT?

soloestoy · 2023-02-09T06:24:50Z

comment regarding my last commit (f71801c): since both replicas and AOF do not check slots we figured that there's no need to avoid lazy-expiry (the original motivation was the fear of a CROSSSLOT on replica)

I don't think so, we should guarantee all propagations are valid, I can give some use cases:

users can use redis-cli --pipe < appendonly.aof to load data from AOF to a running cluster node, the transaction CROSSLOT leads to error.
some data transmission tools like redis-shake are using replication stream, when transfer data from one node to another, they are both master role, and the transaction CROSSLOT in replication stream leads to error.

soloestoy · 2023-02-09T06:43:47Z

TBH, I don't like introduce the ARBITRARY_KEYS flag, the key problem I think is we mixed the expire-del with the commands when do replication, except the SCAN problem, the normal command has the problem too which I mentioned in #9890 but not fixed:

if we touch a expired key in a write command, like set key value, the replication data is:

MULTI
DEL key
SET key value
EXEC

but the right replication data should be DEL key SET key value without multi/exec, the expire-del propagation doesn't have matters with the SET command, it's the lazy expire's work.

the lazy expire deletion should be stripped from command also from transaction IMO, we need find a way to finish it.

soloestoy · 2023-02-09T06:51:00Z

src/server.c

+     * to avoid using a transaction (much like active-expire) */
+    if (server.current_client &&
+        server.current_client->cmd &&
+        server.current_client->cmd->flags & CMD_TOUCHES_ARBITRARY_KEYS)


seems if call SCAN inside MULTI/EXEC, we can still get a CROSSSLOT transaction?

oranagra · 2023-02-09T07:01:06Z

we also have a tool that replicates data from the replication stream into masters, that was the trigger for this PR.
but we realized that in some way the problem always existed, e.g. for SCAN calls that come from scripts or transaction (these would still be wrapped in MULTI-EXEC), so we decided not to mess with that and leave it as it was (revert the change we made there, the one causing SCAN inside scripts not to lazy expire).

I'm not certain that the case of lazy expire inside a SET command is right to be propagated without a transaction. there may be complex cases where touching multiple keys and the DELs are interleaved with other propagations, and taking them out means sending thing out of the order in which they happened. i fear there might be a case where this can break something.
arguably, if the time is properly frozen for the duration of the command, then key is either expired or not expired for all the actions it takes, and it would not be able to notice this reorder, but maybe there are some edge cases (specifically for modules).
we can explore this direction, but i fear it may get complicated (more than the current proposed trick)

soloestoy · 2023-02-09T08:53:51Z

we also have a tool that replicates data from the replication stream into masters, that was the trigger for this PR.
but we realized that in some way the problem always existed, e.g. for SCAN calls that come from scripts or transaction (these would still be wrapped in MULTI-EXEC), so we decided not to mess with that and leave it as it was (revert the change we made there, the one causing SCAN inside scripts not to lazy expire).

yes, you are right, but #9890 expanded impact, I mean a single SCAN outside scripts or transaction is a very common case, I'm afraid this may cause many new problems to other project. As you said SCAN calls from scripts or transaction can lead to the problem too, seems this usage is not a normal case. But I still wanna fix it completely, especially this PR points out the problem now.

I'm not certain that the case of lazy expire inside a SET command is right to be propagated without a transaction. there may be complex cases where touching multiple keys and the DELs are interleaved with other propagations, and taking them out means sending thing out of the order in which they happened.

I understand your worry, the order is clear, should propagate DELs before the read commands, but it's not easy to guarantee, since we have too many special cases... but from another hand, I think this is a good chance to figure it out and sort out the mess all cases about propagation, what do you think?

oranagra · 2023-02-09T09:12:32Z

maybe we can take it in steps then... as we realized that the change in redis 7.0 for scan can cause issues (not for redis, but for the ecosystem), maybe we should merge this fix as is now and even backport that to the next release of 7.0.
and then discuss further cleanup (that involves more risky changes) for 7.2 or 8.0.
WDYT?

soloestoy · 2023-02-09T12:25:10Z

maybe we can take it in steps then... as we realized that the change in redis 7.0 for scan can cause issues (not for redis, but for the ecosystem), maybe we should merge this fix as is now and even backport that to the next release of 7.0. and then discuss further cleanup (that involves more risky changes) for 7.2 or 8.0. WDYT?

makes sense, I opened an issue #11792 to track the problem

oranagra · 2023-02-09T12:53:08Z

@redis/core-team please approve.
The change is summed up in the top comment, major decision is specifically about the new module flag.

soloestoy · 2023-02-09T13:18:49Z

uh... sorry I forgot module flag, if we agree this is just a temporary method, I think we don't need expose the flag to module

oranagra · 2023-02-09T16:00:16Z

In theory, the flag can be kept, and even exposed to users even if we solve the lazy expiry issue. I.e. that flag really describes the command behavior.
But I agree, let's hide it for now so we keep future flexibility.

madolson · 2023-02-13T16:46:11Z

Top comment seems OK to me, but I noticed the major decision was removed so not going to spend more time looking at it.

Starting from Redis 7.0 (#9890) we started wrapping everything a command propagates with MULTI/EXEC. The problem is that both SCAN and RANDOMKEY can lazy-expire arbitrary keys (similar behavior to active-expire), and put DELs in a transaction. Fix: When these commands are called without a parent exec-unit (e.g. not in EVAL or MULTI) we avoid wrapping their DELs in a transaction (for the same reasons active-expire and eviction avoids a transaction) This PR adds a per-command flag that indicates that the command may touch arbitrary keys (not the ones in the arguments), and uses that flag to avoid the MULTI-EXEC. For now, this flag is internal, since we're considering other solutions for the future. Note for cluster mode: if SCAN/RANDOMKEY is inside EVAL/MULTI it can still cause the same situation (as it always did), but it won't cause a CROSSSLOT because replicas and AOF do not perform slot checks. The problem with the above is mainly for 3rd party ecosystem tools that propagate commands from master to master, or feed an AOF file with redis-cli into a master. This PR aims to fix the regression in redis 7.0, and we opened #11792 to try to handle the bigger problem with lazy expire better for another release. (cherry picked from commit fd82bcc)

Starting from Redis 7.0 (redis#9890) we started wrapping everything a command propagates with MULTI/EXEC. The problem is that both SCAN and RANDOMKEY can lazy-expire arbitrary keys (similar behavior to active-expire), and put DELs in a transaction. Fix: When these commands are called without a parent exec-unit (e.g. not in EVAL or MULTI) we avoid wrapping their DELs in a transaction (for the same reasons active-expire and eviction avoids a transaction) This PR adds a per-command flag that indicates that the command may touch arbitrary keys (not the ones in the arguments), and uses that flag to avoid the MULTI-EXEC. For now, this flag is internal, since we're considering other solutions for the future. Note for cluster mode: if SCAN/RANDOMKEY is inside EVAL/MULTI it can still cause the same situation (as it always did), but it won't cause a CROSSSLOT because replicas and AOF do not perform slot checks. The problem with the above is mainly for 3rd party ecosystem tools that propagate commands from master to master, or feed an AOF file with redis-cli into a master. This PR aims to fix the regression in redis 7.0, and we opened redis#11792 to try to handle the bigger problem with lazy expire better for another release.

guybe7 commented Feb 7, 2023

View reviewed changes

src/db.c Outdated Show resolved Hide resolved

oranagra reviewed Feb 8, 2023

View reviewed changes

src/db.c Outdated Show resolved Hide resolved

drop changes when in cluster_mode

f71801c

oranagra approved these changes Feb 8, 2023

View reviewed changes

src/commands/randomkey.json Outdated Show resolved Hide resolved

tests/cluster/tests/16-transactions-on-replica.tcl Outdated Show resolved Hide resolved

src/server.h Outdated Show resolved Hide resolved

cr

d906e2a

oranagra approved these changes Feb 9, 2023

View reviewed changes

oranagra added the state:major-decision Requires core team consensus label Feb 9, 2023

soloestoy reviewed Feb 9, 2023

View reviewed changes

soloestoy mentioned this pull request Feb 9, 2023

Sort out mess around propagation and lazy-expiration #11792

Open

soloestoy approved these changes Feb 9, 2023

View reviewed changes

oranagra added release-notes indication that this issue needs to be mentioned in the release notes approval-needed Waiting for core team approval to be merged labels Feb 9, 2023

dropping module api flag

515218a

oranagra removed the state:major-decision Requires core team consensus label Feb 12, 2023

oranagra merged commit fd82bcc into redis:unstable Feb 14, 2023

oranagra mentioned this pull request Feb 27, 2023

Release 7.0.9 #11848

Merged

Conversation

guybe7 commented Feb 7, 2023 • edited by oranagra Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

soloestoy commented Feb 8, 2023

Uh oh!

oranagra commented Feb 8, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

guybe7 commented Feb 8, 2023

Uh oh!

guybe7 commented Feb 8, 2023

Uh oh!

oranagra Feb 9, 2023

Choose a reason for hiding this comment

Uh oh!

oranagra commented Feb 9, 2023

Uh oh!

soloestoy commented Feb 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

soloestoy commented Feb 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

soloestoy Feb 9, 2023

Choose a reason for hiding this comment

Uh oh!

oranagra commented Feb 9, 2023

Uh oh!

soloestoy commented Feb 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oranagra commented Feb 9, 2023

Uh oh!

soloestoy commented Feb 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oranagra commented Feb 9, 2023

Uh oh!

soloestoy commented Feb 9, 2023

Uh oh!

oranagra commented Feb 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

madolson commented Feb 13, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

guybe7 commented Feb 7, 2023 •

edited by oranagra

Loading

soloestoy commented Feb 9, 2023 •

edited

Loading

soloestoy commented Feb 9, 2023 •

edited

Loading

soloestoy commented Feb 9, 2023 •

edited

Loading

soloestoy commented Feb 9, 2023 •

edited

Loading

oranagra commented Feb 9, 2023 •

edited

Loading