Allow setting locale in lua script. #11041

yourtree · 2022-07-26T14:30:29Z

It is noted that Lua basically uses strcoll() as comparison function, which leads to a problem that, in different regions, for some characters, the comparison result may be different. Below is an example.

This will cause accidental code compatibility issues for Lua scripts. And here provides a solution.
This commit allows the use of setlocale() function of Lua os library. To avoid hidden systematic risk, the parameter category is set to collate and is unchangeable. The packaged function accepts only one parameter and the other functions of os library are unreachable. Allow setting locale offers an opportunity to uniform standards. Below shows how it works.

127.0.0.1:6379> EVAL "return os.setlocale('C')" 0
"C"
127.0.0.1:6379> EVAL "return os.clock()" 0
(error) ERR user_script:1: attempt to call field 'clock' (a nil value) script: ea58cfad299460ea863f576834104a675e48c28c, on @user_script:1.
127.0.0.1:6379> EVAL "return os.setlocale('C', 'numeric')" 0
(error) ERR wrong number of arguments script: 4393e830ac85509505f24894333f199e9a5e3eb6, on @user_script:1.
127.0.0.1:6379> EVAL "return os.setlocale('C')" 0
"C"
127.0.0.1:6379> EVAL "return '*' < ','" 0
(integer) 1
127.0.0.1:6379> EVAL "return '*' > ','" 0
(nil)
127.0.0.1:6379> EVAL "return os.setlocale('fr_FR')" 0
"fr_FR"
127.0.0.1:6379> EVAL "return '*' > ','" 0
(integer) 1
127.0.0.1:6379> EVAL "return '*' < ','" 0
(nil)

sundb · 2022-07-26T15:30:30Z

src/script_lua.c

@@ -50,6 +50,7 @@ static char *libraries_allow_list[] = {
    "math",
    "table",
    "struct",
+    "os",


~~import os library would be dangerous, e.x os.execute, os.rename~~
Pardon my misunderstanding (on small screen phone), but maybe we shouldn't call it os.

Yup, it could be replaced with "ros" or something else. The initial intention to name it 'os' is to conform to the concept of daily use.

oranagra · 2022-07-26T17:01:34Z

src/roslib.c

+        return lua_error(L);
+    }
+    const char *l = luaL_optstring(L, 1, NULL);
+    lua_pushstring(L, setlocale(LC_COLLATE, l));


I don't think we wanna change the system (global) locale in a script, it'll have implications outside that script.
maybe instead we should expose a CONFIG directive to control it?
i.e. currently redis relies on environment variable for that.
see #1074 and #799

CONFIG is indeed better cuz the environment is shared. I'll make changes in later commits.

I think controlling locale through environment is more standard, having a configuration parameter that overrides that might be very counter intuitive in some cases. If a Lua script can change the locale programmatically, and that change only affects that script - I think that's much better.

i'm not sure it's at all possible to have it only affect Lua.
indeed it's standard to control the locale via env var, but that's actually not very convenient.
i think that (despite being non-standard), it's much more useful to set such things via config file and config command).
same as we handle other global server configs like rdbcompression and ziplist threshold.
although granted, these do not affect behavior, just optimizations.

Since it seems necessary to uniform the output format of SORT command globally (eg. the same business deployed in different regions), it may be better to control it from the upper level, i.e. CONFIG command.

The title of this pr is easily misunderstood and I've created a new pr #11059

oranagra · 2022-08-03T07:24:50Z

closing in favor of #11059 (to be controlled by a config, since we can't make sure it only affects Lua)

