1. MIGRATE has a potnetial key arg in argv[3]. It should be reflected in the command table.
2. getKeysUsingCommandTable should never free getKeysResult, it is always freed by the caller)
The reason we never encountered this double-free bug is that almost always getKeysResult
uses the statis buffer and doesn't allocate a new one.
(cherry picked from commit 6aa2285e32a6bc16fe2938bfb40d833db7d3752d)
1. MIGRATE has a potnetial key arg in argv[3]. It should be reflected in the command table.
2. getKeysUsingCommandTable should never free getKeysResult, it is always freed by the caller)
The reason we never encountered this double-free bug is that almost always getKeysResult
uses the statis buffer and doesn't allocate a new one.
(cherry picked from commit 6aa2285e32a6bc16fe2938bfb40d833db7d3752d)
sometimes you can be very deep in the call stack, without access to argv.
once you're there you may want your reply/log to contain the command name.
(cherry picked from commit e16d3eb998f7017c95ef17179de77aa5b6f2e272)
sometimes you can be very deep in the call stack, without access to argv.
once you're there you may want your reply/log to contain the command name.
(cherry picked from commit e16d3eb998f7017c95ef17179de77aa5b6f2e272)
Before this commit using RM_Call without "!" could cause the master
to lazy-expire a key (delete it) but without replicating to replicas.
This could cause the replica's memory usage to gradually grow and
could also cause consistency issues if the master and replica have
a clock diff.
This bug was introduced in #8617
Added a test which demonstrates that scenario.
Before this commit using RM_Call without "!" could cause the master
to lazy-expire a key (delete it) but without replicating to replicas.
This could cause the replica's memory usage to gradually grow and
could also cause consistency issues if the master and replica have
a clock diff.
This bug was introduced in #8617
Added a test which demonstrates that scenario.
Starting redis 6.0 (part of the TLS feature), diskless master uses pipe from the fork
child so that the parent is the one sending data to the replicas.
This mechanism has an issue in which a hung replica will cause the master to wait
for it to read the data sent to it forever, thus preventing the fork child from terminating
and preventing the creations of any other forks.
This PR adds a timeout mechanism, much like the ACK-based timeout,
we disconnect replicas that aren't reading the RDB file fast enough.
Starting redis 6.0 (part of the TLS feature), diskless master uses pipe from the fork
child so that the parent is the one sending data to the replicas.
This mechanism has an issue in which a hung replica will cause the master to wait
for it to read the data sent to it forever, thus preventing the fork child from terminating
and preventing the creations of any other forks.
This PR adds a timeout mechanism, much like the ACK-based timeout,
we disconnect replicas that aren't reading the RDB file fast enough.
If GT/LT fails the operation we need to reply with
nill (like failure due to NX).
Other changes:
Add the missing $encoding suffix to many zset tests
Note: there's a behavior change just in case of INCR + GT/LT that fails.
The old code was replying with the wrong (rejected) score, and now it'll reply with nil.
Note that that's anyway a corner case so this "behavior change" shouldn't have too much affect.
Using GT/LT with INCR has a predictable result even before we run the command
(INCR GT will only only / always fail if the increment is negative).
If GT/LT fails the operation we need to reply with
nill (like failure due to NX).
Other changes:
Add the missing $encoding suffix to many zset tests
Note: there's a behavior change just in case of INCR + GT/LT that fails.
The old code was replying with the wrong (rejected) score, and now it'll reply with nil.
Note that that's anyway a corner case so this "behavior change" shouldn't have too much affect.
Using GT/LT with INCR has a predictable result even before we run the command
(INCR GT will only only / always fail if the increment is negative).
1. moduleReplicateMultiIfNeeded should use server.in_eval like
moduleHandlePropagationAfterCommandCallback
2. server.in_eval could have been set to 1 and not reset back
to 0 (a lot of missed early-exits after in_eval is already 1)
Note: The new assertions in processCommand cover (2) and I added
two module tests to cover (1)
Implications:
If an EVAL that failed (and thus left server.in_eval=1) runs before a module
command that replicates, the replication stream will contain MULTI (because
moduleReplicateMultiIfNeeded used to check server.lua_caller which is NULL
at this point) but not EXEC (because server.in_eval==1)
This only affects modules as module.c the only user of server.in_eval.
Affects versions 6.2.0, 6.2.1
1. moduleReplicateMultiIfNeeded should use server.in_eval like
moduleHandlePropagationAfterCommandCallback
2. server.in_eval could have been set to 1 and not reset back
to 0 (a lot of missed early-exits after in_eval is already 1)
Note: The new assertions in processCommand cover (2) and I added
two module tests to cover (1)
Implications:
If an EVAL that failed (and thus left server.in_eval=1) runs before a module
command that replicates, the replication stream will contain MULTI (because
moduleReplicateMultiIfNeeded used to check server.lua_caller which is NULL
at this point) but not EXEC (because server.in_eval==1)
This only affects modules as module.c the only user of server.in_eval.
Affects versions 6.2.0, 6.2.1
Bug 1:
When a module ctx is freed moduleHandlePropagationAfterCommandCallback
is called and handles propagation. We want to prevent it from propagating
commands that were not replicated by the same context. Example:
1. module1.foo does: RM_Replicate(cmd1); RM_Call(cmd2); RM_Replicate(cmd3)
2. RM_Replicate(cmd1) propagates MULTI and adds cmd1 to also_propagagte
3. RM_Call(cmd2) create a new ctx, calls call() and destroys the ctx.
4. moduleHandlePropagationAfterCommandCallback is called, calling
alsoPropagates EXEC (Note: EXEC is still not written to socket),
setting server.in_trnsaction = 0
5. RM_Replicate(cmd3) is called, propagagting yet another MULTI (now
we have nested MULTI calls, which is no good) and then cmd3
We must prevent RM_Call(cmd2) from resetting server.in_transaction.
REDISMODULE_CTX_MULTI_EMITTED was revived for that purpose.
Bug 2:
Fix issues with nested RM_Call where some have '!' and some don't.
Example:
1. module1.foo does RM_Call of module2.bar without replication (i.e. no '!')
2. module2.bar internally calls RM_Call of INCR with '!'
3. at the end of module1.foo we call RM_ReplicateVerbatim
We want the replica/AOF to see only module1.foo and not the INCR from module2.bar
Introduced a global replication_allowed flag inside RM_Call to determine
whether we need to replicate or not (even if '!' was specified)
Other changes:
Split beforePropagateMultiOrExec to beforePropagateMulti afterPropagateExec
just for better readability
Bug 1:
When a module ctx is freed moduleHandlePropagationAfterCommandCallback
is called and handles propagation. We want to prevent it from propagating
commands that were not replicated by the same context. Example:
1. module1.foo does: RM_Replicate(cmd1); RM_Call(cmd2); RM_Replicate(cmd3)
2. RM_Replicate(cmd1) propagates MULTI and adds cmd1 to also_propagagte
3. RM_Call(cmd2) create a new ctx, calls call() and destroys the ctx.
4. moduleHandlePropagationAfterCommandCallback is called, calling
alsoPropagates EXEC (Note: EXEC is still not written to socket),
setting server.in_trnsaction = 0
5. RM_Replicate(cmd3) is called, propagagting yet another MULTI (now
we have nested MULTI calls, which is no good) and then cmd3
We must prevent RM_Call(cmd2) from resetting server.in_transaction.
REDISMODULE_CTX_MULTI_EMITTED was revived for that purpose.
Bug 2:
Fix issues with nested RM_Call where some have '!' and some don't.
Example:
1. module1.foo does RM_Call of module2.bar without replication (i.e. no '!')
2. module2.bar internally calls RM_Call of INCR with '!'
3. at the end of module1.foo we call RM_ReplicateVerbatim
We want the replica/AOF to see only module1.foo and not the INCR from module2.bar
Introduced a global replication_allowed flag inside RM_Call to determine
whether we need to replicate or not (even if '!' was specified)
Other changes:
Split beforePropagateMultiOrExec to beforePropagateMulti afterPropagateExec
just for better readability
Have a clear separation between in and out flags
Other changes:
delete dead code in RM_ZsetIncrby: if zsetAdd returned error (happens only if
the result of the operation is NAN or if score is NAN) we return immediately so
there is no way that zsetAdd succeeded and returned NAN in the out-flags
Have a clear separation between in and out flags
Other changes:
delete dead code in RM_ZsetIncrby: if zsetAdd returned error (happens only if
the result of the operation is NAN or if score is NAN) we return immediately so
there is no way that zsetAdd succeeded and returned NAN in the out-flags
Scenario:
1. A module client is blocked on keys with a timeout
2. Shortly before the timeout expires, the key is being populated and signaled
as ready
3. Redis calls moduleTryServeClientBlockedOnKey (which replies to client) and
then moduleUnblockClient
4. moduleUnblockClient doesn't really unblock the client, it writes to
server.module_blocked_pipe and only marks the BC as unblocked.
5. beforeSleep kics in, by this time the client still exists and techincally
timed-out. beforeSleep replies to the timeout client (double reply) and
only then moduleHandleBlockedClients is called, reading from module_blocked_pipe
and calling unblockClient
The solution is similar to what was done in moduleTryServeClientBlockedOnKey: we
should avoid re-processing an already-unblocked client
Scenario:
1. A module client is blocked on keys with a timeout
2. Shortly before the timeout expires, the key is being populated and signaled
as ready
3. Redis calls moduleTryServeClientBlockedOnKey (which replies to client) and
then moduleUnblockClient
4. moduleUnblockClient doesn't really unblock the client, it writes to
server.module_blocked_pipe and only marks the BC as unblocked.
5. beforeSleep kics in, by this time the client still exists and techincally
timed-out. beforeSleep replies to the timeout client (double reply) and
only then moduleHandleBlockedClients is called, reading from module_blocked_pipe
and calling unblockClient
The solution is similar to what was done in moduleTryServeClientBlockedOnKey: we
should avoid re-processing an already-unblocked client
the test was misleading because the module would actually woke up on a wrong type and
re-blocked, while the test name suggests the module doesn't not wake up at all on a wrong type..
i changed the name of the test + added verification that indeed the module wakes up and gets
re-blocked after it understand it's the wrong type
the test was misleading because the module would actually woke up on a wrong type and
re-blocked, while the test name suggests the module doesn't not wake up at all on a wrong type..
i changed the name of the test + added verification that indeed the module wakes up and gets
re-blocked after it understand it's the wrong type
The output for COMMAND command was wrong for some commands.
clients can use firstkey,lastkey,step to find (some) key name arguments, and the
"movablekeys" flag to know that they can't know all (or any) of the key name arguments.
These commands had the wrong output:
1. GEORADIUS*_RO used to have "movablekeys" (which it doesn't really need)
2. XREAD and XREADGROUP used to have (1,1,1). but that's completely wrong.
3. Z*STORE used to have (0,0,0) but it can at lest give the index of the dstkey (1,1,1)
The output for COMMAND command was wrong for some commands.
clients can use firstkey,lastkey,step to find (some) key name arguments, and the
"movablekeys" flag to know that they can't know all (or any) of the key name arguments.
These commands had the wrong output:
1. GEORADIUS*_RO used to have "movablekeys" (which it doesn't really need)
2. XREAD and XREADGROUP used to have (1,1,1). but that's completely wrong.
3. Z*STORE used to have (0,0,0) but it can at lest give the index of the dstkey (1,1,1)
The bug occurs when 'callback' re-registers itself to a point
in the future and the execution time in non-negligible:
'now' refers to time BEFORE callback was executed and is used
to calculate 'next_period'.
We must get the actual current time when calculating 'next_period'
(cherry picked from commit 9cbdc8dcdb)
The bug occurs when 'callback' re-registers itself to a point
in the future and the execution time in non-negligible:
'now' refers to time BEFORE callback was executed and is used
to calculate 'next_period'.
We must get the actual current time when calculating 'next_period'
(cherry picked from commit 1a91a2700b)
The bug was introduced by #5021 which only attempted avoid EXIST on an
already expired key from returning 1 on a replica.
Before that commit, dbExists was used instead of
lookupKeyRead (which had an undesired effect to "touch" the LRU/LFU)
Other than that, this commit fixes OBJECT to also come empty handed on
expired keys in replica.
And DEBUG DIGEST-VALUE to behave like DEBUG OBJECT (get the data from
the key regardless of it's expired state)
(cherry picked from commit 168dcb549c)
The bug was introduced by #5021 which only attempted avoid EXIST on an
already expired key from returning 1 on a replica.
Before that commit, dbExists was used instead of
lookupKeyRead (which had an undesired effect to "touch" the LRU/LFU)
Other than that, this commit fixes OBJECT to also come empty handed on
expired keys in replica.
And DEBUG DIGEST-VALUE to behave like DEBUG OBJECT (get the data from
the key regardless of it's expired state)
(cherry picked from commit f8ae991717)
This PR adds another trimming strategy to XADD and XTRIM named MINID
(complements the existing MAXLEN).
It also adds a new LIMIT argument that allows incremental trimming by repeated
calls (rather than all at once).
This provides the ability to trim all records older than a certain ID (which makes it
possible for the user to trim by age too).
Example:
XTRIM mystream MINID ~ 1608540753 will trim entries with id < 1608540753,
but might not trim all (because of the ~ modifier)
The purpose is to ease the use of streams. many users use streams as logs and
the common case is wanting a log
of the last X seconds rather than a log that contains maximum X entries (new
MINID vs existing MAXLEN)
The new LIMIT modifier is only supported when the trim strategy uses ~.
i.e. when the user asked for exact trimming, it all happens in one go (no
possibility for incremental trimming).
However, when ~ is provided, we trim full rax nodes, up to the limit number
of records.
The default limit is 100*stream_node_max_entries (used when LIMIT is not
provided).
I.e. this is a behavior change (even if the existing MAXLEN strategy is used).
An explicit limit of 0 means unlimited (but note that it's not the default).
Other changes:
Refactor arg parsing code for XADD and XTRIM to use common code.
This PR adds another trimming strategy to XADD and XTRIM named MINID
(complements the existing MAXLEN).
It also adds a new LIMIT argument that allows incremental trimming by repeated
calls (rather than all at once).
This provides the ability to trim all records older than a certain ID (which makes it
possible for the user to trim by age too).
Example:
XTRIM mystream MINID ~ 1608540753 will trim entries with id < 1608540753,
but might not trim all (because of the ~ modifier)
The purpose is to ease the use of streams. many users use streams as logs and
the common case is wanting a log
of the last X seconds rather than a log that contains maximum X entries (new
MINID vs existing MAXLEN)
The new LIMIT modifier is only supported when the trim strategy uses ~.
i.e. when the user asked for exact trimming, it all happens in one go (no
possibility for incremental trimming).
However, when ~ is provided, we trim full rax nodes, up to the limit number
of records.
The default limit is 100*stream_node_max_entries (used when LIMIT is not
provided).
I.e. this is a behavior change (even if the existing MAXLEN strategy is used).
An explicit limit of 0 means unlimited (but note that it's not the default).
Other changes:
Refactor arg parsing code for XADD and XTRIM to use common code.
New command: XAUTOCLAIM <key> <group> <consumer> <min-idle-time> <start> [COUNT <count>] [JUSTID]
The purpose is to claim entries from a stale consumer without the usual
XPENDING+XCLAIM combo which takes two round trips.
The syntax for XAUTOCLAIM is similar to scan: A cursor is returned (streamID)
by each call and should be used as start for the next call. 0-0 means the scan is complete.
This PR extends the deferred reply mechanism for any bulk string (not just counts)
This PR carries some unrelated test code changes:
- Renames the term "client" into "consumer" in the stream-cgroups test
- And also changes DEBUG SLEEP into "after"
Co-authored-by: Oran Agra <oran@redislabs.com>
New command: XAUTOCLAIM <key> <group> <consumer> <min-idle-time> <start> [COUNT <count>] [JUSTID]
The purpose is to claim entries from a stale consumer without the usual
XPENDING+XCLAIM combo which takes two round trips.
The syntax for XAUTOCLAIM is similar to scan: A cursor is returned (streamID)
by each call and should be used as start for the next call. 0-0 means the scan is complete.
This PR extends the deferred reply mechanism for any bulk string (not just counts)
This PR carries some unrelated test code changes:
- Renames the term "client" into "consumer" in the stream-cgroups test
- And also changes DEBUG SLEEP into "after"
Co-authored-by: Oran Agra <oran@redislabs.com>
when the same consumer re-claim an entry that it already has, there's
no need to remove-and-insert if it's the same rax.
we do need to update the idle time though.
this commit only improves efficiency (doesn't change behavior).
when the same consumer re-claim an entry that it already has, there's
no need to remove-and-insert if it's the same rax.
we do need to update the idle time though.
this commit only improves efficiency (doesn't change behavior).
One way this was happening is when a module issued an RM_Call which would inject MULTI.
If the module command that does that was itself issued by something else that already did
added MULTI (e.g. another module, or a Lua script), it would have caused nested MULTI.
In fact the MULTI state in the client or the MULTI_EMITTED flag in the context isn't
the right indication that we need to propagate MULTI or not, because on a nested calls
(possibly a module action called by a keyspace event of another module action), these
flags aren't retained / reflected.
instead there's now a global propagate_in_transaction flag for that.
in addition to that, we now have a global in_eval and in_exec flags, to serve the flags
of RM_GetContextFlags, since their dependence on the current client is wrong for the same
reasons mentioned above.
One way this was happening is when a module issued an RM_Call which would inject MULTI.
If the module command that does that was itself issued by something else that already did
added MULTI (e.g. another module, or a Lua script), it would have caused nested MULTI.
In fact the MULTI state in the client or the MULTI_EMITTED flag in the context isn't
the right indication that we need to propagate MULTI or not, because on a nested calls
(possibly a module action called by a keyspace event of another module action), these
flags aren't retained / reflected.
instead there's now a global propagate_in_transaction flag for that.
in addition to that, we now have a global in_eval and in_exec flags, to serve the flags
of RM_GetContextFlags, since their dependence on the current client is wrong for the same
reasons mentioned above.
The bug was introduced by #5021 which only attempted avoid EXIST on an
already expired key from returning 1 on a replica.
Before that commit, dbExists was used instead of
lookupKeyRead (which had an undesired effect to "touch" the LRU/LFU)
Other than that, this commit fixes OBJECT to also come empty handed on
expired keys in replica.
And DEBUG DIGEST-VALUE to behave like DEBUG OBJECT (get the data from
the key regardless of it's expired state)
The bug was introduced by #5021 which only attempted avoid EXIST on an
already expired key from returning 1 on a replica.
Before that commit, dbExists was used instead of
lookupKeyRead (which had an undesired effect to "touch" the LRU/LFU)
Other than that, this commit fixes OBJECT to also come empty handed on
expired keys in replica.
And DEBUG DIGEST-VALUE to behave like DEBUG OBJECT (get the data from
the key regardless of it's expired state)
The bug occurs when 'callback' re-registers itself to a point
in the future and the execution time in non-negligible:
'now' refers to time BEFORE callback was executed and is used
to calculate 'next_period'.
We must get the actual current time when calculating 'next_period'
The bug occurs when 'callback' re-registers itself to a point
in the future and the execution time in non-negligible:
'now' refers to time BEFORE callback was executed and is used
to calculate 'next_period'.
We must get the actual current time when calculating 'next_period'
- Clarify some documentation comments
- Make sure blocked-on-keys client privdata is accessible
from withing the timeout callback
- Handle blocked clients in beforeSleep - In case a key
becomes "ready" outside of processCommand
See #7879#7880
(cherry picked from commit c816eec893)
- Clarify some documentation comments
- Make sure blocked-on-keys client privdata is accessible
from withing the timeout callback
- Handle blocked clients in beforeSleep - In case a key
becomes "ready" outside of processCommand
See #7879#7880
(cherry picked from commit addf47dcac)
If 'delta' is negative 'mem_freed' may underflow and cause
the while loop to exit prematurely (And not evicting enough
memory)
mem_freed can be negative when:
1. We use lazy free (consuming memory by appending to a list)
2. Thread doing an allocation between the two calls to zmalloc_used_memory.
If 'delta' is negative 'mem_freed' may underflow and cause
the while loop to exit prematurely (And not evicting enough
memory)
mem_freed can be negative when:
1. We use lazy free (consuming memory by appending to a list)
2. Thread doing an allocation between the two calls to zmalloc_used_memory.
- Clarify some documentation comments
- Make sure blocked-on-keys client privdata is accessible
from withing the timeout callback
- Handle blocked clients in beforeSleep - In case a key
becomes "ready" outside of processCommand
See #7879#7880
- Clarify some documentation comments
- Make sure blocked-on-keys client privdata is accessible
from withing the timeout callback
- Handle blocked clients in beforeSleep - In case a key
becomes "ready" outside of processCommand
See #7879#7880
1. Call emptyDb even in case of diskless-load: We want modules
to get the same FLUSHDB event as disk-based replication.
2. Do not fire any module events when flushing the backups array.
3. Delete redundant call to signalFlushedDb (Called from emptyDb).
Former-commit-id: aa8a3077a2
1. Call emptyDb even in case of diskless-load: We want modules
to get the same FLUSHDB event as disk-based replication.
2. Do not fire any module events when flushing the backups array.
3. Delete redundant call to signalFlushedDb (Called from emptyDb).
Former-commit-id: aa8a3077a2
guybe7
