Here the idea is that we do not want freeMemoryIfNeeded() to propagate a
DEL command before the script and change what happens in the script
execution once it reaches the slave. For example see this potential
issue (in the words of @soloestoy):
On master, we run the following script:
if redis.call('get','key')
then
redis.call('set','xxx','yyy')
end
redis.call('set','c','d')
Then when redis attempts to execute redis.call('set','xxx','yyy'), we call freeMemoryIfNeeded(), and the key may get deleted, and because redis.call('set','xxx','yyy') has already been executed on master, this script will be replicated to slave.
But the slave received "DEL key" before the script, and will ignore maxmemory, so after that master has xxx and c, slave has only one key c.
Note that this patch (and other related work) was authored collaboratively in
issue #5250 with the help of @soloestoy and @oranagra.
Related to issue #5250.
Technically speaking we don't really need to put the master client in
the clients that need to be processed, since in practice the PING
commands from the master will take care, however it is conceptually more
sane to do so.
A) slave buffers didn't count internal fragmentation and sds unused space,
this caused them to induce eviction although we didn't mean for it.
B) slave buffers were consuming about twice the memory of what they actually needed.
- this was mainly due to sdsMakeRoomFor growing to twice as much as needed each time
but networking.c not storing more than 16k (partially fixed recently in 237a38737).
- besides it wasn't able to store half of the new string into one buffer and the
other half into the next (so the above mentioned fix helped mainly for small items).
- lastly, the sds buffers had up to 30% internal fragmentation that was wasted,
consumed but not used.
C) inefficient performance due to starting from a small string and reallocing many times.
what i changed:
- creating dedicated buffers for reply list, counting their size with zmalloc_size
- when creating a new reply node from, preallocate it to at least 16k.
- when appending a new reply to the buffer, first fill all the unused space of the
previous node before starting a new one.
other changes:
- expose mem_not_counted_for_evict info field for the benefit of the test suite
- add a test to make sure slave buffers are counted correctly and that they don't cause eviction
Implementation notes: as INFO is "already broken", I didn't want to break it further. Instead of computing the server.lua_script dict size on every call, I'm keeping a running sum of the body's length and dict overheads.
This implementation is naive as it **does not** take into consideration dict rehashing, but that inaccuracy pays off in speed ;)
Demo time:
```bash
$ redis-cli info memory | grep "script"
used_memory_scripts:96
used_memory_scripts_human:96B
number_of_cached_scripts:0
$ redis-cli eval "" 0 ; redis-cli info memory | grep "script"
(nil)
used_memory_scripts:120
used_memory_scripts_human:120B
number_of_cached_scripts:1
$ redis-cli script flush ; redis-cli info memory | grep "script"
OK
used_memory_scripts:96
used_memory_scripts_human:96B
number_of_cached_scripts:0
$ redis-cli eval "return('Hello, Script Cache :)')" 0 ; redis-cli info memory | grep "script"
"Hello, Script Cache :)"
used_memory_scripts:152
used_memory_scripts_human:152B
number_of_cached_scripts:1
$ redis-cli eval "return redis.sha1hex(\"return('Hello, Script Cache :)')\")" 0 ; redis-cli info memory | grep "script"
"1be72729d43da5114929c1260a749073732dc822"
used_memory_scripts:232
used_memory_scripts_human:232B
number_of_cached_scripts:2
✔ 19:03:54 redis [lua_scripts-in-info-memory L ✚…⚑] $ redis-cli evalsha 1be72729d43da5114929c1260a749073732dc822 0
"Hello, Script Cache :)"
```
The main change introduced by this commit is pretending that help
arrays are more text than code, thus indenting them at level 0. This
improves readability, and is an old practice when defining arrays of
C strings describing text.
Additionally a few useless return statements are removed, and the HELP
subcommand capitalized when printed to the user.
The function in its initial form, and after the fixes for the PSYNC2
bugs, required code duplication in multiple spots. This commit modifies
it in order to always compute the script name independently, and to
return the SDS of the SHA of the body: this way it can be used in all
the places, including for SCRIPT LOAD, without duplicating the code to
create the Lua function name. Note that this requires to re-compute the
body SHA1 in the case of EVAL seeing a script for the first time, but
this should not change scripting performance in any way because new
scripts definition is a rare event happening the first time a script is
seen, and the SHA1 computation is anyway not a very slow process against
the typical Redis script and compared to the actua Lua byte compiling of
the body.
Note that the function used to assert() if a duplicated script was
loaded, however actually now two times over three, we want the function
to handle duplicated scripts just fine: this happens in SCRIPT LOAD and
in RDB AUX "lua" loading. Moreover the assert was not defending against
some obvious failure mode, so now the function always tests against
already defined functions at start.
Unfortunately, as outlined by @soloestoy in #4505, "lua" AUX RDB field
loading in case of duplicated script was still broken. This commit fixes
this problem and also a memory leak introduced by the past commit.
Note that now we have a regression test able to duplicate the issue, so
this commit was actually tested against the regression. The original PR
also had a valid fix, but I prefer to hide the details of scripting.c
outside scripting.c, and later "SCRIPT LOAD" should also be able to use
the function luaCreateFunction() instead of redoing the work.
In the case of slaves loading the RDB from master, or in other similar
cases, the script is already defined, and the function registering the
script should not fail in the assert() call.
Related to #4483. As suggested by @soloestoy, we can retrieve the SHA1
from the body. Given that in the new implementation using AUX fields we
ended copying around a lot to create new objects and strings, extremize
such concept and trade CPU for space inside the RDB file.
This adds a new `addReplyHelp` helper that's used by commands
when returning a help text. The following commands have been
touched: DEBUG, OBJECT, COMMAND, PUBSUB, SCRIPT and SLOWLOG.
WIP
Fix entry command table entry for OBJECT for HELP option.
After #4472 the command may have just 2 arguments.
Improve OBJECT HELP descriptions.
See #4472.
WIP 2
WIP 3
For example:
1. A module command called within a MULTI section.
2. A Lua script with replicate_commands() called within a MULTI section.
3. A module command called from a Lua script in the above context.
This fix, provided by Paul Kulchenko (@pkulchenko), allows the Lua
scripting engine to evaluate statements with a trailing comment like the
following one:
EVAL "print() --comment" 0
Lua can't parse the above if the string does not end with a newline, so
now a final newline is always added automatically. This does not change
the SHA1 of scripts since the SHA1 is computed on the body we pass to
EVAL, without the other code we add to register the function.
Close#2951.
Example of offending code:
> script debug yes
OK
> eval "local a = {1} a[1] = a\nprint(a)" 0
1) * Stopped at 1, stop reason = step over
2) -> 1 local a = {1} a[1] = a
> next
1) * Stopped at 2, stop reason = step over
2) -> 2 print(a)
> print
... server crash ...
Close#2955.
Now it lists code around the current position by default. Can list any
other part using other arguments, but a new "whole" command was added in
order to show the whole source code easily.
When the debugger exits now it produces an <endsession> tag that informs
redis-cli (or other debugging clients) that the session terminated.
This way the client knows there is yet another reply to read (the one of
the EVAL script itself), and can switch to non-debugging mode ASAP.
It's handly to just eval "5+5" without the return and see it printed on
the screen as result. However prepending "return" does not always result
into valid Lua code. So what we do is to exploit a common Lua community
trick of trying to compile with return prepended, and if compilation
fails then it's not an expression that can be returned, so we try again
without prepending "return". Works great apparently.
Arguments arity and arguments type error of redis.call() were not
reported correctly to Lua, so the command acted in this regard like
redis.pcall(), but just for two commands. Redis.call() should always
raise errors instead.
Currently this feature is only accessible via DEBUG for testing, since
otherwise depending on the instance configuration a given script works
or is broken, which is against the Redis philosophy.
By calling redis.replicate_commands(), the scripting engine of Redis
switches to commands replication instead of replicating whole scripts.
This is useful when the script execution is costly but only results in a
few writes performed to the dataset.
Morover, in this mode, it is possible to call functions with side
effects freely, since the script execution does not need to be
deterministic: anyway we'll capture the outcome from the point of view
of changes to the dataset.
In this mode math.random() returns different sequences at every call.
If redis.replicate_commnads() is not called before any other write, the
command returns false and sticks to whole scripts replication instead.
Rationale is that when re-entering, it is likely due to Lua debugging
hooks. Returning an error will be ignored in most cases, going totally
unnoticed. With the log at least we leave a trace.
Related to issue #2302.