Till now Redis officially supported tuning it via environment variable see #1074. But we had other requests to allow changing it at runtime, see #799, and #11041. Note that `strcoll()` is used as Lua comparison function and also for comparison of certain string objects in Redis, which leads to a problem that, in different regions, for some characters, the result may be different. Below is an example. ``` 127.0.0.1:6333> SORT test alpha 1) "<" 2) ">" 3) "," 4) "*" 127.0.0.1:6333> CONFIG GET locale-collate 1) "locale-collate" 2) "" 127.0.0.1:6333> CONFIG SET locale-collate 1 (error) ERR CONFIG SET failed (possibly related to argument 'locale') 127.0.0.1:6333> CONFIG SET locale-collate C OK 127.0.0.1:6333> SORT test alpha 1) "*" 2) "," 3) "<" 4) ">" ``` That will cause accidental code compatibility issues for Lua scripts and some Redis commands. This commit creates a new config parameter to control the local environment which only affects `Collate` category. Above shows how it affects `SORT` command, and below shows the influence on Lua scripts. ``` 127.0.0.1:6333> CONFIG GET locale-collate 1) " locale-collate" 2) "C" 127.0.0.1:6333> EVAL "return ',' < '*'" 0 (nil) 127.0.0.1:6333> CONFIG SET locale-collate "" OK 127.0.0.1:6333> EVAL "return ',' < '*'" 0 (integer) 1 ``` Co-authored-by: calvincjli <calvincjli@tencent.com> Co-authored-by: Oran Agra <oran@redislabs.com>

