Sort out mess around propagation and MULTI/EXEC #9890

guybe7 · 2021-12-03T12:52:26Z

The mess:
Some parts use alsoPropagate for late propagation, others using an immediate one (propagate()), causing edge cases, ugly/hacky code, and the tendency for bugs

The basic idea is that all commands are propagated via alsoPropagate (i.e. added to a list) and the top-most call() is responsible for going over that list and actually propagating them (and wrapping them in MULTI/EXEC if there's more than one command). This is done in the new function, propagatePendingCommands.

Callers to propagatePendingCommands:

top-most call() (we want all nested call()s to add to the also_propagate array and just the top-most one to propagate them) - via afterCommand
handleClientsBlockedOnKeys: it is out of call() context and it may propagate stuff - via afterCommand.
handleClientsBlockedOnKeys edge case: if the looked-up key is already expired, we will propagate the expire but will not unblock any client so afterCommand isn't called. in that case, we have to propagate the deletion explicitly.
cron stuff: active-expire and eviction may also propagate stuff
modules: the module API allows to propagate stuff from just about anywhere (timers, keyspace notifications, threads). I could have tried to catch all the out-of-call-context places but it seemed easier to handle it in one place: when we free the context. in the spirit of what was done in call(), only the top-most freeing of a module context may cause propagation.
modules: when using a thread-safe ctx it's not clear when/if the ctx will be freed. we do know that the module must lock the GIL before calling RM_Replicate/RM_Call so we propagate the pending commands when releasing the GIL.

A "known limitation", which were actually a bug, was fixed because of this commit (see propagate.tcl):
When using a mix of RM_Call with ! and RM_Replicate, the command would propagate out-of-order: first all the commands from RM_Call, and then the ones from RM_Replicate

Another thing worth mentioning is that if, in the past, a client would issue a MULTI/EXEC with just one write command the server would blindly propagate the MULTI/EXEC too, even though it's redundant. not anymore.

This commit renames propagate() to propagateNow() in order to cause conflicts in pending PRs.
propagatePendingCommands is the only caller of propagateNow, which is now a static, internal helper function.

Optimizations:

alsoPropagate will not add stuff to also_propagate if there's no AOF and replicas
alsoPropagate reallocs also_propagagte exponentially, to save calls to memmove

Bugfixes:

CONFIG SET can create evictions, sending notifications which can cause to dirty++ with modules.
we need to prevent it from propagating to AOF/replicas
We need to set current_client in RM_Call. buggy scenario:
- CONFIG SET maxmemory, eviction notifications, module hook calls RM_Call
- assertion in lookupKey crashes, because current_client has CONFIG SET, which isn't CMD_WRITE
minor: in eviction, call propagateDeletion after notification, like active-expire and all commands
(we always send a notification before propagating the command)

src/multi.c

zuiderkwast · 2021-12-04T17:25:24Z

"Sort out mess" 👍 😁

oranagra · 2021-12-05T15:41:11Z

@guybe7 thanks..

Few requests while i review:

please start off the top comment to describe what was that mess and what complications it caused. i.e. some parts using "also" late propagation, others using an immediate one, causing edge cases, ugly / tacky code, and tendency for bugs?.
please look into the sanitizer timeout failure in LATENCY of expire events are correctly collected, i don't recall seeing it before.

guybe7 · 2021-12-05T17:45:11Z

@oranagra i can't figure it out, i ran it locally (build with SANITIZER=address and ran the tests) and it passed

oranagra · 2021-12-05T18:41:58Z

I've re triggered the tests and got the same outcome.
Maybe it has something to do with a timing issue issue causing a hung, or maybe related to the gcc version being used?

src/aof.c

src/blocked.c

src/evict.c

src/module.c

tests/unit/moduleapi/propagate.tcl

tests/unit/multi.tcl

src/blocked.c

src/server.h

src/blocked.c

src/module.c

src/server.c

tests/unit/multi.tcl

src/server.c

src/expire.c

oranagra · 2021-12-09T09:27:52Z

@soloestoy i'll be happy if you can have a look and see if you can spot any issues with the new approach.

soloestoy · 2021-12-09T09:48:01Z

@soloestoy i'll be happy if you can have a look and see if you can spot any issues with the new approach.

OK, I'll check in a day or two, too busy this week...

bugfixes: 1. CONFIG SET can create evictions, sending notifications which can cause to dirty++ with modules. we need to prevent it from propagating to AOF/replicas 2. we need to set current_client in RM_Call. buggy scenario: - CONFIG SET maxmemory, notifications, module hook calls RM_Call - assertion in lookupKey crashes, because current_client has CONFIG SET, which isn't CMD_WRITE 3. minor: in eviction, call propagateDeletion after notification, like active-expire and all commands (we always send a notification before propagating the command)

This would mean that the effects of `CONFIG SET maxmemory` may not be visible once the command returns. That could anyway happen since incremental eviction was added in redis 6.2 (see redis#7653) We do this to fix one of the propagation bugs about eviction see redis#9890 and redis#10014.

This PR is for future compatability to Redis 7. Till Redis 7, if commands would have executed from background thread, then Redis would not have wrap them with multi exec and the commands would not have executed atomically on replica. Gears solved it by propagating multi exec by itself. Thanks to this PR: redis/redis#9890 It is no longer needed to replicate multi exec, Redis takes care of this for us.

RM_Yield was missing a call to protectClient to prevent redis from processing future commands of the yielding client. Adding tests that fail without this fix. This would be complicated to solve since nested calls to RM_Call used to replace the current_client variable with the module temp client. It looks like it's no longer necessary to do that, since it was added back in redis#9890 to solve two issues, both already gone: 1. call to CONFIG SET maxmemory could trigger a module hook calling RM_Call. although this specific issue is gone, arguably other hooks like keyspace notification, can do the same. 2. an assertion in lookupKey that checks the current command of the current client, introduced in redis#9572 and removed in redis#10248

…10573) RM_Yield was missing a call to protectClient to prevent redis from processing future commands of the yielding client. Adding tests that fail without this fix. This would be complicated to solve since nested calls to RM_Call used to replace the current_client variable with the module temp client. It looks like it's no longer necessary to do that, since it was added back in #9890 to solve two issues, both already gone: 1. call to CONFIG SET maxmemory could trigger a module hook calling RM_Call. although this specific issue is gone, arguably other hooks like keyspace notification, can do the same. 2. an assertion in lookupKey that checks the current command of the current client, introduced in #9572 and removed in #10248

If was first added in redis#9890 to solve the problem of CONFIG SET maxmemory causing eviction inside MULTI/EXEC, but that problem is already fixed (CONFIG SET doesn't evict directly, it just schedules a later eviction) Keep that condition may hide bugs (i.e. performEvictions should always expect to have an empty server.also_propagate)

If was first added in #9890 to solve the problem of CONFIG SET maxmemory causing eviction inside MULTI/EXEC, but that problem is already fixed (CONFIG SET doesn't evict directly, it just schedules a later eviction) Keep that condition may hide bugs (i.e. performEvictions should always expect to have an empty server.also_propagate)

This case is interesting because it originates from cron, rather than from another command. The idea came from looking at #9890 and #10573, and I was wondering if RM_Call would work properly when `server.current_client == NULL`

Redis 7.0 has redis#9890 which added an assertion when the propagation queue was not flushed and we got to beforeSleep. But it turns out that when processCommands calls getNodeByQuery and decides to reject the command, it can lead to a key that was lazy expired and is deleted without later flushing the propagation queue. This change prevents lazy expiry from deleting the key at this stage (not as part of a command being processed in `call`) Closes redis#11014 getNodeByQuery

Redis 7.0 has redis#9890 which added an assertion when the propagation queue was not flushed and we got to beforeSleep. But it turns out that when processCommands calls getNodeByQuery and decides to reject the command, it can lead to a key that was lazy expired and is deleted without later flushing the propagation queue. This change prevents lazy expiry from deleting the key at this stage (not as part of a command being processed in `call`) Closes redis#11014 Co-authored-by: Madelyn Olson <34459052+madolson@users.noreply.github.com>

…#11176) Redis 7.0 has #9890 which added an assertion when the propagation queue was not flushed and we got to beforeSleep. But it turns out that when processCommands calls getNodeByQuery and decides to reject the command, it can lead to a key that was lazy expired and is deleted without later flushing the propagation queue. This change prevents lazy expiry from deleting the key at this stage (not as part of a command being processed in `call`)

…redis#11176) Redis 7.0 has redis#9890 which added an assertion when the propagation queue was not flushed and we got to beforeSleep. But it turns out that when processCommands calls getNodeByQuery and decides to reject the command, it can lead to a key that was lazy expired and is deleted without later flushing the propagation queue. This change prevents lazy expiry from deleting the key at this stage (not as part of a command being processed in `call`) (cherry picked from commit c789fb0)

…#11176) Redis 7.0 has #9890 which added an assertion when the propagation queue was not flushed and we got to beforeSleep. But it turns out that when processCommands calls getNodeByQuery and decides to reject the command, it can lead to a key that was lazy expired and is deleted without later flushing the propagation queue. This change prevents lazy expiry from deleting the key at this stage (not as part of a command being processed in `call`) (cherry picked from commit c789fb0)

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 (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. (cherry picked from commit fd82bcc)

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)

…redis#11176) Redis 7.0 has redis#9890 which added an assertion when the propagation queue was not flushed and we got to beforeSleep. But it turns out that when processCommands calls getNodeByQuery and decides to reject the command, it can lead to a key that was lazy expired and is deleted without later flushing the propagation queue. This change prevents lazy expiry from deleting the key at this stage (not as part of a command being processed in `call`)

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.

…redis#11176) Redis 7.0 has redis#9890 which added an assertion when the propagation queue was not flushed and we got to beforeSleep. But it turns out that when processCommands calls getNodeByQuery and decides to reject the command, it can lead to a key that was lazy expired and is deleted without later flushing the propagation queue. This change prevents lazy expiry from deleting the key at this stage (not as part of a command being processed in `call`)

…edis#10573) RM_Yield was missing a call to protectClient to prevent redis from processing future commands of the yielding client. Adding tests that fail without this fix. This would be complicated to solve since nested calls to RM_Call used to replace the current_client variable with the module temp client. It looks like it's no longer necessary to do that, since it was added back in redis#9890 to solve two issues, both already gone: 1. call to CONFIG SET maxmemory could trigger a module hook calling RM_Call. although this specific issue is gone, arguably other hooks like keyspace notification, can do the same. 2. an assertion in lookupKey that checks the current command of the current client, introduced in redis#9572 and removed in redis#10248

If was first added in redis#9890 to solve the problem of CONFIG SET maxmemory causing eviction inside MULTI/EXEC, but that problem is already fixed (CONFIG SET doesn't evict directly, it just schedules a later eviction) Keep that condition may hide bugs (i.e. performEvictions should always expect to have an empty server.also_propagate)

This case is interesting because it originates from cron, rather than from another command. The idea came from looking at redis#9890 and redis#10573, and I was wondering if RM_Call would work properly when `server.current_client == NULL`

…redis#11176) Redis 7.0 has redis#9890 which added an assertion when the propagation queue was not flushed and we got to beforeSleep. But it turns out that when processCommands calls getNodeByQuery and decides to reject the command, it can lead to a key that was lazy expired and is deleted without later flushing the propagation queue. This change prevents lazy expiry from deleting the key at this stage (not as part of a command being processed in `call`)

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 requested a review from oranagra December 3, 2021 12:52

guybe7 commented Dec 3, 2021

View reviewed changes

src/multi.c Show resolved Hide resolved

guybe7 force-pushed the prop_fix branch from 26bcf5b to c214849 Compare December 3, 2021 12:58

guybe7 removed the request for review from oranagra December 3, 2021 13:08

guybe7 marked this pull request as draft December 3, 2021 13:08

Sort out mess around propagation and MULTI/EXEC

5c674ed

guybe7 force-pushed the prop_fix branch from c214849 to 5c674ed Compare December 3, 2021 13:33

guybe7 marked this pull request as ready for review December 3, 2021 14:29

guybe7 requested a review from oranagra December 3, 2021 14:30

fixes and tests

511d9ae

oranagra added this to Backlog in 7.0 via automation Dec 5, 2021

oranagra moved this from Backlog to In Review in 7.0 Dec 5, 2021

oranagra added the 7.0-not-after-rc1 label Dec 5, 2021

oranagra reviewed Dec 5, 2021

View reviewed changes

guybe7 force-pushed the prop_fix branch from 0d80e80 to ea1db47 Compare December 8, 2021 12:06

CR fixes

cd5e3e7

guybe7 force-pushed the prop_fix branch 2 times, most recently from 82d8f81 to 07f8a39 Compare December 8, 2021 14:22

fix tests + optimizations

e42866b

guybe7 force-pushed the prop_fix branch from 07f8a39 to e42866b Compare December 8, 2021 14:29

oranagra reviewed Dec 8, 2021

View reviewed changes

oranagra reviewed Dec 9, 2021

View reviewed changes

tests/unit/multi.tcl Show resolved Hide resolved

oranagra reviewed Dec 9, 2021

View reviewed changes

src/server.c Show resolved Hide resolved

src/expire.c Show resolved Hide resolved

oranagra mentioned this pull request Apr 12, 2022

Fix RM_Yield bug processing future commands of the current client. #10573

Merged

guybe7 mentioned this pull request Apr 20, 2022

isSafeToPerformEvictions: Remove redundant condition #10610

Merged

guybe7 mentioned this pull request Apr 25, 2022

Test: RM_Call from within "expired" notification #10613

Merged

oranagra mentioned this pull request Aug 23, 2022

Fix assertion when a key is lazy expired during cluster key migration #11176

Merged

This was referenced Feb 8, 2023

SCAN/RANDOMKEY and lazy-expire #11788

Merged

Sort out mess around propagation and lazy-expiration #11792

Open

sundb mentioned this pull request Apr 5, 2024

[CRASH] worker thread call RedisModule_ReplicateVerbatim(ctx); trigger redis crash #12647

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sort out mess around propagation and MULTI/EXEC #9890

Sort out mess around propagation and MULTI/EXEC #9890

guybe7 commented Dec 3, 2021 •

edited

zuiderkwast commented Dec 4, 2021

oranagra commented Dec 5, 2021

guybe7 commented Dec 5, 2021

oranagra commented Dec 5, 2021

oranagra commented Dec 9, 2021

soloestoy commented Dec 9, 2021

Sort out mess around propagation and MULTI/EXEC #9890

Sort out mess around propagation and MULTI/EXEC #9890

Conversation

guybe7 commented Dec 3, 2021 • edited

zuiderkwast commented Dec 4, 2021

oranagra commented Dec 5, 2021

guybe7 commented Dec 5, 2021

oranagra commented Dec 5, 2021

oranagra commented Dec 9, 2021

soloestoy commented Dec 9, 2021

guybe7 commented Dec 3, 2021 •

edited