[Tarantool-patches] [PATCH 0/4] session settings fixes

[Tarantool-patches] [PATCH 0/4] session settings fixes

[Peter Gulutzan]

Hi,

Re:
SETTING SET "sql_default_engine" = 'memtx';

Adding a new reserved word SETTING, without advance notice,
should be against policy. It causes a slight risk
of breaking an existing application.

I had hoped for
UPDATE _SETTINGS_DEFAULT_ENGINE SET VALUE = 'memtx';
I claim that is just as "user friendly" -- it
adds two words, but it removes quotation marks.
And it does not require that users learn new syntax
because UPDATE ... SET is well known.

Notice that I can already do this -- almost.
This is legal:
CREATE VIEW _SETTINGS_DEFAULT_ENGINE AS
     SELECT "value" AS value
     FROM "_session_settings"
     WHERE "name" = 'sql_default_engine';
The flaw is that views are not updatable.
But I don't regard that as a big problem
because this is not a proposal to make
all views updatable. It only requires:
when you see that exact UPDATE statement,
transform it to box.session._settings:update.
And similarly for sql_defer_foreign_keys etc.

Alternatively, users could create a Lua function
X which updates
_session_settings, vaguely like this
box.schema.func.create('X',
     {language = 'LUA',
      returns = 'string',
      body = [[function ()
box.space._session_settings:update('sql_default_engine', {{'=', 2, 
'memtx'}});
               return 'memtx changed'
               end]],
      is_sandboxed = false,
      exports = {'LUA', 'SQL'},
      is_deterministic = true})
then allow this SQL syntax:
CALL X();
Again, I claim that this is just as "user friendly"
because -- as far as I can tell -- some people think
saving keystrokes is the same as being friendly.
Nikita Pettik explained that issue#4711 is for a shortcut
and that is short.
Eventually we will probably want to support CALL anyway,
and it is already a reserved word.

Thus there are "user-friendly" alternatives that do not
require new proprietary syntax.

Peter Gulutzan

Re: [Tarantool-patches] [PATCH 0/4] session settings fixes

[Timur Safin]

: From: Peter Gulutzan <pgulutzan@ocelot.ca>
: 
: Re:
: SETTING SET "sql_default_engine" = 'memtx';
: 
: Adding a new reserved word SETTING, without advance notice,
: should be against policy. It causes a slight risk
: of breaking an existing application.
: 
: I had hoped for
: UPDATE _SETTINGS_DEFAULT_ENGINE SET VALUE = 'memtx';


: I claim that is just as "user friendly" -- it
: adds two words, but it removes quotation marks.
: And it does not require that users learn new syntax
: because UPDATE ... SET is well known.

Yes, I like this way `UPDATE config SET VALUE = value` - 
it's looking very natural for SQL, without any unnecessary 
extension of a language whatsoever.

Timur

Re: [Tarantool-patches] [PATCH 0/4] session settings fixes

[Chris Sosnin]

Hi!

> On 3 Apr 2020, at 20:09, Peter Gulutzan <pgulutzan@ocelot.ca> wrote:
> 
> Hi,
> 
> Re:
> SETTING SET "sql_default_engine" = 'memtx';
> 
> Adding a new reserved word SETTING, without advance notice,
> should be against policy. It causes a slight risk
> of breaking an existing application.
> 
> I had hoped for
> UPDATE _SETTINGS_DEFAULT_ENGINE SET VALUE = 'memtx';

Implementing this syntax would either require creating a table for each setting
or adding new keywords, since this is the same as updating tables.

Moreover, SET VALUE construction also is not supported, so we still need
to reserve a new word.

We can borrow another MySQL syntax — SET SESSION name = value
What do you think about this option?

> I claim that is just as "user friendly" -- it
> adds two words, but it removes quotation marks.
> And it does not require that users learn new syntax
> because UPDATE ... SET is well known.
> 
> Notice that I can already do this -- almost.
> This is legal:
> CREATE VIEW _SETTINGS_DEFAULT_ENGINE AS
>     SELECT "value" AS value
>     FROM "_session_settings"
>     WHERE "name" = 'sql_default_engine';
> The flaw is that views are not updatable.
> But I don't regard that as a big problem
> because this is not a proposal to make
> all views updatable. It only requires:
> when you see that exact UPDATE statement,
> transform it to box.session._settings:update.
> And similarly for sql_defer_foreign_keys etc.
> 
> Alternatively, users could create a Lua function
> X which updates
> _session_settings, vaguely like this
> box.schema.func.create('X',
>     {language = 'LUA',
>      returns = 'string',
>      body = [[function ()
> box.space._session_settings:update('sql_default_engine', {{'=', 2, 'memtx'}});
>               return 'memtx changed'
>               end]],
>      is_sandboxed = false,
>      exports = {'LUA', 'SQL'},
>      is_deterministic = true})
> then allow this SQL syntax:
> CALL X();
> Again, I claim that this is just as "user friendly"
> because -- as far as I can tell -- some people think
> saving keystrokes is the same as being friendly.
> Nikita Pettik explained that issue#4711 is for a shortcut
> and that is short.
> Eventually we will probably want to support CALL anyway,
> and it is already a reserved word.
> 
> Thus there are "user-friendly" alternatives that do not
> require new proprietary syntax.
> 
> Peter Gulutzan
> 