commit bdf7696 Author: sundb <sundbcn@gmail.com> Date: Tue Sep 6 19:50:14 2022 +0800 Fix test fail in eextern test mode commit 6ada91d Author: sundb <sundbcn@gmail.com> Date: Tue Sep 6 17:39:52 2022 +0800 Optimize comment in test commit b972e06 Author: sundb <sundbcn@gmail.com> Date: Tue Sep 6 17:30:29 2022 +0800 Fix crash due to wrongly split quicklist node commit 8e51c95 Author: sundb <sundbcn@gmail.com> Date: Tue Sep 6 16:01:08 2022 +0800 Fix crash due to delete entry from compress quicklist node commit 9022375 Author: Ariel Shtul <ariel.shtul@redislabs.com> Date: Tue Aug 23 09:37:59 2022 +0300 [PERF] use snprintf once in addReplyDouble (redis#11093) The previous implementation calls `snprintf` twice, the second time used to 'memcpy' the output of the first, which could be a very large string. The new implementation reserves space for the protocol header ahead of the formatted double, and then prepends the string length ahead of it. Measured improvement of simple ZADD of some 25%. commit 407b5c9 Author: Itamar Haber <itamar@redis.com> Date: Mon Aug 22 15:05:01 2022 +0300 Replaces a made-up term with a real one (redis#11169) commit a534983 Author: Itamar Haber <itamar@redis.com> Date: Sun Aug 21 18:15:53 2022 +0300 Changes "lower" to "capital" in GEO units history notes (redis#11164) A overlooked mistake in the redis#11162 commit ca6aead Author: yourtree <56780191+yourtree@users.noreply.github.com> Date: Sun Aug 21 22:55:45 2022 +0800 Support setlocale via CONFIG operation. (redis#11059) Till now Redis officially supported tuning it via environment variable see redis#1074. But we had other requests to allow changing it at runtime, see redis#799, and redis#11041. Note that `strcoll()` is used as Lua comparison function and also for comparison of certain string objects in Redis, which leads to a problem that, in different regions, for some characters, the result may be different. Below is an example. ``` 127.0.0.1:6333> SORT test alpha 1) "<" 2) ">" 3) "," 4) "*" 127.0.0.1:6333> CONFIG GET locale-collate 1) "locale-collate" 2) "" 127.0.0.1:6333> CONFIG SET locale-collate 1 (error) ERR CONFIG SET failed (possibly related to argument 'locale') 127.0.0.1:6333> CONFIG SET locale-collate C OK 127.0.0.1:6333> SORT test alpha 1) "*" 2) "," 3) "<" 4) ">" ``` That will cause accidental code compatibility issues for Lua scripts and some Redis commands. This commit creates a new config parameter to control the local environment which only affects `Collate` category. Above shows how it affects `SORT` command, and below shows the influence on Lua scripts. ``` 127.0.0.1:6333> CONFIG GET locale-collate 1) " locale-collate" 2) "C" 127.0.0.1:6333> EVAL "return ',' < '*'" 0 (nil) 127.0.0.1:6333> CONFIG SET locale-collate "" OK 127.0.0.1:6333> EVAL "return ',' < '*'" 0 (integer) 1 ``` Co-authored-by: calvincjli <calvincjli@tencent.com> Co-authored-by: Oran Agra <oran@redislabs.com> commit 31ef410 Author: Itamar Haber <itamar@redis.com> Date: Sun Aug 21 17:01:17 2022 +0300 Adds historical note about lower-case geo units support (redis#11162) This change was part of redis#9656 (Redis 7.0) commit c3a0253 Author: Wen Hui <wen.hui.ware@gmail.com> Date: Sun Aug 21 00:52:57 2022 -0400 Add 2 test cases for XDEL and XGROUP CREATE command (redis#11137) This PR includes 2 missed test cases of XDEL and XGROUP CREATE command 1. one test case: XDEL delete multiply id once 2. 3 test cases: XGROUP CREATE has ENTRIESREAD parameter, which equal 0 (special positive number), 3 and negative value. Co-authored-by: Ubuntu <lucas.guang.yang1@huawei.com> Co-authored-by: Oran Agra <oran@redislabs.com> Co-authored-by: Binbin <binloveplay1314@qq.com>

Till now Redis officially supported tuning it via environment variable see redis#1074. But we had other requests to allow changing it at runtime, see redis#799, and redis#11041. Note that `strcoll()` is used as Lua comparison function and also for comparison of certain string objects in Redis, which leads to a problem that, in different regions, for some characters, the result may be different. Below is an example. ``` 127.0.0.1:6333> SORT test alpha 1) "<" 2) ">" 3) "," 4) "*" 127.0.0.1:6333> CONFIG GET locale-collate 1) "locale-collate" 2) "" 127.0.0.1:6333> CONFIG SET locale-collate 1 (error) ERR CONFIG SET failed (possibly related to argument 'locale') 127.0.0.1:6333> CONFIG SET locale-collate C OK 127.0.0.1:6333> SORT test alpha 1) "*" 2) "," 3) "<" 4) ">" ``` That will cause accidental code compatibility issues for Lua scripts and some Redis commands. This commit creates a new config parameter to control the local environment which only affects `Collate` category. Above shows how it affects `SORT` command, and below shows the influence on Lua scripts. ``` 127.0.0.1:6333> CONFIG GET locale-collate 1) " locale-collate" 2) "C" 127.0.0.1:6333> EVAL "return ',' < '*'" 0 (nil) 127.0.0.1:6333> CONFIG SET locale-collate "" OK 127.0.0.1:6333> EVAL "return ',' < '*'" 0 (integer) 1 ``` Co-authored-by: calvincjli <calvincjli@tencent.com> Co-authored-by: Oran Agra <oran@redislabs.com>

Allow setting locale in lua script.

a5d850f

enjoy-binbin requested a review from MeirShpilraien July 26, 2022 14:44

sundb reviewed Jul 26, 2022

View reviewed changes

oranagra reviewed Jul 26, 2022

View reviewed changes

yourtree mentioned this pull request Jul 29, 2022

Support setlocale via CONFIG operation. #11059

Merged

oranagra closed this Aug 3, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow setting locale in lua script. #11041

Allow setting locale in lua script. #11041

yourtree commented Jul 26, 2022 •

edited

sundb Jul 26, 2022 •

edited

yourtree Jul 26, 2022

oranagra Jul 26, 2022

yourtree Jul 27, 2022

yossigo Jul 27, 2022

oranagra Jul 27, 2022

yourtree Jul 29, 2022

yourtree Jul 29, 2022

oranagra commented Aug 3, 2022

Allow setting locale in lua script. #11041

Allow setting locale in lua script. #11041

Conversation

yourtree commented Jul 26, 2022 • edited

sundb Jul 26, 2022 • edited

Choose a reason for hiding this comment

yourtree Jul 26, 2022

Choose a reason for hiding this comment

oranagra Jul 26, 2022

Choose a reason for hiding this comment

yourtree Jul 27, 2022

Choose a reason for hiding this comment

yossigo Jul 27, 2022

Choose a reason for hiding this comment

oranagra Jul 27, 2022

Choose a reason for hiding this comment

yourtree Jul 29, 2022

Choose a reason for hiding this comment

yourtree Jul 29, 2022

Choose a reason for hiding this comment

oranagra commented Aug 3, 2022

yourtree commented Jul 26, 2022 •

edited

sundb Jul 26, 2022 •

edited