Tarantool development patches archive
 help / color / mirror / Atom feed
* [Tarantool-patches] [PATCH 0/4] session settings fixes
@ 2020-03-30  9:13 Chris Sosnin
  2020-03-30  9:13 ` [Tarantool-patches] [PATCH 1/4] box: replace session_settings modules with a single array Chris Sosnin
                   ` (6 more replies)
  0 siblings, 7 replies; 27+ messages in thread
From: Chris Sosnin @ 2020-03-30  9:13 UTC (permalink / raw)
  To: v.shpilevoy, korablev, tarantool-patches

issue #1:https://github.com/tarantool/tarantool/issues/4711
issue #2:https://github.com/tarantool/tarantool/issues/4712
branch:https://github.com/tarantool/tarantool/tree/ksosnin/gh-4712-session-settings-v2

Chris Sosnin (4):
  box: replace session_settings modules with a single array
  box: add binary search for _session_settings space
  box: provide a user friendly frontend for accessing session settings
  sql: provide a user friendly frontend for accessing session settings

 extra/mkkeywordhash.c                         |   1 +
 src/box/lua/session.c                         | 111 +++++++++
 src/box/session.cc                            |   1 +
 src/box/session.h                             |   2 +
 src/box/session_settings.c                    | 214 +++++++++++-------
 src/box/session_settings.h                    |  53 +++--
 src/box/sql.c                                 |   5 -
 src/box/sql/build.c                           | 104 ++++-----
 src/box/sql/parse.y                           |   5 +
 src/box/sql/sqlInt.h                          |  11 +
 src/box/sql/vdbe.c                            |  50 ++++
 ...rontend.result => session_settings.result} | 147 ++++++++++--
 ...end.test.lua => session_settings.test.lua} |  61 ++++-
 13 files changed, 589 insertions(+), 176 deletions(-)
 rename test/box/{gh-4511-access-settings-from-any-frontend.result => session_settings.result} (71%)
 rename test/box/{gh-4511-access-settings-from-any-frontend.test.lua => session_settings.test.lua} (64%)

-- 
2.21.1 (Apple Git-122.3)

^ permalink raw reply	[flat|nested] 27+ messages in thread
* [Tarantool-patches] [PATCH 0/4] box: session settings fixes
@ 2020-02-17 12:12 Chris Sosnin
  2020-02-17 12:12 ` [Tarantool-patches] [PATCH 3/4] box: provide a user friendly frontend for accessing session settings Chris Sosnin
  0 siblings, 1 reply; 27+ messages in thread
From: Chris Sosnin @ 2020-02-17 12:12 UTC (permalink / raw)
  To: korablev, tarantool-patches

The first patch just merges all modules into one array, so the
binary search will work once for all settings.

The second patch is implementation of the binary search.

The last two patches add frontend for accessing session settings:
Lua table and SQL statement respectively.

branch: https://github.com/tarantool/tarantool/tree/ksosnin/gh-4712-search-settings
issue #1: https://github.com/tarantool/tarantool/issues/4711
issue #2: https://github.com/tarantool/tarantool/issues/4712

Chris Sosnin (4):
  box: replace session_settings modules with a single array
  box: add binary search for _session_settings space
  box: provide a user friendly frontend for accessing session settings
  sql: provide a user friendly frontend for accessing session settings

 src/box/lua/session.c                         |  92 ++++++++
 src/box/session.cc                            |   1 +
 src/box/session.h                             |   2 +
 src/box/session_settings.c                    | 214 +++++++++++-------
 src/box/session_settings.h                    |  53 +++--
 src/box/sql.c                                 |   5 -
 src/box/sql/build.c                           | 104 ++++-----
 src/box/sql/parse.y                           |   5 +
 src/box/sql/sqlInt.h                          |  11 +
 src/box/sql/vdbe.c                            |  50 ++++
 ...rontend.result => session_settings.result} | 149 ++++++++++--
 ...end.test.lua => session_settings.test.lua} |  61 ++++-
 12 files changed, 571 insertions(+), 176 deletions(-)
 rename test/box/{gh-4511-access-settings-from-any-frontend.result => session_settings.result} (71%)
 rename test/box/{gh-4511-access-settings-from-any-frontend.test.lua => session_settings.test.lua} (65%)

-- 
2.21.1 (Apple Git-122.3)

^ permalink raw reply	[flat|nested] 27+ messages in thread
* Re: [Tarantool-patches] [PATCH v4 3/3] box: provide a user friendly frontend for accessing session settings
@ 2020-02-03 22:17 Vladislav Shpilevoy
  2020-02-04 19:31 ` [Tarantool-patches] [PATCH 3/4] " Chris Sosnin
  0 siblings, 1 reply; 27+ messages in thread