Re: [Tarantool-patches] [PATCH 0/4] session settings fixes

[Timur Safin]

: -----Original Message-----
: From: Tarantool-patches <tarantool-patches-bounces@dev.tarantool.org> On
: Behalf Of Chris Sosnin
: 
: > On 3 Apr 2020, at 20:09, Peter Gulutzan <pgulutzan@ocelot.ca> wrote:
: >
: > Hi,
: >
: > Re:
: > SETTING SET "sql_default_engine" = 'memtx';
: >
: > Adding a new reserved word SETTING, without advance notice,
: > should be against policy. It causes a slight risk
: > of breaking an existing application.
: >
: > I had hoped for
: > UPDATE _SETTINGS_DEFAULT_ENGINE SET VALUE = 'memtx';
: 
: Implementing this syntax would either require creating a table for each
: setting
: or adding new keywords, since this is the same as updating tables.
: 
: Moreover, SET VALUE construction also is not supported, so we still need
: to reserve a new word.
: 
: We can borrow another MySQL syntax — SET SESSION name = value
: What do you think about this option?
: 

Peter, after multiple rounds of discussions here (one incompatible way against another incompatible way) we tend to prefer the simpler syntax

    SET SESSION sqlconfigname = value

because it's quite simpler, looks similar to MySQL way (though still is not part of standard), and would require less intrusive patch to parser. Introducing all configuration settins as separate system spaces (via hardcoding in the engine) looked to us slightly more intrusive (while still being not as much closer to the standard way).

Minuses against minuses on both sides. 

What do you think, Peter?

Thanks,
Timur

Re: [Tarantool-patches] [PATCH 0/4] session settings fixes

[Peter Gulutzan]

On 2020-04-08 3:40 a.m., Timur Safin wrote:
<cut>
 > Peter, after multiple rounds of discussions here (one incompatible 
way against another incompatible way) we tend to prefer the simpler syntax
 >
 >     SET SESSION sqlconfigname = value
 >
 > because it's quite simpler, looks similar to MySQL way (though still 
is not part of standard), and would require less intrusive patch to 
parser. Introducing all configuration settins as separate system spaces 
(via hardcoding in the engine) looked to us slightly more intrusive 
(while still being not as much closer to the standard way).
 >
 > Minuses against minuses on both sides.
 >
 > What do you think, Peter?

My main objection to SETTING SET was that it
might cause an existing legal statement to become illegal.
I have asked C. Sosnin whether SET SESSION solves that.

My other objection was about proprietary syntax.
SET SESSION is not extremely different from standard SQL
SET SESSION CHARACTERISTICS (which I realize is too long),
and proprietary syntax is less bad if another vendor
does it. Incidentally MySQL handles this correctly:
CREATE PROCEDURE p()
BEGIN
   DECLARE session INTEGER;
   SET session = 5;
END;
Eventually, when Tarantool supports declared variables,
I hope everyone will remember that should be legal.

Thank you for considering alternative suggestions.
Please proceed with what you all regard as least intrusive.

Peter Gulutzan

Re: [Tarantool-patches] [PATCH 0/4] session settings fixes

[Peter Gulutzan]

Hi,

On 2020-04-07 10:32 a.m., Chris Sosnin wrote:
 > Hi!
 >
 >> On 3 Apr 2020, at 20:09, Peter Gulutzan <pgulutzan@ocelot.ca> wrote:
 >>
 >> Hi,
 >>
 >> Re:
 >> SETTING SET "sql_default_engine" = 'memtx';
 >>
 >> Adding a new reserved word SETTING, without advance notice,
 >> should be against policy. It causes a slight risk
 >> of breaking an existing application.
 >>
 >> I had hoped for
 >> UPDATE _SETTINGS_DEFAULT_ENGINE SET VALUE = 'memtx';
 >
 > Implementing this syntax would either require creating a table for 
each setting
 > or adding new keywords, since this is the same as updating tables.
 >

It would definitely require creating a (viewed) table for each setting.
That is why I said "And similarly for sql_defer_foreign_keys etc."

 > Moreover, SET VALUE construction also is not supported, so we still need
 > to reserve a new word.
 >

As I pointed out in my earlier email, I can say
UPDATE _settings_default_engine SET value = 'memtx';
now, in a current Tarantool version, without a parser error.
If _settings_default_engine is not a view, it works fine.
So I didn't see why there is a need to reserve VALUE.

(Digression: That does not mean I like using VALUE for a
column name. I objected about it in an earlier email.
But I did not propose a better name, and now it is too late.)

 > We can borrow another MySQL syntax — SET SESSION name = value
 > What do you think about this option?

In MySQL, SESSION is not a reserved word.
Let us go back to my original complaint.
CREATE TABLE setting (setting INTEGER PRIMARY KEY);
That statement is legal in Tarantool-2.4.
That statement is illegal in branch ksosnin/gh-4712-session-settings-v2.
If you say SET SESSION instead of SETTING SET,
is there still a need to add a new reserved word without notice?