From: Vladislav Shpilevoy @ 2020-02-03 22:17 UTC (permalink / raw)
  To: Chris Sosnin, tarantool-patches

Thanks for the patch!

See 12 comments below.

On 28/01/2020 13:50, Chris Sosnin wrote:
> - space_object:update() is hard to use for configuring session settings,
> so we provide box.session.setting table, which can be used in a much more
> native way.
> 
> - Prior to this patch sql settings were not accessible before box.cfg()
> call, even though these flags can be set right after session creation.
> 
> Part of #4711
> ---
>  src/box/lua/session.c                         | 106 ++++++++++++++++++
>  src/box/sql.c                                 |   5 -
>  ...1-access-settings-from-any-frontend.result |  57 ++++++++++
>  ...access-settings-from-any-frontend.test.lua |  17 +++
>  4 files changed, 180 insertions(+), 5 deletions(-)
> 
> diff --git a/src/box/lua/session.c b/src/box/lua/session.c
> index c6a600f6f..3ba1253c7 100644
> --- a/src/box/lua/session.c
> +++ b/src/box/lua/session.c
> @@ -42,6 +42,9 @@
>  #include "box/user.h"
>  #include "box/schema.h"
>  #include "box/port.h"
> +#include "box/session_settings.h"
> +
> +extern void sql_session_settings_init();

1. Please, don't mix SQL and Lua. At least here. Call it in
session.cc in session_init(). This file should not initialize
any session backends.