(Digression: Currently, every word that can be the start of a statement
is reserved. Also, a few words that might in future be the start of a
statement are reserved -- ANALYZE, BEGIN, CALL, DECLARE, END, FETCH, FOR,
GRANT, IF, ITERATE, LEAVE, LOOP, RENAME, REPEAT, RESIGNAL, REVOKE, SET,
SIGNAL. I believe that is a good thing. I do not believe that
SETTING should be unreserved if it is the start of a statement.)

Peter Gulutzan

[Tarantool-patches] [PATCH 0/4] session settings fixes

[Chris Sosnin]

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)

Re: [Tarantool-patches] [PATCH 0/4] session settings fixes

[Timur Safin]

I'm late here on this party, but I very much dislike proposed syntax of

	SETTING SET configuration_name = value;

It feels not SQL-ish, and I see no much value introducing new statement 
while there is still SET statement available. 

Why we could not simply use something like:
	SET box.session.settings.name = value;
?

Thanks,
Timur


: -----Original Message-----
: From: Tarantool-patches <tarantool-patches-bounces@dev.tarantool.org> On
: Behalf Of Chris Sosnin
: Sent: Monday, March 30, 2020 12:14 PM
: To: v.shpilevoy@tarantool.org; korablev@tarantool.org; tarantool-
: patches@dev.tarantool.org
: Subject: [Tarantool-patches] [PATCH 0/4] session settings fixes
: 
: 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)

Re: [Tarantool-patches] [PATCH 0/4] session settings fixes

[Chris Sosnin]

> On 2 Apr 2020, at 12:14, Timur Safin <tsafin@tarantool.org> wrote:
> 
> I'm late here on this party, but I very much dislike proposed syntax of
> 
> 	SETTING SET configuration_name = value;
> 
> It feels not SQL-ish, and I see no much value introducing new statement 
> while there is still SET statement available. 
> 
> Why we could not simply use something like:
> 	SET box.session.settings.name = value;
> ?
> 
> Thanks,
> Timur

It was SET <setting_name> = <new_value> in the first version.

However, we agreed on this syntax being too generic for configuring
session settings, so I added SETTING keyword.

> 
> 
> : -----Original Message-----
> : From: Tarantool-patches <tarantool-patches-bounces@dev.tarantool.org> On
> : Behalf Of Chris Sosnin
> : Sent: Monday, March 30, 2020 12:14 PM
> : To: v.shpilevoy@tarantool.org; korablev@tarantool.org; tarantool-
> : patches@dev.tarantool.org
> : Subject: [Tarantool-patches] [PATCH 0/4] session settings fixes
> : 
> : 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)
> 
> 

Re: [Tarantool-patches] [PATCH 0/4] session settings fixes

[Nikita Pettik]

On 02 Apr 12:14, Timur Safin wrote:
> I'm late here on this party, but I very much dislike proposed syntax of

Patch is not merged, so it's still okay to suggest changes.

> 	SETTING SET configuration_name = value;
> 
> It feels not SQL-ish, and I see no much value introducing new statement 
> while there is still SET statement available. 

SET is not available as a separated statements; it's only a part
of UPDATE statements syntax. Rolling it back (SETTING SET -> SET)
is not a big deal - only one line of parse.y requires patching.

As I already said, I'm okay with all variations, whether it is
SET, SET SETTING or SETTING SET.
 
> Why we could not simply use something like:
> 	SET box.session.settings.name = value;

box.session.settings -- too long prefix. Originally, SET was
suggested as a shortcat for normal UPDATE "_session_settings" ...
SQL statement. Since it is shortcat, let's keep it short.

> ?
> 
> Thanks,
> Timur
> 

Re: [Tarantool-patches] [PATCH 0/4] session settings fixes

[Nikita Pettik]

On 30 Mar 12:13, Chris Sosnin wrote:
> 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

Nit: while sending next version of patch-set please specify it
explicitly with --subject-prefix='PATCH v2' and attach changelog
between versions. For example: https://lists.tarantool.org/pipermail/tarantool-patches/2020-March/015103.html
 
> 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)
> 

Re: [Tarantool-patches] [PATCH 0/4] session settings fixes

[Chris Sosnin]

It was v2, Vlad asked me to resend the whole patchset for 
convenience. Won’t forget to do this next time.

> On 3 Apr 2020, at 16:09, Nikita Pettik <korablev@tarantool.org> wrote:
> 
> On 30 Mar 12:13, Chris Sosnin wrote:
>> 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
> 
> Nit: while sending next version of patch-set please specify it
> explicitly with --subject-prefix='PATCH v2' and attach changelog
> between versions. For example: https://lists.tarantool.org/pipermail/tarantool-patches/2020-March/015103.html
> 
>> 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)
>> 

Re: [Tarantool-patches] [PATCH 0/4] session settings fixes

[Kirill Yukhin]

Hello,

On 30 Mar 12:13, Chris Sosnin wrote:
> 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%)

I've checked 1, 3 and 4 patches into master.

--
Regards, Kirill Yukhin