>  static const char *sessionlib_name = "box.session";
>  
> @@ -411,6 +414,108 @@ lbox_session_on_access_denied(struct lua_State *L)
>  				  lbox_push_on_access_denied_event, NULL);
>  }
>  
> +static int
> +session_setting_serialize(lua_State *L)
> +{
> +	lua_getfield(L, -1, "_id");
> +	int sid = lua_tointeger(L, -1);
> +	const char *mp_pair, *mp_pair_end;
> +	session_settings[sid].get(sid, &mp_pair, &mp_pair_end);
> +	uint32_t len;
> +	mp_decode_array(&mp_pair); /* [setting_name : value] */

2. Avoid writing comments in the same line as the code, unless
this is a special case like a structure inlined initialization.
This has simple reasons: lines are shorter and therefore easier
to read; diff is smaller when only the code or only the comment
are changed.

> +	mp_decode_str(&mp_pair, &len);
> +	uint32_t field_type = session_settings[sid].metadata.field_type;
> +	if (field_type == FIELD_TYPE_BOOLEAN) {
> +		bool value = mp_decode_bool(&mp_pair);
> +		lua_pushboolean(L, value);
> +	} else {
> +		const char *str = mp_decode_str(&mp_pair, &len);
> +		lua_pushlstring(L, str, len);
> +	}
> +	return 1;
> +}
> +
> +static int
> +session_setting_set(lua_State *L)
> +{
> +	if (lua_gettop(L) != 2)
> +		goto error;
> +	int arg_type = lua_type(L, -1);
> +	lua_getfield(L, -2, "_id");
> +	int sid = lua_tointeger(L, -1);
> +	struct session_setting *setting = &session_settings[sid];
> +	lua_pop(L, 1);
> +	switch (arg_type) {
> +		case LUA_TBOOLEAN: {
> +			bool value = lua_toboolean(L, -1);
> +			size_t size = mp_sizeof_bool(value);
> +			char *mp_value = (char *)region_alloc(&fiber()->gc,
> +							      size);

3. Just use static_alloc. The same below.

> +			mp_encode_bool(mp_value, value);
> +			if (setting->set(sid, mp_value) != 0)
> +				luaT_error(L);

4. According to the new agreement, all new Lua functions should
return nil, err when an error happens. Except OOM and invalid
usage of API. So if set() returns != 0, then please, use
luaT_push_nil_and_error().

> +			lua_pushstring(L, setting->name);
> +			lua_pushboolean(L, value);
> +			break;
> +		}
> +		case LUA_TSTRING: {
> +			const char *str = lua_tostring(L, -1);
> +			size_t len = strlen(str);
> +			uint32_t size = mp_sizeof_str(len);
> +			char *mp_value = (char *)region_alloc(&fiber()->gc,
> +							      size);
> +			mp_encode_str(mp_value, str, len);
> +			if (setting->set(sid, mp_value) != 0)
> +				luaT_error(L);
> +			lua_pushstring(L, setting->name);
> +			lua_pushstring(L, str);
> +			break;
> +		}
> +		default:
> +			goto error;
> +	}
> +	return 2;
> +error:
> +	luaL_error(L, "box.session.settings.set(): bad arguments");
> +	return -1;
> +}
> +
> +static void
> +session_setting_create(struct lua_State *L, int id)
> +{
> +	lua_newtable(L);
> +	lua_pushstring(L, "_id");
> +	lua_pushinteger(L, id);
> +	lua_settable(L, -3);
> +	lua_newtable(L); /* setting metatable */

5. The same as above about code + comment in one line. Also,
please, use capital letter to start a sentence, and put a dot
in the end.

> +	lua_pushstring(L, "__serialize");
> +	lua_pushcfunction(L, session_setting_serialize);
> +	lua_settable(L, -3);
> +	lua_setmetatable(L, -2);

6. You create lots of duplicate metatables here. I would
try to create it only once and use that one object for
all lua_setmetatable().

> +	lua_pushcfunction(L, session_setting_set);
> +	lua_setfield(L, -2, "set");
> +}
> +
> +void
> +session_settings_init(struct lua_State *L)
> +{
> +	/* Init settings that are avaliable right after session creation. */

7. avaliable -> available.

8. Please, keep comments in 66 line width limit.

> +	sql_session_settings_init();
> +	static const struct luaL_Reg settingslib[] = {
> +		{NULL, NULL}
> +	};
> +	luaL_register_module(L, "box.session.settings", settingslib);
> +	int id = sql_session_setting_BEGIN;

9. If you start from SQL, you will need to change this code
in case something will be added prior to SQL settings. Lets
start from 0 and iterate to SESSION_SETTING_COUNT.

> +	for (; id <= sql_session_setting_END; ++id) {
> +		/* Store settings as name : id */

10. Not sure this comment is still correct. You use
name : table, rather than name : id. But honestly here
this is obvious anyway, so I would drop that comment
at all.

> +		struct session_setting *setting = &session_settings[id];
> +		lua_pushstring(L, setting->name);
> +		session_setting_create(L, id);
> +		lua_settable(L, -3);
> +	}
> +	lua_pop(L, 1);
> +}
> +
>  void
>  session_storage_cleanup(int sid)
>  {
> @@ -448,6 +553,7 @@ exit:
>  void
>  box_lua_session_init(struct lua_State *L)
>  {
> +	session_settings_init(L);

11. I propose you to move it to the end of this function. You
initialize box.session.settings before box.session is
created. If you move it to the end, you will be able to drop
this dummy module registration, which you are doing above:

    static const struct luaL_Reg settingslib[] = {
    	{NULL, NULL}
    };

Because after

    luaL_register_module(L, sessionlib_name, sessionlib)

you have box.session on the stack and can add whatever you
want there, including settings.

>  	static const struct luaL_Reg session_internal_lib[] = {
>  		{"create", lbox_session_create},
>  		{"run_on_connect",    lbox_session_run_on_connect},
> diff --git a/test/box/gh-4511-access-settings-from-any-frontend.test.lua b/test/box/gh-4511-access-settings-from-any-frontend.test.lua
> index 40a58ad04..53f03450d 100644
> --- a/test/box/gh-4511-access-settings-from-any-frontend.test.lua
> +++ b/test/box/gh-4511-access-settings-from-any-frontend.test.lua
> @@ -118,3 +118,20 @@ s:update('sql_defer_foreign_keys', {{'=', 'some text', true}})
>  s:update('sql_defer_foreign_keys', {{'=', 'value', 1}})
>  s:update('sql_defer_foreign_keys', {{'=', 'value', {1}}})
>  s:update('sql_defer_foreign_keys', {{'=', 'value', '1'}})
> +
> +-- gh-4711: Provide a user-friendly frontend for accessing session settings.

12. The file has name 'gh-4511-*', so you can't add issue 4711 tests
here. Either add to a new file, or rename the test file to
box/session_settings.

> +settings = box.session.settings
> +assert(settings ~= nil)
> +
> +s:update('sql_default_engine', {{'=', 2, 'vinyl'}})
> +settings.sql_default_engine
> +settings.sql_default_engine:set('memtx')
> +assert(s:get('sql_default_engine').value == 'memtx')
> +settings.sql_defer_foreign_keys:set(true)
> +assert(s:get('sql_defer_foreign_keys').value == true)
> +s:update('sql_defer_foreign_keys', {{'=', 2, false}})
> +settings.sql_defer_foreign_keys
> +
> +settings.sql_default_engine:set(true)
> +settings.sql_defer_foreign_keys:set(false, 1, 2, 3)
> +settings.sql_parser_debug:set('string')
> 

^ permalink raw reply	[flat|nested] 27+ messages in thread
end of thread, other threads:[~2020-04-13 14:18 UTC | newest]

Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-30  9:13 [Tarantool-patches] [PATCH 0/4] session settings fixes Chris Sosnin
2020-03-30  9:13 ` [Tarantool-patches] [PATCH 1/4] box: replace session_settings modules with a single array Chris Sosnin
2020-04-03 13:32   ` Nikita Pettik
2020-03-30  9:13 ` [Tarantool-patches] [PATCH 2/4] box: add binary search for _session_settings space Chris Sosnin
2020-04-03 14:00   ` Nikita Pettik
2020-04-13 13:40     ` Kirill Yukhin
2020-03-30  9:13 ` [Tarantool-patches] [PATCH 3/4] box: provide a user friendly frontend for accessing session settings Chris Sosnin
2020-04-03 14:47   ` Nikita Pettik
2020-03-30  9:13 ` [Tarantool-patches] [PATCH 4/4] sql: " Chris Sosnin
2020-04-03 15:19   ` Nikita Pettik
2020-04-04 21:56   ` Vladislav Shpilevoy
2020-04-10 15:40     ` Chris Sosnin
2020-04-11 17:18       ` Vladislav Shpilevoy
2020-04-13  7:50       ` Timur Safin
2020-04-02  9:14 ` [Tarantool-patches] [PATCH 0/4] session settings fixes Timur Safin
2020-04-02 10:18   ` Chris Sosnin
2020-04-03 12:47   ` Nikita Pettik
2020-04-03 13:09 ` Nikita Pettik
2020-04-03 14:02   ` Chris Sosnin
2020-04-13 14:18 ` Kirill Yukhin
  -- strict thread matches above, loose matches on Subject: below --
2020-02-17 12:12 [Tarantool-patches] [PATCH 0/4] box: " Chris Sosnin
2020-02-17 12:12 ` [Tarantool-patches] [PATCH 3/4] box: provide a user friendly frontend for accessing session settings Chris Sosnin
2020-03-16 16:08   ` Nikita Pettik
2020-03-16 22:53     ` Vladislav Shpilevoy
2020-03-17 14:27       ` Nikita Pettik
2020-03-17 14:36         ` Chris Sosnin
2020-02-03 22:17 [Tarantool-patches] [PATCH v4 3/3] " Vladislav Shpilevoy
2020-02-04 19:31 ` [Tarantool-patches] [PATCH 3/4] " Chris Sosnin
2020-02-06 22:15   ` Vladislav Shpilevoy

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox